On Fri, Jan 3, 2025 at 12:16 AM Patrick Steinhardt <ps@xxxxxx> wrote:
>
> On Wed, Jan 01, 2025 at 02:53:09AM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > 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.
> 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.
> > 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.
