On 12.08.2014 21:56, Ivan Zhakov wrote: > On 11 August 2014 20:51, Ben Reser <b...@reser.org> wrote: >> On 8/11/14 1:59 AM, Ivan Zhakov wrote: >>> My primary concerns was that with svn_checksum_to_cstring_display2() >>> we have to care at every place to use proper flags to get some >>> canonical representation for protocol/storage. >> How about svn_checksum_to_cstring_canonical() which is just this macro: >> #define svn_checksum_to_cstring_canonical(checksum, pool) >> svn_checksum_to_cstring_display2((checksum), SVN_CHECKSUM_CSTRING_LOWER, >> (pool)) >> > I found macro as unnecessary hack in this case, while I like > svn_checksum_to_cstring_canonical() name. Do you have any reasons > against just making it public function. > >>> Exactly. I want to leave svn_checksum_to_cstring_display() and do not >>> change all callers on the branch: >>> 1. It's a little bit out of scope of the branch (many unrelated >>> changes, that should be reviewed that means less focus on x509 >>> changes) >> I think we've spent more time talking about this than the time it takes to >> review those changes. Those are really easy changes since all they're doing >> is >> adding the SVN_CHECKSUM_CSTRING_LOWER and changing >> svn_checksum_to_cstring_display to svn_checksum_to_cstring_display2. >> >>> 2. We currently use svn_checksum_to_cstring_display() as canonical >>> representation of checksum in many places. And replacing them with >>> calls to functions with flags is error prone since we have to care to >>> use the same flags. >>> >>> Brane asked me to reverted my branch changes for some reason, while I >>> just wanted to help you: >>> http://svn.haxx.se/dev/archive-2014-08/0113.shtml >>> >>> I've reverted my branch changes in r1617225. So I'm leaving solution >>> of svn_checksum_to_cstring_display() problem to you. Please let me >>> know when you are finished and I'll review branch again. >>> I'm -1 on merging this branch until the >>> svn_checksum_to_cstring_display2() issue is resolved. >> Does the macro I suggest above resolve the problem for you? I'll be happy to >> go find those cases and change to them, before or after the branch merges. >> But >> I don't really want to bother with that if you're not willing to budge on the >> leaving svn_checksum_to_cstring_display() alone. >> > My concerns are the following: > 1. Avoid unrelated branch changes > 2. Have some function for converting checksum to canonical form: > a) one option is just leave svn_checksum_to_cstring_display() and > add svn_checksum_to_cstring_display_ex() > b) another option is deprecate svn_checksum_to_cstring_display(), > replacing existing callers with > new svn_checksum_to_cstring_canonical(), but I think it's > better do this on trunk to make review easier. > > Btw it would be nice to have tests for > svn_checksum_to_cstring_display2()/svn_checksum_to_cstring_display_ex().
At this point, just reverting the change that introduced svn_checksum_to_cstring_display2 from the x509 branch would resolve your objections, right, Ivan? We can live with not having a canonical checksum representation for display purposes that's different from the one in our wire protocol, and we can certainly address this mess separately from the x509 parser — which is, after all, the main purpose of the branch. To clarify, I would be against releasing the authn code as it is, with the extra info stored in the authn cache just because we don't have a cert parser on trunk; and I think the branch, even without the display changes, solves that problem just fine. -- Brane -- Branko Čibej | Director of Subversion WANdisco | Realising the impossibilities of Big Data e. br...@wandisco.com
