Re: [PATCH 3/3] shorten_unambiguous_ref(): avoid sscanf()

From

Jeff King <peff@peff.net>

Date

Feb 14, 2023, 22:30 UTC

Message-ID

<Y+wLoFKXhlugxrh1@coredump.intra.peff.net>

In-Reply-To

<xmqqmt5f535i.fsf@gitster.g>

On Tue, Feb 14, 2023 at 01:48:57PM -0800, Junio C Hamano wrote:

> > +	do {
> > +		if (!*rule)
> > +			BUG("rev-parse rule did not have percent");
> > +		if (*rule == '%')
> > +			break;
> > +	} while (*refname++ == *rule++);
> 
> So, if we have refname="refs/heads/frotz" and rule="refs/%.*s", then
> we'll scan refname and rule to skip over their "refs/" prefix, and
> the next iteration, where post-increment moved the pointers to point
> at 'h' (at the beginning of "heads/frotz") on the refname side and
> '%' on the rule side, we iterate once more, notice *rule is '%', and
> break out of the loop.  We have refname="heads/frotz" and rule="%.*s"
> 
> If we have refname="refsXheads/frotz" and rule="refs/%.*s", after
> skipping over "refs", refname points at 'X' while rule points at '/'
> and the loop needs to break.  Both pointers are post-incremented,
> and now we have refname="heads/frotz" and rule="%.*s".

This would fix it:

diff --git a/refs.c b/refs.c
index d8ce7e9ee1..2c26cf02d3 100644
--- a/refs.c
+++ b/refs.c
@@ -1318,6 +1318,8 @@ int update_ref(const char *msg, const char *refname,
 static const char *match_parse_rule(const char *refname, const char *rule,
 				    size_t *len)
 {
+	int matched = 0;
+
 	/*
 	 * Check that rule matches refname up to the first percent
 	 * in the rule. This is basically skip_prefix(), but
@@ -1326,10 +1328,15 @@ static const char *match_parse_rule(const char *refname, const char *rule,
 	do {
 		if (!*rule)
 			BUG("rev-parse rule did not have percent");
-		if (*rule == '%')
+		if (*rule == '%') {
+			matched = 1;
 			break;
+		}
 	} while (*refname++ == *rule++);
 
+	if (!matched)
+		return 0;
+
 	/*
 	 * Check that we matched all the way to the "%" placeholder,
 	 * and skip past it within the rule string. If so, "refname" at

but I have a feeling that it gets more readable if we flip the break
conditional and the loop condition.

I had also imagined this as a skip_prefix_to_percent() helper, which
makes the logic nicer, but we actually need to advance in both the
refname and the prefix, which makes for a weird interface.

-Peff