On 13 August 2014 01:24, Branko Čibej <br...@wandisco.com> wrote: > 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? Yes, it does.
> 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. > Yes, but I suggest just add tiny static function with nice printing in auth-cmd.c: simple solution for release. > 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. > Agree. -- Ivan Zhakov CTO | VisualSVN | http://www.visualsvn.com
