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(). -- Ivan Zhakov
