git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Richard Hansen <rhansen@bbn.com>
Cc: git@vger.kernel.org, gitster@pobox.com, sunshine@sunshineco.com
Subject: [PATCH 2/1] peel_onion: do not assume length of x_type globals
Date: Tue, 3 Sep 2013 16:20:41 -0400	[thread overview]
Message-ID: <20130903202041.GA7463@sigill.intra.peff.net> (raw)
In-Reply-To: <1378237816-28671-1-git-send-email-rhansen@bbn.com>

When we are parsing "rev^{foo}", we check "foo" against the
various global type strings, like "commit_type",
"tree_type", etc. This is nicely abstracted, but then we
destroy the abstraction completely by using magic numbers
that must match the length of the type strings.

We can avoid these magic numbers by using skip_prefix. In an
ideal world, the compiler would compute the magic number
without even calling strlen() at runtime. However, it cannot
do so in this case because we hide the definition of
commit_type and friends behind an extern declaration.

This shouldn't matter, though, as this is not a performance
critical code path. A few short strlens inside get_sha1()
will not even be measurable. And if we care, we can adjust
the declaration of commit_type and friends later on.

Signed-off-by: Jeff King <peff@peff.net>
---
This is the potential cleanup we discussed. Having written that commit
message, I wonder if it is really even worth using commit_type at all.
It is not like that variable can ever change without the git data
structure changing, and even if it did, we would almost certainly want
to revisit how that change affects the user-visible "rev^{}" syntax
anyway.

So maybe simply:

  if (!prefixcmp("commit}"))

would be fine, and it is way more readable. That's what we do already
for "rev^{object}".

 sha1_name.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 6dc496d..23d0d71 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -652,7 +652,7 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 static int peel_onion(const char *name, int len, unsigned char *sha1)
 {
 	unsigned char outer[20];
-	const char *sp;
+	const char *sp, *x;
 	unsigned int expected_type = 0;
 	unsigned lookup_flags = 0;
 	struct object *o;
@@ -677,13 +677,13 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 		return -1;
 
 	sp++; /* beginning of type name, or closing brace for empty */
-	if (!strncmp(commit_type, sp, 6) && sp[6] == '}')
+	if ((x = skip_prefix(sp, commit_type)) && *x == '}')
 		expected_type = OBJ_COMMIT;
-	else if (!strncmp(tag_type, sp, 3) && sp[3] == '}')
+	else if ((x = skip_prefix(sp, tag_type)) && *x == '}')
 		expected_type = OBJ_TAG;
-	else if (!strncmp(tree_type, sp, 4) && sp[4] == '}')
+	else if ((x = skip_prefix(sp, tree_type)) && *x == '}')
 		expected_type = OBJ_TREE;
-	else if (!strncmp(blob_type, sp, 4) && sp[4] == '}')
+	else if ((x = skip_prefix(sp, blob_type)) && *x == '}')
 		expected_type = OBJ_BLOB;
 	else if (!prefixcmp(sp, "object}"))
 		expected_type = OBJ_ANY;
-- 
1.8.4.2.g87d4a77

  parent reply	other threads:[~2013-09-03 20:20 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-09-03 19:50 [PATCH v4] peel_onion(): add support for <rev>^{tag} Richard Hansen
2013-09-03 20:10 ` Junio C Hamano
2013-09-03 20:20 ` Jeff King [this message]
2013-09-03 20:27   ` [PATCH 2/1 alt] peel_onion: do not assume length of x_type globals Jeff King
2013-09-03 20:46     ` 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=20130903202041.GA7463@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=rhansen@bbn.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).