Re: [PATCH] introduce inline is_same_sha1

From: Junio C Hamano <junkio@cox.net>
Date: Aug 17, 2006, 01:12 UTC
Message-ID: <7vveos17ym.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.63.0608161721020.11465@chino.corp.google.com>

David Rientjes <rientjes@google.com> writes:

Show 15 quoted lines

> Introduces global inline:
> 	is_same_sha1(const unsigned char *sha1,
> 		     const unsigned char *sha2);
>
> Uses memcmp for comparison and returns the result.  All tests
> for comparisons for sha1 have been changed to use the new
> global inline, but existing uses of memcmp have been preserved
> where the return value other than 0 is needed, i.e.:
>
> 	int cmp = !memcmp(old_sha1, new_sha1, 20);
>
> Conversions from char* to unsigned char* casts have been made
> where appropriate.
>
> Signed-off-by: David Rientjes <rientjes@google.com>

I would have expected the inline function to be:

	int cmp_object_name(const void *, const void *)

No need for "unsigned char *" that way [*1*].

I do not know what your ultimate goal with this patch is, but I like the fact that we do not have to hardcode "20" everywhere. With a yet-to-be-written companion patch to make the "20" into a symbolic constant OBJECT_NAME_LENGTH, we could someday have a flag day to use a hash different from SHA-1 with an updated 'git-convert-objects' ;-).

I would have liked if the function were to give the comparison results similar to standard comparison functions such as memcmp and strcmp. I do not know off-hand if we order by the object names, and we might only be interested in equality tests, but still...

Line-wrap the message for comfortable viewing on 80-column terminals, please.

[Footnote]

*1* On the other hand if stricter type checking is wanted,
    "const unsigned char *" is too weak.  We would need either
    sparse annotation or a new type (maybe a struct that has a
    single member that has 20-byte uchar array).  I personally
    think that is an overkill, though.