 |
 |
I see you've already pulled Brian's pull request: #1001. In essence, Brian's patch and mine address the same issue. This explains the merge-conflict. As it happens, I developed my patch independently from Brian -- I only learn of his pull-request when I posted my pull-request to the email discussion about this issue. Since you asked my opinion, here it is! For me, there are some stylistic issues with Brian's patch. - The patch violates the DRY principle (Don't Repeat Yourself), since the new method (`convert_digest_rfc_name`) is just the composition of `convert_digest_name` and `convert_xrootd_to_rfc_name` with a minor optimisation. - The patch results in the input string being parsed multiple times, including the if-else-if chain using the (relatively) expensive strcasecmp (once to discover which algorithm to query xrootd; once again to discover which RFC-3230 algorithm name to include in response). This is, perhaps, not terrible, but it's unnecessary. - It "smells" like premature optimisation: given `convert_digest_rfc_name` is just a composition, why not just apply `convert_digest_name` and `convert_xrootd_to_rfc_name` to make this obvious? Given the method will be delayed by calculating the checksum value, the additional overhead will likely be insignificant. - It makes it harder to fix the checksum priority ordering (the "p" values where the client says which algorithm it prefers), since the p-values would now need to be parsed in two functions (`convert_digest_rfc_name` and `convert_digest_name`). This is a consequence of DRY violation. - It also fails to close a potential cause of bugs in the future, since it continues to assume the returned checksum is the first algorithm the client requested. A future change could modify which checksum is returned (e.g., prefer returning a cached checksum, if one of the requested checksums is already known). As the returned checksum now comes from (re-)parsing the request header, updating the code to support checksum caching would require the author to be aware how the returned checksum name is calculated and modify it. That all said, I believe his patch works. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/xrootd/xrootd/pull/1002#issuecomment-502611009 ######################################################################## Use REPLY-ALL to reply to list To unsubscribe from the XROOTD-DEV list, click the following link: https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1