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 - To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html