Re: [PATCH v5 1/2] name-hash.c: replace cache_name_compare() with memcmp()

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 20, 2014, 17:10 UTC
Message-ID: <xmqqvbro7k54.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1403230004-11034-2-git-send-email-jmmahler@gmail.com>

Jeremiah Mahler <jmmahler@gmail.com> writes:

> When cache_name_compare() is used on counted strings of the same
> length, it is equivalent to a memcmp().  Since the one use of
> cache_name_compare() in name-hash.c requires that the lengths are
> equal, just replace it with memcmp().

I do not think it is not "requires that the lengths are equal"; it merely is a premature optimization, as it wants to catch only the case where the names are the same.

Your patch is not wrong per-se, but with the above justification of yours, I would actually have expected to see it updated to use !cache_name_compare() and then later !name_compare(). That way, if it ever turns out that giving name_compare() semantics specific to "name" (as opposed to just byte-for-byte comparison given by memcmp(3)) is a good idea, we will use that comparison with semantics specific to "name"s here, without having to change it from memcmp().

Having said all that, I think we see a more correct justification for this change in the pre-context of the patch. We want the exact comparison, without any funky "name"-specific semantics in the quick-and-exact case.

I've queued it like this (no need to reroll). Thanks.

    name-hash.c: replace cache_name_compare() with memcmp(3)
    
    The same_name() private function wants a quick-and-exact check to
    see if they two names are byte-for-byte identical first and then
    fall back to the slow path.  Use memcmp(3) for the former to make it
    clear that we do not want any "name" specific comparison.
    
    Signed-off-by: Jeremiah Mahler <jmmahler@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/name-hash.c b/name-hash.c
index 97444d0..49fd508 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -179,7 +179,7 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen
 	 * Always do exact compare, even if we want a case-ignoring comparison;
 	 * we do the quick exact one first, because it will be the common case.
 	 */
-	if (len == namelen && !cache_name_compare(name, namelen, ce->name, len))
+	if (len == namelen && !memcmp(name, ce->name, len))
 		return 1;
 
 	if (!icase)