Re: [PATCH 3/3] shorten_unambiguous_ref(): avoid sscanf()
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Feb 14, 2023, 23:20 UTC
- Message-ID
- <CAPig+cQPsrEJ3thaJ1cTkqFaKe34vmrtz_jNLHYTPkBoyMPm8g@mail.gmail.com>
- In-Reply-To
- <xmqqmt5f535i.fsf@gitster.g>
On Tue, Feb 14, 2023 at 4:48 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> Jeff King <peff@peff.net> writes:
> > + 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".Nice catch. I had sat staring at and worrying about the combined comparison and post-increment, trying to come up with a failing edge case but missed this one entirely.
This logic error missed by two people suggests that the patch probably ought to be accompanied by some new tests.