On 12.08.2014 21:56, Ivan Zhakov wrote:
> 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
> 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. brane_at_wandisco.com
Received on 2014-08-12 23:25:15 CEST