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))
> 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.
>>> 2. May be 'svn auth' should print warning and continue on certificate
>>> parsing error?
>>
>> Yes that should be the case.
>>
> Great, that would be great to have.
I'll fix this as well (actually I already fixed it but I'm seeing error leak
aborts that appear to be pre-existing that I haven't figured out yet, they may
be from my local changes that turn on the automatic pager).
Received on 2014-08-11 18:50:19 CEST