git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 2/1 alt] peel_onion: do not assume length of x_type globals

From
Jeff King <peff@peff.net>
Date
Sep 3, 2013, 20:27 UTC
Message-ID
<20130903202729.GA26217@sigill.intra.peff.net>
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
Previous: Jeff KingNext: Junio C Hamano
Message 4 of 5 in “peel_onion(): add support for <rev>^{tag}”
  1. peel_onion(): add support for <rev>^{tag}Richard Hansen, Sep 3, 2013
  2. Junio C HamanoSep 3, 2013
  3. 2/1 peel_onion: do not assume length of x_type globalsJeff King, Sep 3, 2013
  4. 2/1 peel_onion: do not assume length of x_type globalsJeff King, Sep 3, 2013
  5. Junio C HamanoSep 3, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.