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

[PATCHv2 3/3] shorten_unambiguous_ref(): Fix shortening refs/remotes/origin/HEAD to origin

From
Johan Herland <johan@herland.net>
Date
May 7, 2013, 18:54 UTC
Message-ID
<1367952856-30729-3-git-send-email-johan@herland.net>
In-Reply-To
<1367952856-30729-1-git-send-email-johan@herland.net>

When expanding shorthand refs to full ref names (e.g. in dwim_ref()), we use the ref_rev_parse_rules list of expansion patterns. This list allows "origin" to be expanded into "refs/remotes/origin/HEAD", by using the "refs/remotes/%.*s/HEAD" pattern from that list.

shorten_unambiguous_ref() exists to provide the reverse operation: turning a full ref name into a shorter (but still unambiguous) name. It does so by matching the given refname against each pattern from the ref_rev_parse_rules list (in reverse), and extracting the short- hand name from the matching rule.

However, when given "refs/remotes/origin/HEAD" it fails to shorten it into "origin", because we misuse the sscanf() function when matching "refs/remotes/origin/HEAD" against "refs/remotes/%.*s/HEAD": We end up calling sscanf like this:

  sscanf("refs/remotes/origin/HEAD", "refs/remotes/%s/HEAD", short_name)

In this case, sscanf() will match the initial "refs/remotes/" part, and then match the remainder of the refname against the "%s", and place it ("origin/HEAD") into short_name. The part of the pattern following the "%s" format is never verified, because sscanf() apparently does not need to do that (it has performed the one expected format extraction, and will return 1 correspondingly; see [1] for more details).

This patch replaces the misuse of sscanf() with a fairly simple function that manually matches the refname against patterns, and extracts the shorthand name.

[1]: If we assume that sscanf() does not do a verification pass prior to format extraction, there is AFAICS _no_ way for sscanf() - having already done one or more format extractions - to indicate to its caller that the input fails to match the trailing part of the format string. In other words, AFAICS, the scanf() family of function will only verify matching input up to and including the last format specifier in the format string. Any data following the last format specifier will not be verified. Yet another reason to consider the scanf functions harmful...

Cc: Bert Wesarg <bert.wesarg@googlemail.com>
Improved-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johan Herland <johan@herland.net>
---
 refs.c                                       | 77 ++++++++++------------------
 t/t1514-rev-parse-shorten_unambiguous_ref.sh |  4 +-
 t/t6300-for-each-ref.sh                      |  2 +-
 3 files changed, 29 insertions(+), 54 deletions(-)
diff --git a/refs.c b/refs.c
index d17931a..a0ba2fd 100644
--- a/refs.c
+++ b/refs.c
@@ -2945,80 +2945,55 @@ struct ref *find_ref_by_name(const struct ref *list, const char *name)
 	return NULL;
 }
 
-/*
- * generate a format suitable for scanf from a ref_rev_parse_rules
- * rule, that is replace the "%.*s" spec with a "%s" spec
- */
-static void gen_scanf_fmt(char *scanf_fmt, const char *rule)
+static int shorten_ref(const char *refname, const char *pattern, char *short_name)
 {
-	char *spec;
-
-	spec = strstr(rule, "%.*s");
-	if (!spec || strstr(spec + 4, "%.*s"))
-		die("invalid rule in ref_rev_parse_rules: %s", rule);
-
-	/* copy all until spec */
-	strncpy(scanf_fmt, rule, spec - rule);
-	scanf_fmt[spec - rule] = '\0';
-	/* copy new spec */
-	strcat(scanf_fmt, "%s");
-	/* copy remaining rule */
-	strcat(scanf_fmt, spec + 4);
-
-	return;
+	/*
+	 * pattern must be of the form "[pre]%.*s[post]". If refname
+	 * starts with "[pre]" and ends with "[post]", extract the middle
+	 * part into short_name, and return the number of chars in the
+	 * middle part (not counting the added NUL-terminator). Otherwise,
+	 * if refname does not match pattern, return 0.
+	 */
+	int match_len;
+	const char *match_start, *sep = strstr(pattern, "%.*s");
+	if (!sep || strstr(sep + 4, "%.*s"))
+		die("invalid pattern in ref_rev_parse_rules: %s", pattern);
+	match_start = refname + (sep - pattern);
+	match_len = strlen(refname) - (strlen(pattern) - 4);
+	if (match_len <= 0 ||
+	    strncmp(refname, pattern, match_start - refname) ||
+	    strcmp(match_start + match_len, sep + 4))
+		return 0; /* refname does not match */
+	memcpy(short_name, match_start, match_len);
+	short_name[match_len] = '\0';
+	return match_len;
 }
 
 char *shorten_unambiguous_ref(const char *refname, int strict)
 {
 	int i;
-	static char **scanf_fmts;
-	static int nr_rules;
 	char *short_name;
 
-	/* pre generate scanf formats from ref_rev_parse_rules[] */
-	if (!nr_rules) {
-		size_t total_len = 0;
-
-		/* the rule list is NULL terminated, count them first */
-		for (; ref_rev_parse_rules[nr_rules]; nr_rules++)
-			/* no +1 because strlen("%s") < strlen("%.*s") */
-			total_len += strlen(ref_rev_parse_rules[nr_rules]);
-
-		scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);
-
-		total_len = 0;
-		for (i = 0; i < nr_rules; i++) {
-			scanf_fmts[i] = (char *)&scanf_fmts[nr_rules]
-					+ total_len;
-			gen_scanf_fmt(scanf_fmts[i], ref_rev_parse_rules[i]);
-			total_len += strlen(ref_rev_parse_rules[i]);
-		}
-	}
-
-	/* bail out if there are no rules */
-	if (!nr_rules)
-		return xstrdup(refname);
-
 	/* buffer for scanf result, at most refname must fit */
 	short_name = xstrdup(refname);
 
 	/* skip first rule, it will always match */
-	for (i = nr_rules - 1; i > 0 ; --i) {
+	for (i = ARRAY_SIZE(ref_rev_parse_rules) - 2; i > 0 ; --i) {
 		int j;
 		int rules_to_fail = i;
 		int short_name_len;
 
-		if (1 != sscanf(refname, scanf_fmts[i], short_name))
+		if (!(short_name_len = shorten_ref(refname,
+						   ref_rev_parse_rules[i],
+						   short_name)))
 			continue;
 
-		short_name_len = strlen(short_name);
-
 		/*
 		 * in strict mode, all (except the matched one) rules
 		 * must fail to resolve to a valid non-ambiguous ref
 		 */
 		if (strict)
-			rules_to_fail = nr_rules;
+			rules_to_fail = ARRAY_SIZE(ref_rev_parse_rules) - 1;
 
 		/*
 		 * check if the short name resolves to a valid ref,
diff --git a/t/t1514-rev-parse-shorten_unambiguous_ref.sh b/t/t1514-rev-parse-shorten_unambiguous_ref.sh
index ad75436..fd87ce3 100755
--- a/t/t1514-rev-parse-shorten_unambiguous_ref.sh
+++ b/t/t1514-rev-parse-shorten_unambiguous_ref.sh
@@ -58,7 +58,7 @@ test_shortname () {
 	test_cmp expect.sha1 actual.sha1
 }
 
-test_expect_failure 'shortening refnames in strict mode' '
+test_expect_success 'shortening refnames in strict mode' '
 	test_shortname refs/heads/master strict heads/master master_f &&
 	test_shortname refs/heads/origin/master strict heads/origin/master master_b &&
 	test_shortname refs/master strict refs/master master_d &&
@@ -67,7 +67,7 @@ test_expect_failure 'shortening refnames in strict mode' '
 	test_shortname refs/tags/master strict tags/master master_c
 '
 
-test_expect_failure 'shortening refnames in loose mode' '
+test_expect_success 'shortening refnames in loose mode' '
 	test_shortname refs/heads/master loose heads/master master_f &&
 	test_shortname refs/heads/origin/master loose origin/master master_b &&
 	test_shortname refs/master loose master master_d &&
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 5d716c8..57e3109 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -472,7 +472,7 @@ origin
 origin/master
 EOF
 
-test_expect_failure 'Check refs/remotes/origin/HEAD shortens to origin' '
+test_expect_success 'Check refs/remotes/origin/HEAD shortens to origin' '
 	git remote set-head origin master &&
 	git for-each-ref --format="%(refname:short)" refs/remotes >actual &&
 	test_cmp expected actual

-- 
1.8.1.3.704.g33f7d4f
Previous: Johan HerlandNext: Junio C Hamano
Message 8 of 39 in “Make "$remote/$branch" work with unconventional refspecs”
  1. 0/7 Make "$remote/$branch" work with unconventional refspecsJohan Herland, May 4, 2013
  2. 1/7 shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to originJohan Herland, May 4, 2013
  3. Bert WesargMay 5, 2013
  4. Junio C HamanoMay 6, 2013
  5. Johan HerlandMay 7, 2013
  6. 1/3 t1514: Add tests of shortening refnames in strict/loose modeJohan Herland, May 7, 2013
  7. 2/3 t1514: Demonstrate failure to correctly shorten "refs/remotes/origin/HEAD"Johan Herland, May 7, 2013
  8. 3/3 shorten_unambiguous_ref(): Fix shortening refs/remotes/origin/HEAD to originJohan Herland, May 7, 2013
  9. Junio C HamanoMay 7, 2013
  10. Junio C HamanoMay 7, 2013
  11. Johan HerlandMay 7, 2013
  12. Junio C HamanoMay 7, 2013
  13. Johan HerlandMay 7, 2013
  14. 2/7 t7900: Start testing usability of namespaced remote refsJohan Herland, May 4, 2013
  15. Junio C HamanoMay 7, 2013
  16. Johan HerlandMay 7, 2013
  17. Junio C HamanoMay 7, 2013
  18. 3/7 t7900: Demonstrate failure to expand "$remote/$branch" according to refspecsJohan Herland, May 4, 2013
  19. Junio C HamanoMay 7, 2013
  20. 4/7 refs.c: Refactor rules for expanding shorthand names into full refnamesJohan Herland, May 4, 2013
  21. Junio C HamanoMay 7, 2013
  22. 5/7 refs.c: Refactor code for shortening full refnames into shorthand namesJohan Herland, May 4, 2013
  23. Junio C HamanoMay 7, 2013
  24. 6/7 refname_match(): Caller must declare if we're matching local or remote refsJohan Herland, May 4, 2013
  25. Junio C HamanoMay 7, 2013
  26. 7/7 refs.c: Add rules for resolving refs using remote refspecsJohan Herland, May 4, 2013
  27. Junio C HamanoMay 5, 2013
  28. Johan HerlandMay 5, 2013
  29. Junio C HamanoMay 5, 2013
  30. Johan HerlandMay 5, 2013
  31. Junio C HamanoMay 5, 2013
  32. Santi BéjarMay 6, 2013
  33. Santi BéjarMay 6, 2013
  34. Junio C HamanoMay 6, 2013
  35. Santi BéjarMay 6, 2013
  36. Junio C HamanoMay 6, 2013
  37. Junio C HamanoMay 6, 2013
  38. Johan HerlandMay 6, 2013
  39. Junio C HamanoMay 7, 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.