[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: checksum / compare_files question

From: Stefan Sperling <stsp_at_elego.de>
Date: Tue, 19 May 2009 12:59:53 +0100

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

This is an archived mail posted to the Subversion Dev mailing list.