Re: [PATCH v2 1/2] object-name: fix resolution of object names containing curly braces

From: Elijah Newren <newren@gmail.com>
Date: Jan 4, 2025, 18:54 UTC
Message-ID: <CABPp-BHACrtkMJAKpOsPw5bw8Fvnhe_wFLMNntsPifmg8yomfw@mail.gmail.com>
In-Reply-To: <xmqq34hy4hii.fsf@gitster.g>

On Sat, Jan 4, 2025 at 9:26 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> >       for (cp = name, bracket_depth = 0; *cp; cp++) {
> > -             if (*cp == '{')
> > +             if (*(cp+1) == '{' && (*cp == '@' || *cp == '^')) {
> > +                     cp++;
> >                       bracket_depth++;
>
> Checking cp[1] before even knowing if cp[0] is the end of the string
> (hence cp[1] is an out of bounds access) smells fishy.

We checked *cp in the loop already, so we know cp[0] != '\0'. Combined with the fact that this is a NUL-terminated string, we therefore also know that cp[1] is not an out-of-bounds access.

> If it were
> something like ...
>
>         if (cp[0] && strchr("@^", cp[0]) && cp[1] == '{')

Since we know cp[0] != '\0' already, couldn't this be simplified to

    if (strchr("@^", *cp) && cp[1] == '{')

I do like this form better though, yes.

Show 5 quoted lines

> ... it may be a bit more palatable, perhaps?  At least writing it
> this way we can easily scale when we find the third character we
> need to special case, hopefully, but again, I do prefer if we can
> find a solution that does not have such an intimate knowledge about
> "@^", which I just failed to do here X-<.

Yeah, I have failed to come up with an alternative as well. If I and others can't come up with something better in a few days, I'll resubmit with the above change and a comment in the commit message that we'd prefer something better but were unable to come up with anything.