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. 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 '@'?
> 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?
Thanks!
Patrick
