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
To change the state, the "flaggy" design would be
but the obviously more readable design uses two methods,
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
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
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.
Branko Čibej | Director of Subversion
WANdisco | Realising the impossibilities of Big Data
Received on 2014-08-13 01:39:45 CEST