Re: [PATCH] object-name: fix resolution of object names containing curly braces

From: Elijah Newren <newren@gmail.com>
Date: Jan 3, 2025, 23:43 UTC
Message-ID: <CABPp-BF7QQ02OFOzSaUsSU57yB9SZFRT=KnuDx86ABRAAob4Pw@mail.gmail.com>
In-Reply-To: <Z3ec0768O64j-JeJ@pks.im>

On Fri, Jan 3, 2025 at 12:16 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 25 quoted lines

>
> On Wed, Jan 01, 2025 at 02:53:09AM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > Given a branch name of 'foo{bar', commands like
> >
> >     git cat-file -p foo{bar:README.md
> >
> > should succeed (assuming that branch had a README.md file, of course).
> > However, the change in cce91a2caef9 (Change 'master@noon' syntax to
> > 'master@{noon}'., 2006-05-19) presumed that curly braces would always
> > come after an '@' and be paired, causing 'foo{bar:README.md' to
> > entirely miss the ':' and assume there's no object being referenced.
> > In short, git would report:
> >
> >     fatal: Not a valid object name foo{bar:README.md
> >
> > Change the parsing to only make the assumption of paired curly braces
> > immediately after a '@' character appears.
> >
> > Add tests for both this and 'foo@@{...}' cases, which an initial version
> > of this patch broke.
>
> Curious. I was kind of surprised to see that it's perfectly legal to
> have branch names with curly braces in them in the first place.

I was surprised too, but apparently they are valid and we have real world repositories where people have used such bad names.

Show 36 quoted lines

> Even
> something like `foo{bar}` is legal, even though it might be confusing
> when one knows the above syntax. But sans your finding, this should be
> fine given that curly braces are only interpreted specially when
> preceded by '@', and the '@{' sequence is indeed disallowed by
> `check_refname_compoment()`.
>
> > diff --git a/object-name.c b/object-name.c
> > index c892fbe80aa..e92f26b3256 100644
> > --- a/object-name.c
> > +++ b/object-name.c
> > @@ -2087,12 +2087,14 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
> >               return -1;
> >       }
> >       for (cp = name, bracket_depth = 0; *cp; cp++) {
> > -             if (*cp == '{')
> > +             if (*cp == '@' && *(cp+1) == '{') {
> > +                     cp++;
> >                       bracket_depth++;
> > -             else if (bracket_depth && *cp == '}')
> > +             } else if (bracket_depth && *cp == '}') {
> >                       bracket_depth--;
> > -             else if (!bracket_depth && *cp == ':')
> > +             } else if (!bracket_depth && *cp == ':') {
> >                       break;
> > +             }
> >       }
> >       if (*cp == ':') {
> >               struct object_id tree_oid;
>
> Makes sense. Only the first hunk actually changes anything, the
> remaining changes are only required to make us stick to our coding
> style.
>
> I wonder though: does this have any impact on '<rev>^{<type>}' and other
> syntaxes where we use '^' instead of '@'?

<type> is pretty limited, so I see no problem there. However <rev>^{/<search text>} is problematic, as Junio pointed out. I've fixed up the patch and added a testcase to cover all the '^{...}' cases.

Show 28 quoted lines

> > diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> > index d36cd7c0863..252485dac78 100755
> > --- a/t/t1006-cat-file.sh
> > +++ b/t/t1006-cat-file.sh
> > @@ -603,6 +603,23 @@ test_expect_success FUNNYNAMES '--batch-check, -Z with newline in input' '
> >       test_cmp expect actual
> >  '
> >
> > +test_expect_success FUNNYNAMES 'setup with curly braches in input' '
> > +     git branch "foo{bar" &&
> > +     git branch "foo@"
> > +'
> > +
> > +test_expect_success FUNNYNAMES 'object reference with curly brace' '
> > +     git cat-file -p "foo{bar:hello" >actual &&
> > +     git cat-file -p HEAD:hello >expect &&
> > +     test_cmp expect actual
> > +'
> > +
> > +test_expect_success FUNNYNAMES 'object reference with at-sign' '
> > +     git cat-file -p "foo@@{0}:hello" >actual &&
> > +     git cat-file -p HEAD:hello >expect &&
> > +     test_cmp expect actual
> > +'
>
> Do these really need the FUNNYNAMES prereq? The prereq seems to only be
> about embedded quotes, tabs and newlines and is disallowed on MinGW. But
> I think both '{' and '@' should work alright there, shouldn't they?

Oh, I misread the failures.  It turns out the FUNNYNAMES prereq fixed
things in CI on windows for me because the only commit ever created in
the repository is created by a testcase with a FUNNYNAMES prereq.
Since the setup for my tests relied on HEAD existing (because I run
   git branch "foo{bar" HEAD
in a setup test of my own), the tests were failing.  I didn't look
closely enough and assumed that command was failing because Windows
didn't like a branch name with a curly brace, but the real reason it
was failing was because HEAD didn't exist.

I'll tweak one of the earlier setup tests to create a commit so HEAD exists.

Thanks for pointing this out.