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

Since WC-NG we tried not to introduce new functions with flag arguments as in general functions like that are hard to maintain, while it is easy to rev functions to add another separate argument. (Another less preferred option is using a struct with separate args)

I remember arguments from gstein but I have a hard time finding a mail reference.

I think this would be the first new flag style argument in a public function since 1.2 or so.... If possible I would try using a different pattern here.

I like the intermediate option of getting at least the feature merged to trunk without this function. I don't see any arguments against that.

Bert

-----Original Message-----
From: "Branko Čibej" <brane_at_wandisco.com>
Sent: ‎12/‎08/‎2014 23:25
To: "dev_at_subversion.apache.org" <dev_at_subversion.apache.org>
Subject: Re: [VOTE] Merge svn-auth-x509 branch to trunk?

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