Re: [PATCH] introduce inline is_same_sha1

David Rientjes [off-list ref] writes:

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

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.

Good, we are on the same wavelength.  But that does not explain
why you do not use (void *) which would let you avoid casts at
the calling sites.

quoted

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.

I may be slower than I usually am, but I fail to see the logic.
If we are isolating the comparison length and nothing else,
which I agree is a good thing, I would expect the function to
return 0 when same, just like memcmp(), which means "is_same" is
not the interface we would want.  "is_same" is justified only
when all the callers want to know equality and not comparison.

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

Exactly.  So if some other call sites do want comparison, they
would either need to do memcmp() with 20 or needs cmp_object_name()
like interface, wouldn't they?

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

Correct, pack idx is sorted by object name.

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

I am interested in this clean-up, but please convince me that
having two interfaces (is_same and compare) is better way to
abstract out 20 than one interface (compare), or if you agree
with me that one interface is better, please redo the patch and
the change the call sites you deliberately left as memcmp() in
the patch as well.

I sometimes say "I'd apply this as is, but you need to fix such
and such", when the contributor has a good track record of
responding.  Please don't get this wrong, but you are still
unknown quantity to me.  This is a learning process for both of
us.

By the way, Wednesdays and Saturdays (my time) are my git days,
so I try to summarize the current status of "master" and "next"
branches on these days.  On other days I hack on git only during
lunch and evenings so the turnaround and response tend be slower
and sketchier.