From: Junio C Hamano <gitster@pobox•com>
To: Jeremiah Mahler <jmmahler@gmail•com>
Cc: Jonathan Nieder <jrnieder@gmail•com>, git@vger•kernel.org
Subject: Re: [PATCH v5 1/2] name-hash.c: replace cache_name_compare() with memcmp()
Date: Fri, 20 Jun 2014 10:10:56 -0700 [thread overview]
Message-ID: <xmqqvbro7k54.fsf@gitster.dls.corp.google.com> (raw)
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)
next prev parent reply other threads:[~2014-06-25 21:29 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-20 2:06 [PATCH v5 0/2] cleanup duplicate name_compare() functions Jeremiah Mahler
2014-06-20 2:06 ` [PATCH v5 1/2] name-hash.c: replace cache_name_compare() with memcmp() Jeremiah Mahler
2014-06-20 17:10 ` Junio C Hamano [this message]
2014-06-20 2:06 ` [PATCH v5 2/2] cleanup duplicate name_compare() functions Jeremiah Mahler
2014-06-20 17:15 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqvbro7k54.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jmmahler@gmail$(echo .)com \
--cc=jrnieder@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox