On Tue, May 19, 2009 at 04:32:13AM -0400, John Szakmeister wrote:
> On Mon, May 18, 2009 at 10:16 PM, Neels Janosch Hofmeyr <neels_at_elego.de> wrote:
> > Hi all,
> >
> > I came across this snippet of code:
> >
> > subversion/libsvn_subr/checksum.c:
> > [[[
> > svn_boolean_t
> > svn_checksum_match(const svn_checksum_t *checksum1,
> > const svn_checksum_t *checksum2)
> > {
> > if (checksum1 == NULL || checksum2 == NULL)
> > return TRUE;
> > ]]]
> >
> > I don't see how that makes sense. If it does, then the function name is
> > misleading. The comment from include/checksum.h is ... tricky:
>
> It is a bit tricky, but the answer lies in svn_checksum_parse_hex().
> If the checksum provided is all zeros (e.g., if the string read in
> from node rep was '0000000000000000'), then svn_checksum_parse_hex()
> will actually return NULL. IOW, NULL is the wildcard checksum even
> though it's a string of zeros in the rep.
>
> I'd say that better documentation of that fact is in order, although
> I'm not sure where exactly that should be documented.
I'd say a better representation of a wildcard checksum is in order.
The API currently overloads the meaning of NULL, which I don't think
is a good idea. Whenever I see a NULL pointer, I associate that with
"uninitialized" and hence "not OK to use".
I don't really care if it's done by setting pointers to NULL,
but then we should at least
#define SVN_WILDCARD_CHECKSUM NULL
and have the code say:
if (checksum1 == SVN_WILDCARD_CHECKSUM ||
checksum2 == SVN_WILDCARD_CHECKSUM)
return TRUE;
Or going even further, we could add a "wildchard" checksum type.
Something like this:
Index: subversion/include/svn_checksum.h
===================================================================
--- subversion/include/svn_checksum.h (revision 37767)
+++ subversion/include/svn_checksum.h (working copy)
@@ -43,7 +43,11 @@ typedef enum
svn_checksum_md5,
/** The checksum is (or should be set to) a SHA1 checksum. */
- svn_checksum_sha1
+ svn_checksum_sha1,
+
+ /** The checksum is a wildcard.
+ * @since New in 1.7 */
+ svn_checksum_wildcard
} svn_checksum_kind_t;
/**
@@ -128,8 +132,9 @@ svn_checksum_to_cstring(const svn_checksum_t *chec
/** Parse the hex representation @a hex of a checksum of kind @a kind and
* set @a *checksum to the result, allocating in @a pool.
*
- * If @a hex is @c NULL or is the all-zeros checksum, then set @a *checksum
- * to @c NULL.
+ * If @a hex is @c NULL then set @a *checksum * to @c NULL.
+ * If @a hex is the all-zeros checksum, then set @a *checksum to
+ * a wildcard checksum instead of a checksum of the requested kind.
*
* @since New in 1.6.
*/
svn_checksum_parse_hex() would need a bump of course, but the diff
above shows just the docstring diff on purpose.
Either approach would make the API more clear I think.
Stefan
Received on 2009-05-19 14:01:06 CEST