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

On 12.08.2014 23:47, Ben Reser wrote:
> On 8/12/14 12:56 PM, Ivan Zhakov wrote:
>> 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.
> Per our discussion I'll just remove the svn_checksum_* changes on the branch.
> After we get the X.509 stuff merged to trunk I'll commit something that changes
> the checksum output formatting and that accommodates most of your comments. I
> am still inclined to use flags so that the function can be used in multiple
> differing cases.

This thing against flags and booleans that change API behaviour does
have a very good argument going for it: it makes the code less obvious
for the sake of saving a few function names.

Consider the standard example, which (these days) would be an API to a
DOM-like object that can be displayed or not; to query its state, you'd
have a method called

    bool is_visible();

To change the state, the "flaggy" design would be

    void set_visibility(bool);

but the obviously more readable design uses two methods,

    void show();
    void hide();

With the former, you always have to worry about what the boolean
parameter actually means; with the latter, there can be no doubt.

So in the case of displaying the contents of checksums, first of all
something called

    svn_checksum_to_cstring_display

is clearly a broken API, especially if it's used to format the checksum
for the protocol, i.e., has nothing to do with displaying it at all. The
docstring says nothing about displaying; and there's no difference
between this function and svn_checksum_to_cstring, other than that the
latter accepts (and possibly returns) a NULL, where the former does not.

Then we have svn_checksum_serialize and svn_checksum_deserialize, which
I would expect to be always used for the protocol; and
svn_checksum_parse_hex, where it's not clear if it's the inverse of
svn_checksum_to_cstring, or the _display variant, or both, or WTF.

In other words, at least one of these APIs needs to be deprecated
because it has a terrible name, and all of them need better
documentation. If we want a function that's intended specifically for
printing fingerprints, then by all means call it

    svn_checksum_fingerprint

although, really, I don't know why such a thing would have to be in
libsvn_subr, or perhaps in svn_checksum.h; I could imagine such a thing
living in subr but exposed in svn_cmdline.h, called
svn_cmdline_fingerprint or some such.

-- Brane

-- 
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
e. brane_at_wandisco.com