[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [VOTE] Merge svn-auth-x509 branch to trunk?

From: Ivan Zhakov <ivan_at_visualsvn.com>
Date: Tue, 12 Aug 2014 23:56:34 +0400

On 11 August 2014 20:51, Ben Reser <ben_at_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

Ivan Zhakov
Received on 2014-08-12 21:57:22 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.