Re: [PATCH 5/7] refs.c: Refactor code for shortening full refnames into shorthand names

Johan Herland [off-list ref] writes:

This patch removes the only remaining user of ref_rev_parse_rules.
It has now been fully replaced by ref_expand_rules. Hence this patch
also removes ref_rev_parse_rules.

Yeah, I was wondering when you would do this while reading [4/7], as
removing the parse_rules[] would break shortener side, and leaving
it in for long would risk allowing parse_rules[] and expand_rules[]
to drift apart.

-const struct ref_expand_rule ref_expand_rules[] = {
-	{ ref_expand_txtly, "%.*s" },
-	{ ref_expand_txtly, "refs/%.*s" },
-	{ ref_expand_txtly, "refs/tags/%.*s" },
-	{ ref_expand_txtly, "refs/heads/%.*s" },
-	{ ref_expand_txtly, "refs/remotes/%.*s" },
-	{ ref_expand_txtly, "refs/remotes/%.*s/HEAD" },
-	{ NULL, NULL }
-};

I wonder if you planned the previous step a bit better, this removal
of a large block of text could have come next to the replacement of
it we see after the addition of ref_shorten_txtly() function.

+static char *ref_shorten_txtly(const struct ref_expand_rule *rule,
+			       const char *refname)
+{
+...
+}
 
-static const char *ref_rev_parse_rules[] = {
-	"%.*s",
-	"refs/%.*s",
-	"refs/tags/%.*s",
-	"refs/heads/%.*s",
-	"refs/remotes/%.*s",
-	"refs/remotes/%.*s/HEAD",
-	NULL
+const struct ref_expand_rule ref_expand_rules[] = {
+	{ ref_expand_txtly, NULL, "%.*s" },
+	{ ref_expand_txtly, ref_shorten_txtly, "refs/%.*s" },
+	{ ref_expand_txtly, ref_shorten_txtly, "refs/tags/%.*s" },
+	{ ref_expand_txtly, ref_shorten_txtly, "refs/heads/%.*s" },
+	{ ref_expand_txtly, ref_shorten_txtly, "refs/remotes/%.*s" },
+	{ ref_expand_txtly, ref_shorten_txtly, "refs/remotes/%.*s/HEAD" },
+	{ NULL, NULL, NULL }
 };
 
 int refname_match(const char *abbrev_name, const char *full_name,

 char *shorten_unambiguous_ref(const char *refname, int strict)
 {
 	int i;
 	char *short_name;
 
-	/* buffer for scanf result, at most refname must fit */
-	short_name = xstrdup(refname);
-
-	/* skip first rule, it will always match */
-	for (i = ARRAY_SIZE(ref_rev_parse_rules) - 1; i > 0 ; --i) {
+	for (i = ARRAY_SIZE(ref_expand_rules) - 1; i >= 0 ; --i) {
...
 		/*
 		 * in strict mode, all (except the matched one) rules
 		 * must fail to resolve to a valid non-ambiguous ref
 		 */
 		if (strict)
-			rules_to_fail = ARRAY_SIZE(ref_rev_parse_rules);
+			rules_to_fail = ARRAY_SIZE(ref_expand_rules);

This part obviously depends on 1/7; do we still have an off-by-one
change from the original, or did I miscount when I reviewed 1/7?

Again, the overall strategy to refactor sounds sound.

It may be a lot simpler if you have ref_expand/shorten_append() and
ref_expand/shortn_append_with_HEAD() built-in helper functions.
Then you can perform the expansion and contraction without "%.*s" at
all.