Re: [PATCH 1/7] shorten_unambiguous_ref(): Allow shortening refs/remotes/origin/HEAD to origin

Johan Herland [off-list ref] writes:

... 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.

Yeah, we can detect when we did not have enough, but we cannot tell
where it stopped matching.

It is interesting that this bug has stayed so long with us, which
may indicate that nobody actually uses the feature at all.

Good eyes.

Does this need to be an extern?

 {
+	/*
+	 * pattern must be of the form "[pre]%.*s[post]". Check if refname
+	 * starts with "[pre]" and ends with "[post]". If so, write the
+	 * middle part into short_name, and return the number of chars
+	 * written (not counting the added NUL-terminator). Otherwise,
+	 * if refname does not match pattern, return 0.
+	 */
+	size_t pre_len, post_start, post_len, match_len;
+	size_t ref_len = strlen(refname);
+	char *sep = strstr(pattern, "%.*s");
+	if (!sep || strstr(sep + 4, "%.*s"))
+		die("invalid pattern in ref_rev_parse_rules: %s", pattern);
+	pre_len = sep - pattern;
+	post_start = pre_len + 4;
+	post_len = strlen(pattern + post_start);
+	if (pre_len + post_len >= ref_len)
+		return 0; /* refname too short */
+	match_len = ref_len - (pre_len + post_len);
+	if (strncmp(refname, pattern, pre_len) ||
+	    strncmp(refname + ref_len - post_len, pattern + post_start, post_len))
+		return 0; /* refname does not match */
+	memcpy(short_name, refname + pre_len, match_len);
+	short_name[match_len] = '\0';
+	return match_len;
 }

OK. Looks correct, even though I suspect some people might come up
with a more concise way to express the above.

 char *shorten_unambiguous_ref(const char *refname, int strict)
 {
 	int i;
 	char *short_name;
 
 	/* skip first rule, it will always match */
-	for (i = nr_rules - 1; i > 0 ; --i) {
+	for (i = ARRAY_SIZE(ref_rev_parse_rules) - 1; i > 0 ; --i) {
 		int j;
 		int rules_to_fail = i;
 		int short_name_len;
 
+		if (!ref_rev_parse_rules[i] ||

What is this skippage about?  Isn't it what you already compensated
away by starting from "ARRAY_SIZE() - 1"?

Ahh, no.  But wait.  Isn't there a larger issue here?

+		    !(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);

Isn't nr_rules in the original is "ARRAY_SIZE()-1"?

 
 		/*
 		 * check if the short name resolves to a valid ref,

Could you add a test to trigger the "strict" codepath?

Thanks.