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

From: Junio C Hamano <[email-masked]>
Date: Tue, 14 Feb 2023 13:48:57 -0800

Jeff King <[email-masked]> writes:

> +/*
> + * Check that the string refname matches a rule of the form
> + * "{prefix}%.*s{suffix}". So "foo/bar/baz" would match the rule
> + * "foo/%.*s/baz", and return the string "bar".
> + */
> +static const char *match_parse_rule(const char *refname, const char *rule,
> +				    size_t *len)
> +{
> +	/*
> +	 * Check that rule matches refname up to the first percent
> +	 * in the rule. This is basically skip_prefix(), but
> +	 * ending at percent in the prefix, rather than end-of-string.
> +	 */
> +	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".

Am I reading the loop correctly?  I wanted the bogus refname not to
match the rule, but without peeking back refname[-1], I cannot tell
the two cases apart at this point.

> +	/*
> +	 * Check that we matched all the way to the "%" placeholder,
> +	 * and skip past it within the rule string. If so, "refname" at
> +	 * this point is the beginning of a potential match.
> +	 */
> +	if (!skip_prefix(rule, "%.*s", &rule))
> +		return NULL;

And we now have rule pointing at "" (i.e. "refs/%.*s" has been fully
consumed).  refname points at "heads/frotz".

> +	/*
> +	 * And now check that our suffix (if any) matches.
> +	 */
> +	if (!strip_suffix(refname, rule, len))
> +		return NULL;
> +
> +	return refname; /* len set by strip_suffix() */
> +}

And the suffix "" is stripped and we yield "heads/frotz".