Re: [PATCH] introduce inline is_same_sha1

[David Rientjes <rientjes@google.com>]

On Wed, 16 Aug 2006, Junio C Hamano wrote:

> 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' ;-).
> 

The reason it is not
	int cmp_object_name(const void *, const void *)

ties into the ultimate goal.  As you said, 20 is hardcoded everywhere in 
the code as the length sha1's name.  Since my own development tree uses 
two different hashes configurable at runtime, I decided to create a single 
static inline that would deal with name comparisons.  I submitted a 
similar change to your tree because, like you, I envisioned that someday 
you may have several different hashes that require different comparison 
lengths.  The easy solution is using strcmp in the inline but then you're 
required to cast based on signness and sometimes you want to truncate the 
comparison to n bytes anyway depending on your hash of choice.  If you 
want to open your project to the possibility of implementing other 
hashes, my patch is a step in that direction.

> 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...
> 

Remember, this is an inline function.  The only reason for writing it 
would be to isolate the number 20 to this particular function (those that 
require comparisons were untouched, as I previously stated, so it still 
exists there as well), otherwise it's useless.  If you're going to allow a 
configurable hash function, then you'll need to isolate the n-bytes 
somewhere if you don't want to pass HASH_NAME_LENGTH around everywhere.

git does sort on sha1 name, specifically with qsort in pack-objects using 
sha1_sort (which isn't an inline, but should be).

(It would be helpful if you were to specifically request changes to a 
patch or explicitly state whether or not you queued to apply it, I can 
never tell).

		David