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. > 2. There are several new compilation warnings on Windows using VS2013: > ..\..\..\subversion\libsvn_subr\x509parse.c(101): warning C4018: '>' : > signed/unsigned mismatch > ..\..\..\subversion\libsvn_subr\x509parse.c(1054): warning C4389: '!=' > : signed/unsigned mismatch This appears to be because pointers are unsigned and apr_size_t is signed. Guess we can just cast the pointer arithmetic to apr_size_t. So they end up looking like this respectively: if (*len > (apr_size_t)(end - *p)) if (len != (apr_size_t)(end - p)) > ..\..\..\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;
