Re: [PATCH] introduce inline is_same_sha1

From: Junio C Hamano <junkio@cox.net>
Date: Aug 18, 2006, 04:09 UTC
Message-ID: <7vejveelcs.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.63.0608171152110.22819@chino.corp.google.com>

David Rientjes <rientjes@google.com> writes:

Show 16 quoted lines

> On Thu, 17 Aug 2006, Alex Riesen wrote:
>
>> Why not just sha1cmp? And if you're aiming at hash-type independence,
>> why not hashcmp?
>
> hashcmp is a very good suggestion.
>
> 		David
> ---
> Introduces global inline:
> 	hashcmp(const unsigned char *sha1, const unsigned char *sha2)
>
> Uses memcmp for comparison and returns the result based on the length of 
> the hash name (a future runtime decision).
>
> Signed-off-by: David Rientjes <rientjes@google.com>

Looks very very good.  Much cleaner.

Also, I retract my previous comment that the parameters should be (void*). Looking at the call sites that need casts, they would need either (unsigned char*) or (char*) cast regardless of the type of the argument hashcmp() function takes anyway.

Thanks.  Will apply.

Oh, by the way, two minor procedural requests.

Pick a good one-line description of the patch; as it was sent, the commit would have shown (in "git shortlog" output):

	Introduces global inline:

which does not make much sense to the first time readers.

Please use something other than '---' as the delimiter after the cover letter, if you write one _before_ the commit log message. E-mailed patch acceptance tools consider anything after '---' is patch and does not contribute to the commit log, so it is lost from the resulting commit.

A short cover letter like this is better placed _after_ the real separator '---' that comes after your commit log message, like this:

From: David Rientjes <rientjes@google.com>
Subject: Do not use memcmp(sha1_1, sha1_2, 20) with hardcoded length.

Introduces global inline:

	hashcmp(const unsigned char *sha1, const unsigned char *sha2)

Uses memcmp for comparison and returns the result based on the length of the hash name (a future runtime decision).

Signed-off-by: David Rientjes <rientjes@google.com>
---

 On Thu, 17 Aug 2006, Alex Riesen wrote:

 > Why not just sha1cmp? And if you're aiming at hash-type independence,
 > why not hashcmp?

 hashcmp is a very good suggestion.  Please ack the following.

 builtin-commit-tree.c    |    2 +-
 builtin-diff-stages.c    |    2 +-
 ...