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".
Thanks for being careful. I had originally detected a match by setting a
flag in the loop when we see the "%", but then thought it wasn't needed.
And it's not for the matching case, but it is for the non-match.
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
