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 alt] peel_onion: do not assume length of x_type globals
Date: Tue, 3 Sep 2013 16:27:30 -0400 [thread overview]
Message-ID: <20130903202729.GA26217@sigill.intra.peff.net> (raw)
In-Reply-To: <20130903202041.GA7463@sigill.intra.peff.net>
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 could avoid these magic numbers by using skip_prefix. But
taking a step back, we can realize that using the
"commit_type" global is not really buying us anything. It is
not ever going to change from being "commit" without causing
severe breakage to existing uses. And even if it did change
for some crazy reason, we would want to evaluate its effects
on the "rev^{}" syntax, anyway.
Let's just switch these to using a custom string literal, as
we do for "rev^{object}". The resulting code is more robust
to changes in the type strings, and is more readable.
Signed-off-by: Jeff King <peff@peff.net>
---
If you _really_ wanted to abstract it, you could make commit_type a
macro and use string concatenation along with prefixcmp. But that is
going in the direction of less readable, I think. :)
This has probably consumed enough brain cycles for such a small and
probably unimportant cleanup. I'll let Junio pick from the 2 options (or
choose to do nothing at all) as he sees fit.
sha1_name.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 6dc496d..2f6e5ab 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -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 (!prefixcmp(sp, "commit}"))
expected_type = OBJ_COMMIT;
- else if (!strncmp(tag_type, sp, 3) && sp[3] == '}')
+ else if (!prefixcmp(sp, "tag}"))
expected_type = OBJ_TAG;
- else if (!strncmp(tree_type, sp, 4) && sp[4] == '}')
+ else if (!prefixcmp(sp, "tree}"))
expected_type = OBJ_TREE;
- else if (!strncmp(blob_type, sp, 4) && sp[4] == '}')
+ else if (!prefixcmp(sp, "blob}"))
expected_type = OBJ_BLOB;
else if (!prefixcmp(sp, "object}"))
expected_type = OBJ_ANY;
--
1.8.4.2.g87d4a77
next prev parent reply other threads:[~2013-09-03 20:27 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 ` [PATCH 2/1] peel_onion: do not assume length of x_type globals Jeff King
2013-09-03 20:27 ` Jeff King [this message]
2013-09-03 20:46 ` [PATCH 2/1 alt] " 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=20130903202729.GA26217@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).