On Tue, May 19, 2009 at 6:12 PM, Neels Janosch Hofmeyr <neels_at_elego.de> wrote:
[snip]
> I'd agree, but have one problem remaining: is "the all-zeros wildcard"
> applicable to both SHA1 and MD5 checksums? Futhermore, is it then desired
> that an MD5 wildcard matches a SHA1 checksum (and vice versa)?
>
> The current code returns true for a wildcard checksum, even if the kinds
> were originally different. Say if I give it an MD5 all-zeros and check it
> against any SHA1 checksum, I still get a match.
I personally don't think this is a huge issue. Is there potential we
could confuse something, and accidentally compare a wildcard intended
for MD5 with SHA1 (assuming your using a wildcard "kind")? Sure.
Would it go unnoticed for years to come? No. The wildcard matching
occurs extremely infrequently. At the moment, it would only occur if
you edited the backend repository and insert the wildcard checksum in
place of what was there. Chances are you'd end up with some other
sort of failure (probably from one of the tests) showing the error of
your ways because you were trying to compare an MD5 with a SHA1, for
real.
> To stay out of that kind of trouble, I'd suggest something like this:
>
> [[[
> const svn_checksum_t svn_checksum_wildcard_md5 =
> { "00000000000000000000000000000000", svn_checksum_md5 };
> #define SVN_CHECKSUM_WILDCARD_MD5 (&svn_checksum_wildcard_md5)
>
> const svn_checksum_t svn_checksum_wildcard_sha1 =
> { "00000000 00000000 00000000 00000000 00000000", svn_checksum_sha1 };
>
> #define SVN_CHECKSUM_WILDCARD_SHA1 (&svn_checksum_wildcard_sha1)
> ]]]
>
> Wherever the current code replaces an all-zero checksum with a NULL
> svn_checksum_t*, do this instead:
> [[[
> if (checksum.kind == svn_checksum_md5 &&
> strcmp(checksum->digest, SVN_CHECKSUM_WILDCARD_MD5->digest) == 0)
> checksum = SVN_CHECKSUM_WILDCARD_MD5; // replace with constant
> ]]]
> (And same for sha1)
I'm not sure you can do that, since you have to return a
'svn_checksum_t *' (a non-const pointer).
You could create a copy though.
At any rate, I'm not trying to stand guard for the original code. I
was simply trying to answer your original question and show that NULL
does have a meaning. :-) If you think this is a better way, go for
it! :-) I certainly won't stand in your way. Anything that makes the
current code more understandable is a Good Thing. How far you want to
go with it is completely up to you. :-)
-John
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2326883
Received on 2009-05-20 10:15:53 CEST