On 7 August 2014 21:03, Ben Reser <ben_at_reser.org> wrote:
> On 8/7/14 4:10 AM, Ivan Zhakov wrote:
>> Several comments on branch code itself:
>> 1. Probably it makes sense to do not deprecate
>> svn_checksum_to_cstring_display() or have local x509 implementation
>> for fingerprint formatting because we use
>> svn_checksum_to_cstring_display() as canonical representation of
>> checksum in libsvn_fs/libsvn_repos. So I suggest revert r1616093 from
>> branch and commit attached patch.
>>
>> This avoid a lot of unrelated changes. I'm ready to do this. Do you
>> have any objections?
>
> The biggest reason I went ahead and deprecated the old function is to try and
> drive us to think about what format we're outputting. Quite a few of the
> places we're outputting the rather difficult to read and compare format to end
> users. We should probably change that but I think it's well outside the scope
> of the branch so I didn't do it.
>
> As you point out there are also a number of places where the formatting is part
> of our format. It's not just the repository, but it's also used in the
> filenames for pristines.
>
> I think I would much rather have these sorts of uses move to some other
> function name (or maybe a macro) since frankly
> svn_checksum_to_cstring_display() seems like the wrong name for a function
> that's outputting as you put it "canonical representations of checksums", which
> are not ended to be displayed.
>
> I certainly was surprised by all the places we ended up using it and felt that
> someone could have easily broken the repository by making a change to the
> formatting.
>
> I'm not fond of the idea of making that format private to the auth command
> because then 3rd parties have to duplicate it to match our output and we can't
> use it in other places that I think we probably ought to be using.
>
I agree svn_checksum_to_cstring_display() is wrong name and the proper
solution to make separated (probably) optimized function for
converting checksum to canonical string representation. But this is
definitely out of scope of this branch. I've gone ahead, reverted
r1616093 from branch and implemented svn_x509_fingerprint_to_display()
for converting cert fingerprints to display string. This is pubic and
tested function so other Subversion clients could use it. Please let
me know you're not happy with my solution.
Btw I've noticed problem that svn auth ends with 'svn: E240018:
Certificate signature mismatch' error for one of certificate stored on
my laptop. There are two questions here:
1. Why Subverison fails to parse this certificate? I could email this
certificate file privately if you're interested.
2. May be 'svn auth' should print warning and continue on certificate
parsing error?
>> ..\..\..\subversion\libsvn_subr\x509parse.c(939): warning C4018: '<' :
>> signed/unsigned mismatch
>> ..\..\..\subversion\libsvn_subr\x509parse.c(946): warning C4389: '!='
>> : signed/unsigned mismatch
>
> This is an easy fix. int i should be apr_size_t i;
>
Great!
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Received on 2014-08-10 14:04:46 CEST