On 13 August 2014 01:24, Branko Čibej <brane_at_wandisco.com> wrote:
> 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?
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
Received on 2014-08-12 23:35:32 CEST