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, view it on GitHub, or mute the thread.

[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/xrootd/xrootd/pull/1002?email_source=notifications\u0026email_token=AA7NRDQGNMC6GMXT4YTJDE3P25LQNA5CNFSM4HYGFXQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX2TYQI#issuecomment-502611009", "url": "https://github.com/xrootd/xrootd/pull/1002?email_source=notifications\u0026email_token=AA7NRDQGNMC6GMXT4YTJDE3P25LQNA5CNFSM4HYGFXQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX2TYQI#issuecomment-502611009", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

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