object-name: fix a pair of object name resolution issues

[newren]

Changes since v2:

• Readability improvement to the first patch, which fixes object name resolution with refs containing a curly brace
• Fixed the second patch for cases like OBJ-COUNT-gHASH~13 and added a couple test cases for that. Also, extended the commit message a bit to discuss the cases brought up on the list.

For the second patch, if folks want some open source examples where it could be triggered, I found two examples:

• lore.git: git cat-file -t master:random/path/major-gaffed
• git.git: git cat-file -t super-invalid~///\\[email protected]

cc: Patrick Steinhardt [email protected]
cc: Elijah Newren [email protected]

[newren]

/submit

[gitgitgadget]

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1844/newren/object-name-fix-v1

To fetch this version to local tag pr-1844/newren/object-name-fix-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1844/newren/object-name-fix-v1

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Elijah Newren via GitGitGadget" <[email protected]> writes:

> From: Elijah Newren <[email protected]>
>
> 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.

Interesting.  I wonder if this looseness was to ensure that we won't
mistake a colon inside "master^{/title with : a colon}" as a start
of a subpath, instead of asking for a commit with a title that
happens to have a colon in it?

> Add tests for both this and 'foo@@{...}' cases, which an initial version
> of this patch broke.

Thanks for being extra careful here.


> Reported-by: Gabriel Amaral <[email protected]>
> Helped-by: Michael Haggerty <[email protected]>
> Signed-off-by: Elijah Newren <[email protected]>
> ---
>     object-name: fix resolution of object names containing curly braces
>     
>     Maintainer note: this bug dates back to 2006; it is not a regression in
>     this cycle.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1844%2Fnewren%2Fobject-name-fix-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1844/newren/object-name-fix-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1844
>
>  object-name.c       |  8 +++++---
>  t/t1006-cat-file.sh | 17 +++++++++++++++++
>  2 files changed, 22 insertions(+), 3 deletions(-)
>
> 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;
> 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
> +'
> +
>  test_expect_success 'setup blobs which are likely to delta' '
>  	test-tool genrandom foo 10240 >foo &&
>  	{ cat foo && echo plus; } >foo-plus &&
>
> base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f

[gitgitgadget]

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Wed, Jan 01, 2025 at 02:53:09AM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[email protected]>
> 
> 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

[gitgitgadget]

User Patrick Steinhardt <[email protected]> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Patrick Steinhardt <[email protected]> writes:

> I wonder though: does this have any impact on '<rev>^{<type>}' and other
> syntaxes where we use '^' instead of '@'?
> ...
> 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 for a review.  I am too curious how this change interacts
with syntax with {braces} that do not use "@".

[gitgitgadget]

There are issues in commit f6e6e7a:
Can I get rid of the FUNNYNAME requirement?
Commit checks stopped - the message is too short
Commit not signed off

[gitgitgadget]

There are issues in commit 4759f9c:
Can I get rid of the FUNNYNAME requirement?
Commit checks stopped - the message is too short
Commit not signed off

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Wed, Jan 1, 2025 at 9:01 AM Junio C Hamano <[email protected]> wrote:
>
> "Elijah Newren via GitGitGadget" <[email protected]> writes:
>
> > From: Elijah Newren <[email protected]>
> >
> > 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.
>
> Interesting.  I wonder if this looseness was to ensure that we won't
> mistake a colon inside "master^{/title with : a colon}" as a start
> of a subpath, instead of asking for a commit with a title that
> happens to have a colon in it?

Yeah, good catch, my changes would for example break parsing
  master^{/object-name:}:t/t1006-cat-file.sh

I'll fix that and add a testcase.

[gitgitgadget]

User Elijah Newren <[email protected]> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Fri, Jan 3, 2025 at 12:16 AM Patrick Steinhardt <[email protected]> wrote:
>
> On Wed, Jan 01, 2025 at 02:53:09AM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <[email protected]>
> >
> > 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.

[newren]

/submit

[gitgitgadget]

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1844/newren/object-name-fix-v2

To fetch this version to local tag pr-1844/newren/object-name-fix-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1844/newren/object-name-fix-v2

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Elijah Newren <[email protected]> writes:

>> Interesting.  I wonder if this looseness was to ensure that we won't
>> mistake a colon inside "master^{/title with : a colon}" as a start
>> of a subpath, instead of asking for a commit with a title that
>> happens to have a colon in it?
>
> Yeah, good catch, my changes would for example break parsing
>   master^{/object-name:}:t/t1006-cat-file.sh
>
> I'll fix that and add a testcase.

I am not sure what the updated approach would be, but I kind of
prefer if the parser does not have to be intimately familiar with
the fact that we know about '@' and '^' as possible characters that
can appear before the opening '{'.  That same attitude of "We know
that before a '{'" '@' is valid thing to appear, so let's special
case '@'" was what got us into this exchange in the first place, and
I am not confident that we now are exhaustive, knowing about '@' and
'^'.

Thanks.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Elijah Newren via GitGitGadget" <[email protected]> writes:

>  * Added a second patch for another bug discovered by the same reporter,
>    where branch:path/to/file/named/major-gaffed is interpreted as a request
>    for a commit (namely affed) rather than a blob. (At least, assuming
>    commit affed exists)
>
> The second patch has some backward compatibility concerns. People used to be
> able to do e.g. git show ${garbage}-g${hash}. I tightened it to
> ${valid_refname}-${number}-g${hash}, but do we want to allow e.g.
> ${valid_refname}-g${hash} (allowing the count to be omitted) or maybe even
> allow a subset of invalid refnames?

My take on it is that it is an absolute no-no if we require that
${valid_refname} exists locally, and it is still iffy if we checked
${valid_refname} with check_format() (because the definition of
validity can change over time, and we would not know the rules that
were valid back when the reference to the commit was written).

Otherwise a tightened rule would make "${garbage}-g${hash}" less
useful to copy-and-paste from a text file to command line.

In general what would we do if a string can be interpreted in
multiple ways in _different_ parts of the object-name codepaths.  We
all know that "affed" would trigger the "ambiguous object name"
error if there are more than one object whose object name begins
with "affed", but if "${garbage}-gaffed" can be interpreted as the
name of an object whose object name begins with "affed" and also can
be interpreted as the name of another object that sits at a path
that ends with "-gaffed" in some tree object, regardless of how the
leading part "${garbage}" looks like, it would be desirable if we
declared such a string as "ambiguous" the same way.

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Sat, Jan 4, 2025 at 6:35 AM Junio C Hamano <[email protected]> wrote:
>
> "Elijah Newren via GitGitGadget" <[email protected]> writes:
>
> >  * Added a second patch for another bug discovered by the same reporter,
> >    where branch:path/to/file/named/major-gaffed is interpreted as a request
> >    for a commit (namely affed) rather than a blob. (At least, assuming
> >    commit affed exists)
> >
> > The second patch has some backward compatibility concerns. People used to be
> > able to do e.g. git show ${garbage}-g${hash}. I tightened it to
> > ${valid_refname}-${number}-g${hash}, but do we want to allow e.g.
> > ${valid_refname}-g${hash} (allowing the count to be omitted) or maybe even
> > allow a subset of invalid refnames?
>
> My take on it is that it is an absolute no-no if we require that
> ${valid_refname} exists locally, and it is still iffy if we checked
> ${valid_refname} with check_format() (because the definition of
> validity can change over time, and we would not know the rules that
> were valid back when the reference to the commit was written).

Fair enough.  However...

> Otherwise a tightened rule would make "${garbage}-g${hash}" less
> useful to copy-and-paste from a text file to command line.
>
> In general what would we do if a string can be interpreted in
> multiple ways in _different_ parts of the object-name codepaths.  We
> all know that "affed" would trigger the "ambiguous object name"
> error if there are more than one object whose object name begins
> with "affed", but if "${garbage}-gaffed" can be interpreted as the
> name of an object whose object name begins with "affed" and also can
> be interpreted as the name of another object that sits at a path
> that ends with "-gaffed" in some tree object, regardless of how the
> leading part "${garbage}" looks like, it would be desirable if we
> declared such a string as "ambiguous" the same way.

How would that be desirable?  There's no possible way to disambiguate.
While abbreviated revisions can just be modified to be less
abbreviated, paths cannot be spelled any other way.  How would you
spell
      master:path/to/who-gabbed
in a "less ambiguous" way to differentiate it from commit abbed?  As
far as I can tell, this proposal just leaves the user stuck with an
error with no way to get the path they want.

If you don't like check_format() being called on the leading part of
the string, can we at least enforce that there is no ':', so that we
can successfully request explicit paths of given revisions and know
that we'll get them?  (That'd disallow e.g. next^{/doc:}-12-gabbed,
but that clearly was never a valid describe output anyway.)

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Elijah Newren via GitGitGadget" <[email protected]> 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.  If it were
something like ...

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

... 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-<.

Thanks.

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Sat, Jan 4, 2025 at 9:26 AM Junio C Hamano <[email protected]> wrote:
>
> "Elijah Newren via GitGitGadget" <[email protected]> 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.

> ... 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.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Elijah Newren <[email protected]> writes:

>> In general what would we do if a string can be interpreted in
>> multiple ways in _different_ parts of the object-name codepaths.  We
>> all know that "affed" would trigger the "ambiguous object name"
>> error if there are more than one object whose object name begins
>> with "affed", but if "${garbage}-gaffed" can be interpreted as the
>> name of an object whose object name begins with "affed" and also can
>> be interpreted as the name of another object that sits at a path
>> that ends with "-gaffed" in some tree object, regardless of how the
>> leading part "${garbage}" looks like, it would be desirable if we
>> declared such a string as "ambiguous" the same way.
>
> How would that be desirable?

In "a:b/c-0-gabcde", *if* "a:b/c-0" *were* a valid way to spell a
valid refname, then the whole thing is an ambiguous object name,
i.e. it could be "something reachable from object 'a:b/c' whose
object name begins with abcde", or it could be "object at the path
b/c-0-gabcde in a tree-ish a", and in such a case our code should be
set up to allow us to give a "that's ambiguous" error, instead of
yielding the first possible interpretation (i.e. if we happen to
have checked the describe name first and "$garbage-0-gabcde", we
yield "abcde" before even checking if $garbage part gives a possible
leading part of a tree-ish; but if a future refactoring of the code
flips the order of checking, we may end up yielding 'an object at a
path, which ends with -0-gabcde, sitting in a tree-ish', without
checking if that could be a valid describe name).

Of course we should make sure that the syntax cannot be ambiguous
when we introduce a new syntax to represent a new feature ;-)

Now, I think ":" has always been a byte that is invalid as a part of
any refname, so "${garbage}-gabcde" with a colon in ${garbage}
cannot be a describe name.  So in the above about "a:b/c-0" is an
impossible example, but I was wondering more about the general
principle we should follow.

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Sat, Jan 4, 2025 at 9:51 AM Junio C Hamano <[email protected]> wrote:
>
> Elijah Newren <[email protected]> writes:
>
> >> In general what would we do if a string can be interpreted in
> >> multiple ways in _different_ parts of the object-name codepaths.  We
> >> all know that "affed" would trigger the "ambiguous object name"
> >> error if there are more than one object whose object name begins
> >> with "affed", but if "${garbage}-gaffed" can be interpreted as the
> >> name of an object whose object name begins with "affed" and also can
> >> be interpreted as the name of another object that sits at a path
> >> that ends with "-gaffed" in some tree object, regardless of how the
> >> leading part "${garbage}" looks like, it would be desirable if we
> >> declared such a string as "ambiguous" the same way.
> >
> > How would that be desirable?
>
> In "a:b/c-0-gabcde", *if* "a:b/c-0" *were* a valid way to spell a
> valid refname, then the whole thing is an ambiguous object name,
> i.e. it could be "something reachable from object 'a:b/c' whose
> object name begins with abcde", or it could be "object at the path
> b/c-0-gabcde in a tree-ish a", and in such a case our code should be
> set up to allow us to give a "that's ambiguous" error, instead of
> yielding the first possible interpretation (i.e. if we happen to
> have checked the describe name first and "$garbage-0-gabcde", we
> yield "abcde" before even checking if $garbage part gives a possible
> leading part of a tree-ish; but if a future refactoring of the code
> flips the order of checking, we may end up yielding 'an object at a
> path, which ends with -0-gabcde, sitting in a tree-ish', without
> checking if that could be a valid describe name).
>
> Of course we should make sure that the syntax cannot be ambiguous
> when we introduce a new syntax to represent a new feature ;-)
>
> Now, I think ":" has always been a byte that is invalid as a part of
> any refname, so "${garbage}-gabcde" with a colon in ${garbage}
> cannot be a describe name.  So in the above about "a:b/c-0" is an
> impossible example, but I was wondering more about the general
> principle we should follow.

Are you only interested in the general principle for the "possible
examples"?  What about the general principle for the "impossible
examples"?  Things like "master:path/to/who-gabbed" are unambiguously
a reference to a path within a revision that cannot be spelled any
alternate way, but the code currently gives the user a commit instead.
What's the right way to fix these "impossible examples"?  I've given
three proposals and implemented the first of them:
  - ${POSSIBLY_VALID_REFNAME}-${INTEGER}-g${HASH}
  - ${POSSIBLY_VALID_REFNAME}-g${HASH}
  - ${ANYTHING_WITHOUT_A_COLON}-g${HASH}

You said you don't like the first two because check_refname() rules
might change, and not commented on the third.

Also, as far as I can tell, the set of "possible examples" you are
focusing on is currently the empty set.  A change of syntax might in
the future expand that to a non-empty-set, and then bring us backward
compatibility headaches because we have been allowing
"${garbage}-g${hash}" to mean a reference to ${hash} and we'd then
have to deal with it becoming ambiguous (and potentially also having
no way to disambiguate those cases, similar to how if colon is allowed
in garbage then we have no way to disambiguate paths).  If we want to
allow future object naming extensions, it seems like we should lock
down and rule out as many existing forms of known ${garbage} as we
can, but that'd push us towards the
${POSSIBLY_VALID_REFNAME}-${INTEGER}-g${HASH} solution I implemented
that you don't seem to like.  Is there a middle ground that you do
like?

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Elijah Newren via GitGitGadget" <[email protected]> writes:

> 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 '@' or '^' and be paired, causing e.g. '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

Naïvely, it seems that a solution is to parse from left to right,
i.e., (1) notice there is a colon, (2) see if everything before that
colon resolves to a treeish, and (3) see if everything after it is a
path that appears in the treeish.

 - When we are given foo@{some:thing}, if we did that, we realize
   that "foo@{some" is not a valid tree-ish object name (since "@{"
   cannot appear in a refname) and then can backtrack by realizing
   "foo" is a ref, and @{...} could be a reflog reference (most
   likely a way to spell some sort of timestamp), and try that.

 - Similarly, for foo:path-gaffed, we would notice "foo" is a valid
   tree-ish object name, and if path-gaffed is a path in it, we'd be
   happy.  Or foo may not be a tree-ish, or path-gaffed may not
   exist in that tree-ish.  In which case, we can backtrack and see
   foo:path-g is an allowed prefix in a desribe name.

Now in the above description, I have assumed that an alternative
interpretation kicks in only as a fallback when we backtrack, but
we could make sure we try all possibilities and notice ambiguity if
we wanted to.

In any case, such an updated structure of the parsing code paths
(whether alternative interpretations are treated as fallbacks or
equally plausible candidates subject to disambiguation) would be
a vast departure from what we currently have, so a targeted "fix"
like these two patches attempt would be more appropriate as an
initial approach, I think.

Thanks, will queue, but probably we'd look at in any seriousness
after the 2.48 final gets tagged.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Elijah Newren via GitGitGadget" <[email protected]> writes:

> Maintainer note: these bugs both date back to 2006; neither is a regression
> in this cycle.

While I was preparing today's -rc2 release, I noticed that this
change broke some of my release scripts with

    $ git rev-parse --verify v2.48.0-rc2-161-g6c2274cdbc^0
    fatal: Needed a single revision

which is the construct that has been there almost forever.  Its
expected output is

    $ git rev-parse --verify v2.48.0-rc2-161-g6c2274cdbc^0
    6c2274cdbca14b7eb70fb182ffac80bf6950e137

The series seems to need a bit more work.

Thanks.

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Mon, Jan 6, 2025 at 9:29 AM Junio C Hamano <[email protected]> wrote:
>
> "Elijah Newren via GitGitGadget" <[email protected]> writes:
>
> > Maintainer note: these bugs both date back to 2006; neither is a regression
> > in this cycle.
>
> While I was preparing today's -rc2 release, I noticed that this
> change broke some of my release scripts with
>
>     $ git rev-parse --verify v2.48.0-rc2-161-g6c2274cdbc^0
>     fatal: Needed a single revision
>
> which is the construct that has been there almost forever.  Its
> expected output is
>
>     $ git rev-parse --verify v2.48.0-rc2-161-g6c2274cdbc^0
>     6c2274cdbca14b7eb70fb182ffac80bf6950e137
>
> The series seems to need a bit more work.

Gah, I made sure to copy the object name into a string buf, so I could
operate on just the relevant part, but then ignored that and operated
on the full thing.

This fixes it:

diff --git a/object-name.c b/object-name.c
index 614520954c7..cb96a0e6161 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1318,7 +1318,7 @@ static int ref_and_count_parts_valid(const char
*name, int len)
        len = cp - name;
        strbuf_init(&sb, len);
        strbuf_add(&sb, name, len);
-       ret = !check_refname_format(name, flags);
+       ret = !check_refname_format(sb.buf, flags);
        strbuf_release(&sb);
        return ret;
 }

I'll include it with all my other fixes in a reroll, which I'll
probably send out after 2.48 to avoid distracting from the release.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Elijah Newren <[email protected]> writes:

> On Mon, Jan 6, 2025 at 9:29 AM Junio C Hamano <[email protected]> wrote:
>>
>> "Elijah Newren via GitGitGadget" <[email protected]> writes:
>>
>> > Maintainer note: these bugs both date back to 2006; neither is a regression
>> > in this cycle.
>>
>> While I was preparing today's -rc2 release, I noticed that this
>> change broke some of my release scripts with
>>
>>     $ git rev-parse --verify v2.48.0-rc2-161-g6c2274cdbc^0
>>     fatal: Needed a single revision
>>
>> which is the construct that has been there almost forever.  Its
>> expected output is
>>
>>     $ git rev-parse --verify v2.48.0-rc2-161-g6c2274cdbc^0
>>     6c2274cdbca14b7eb70fb182ffac80bf6950e137
>>
>> The series seems to need a bit more work.
>
> Gah, I made sure to copy the object name into a string buf, so I could
> operate on just the relevant part, but then ignored that and operated
> on the full thing.
>
> This fixes it:
>
> diff --git a/object-name.c b/object-name.c
> index 614520954c7..cb96a0e6161 100644
> --- a/object-name.c
> +++ b/object-name.c
> @@ -1318,7 +1318,7 @@ static int ref_and_count_parts_valid(const char
> *name, int len)
>         len = cp - name;
>         strbuf_init(&sb, len);
>         strbuf_add(&sb, name, len);
> -       ret = !check_refname_format(name, flags);
> +       ret = !check_refname_format(sb.buf, flags);
>         strbuf_release(&sb);
>         return ret;
>  }
>
> I'll include it with all my other fixes in a reroll, which I'll
> probably send out after 2.48 to avoid distracting from the release.

In existing tests, we seem to be lacking coverage to notice this
breakage, so let's make sure we add one or two.

Thanks.