{"thread":{"id":"62719","subject":"[PATCH] object-name: fix resolution of object names containing curly braces","startedAt":"2025-01-01T02:53:13Z","lastAt":"2025-01-13T19:26:54Z","messageCount":25,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano","Patrick Steinhardt","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"509766","messageId":"pull.1844.git.1735699989371.gitgitgadget@gmail.com","threadId":"62719","inReplyTo":null,"subject":"[PATCH] object-name: fix resolution of object names containing curly braces","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-01-01T02:53:09Z","receivedAt":"2025-01-01T02:53:13Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nGiven a branch name of 'foo{bar', commands like\n\n git cat-file -p foo{bar:README.md\n\nshould succeed (assuming that branch had a README.md file, of course).\nHowever, the change in cce91a2caef9 (Change 'master@noon' syntax to\n'master@{noon}'., 2006-05-19) presumed that curly braces would always\ncome after an '@' and be paired, causing 'foo{bar:README.md' to\nentirely miss the ':' and assume there's no object being referenced.\nIn short, git would report:\n\n fatal: Not a valid object name foo{bar:README.md\n\nChange the parsing to only make the assumption of paired curly braces\nimmediately after a '@' character appears.\n\nAdd tests for both this and 'foo@@{...}' cases, which an initial version\nof this patch broke.\n\nReported-by: Gabriel Amaral \nHelped-by: Michael Haggerty \nSigned-off-by: Elijah Newren \n---\n object-name: fix resolution of object names containing curly braces\n \n Maintainer note: this bug dates back to 2006; it is not a regression in\n this cycle.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1844%2Fnewren%2Fobject-name-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1844/newren/object-name-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1844\n\n object-name.c | 8 +++++---\n t/t1006-cat-file.sh | 17 +++++++++++++++++\n 2 files changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex c892fbe80aa..e92f26b3256 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -2087,12 +2087,14 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n \t\treturn -1;\n \t}\n \tfor (cp = name, bracket_depth = 0; *cp; cp++) {\n-\t\tif (*cp == '{')\n+\t\tif (*cp == '@' && *(cp+1) == '{') {\n+\t\t\tcp++;\n \t\t\tbracket_depth++;\n-\t\telse if (bracket_depth && *cp == '}')\n+\t\t} else if (bracket_depth && *cp == '}') {\n \t\t\tbracket_depth--;\n-\t\telse if (!bracket_depth && *cp == ':')\n+\t\t} else if (!bracket_depth && *cp == ':') {\n \t\t\tbreak;\n+\t\t}\n \t}\n \tif (*cp == ':') {\n \t\tstruct object_id tree_oid;\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex d36cd7c0863..252485dac78 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -603,6 +603,23 @@ test_expect_success FUNNYNAMES '--batch-check, -Z with newline in input' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success FUNNYNAMES 'setup with curly braches in input' '\n+\tgit branch \"foo{bar\" &&\n+\tgit branch \"foo@\"\n+'\n+\n+test_expect_success FUNNYNAMES 'object reference with curly brace' '\n+\tgit cat-file -p \"foo{bar:hello\" >actual &&\n+\tgit cat-file -p HEAD:hello >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success FUNNYNAMES 'object reference with at-sign' '\n+\tgit cat-file -p \"foo@@{0}:hello\" >actual &&\n+\tgit cat-file -p HEAD:hello >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'setup blobs which are likely to delta' '\n \ttest-tool genrandom foo 10240 >foo &&\n \t{ cat foo && echo plus; } >foo-plus &&\n\nbase-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f\n-- \ngitgitgadget\n"},{"id":"509772","messageId":"xmqqpll6wjsk.fsf@gitster.g","threadId":"62719","inReplyTo":"pull.1844.git.1735699989371.gitgitgadget@gmail.com","subject":"Re: [PATCH] object-name: fix resolution of object names containing curly braces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-01T17:00:59Z","receivedAt":"2025-01-01T17:01:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> Given a branch name of 'foo{bar', commands like\n>\n> git cat-file -p foo{bar:README.md\n>\n> should succeed (assuming that branch had a README.md file, of course).\n> However, the change in cce91a2caef9 (Change 'master@noon' syntax to\n> 'master@{noon}'., 2006-05-19) presumed that curly braces would always\n> come after an '@' and be paired, causing 'foo{bar:README.md' to\n> entirely miss the ':' and assume there's no object being referenced.\n> In short, git would report:\n>\n> fatal: Not a valid object name foo{bar:README.md\n>\n> Change the parsing to only make the assumption of paired curly braces\n> immediately after a '@' character appears.\n\nInteresting. I wonder if this looseness was to ensure that we won't\nmistake a colon inside \"master^{/title with : a colon}\" as a start\nof a subpath, instead of asking for a commit with a title that\nhappens to have a colon in it?\n\n> Add tests for both this and 'foo@@{...}' cases, which an initial version\n> of this patch broke.\n\nThanks for being extra careful here.\n\n\n> Reported-by: Gabriel Amaral \n> Helped-by: Michael Haggerty \n> Signed-off-by: Elijah Newren \n> ---\n> object-name: fix resolution of object names containing curly braces\n> \n> Maintainer note: this bug dates back to 2006; it is not a regression in\n> this cycle.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1844%2Fnewren%2Fobject-name-fix-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1844/newren/object-name-fix-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1844\n>\n> object-name.c | 8 +++++---\n> t/t1006-cat-file.sh | 17 +++++++++++++++++\n> 2 files changed, 22 insertions(+), 3 deletions(-)\n>\n> diff --git a/object-name.c b/object-name.c\n> index c892fbe80aa..e92f26b3256 100644\n> --- a/object-name.c\n> +++ b/object-name.c\n> @@ -2087,12 +2087,14 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n> \t\treturn -1;\n> \t}\n> \tfor (cp = name, bracket_depth = 0; *cp; cp++) {\n> -\t\tif (*cp == '{')\n> +\t\tif (*cp == '@' && *(cp+1) == '{') {\n> +\t\t\tcp++;\n> \t\t\tbracket_depth++;\n> -\t\telse if (bracket_depth && *cp == '}')\n> +\t\t} else if (bracket_depth && *cp == '}') {\n> \t\t\tbracket_depth--;\n> -\t\telse if (!bracket_depth && *cp == ':')\n> +\t\t} else if (!bracket_depth && *cp == ':') {\n> \t\t\tbreak;\n> +\t\t}\n> \t}\n> \tif (*cp == ':') {\n> \t\tstruct object_id tree_oid;\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index d36cd7c0863..252485dac78 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -603,6 +603,23 @@ test_expect_success FUNNYNAMES '--batch-check, -Z with newline in input' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success FUNNYNAMES 'setup with curly braches in input' '\n> +\tgit branch \"foo{bar\" &&\n> +\tgit branch \"foo@\"\n> +'\n> +\n> +test_expect_success FUNNYNAMES 'object reference with curly brace' '\n> +\tgit cat-file -p \"foo{bar:hello\" >actual &&\n> +\tgit cat-file -p HEAD:hello >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success FUNNYNAMES 'object reference with at-sign' '\n> +\tgit cat-file -p \"foo@@{0}:hello\" >actual &&\n> +\tgit cat-file -p HEAD:hello >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'setup blobs which are likely to delta' '\n> \ttest-tool genrandom foo 10240 >foo &&\n> \t{ cat foo && echo plus; } >foo-plus &&\n>\n> base-commit: 92999a42db1c5f43f330e4f2bca4026b5b81576f\n"},{"id":"509829","messageId":"Z3ec0768O64j-JeJ@pks.im","threadId":"62719","inReplyTo":"pull.1844.git.1735699989371.gitgitgadget@gmail.com","subject":"Re: [PATCH] object-name: fix resolution of object names containing curly braces","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-03T08:16:27Z","receivedAt":"2025-01-03T08:16:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 01, 2025 at 02:53:09AM +0000, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> Given a branch name of 'foo{bar', commands like\n> \n> git cat-file -p foo{bar:README.md\n> \n> should succeed (assuming that branch had a README.md file, of course).\n> However, the change in cce91a2caef9 (Change 'master@noon' syntax to\n> 'master@{noon}'., 2006-05-19) presumed that curly braces would always\n> come after an '@' and be paired, causing 'foo{bar:README.md' to\n> entirely miss the ':' and assume there's no object being referenced.\n> In short, git would report:\n> \n> fatal: Not a valid object name foo{bar:README.md\n> \n> Change the parsing to only make the assumption of paired curly braces\n> immediately after a '@' character appears.\n> \n> Add tests for both this and 'foo@@{...}' cases, which an initial version\n> of this patch broke.\n\nCurious. I was kind of surprised to see that it's perfectly legal to\nhave branch names with curly braces in them in the first place. Even\nsomething like `foo{bar}` is legal, even though it might be confusing\nwhen one knows the above syntax. But sans your finding, this should be\nfine given that curly braces are only interpreted specially when\npreceded by '@', and the '@{' sequence is indeed disallowed by\n`check_refname_compoment()`.\n\n> diff --git a/object-name.c b/object-name.c\n> index c892fbe80aa..e92f26b3256 100644\n> --- a/object-name.c\n> +++ b/object-name.c\n> @@ -2087,12 +2087,14 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n> \t\treturn -1;\n> \t}\n> \tfor (cp = name, bracket_depth = 0; *cp; cp++) {\n> -\t\tif (*cp == '{')\n> +\t\tif (*cp == '@' && *(cp+1) == '{') {\n> +\t\t\tcp++;\n> \t\t\tbracket_depth++;\n> -\t\telse if (bracket_depth && *cp == '}')\n> +\t\t} else if (bracket_depth && *cp == '}') {\n> \t\t\tbracket_depth--;\n> -\t\telse if (!bracket_depth && *cp == ':')\n> +\t\t} else if (!bracket_depth && *cp == ':') {\n> \t\t\tbreak;\n> +\t\t}\n> \t}\n> \tif (*cp == ':') {\n> \t\tstruct object_id tree_oid;\n\nMakes sense. Only the first hunk actually changes anything, the\nremaining changes are only required to make us stick to our coding\nstyle.\n\nI wonder though: does this have any impact on '^{}' and other\nsyntaxes where we use '^' instead of '@'?\n\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index d36cd7c0863..252485dac78 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -603,6 +603,23 @@ test_expect_success FUNNYNAMES '--batch-check, -Z with newline in input' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success FUNNYNAMES 'setup with curly braches in input' '\n> +\tgit branch \"foo{bar\" &&\n> +\tgit branch \"foo@\"\n> +'\n> +\n> +test_expect_success FUNNYNAMES 'object reference with curly brace' '\n> +\tgit cat-file -p \"foo{bar:hello\" >actual &&\n> +\tgit cat-file -p HEAD:hello >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success FUNNYNAMES 'object reference with at-sign' '\n> +\tgit cat-file -p \"foo@@{0}:hello\" >actual &&\n> +\tgit cat-file -p HEAD:hello >expect &&\n> +\ttest_cmp expect actual\n> +'\n\nDo these really need the FUNNYNAMES prereq? The prereq seems to only be\nabout embedded quotes, tabs and newlines and is disallowed on MinGW. But\nI think both '{' and '@' should work alright there, shouldn't they?\n\nThanks!\n\nPatrick\n"},{"id":"509862","messageId":"xmqqwmfbsxxc.fsf@gitster.g","threadId":"62719","inReplyTo":"Z3ec0768O64j-JeJ@pks.im","subject":"Re: [PATCH] object-name: fix resolution of object names containing curly braces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-03T15:46:07Z","receivedAt":"2025-01-03T15:46:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> I wonder though: does this have any impact on '^{}' and other\n> syntaxes where we use '^' instead of '@'?\n> ...\n> Do these really need the FUNNYNAMES prereq? The prereq seems to only be\n> about embedded quotes, tabs and newlines and is disallowed on MinGW. But\n> I think both '{' and '@' should work alright there, shouldn't they?\n\nThanks for a review. I am too curious how this change interacts\nwith syntax with {braces} that do not use \"@\".\n"},{"id":"509896","messageId":"CABPp-BG47KNt5v3vEzPCCb+qRyz9473qPTfoOcTytb_Eis96SQ@mail.gmail.com","threadId":"62719","inReplyTo":"xmqqpll6wjsk.fsf@gitster.g","subject":"Re: [PATCH] object-name: fix resolution of object names containing curly braces","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-01-03T23:34:41Z","receivedAt":"2025-01-03T23:34:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 1, 2025 at 9:01 AM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > From: Elijah Newren \n> >\n> > Given a branch name of 'foo{bar', commands like\n> >\n> > git cat-file -p foo{bar:README.md\n> >\n> > should succeed (assuming that branch had a README.md file, of course).\n> > However, the change in cce91a2caef9 (Change 'master@noon' syntax to\n> > 'master@{noon}'., 2006-05-19) presumed that curly braces would always\n> > come after an '@' and be paired, causing 'foo{bar:README.md' to\n> > entirely miss the ':' and assume there's no object being referenced.\n> > In short, git would report:\n> >\n> > fatal: Not a valid object name foo{bar:README.md\n> >\n> > Change the parsing to only make the assumption of paired curly braces\n> > immediately after a '@' character appears.\n>\n> Interesting. I wonder if this looseness was to ensure that we won't\n> mistake a colon inside \"master^{/title with : a colon}\" as a start\n> of a subpath, instead of asking for a commit with a title that\n> happens to have a colon in it?\n\nYeah, good catch, my changes would for example break parsing\n master^{/object-name:}:t/t1006-cat-file.sh\n\nI'll fix that and add a testcase.\n"},{"id":"509898","messageId":"CABPp-BF7QQ02OFOzSaUsSU57yB9SZFRT=KnuDx86ABRAAob4Pw@mail.gmail.com","threadId":"62719","inReplyTo":"Z3ec0768O64j-JeJ@pks.im","subject":"Re: [PATCH] object-name: fix resolution of object names containing curly braces","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-01-03T23:43:34Z","receivedAt":"2025-01-03T23:43:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jan 3, 2025 at 12:16 AM Patrick Steinhardt wrote:\n>\n> On Wed, Jan 01, 2025 at 02:53:09AM +0000, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > Given a branch name of 'foo{bar', commands like\n> >\n> > git cat-file -p foo{bar:README.md\n> >\n> > should succeed (assuming that branch had a README.md file, of course).\n> > However, the change in cce91a2caef9 (Change 'master@noon' syntax to\n> > 'master@{noon}'., 2006-05-19) presumed that curly braces would always\n> > come after an '@' and be paired, causing 'foo{bar:README.md' to\n> > entirely miss the ':' and assume there's no object being referenced.\n> > In short, git would report:\n> >\n> > fatal: Not a valid object name foo{bar:README.md\n> >\n> > Change the parsing to only make the assumption of paired curly braces\n> > immediately after a '@' character appears.\n> >\n> > Add tests for both this and 'foo@@{...}' cases, which an initial version\n> > of this patch broke.\n>\n> Curious. I was kind of surprised to see that it's perfectly legal to\n> have branch names with curly braces in them in the first place.\n\nI was surprised too, but apparently they are valid and we have real\nworld repositories where people have used such bad names.\n\n> Even\n> something like `foo{bar}` is legal, even though it might be confusing\n> when one knows the above syntax. But sans your finding, this should be\n> fine given that curly braces are only interpreted specially when\n> preceded by '@', and the '@{' sequence is indeed disallowed by\n> `check_refname_compoment()`.\n>\n> > diff --git a/object-name.c b/object-name.c\n> > index c892fbe80aa..e92f26b3256 100644\n> > --- a/object-name.c\n> > +++ b/object-name.c\n> > @@ -2087,12 +2087,14 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n> > return -1;\n> > }\n> > for (cp = name, bracket_depth = 0; *cp; cp++) {\n> > - if (*cp == '{')\n> > + if (*cp == '@' && *(cp+1) == '{') {\n> > + cp++;\n> > bracket_depth++;\n> > - else if (bracket_depth && *cp == '}')\n> > + } else if (bracket_depth && *cp == '}') {\n> > bracket_depth--;\n> > - else if (!bracket_depth && *cp == ':')\n> > + } else if (!bracket_depth && *cp == ':') {\n> > break;\n> > + }\n> > }\n> > if (*cp == ':') {\n> > struct object_id tree_oid;\n>\n> Makes sense. Only the first hunk actually changes anything, the\n> remaining changes are only required to make us stick to our coding\n> style.\n>\n> I wonder though: does this have any impact on '^{}' and other\n> syntaxes where we use '^' instead of '@'?\n\n is pretty limited, so I see no problem there. However\n^{/} is problematic, as Junio pointed out. I've\nfixed up the patch and added a testcase to cover all the '^{...}'\ncases.\n\n> > diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> > index d36cd7c0863..252485dac78 100755\n> > --- a/t/t1006-cat-file.sh\n> > +++ b/t/t1006-cat-file.sh\n> > @@ -603,6 +603,23 @@ test_expect_success FUNNYNAMES '--batch-check, -Z with newline in input' '\n> > test_cmp expect actual\n> > '\n> >\n> > +test_expect_success FUNNYNAMES 'setup with curly braches in input' '\n> > + git branch \"foo{bar\" &&\n> > + git branch \"foo@\"\n> > +'\n> > +\n> > +test_expect_success FUNNYNAMES 'object reference with curly brace' '\n> > + git cat-file -p \"foo{bar:hello\" >actual &&\n> > + git cat-file -p HEAD:hello >expect &&\n> > + test_cmp expect actual\n> > +'\n> > +\n> > +test_expect_success FUNNYNAMES 'object reference with at-sign' '\n> > + git cat-file -p \"foo@@{0}:hello\" >actual &&\n> > + git cat-file -p HEAD:hello >expect &&\n> > + test_cmp expect actual\n> > +'\n>\n> Do these really need the FUNNYNAMES prereq? The prereq seems to only be\n> about embedded quotes, tabs and newlines and is disallowed on MinGW. But\n> I think both '{' and '@' should work alright there, shouldn't they?\n\nOh, I misread the failures. It turns out the FUNNYNAMES prereq fixed\nthings in CI on windows for me because the only commit ever created in\nthe repository is created by a testcase with a FUNNYNAMES prereq.\nSince the setup for my tests relied on HEAD existing (because I run\n git branch \"foo{bar\" HEAD\nin a setup test of my own), the tests were failing. I didn't look\nclosely enough and assumed that command was failing because Windows\ndidn't like a branch name with a curly brace, but the real reason it\nwas failing was because HEAD didn't exist.\n\nI'll tweak one of the earlier setup tests to create a commit so HEAD exists.\n\nThanks for pointing this out.\n"},{"id":"509899","messageId":"pull.1844.v2.git.1735949870.gitgitgadget@gmail.com","threadId":"62719","inReplyTo":"pull.1844.git.1735699989371.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] object-name: fix resolution of object names containing curly braces","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-01-04T00:17:48Z","receivedAt":"2025-01-04T00:17:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Maintainer note: these bugs both date back to 2006; neither is a regression\nin this cycle.\n\nChanges since v1:\n\n * Covered the ^{...} cases, and added a testcase for those\n * Added a second patch for another bug discovered by the same reporter,\n where branch:path/to/file/named/major-gaffed is interpreted as a request\n for a commit (namely affed) rather than a blob. (At least, assuming\n commit affed exists)\n\nThe second patch has some backward compatibility concerns. People used to be\nable to do e.g. git show ${garbage}-g${hash}. I tightened it to\n${valid_refname}-${number}-g${hash}, but do we want to allow e.g.\n${valid_refname}-g${hash} (allowing the count to be omitted) or maybe even\nallow a subset of invalid refnames?\n\nAlso for the second patch, while the repository the reporter found the issue\nin was something else, I found two open source examples:\n\n * lore.git: git cat-file -t master:random/path/major-gaffed\n * git.git: git cat-file -t super-invalid~///\\\\.....@.lock-gfd0bba94e\n\nElijah Newren (2):\n object-name: fix resolution of object names containing curly braces\n object-name: be more strict in parsing describe-like output\n\n object-name.c | 63 ++++++++++++++++++++++++++++++++++++++++++---\n t/t1006-cat-file.sh | 31 +++++++++++++++++++++-\n t/t6120-describe.sh | 22 ++++++++++++++++\n 3 files changed, 111 insertions(+), 5 deletions(-)\n\n\nbase-commit: 063bcebf0c917140ca0e705cbe0fdea127e90086\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1844%2Fnewren%2Fobject-name-fix-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1844/newren/object-name-fix-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1844\n\nRange-diff vs v1:\n\n 1: 1671b773fcc ! 1: 13f68bebe90 object-name: fix resolution of object names containing curly braces\n @@ Commit message\n should succeed (assuming that branch had a README.md file, of course).\n However, the change in cce91a2caef9 (Change 'master@noon' syntax to\n 'master@{noon}'., 2006-05-19) presumed that curly braces would always\n - come after an '@' and be paired, causing 'foo{bar:README.md' to\n - entirely miss the ':' and assume there's no object being referenced.\n + come after an '@' or '^' and be paired, causing e.g. 'foo{bar:README.md'\n + to entirely miss the ':' and assume there's no object being referenced.\n In short, git would report:\n \n fatal: Not a valid object name foo{bar:README.md\n \n Change the parsing to only make the assumption of paired curly braces\n - immediately after a '@' character appears.\n + immediately after either a '@' or '^' character appears.\n \n - Add tests for both this and 'foo@@{...}' cases, which an initial version\n - of this patch broke.\n + Add tests for this, as well as for a few other test cases that initial\n + versions of this patch broke:\n + * 'foo@@{...}'\n + * 'foo^{/${SEARCH_TEXT_WITH_COLON}}:${PATH}'\n \n Reported-by: Gabriel Amaral \n Helped-by: Michael Haggerty \n @@ object-name.c: static enum get_oid_result get_oid_with_context_1(struct reposito\n \t}\n \tfor (cp = name, bracket_depth = 0; *cp; cp++) {\n -\t\tif (*cp == '{')\n -+\t\tif (*cp == '@' && *(cp+1) == '{') {\n ++\t\tif (*(cp+1) == '{' && (*cp == '@' || *cp == '^')) {\n +\t\t\tcp++;\n \t\t\tbracket_depth++;\n -\t\telse if (bracket_depth && *cp == '}')\n @@ object-name.c: static enum get_oid_result get_oid_with_context_1(struct reposito\n \t\tstruct object_id tree_oid;\n \n ## t/t1006-cat-file.sh ##\n +@@ t/t1006-cat-file.sh: test_expect_success \"setup\" '\n + \tgit config extensions.objectformat $test_hash_algo &&\n + \tgit config extensions.compatobjectformat $test_compat_hash_algo &&\n + \techo_without_newline \"$hello_content\" > hello &&\n +-\tgit update-index --add hello\n ++\tgit update-index --add hello &&\n ++\tgit commit -m \"add hello file\"\n + '\n + \n + run_blob_tests () {\n @@ t/t1006-cat-file.sh: test_expect_success FUNNYNAMES '--batch-check, -Z with newline in input' '\n \ttest_cmp expect actual\n '\n \n -+test_expect_success FUNNYNAMES 'setup with curly braches in input' '\n -+\tgit branch \"foo{bar\" &&\n -+\tgit branch \"foo@\"\n ++test_expect_success 'setup with curly braches in input' '\n ++\tgit branch \"foo{bar\" HEAD &&\n ++\tgit branch \"foo@\" HEAD\n +'\n +\n -+test_expect_success FUNNYNAMES 'object reference with curly brace' '\n ++test_expect_success 'object reference with curly brace' '\n +\tgit cat-file -p \"foo{bar:hello\" >actual &&\n +\tgit cat-file -p HEAD:hello >expect &&\n +\ttest_cmp expect actual\n +'\n +\n -+test_expect_success FUNNYNAMES 'object reference with at-sign' '\n ++test_expect_success 'object reference with at-sign' '\n +\tgit cat-file -p \"foo@@{0}:hello\" >actual &&\n +\tgit cat-file -p HEAD:hello >expect &&\n +\ttest_cmp expect actual\n +'\n ++\n ++test_expect_success 'setup with commit with colon' '\n ++\tgit commit-tree -m \"testing: just a bunch of junk\" HEAD^{tree} >out &&\n ++\tgit branch other $(cat out)\n ++'\n ++\n ++test_expect_success 'object reference via commit text search' '\n ++\tgit cat-file -p \"other^{/testing:}:hello\" >actual &&\n ++\tgit cat-file -p HEAD:hello >expect &&\n ++\ttest_cmp expect actual\n ++'\n +\n test_expect_success 'setup blobs which are likely to delta' '\n \ttest-tool genrandom foo 10240 >foo &&\n -: ----------- > 2: 31f1c37b31a object-name: be more strict in parsing describe-like output\n\n-- \ngitgitgadget\n"},{"id":"509900","messageId":"13f68bebe90549ba19452f12abb6fea41c2517fb.1735949870.git.gitgitgadget@gmail.com","threadId":"62719","inReplyTo":"pull.1844.v2.git.1735949870.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] object-name: fix resolution of object names containing curly braces","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-01-04T00:17:49Z","receivedAt":"2025-01-04T00:17:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nGiven a branch name of 'foo{bar', commands like\n\n git cat-file -p foo{bar:README.md\n\nshould succeed (assuming that branch had a README.md file, of course).\nHowever, the change in cce91a2caef9 (Change 'master@noon' syntax to\n'master@{noon}'., 2006-05-19) presumed that curly braces would always\ncome after an '@' or '^' and be paired, causing e.g. 'foo{bar:README.md'\nto entirely miss the ':' and assume there's no object being referenced.\nIn short, git would report:\n\n fatal: Not a valid object name foo{bar:README.md\n\nChange the parsing to only make the assumption of paired curly braces\nimmediately after either a '@' or '^' character appears.\n\nAdd tests for this, as well as for a few other test cases that initial\nversions of this patch broke:\n * 'foo@@{...}'\n * 'foo^{/${SEARCH_TEXT_WITH_COLON}}:${PATH}'\n\nReported-by: Gabriel Amaral \nHelped-by: Michael Haggerty \nSigned-off-by: Elijah Newren \n---\n object-name.c | 8 +++++---\n t/t1006-cat-file.sh | 31 ++++++++++++++++++++++++++++++-\n 2 files changed, 35 insertions(+), 4 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex a563635a8cb..e54ef1f621e 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -2051,12 +2051,14 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n \t\treturn -1;\n \t}\n \tfor (cp = name, bracket_depth = 0; *cp; cp++) {\n-\t\tif (*cp == '{')\n+\t\tif (*(cp+1) == '{' && (*cp == '@' || *cp == '^')) {\n+\t\t\tcp++;\n \t\t\tbracket_depth++;\n-\t\telse if (bracket_depth && *cp == '}')\n+\t\t} else if (bracket_depth && *cp == '}') {\n \t\t\tbracket_depth--;\n-\t\telse if (!bracket_depth && *cp == ':')\n+\t\t} else if (!bracket_depth && *cp == ':') {\n \t\t\tbreak;\n+\t\t}\n \t}\n \tif (*cp == ':') {\n \t\tstruct object_id tree_oid;\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex ff9bf213aa2..398865d6ebe 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -240,7 +240,8 @@ test_expect_success \"setup\" '\n \tgit config extensions.objectformat $test_hash_algo &&\n \tgit config extensions.compatobjectformat $test_compat_hash_algo &&\n \techo_without_newline \"$hello_content\" > hello &&\n-\tgit update-index --add hello\n+\tgit update-index --add hello &&\n+\tgit commit -m \"add hello file\"\n '\n \n run_blob_tests () {\n@@ -602,6 +603,34 @@ test_expect_success FUNNYNAMES '--batch-check, -Z with newline in input' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup with curly braches in input' '\n+\tgit branch \"foo{bar\" HEAD &&\n+\tgit branch \"foo@\" HEAD\n+'\n+\n+test_expect_success 'object reference with curly brace' '\n+\tgit cat-file -p \"foo{bar:hello\" >actual &&\n+\tgit cat-file -p HEAD:hello >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'object reference with at-sign' '\n+\tgit cat-file -p \"foo@@{0}:hello\" >actual &&\n+\tgit cat-file -p HEAD:hello >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'setup with commit with colon' '\n+\tgit commit-tree -m \"testing: just a bunch of junk\" HEAD^{tree} >out &&\n+\tgit branch other $(cat out)\n+'\n+\n+test_expect_success 'object reference via commit text search' '\n+\tgit cat-file -p \"other^{/testing:}:hello\" >actual &&\n+\tgit cat-file -p HEAD:hello >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'setup blobs which are likely to delta' '\n \ttest-tool genrandom foo 10240 >foo &&\n \t{ cat foo && echo plus; } >foo-plus &&\n-- \ngitgitgadget\n\n"},{"id":"509901","messageId":"31f1c37b31a2ac5cecee0f7f5913e0c466b7561c.1735949870.git.gitgitgadget@gmail.com","threadId":"62719","inReplyTo":"pull.1844.v2.git.1735949870.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] object-name: be more strict in parsing describe-like output","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-01-04T00:17:50Z","receivedAt":"2025-01-04T00:17:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nFrom Documentation/revisions.txt:\n '', e.g. 'v1.7.4.2-679-g3bee7fb'::\n Output from `git describe`; i.e. a closest tag, optionally\n followed by a dash and a number of commits, followed by a dash, a\n 'g', and an abbreviated object name.\nwhich means that output of the format\n ${REFNAME}-${INTEGER}-g${HASH}\nshould parse to fully expand ${HASH}. This is fine. However, we\ncurrently don't validate any of ${REFNAME}-${INTEGER}, we only parse\n-g${HASH} and assume the rest is valid. That is problematic, since it\nbreaks things like\n\n git cat-file -p branchname:path/to/file/named/i-gaffed\n\nwhich, when commit affed exists, will not return us information about a\nfile we are looking for but will instead tell us about commit affed.\n\nSimilarly, we should probably not treat\n refs/tags/invalid/./../...../// ~^:/?*\\\\&[}/busted.lock-g049e0ef6\nas a request for commit 050e0ef6 either.\n\nTighten up the parsing to make sure ${REFNAME} and ${INTEGER} are\npresent and valid.\n\nReported-by: Gabriel Amaral \nSigned-off-by: Elijah Newren \n---\n object-name.c | 55 ++++++++++++++++++++++++++++++++++++++++++++-\n t/t6120-describe.sh | 22 ++++++++++++++++++\n 2 files changed, 76 insertions(+), 1 deletion(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex e54ef1f621e..71207729f6f 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1271,6 +1271,58 @@ static int peel_onion(struct repository *r, const char *name, int len,\n \treturn 0;\n }\n \n+/*\n+ * Documentation/revisions.txt says:\n+ * '', e.g. 'v1.7.4.2-679-g3bee7fb'::\n+ * Output from `git describe`; i.e. a closest tag, optionally\n+ * followed by a dash and a number of commits, followed by a dash, a\n+ * 'g', and an abbreviated object name.\n+ *\n+ * which means that the stuff before '-g${HASH}' needs to be a valid\n+ * refname, a dash, and a non-negative integer. This function verifies\n+ * that.\n+ *\n+ * In particular, we do not want to treat\n+ * branchname:path/to/file/named/i-gaffed\n+ * as a request for commit affed.\n+ *\n+ * More generally, we should probably not treat\n+ * 'refs/heads/./../.../ ~^:/?*[////\\\\\\&}/busted.lock-g050e0ef6ead'\n+ * as a request for object 050e0ef6ead either.\n+ *\n+ * We are called with name[len] == '-' and name[len+1] == 'g', i.e.\n+ * we are verifying ${REFNAME}-{INTEGER} part of the name.\n+ */\n+static int ref_and_count_parts_valid(const char *name, int len)\n+{\n+\tstruct strbuf sb;\n+\tconst char *cp;\n+\tint flags = REFNAME_ALLOW_ONELEVEL;\n+\tint ret = 1;\n+\n+\t/* Ensure we have at least one digit */\n+\tif (!isxdigit(name[len-1]))\n+\t\treturn 0;\n+\n+\t/* Skip over digits backwards until we get to the dash */\n+\tfor (cp = name + len - 2; name < cp; cp--) {\n+\t\tif (*cp == '-')\n+\t\t\tbreak;\n+\t\tif (!isxdigit(*cp))\n+\t\t\treturn 0;\n+\t}\n+\t/* Ensure we found the leading dash */\n+\tif (*cp != '-')\n+\t\treturn 0;\n+\n+\tlen = cp - name;\n+\tstrbuf_init(&sb, len);\n+\tstrbuf_add(&sb, name, len);\n+\tret = !check_refname_format(name, flags);\n+\tstrbuf_release(&sb);\n+\treturn ret;\n+}\n+\n static int get_describe_name(struct repository *r,\n \t\t\t const char *name, int len,\n \t\t\t struct object_id *oid)\n@@ -1284,7 +1336,8 @@ static int get_describe_name(struct repository *r,\n \t\t\t/* We must be looking at g in \"SOMETHING-g\"\n \t\t\t * for it to be describe output.\n \t\t\t */\n-\t\t\tif (ch == 'g' && cp[-1] == '-') {\n+\t\t\tif (ch == 'g' && cp[-1] == '-' &&\n+\t\t\t ref_and_count_parts_valid(name, cp - 1 - name)) {\n \t\t\t\tcp++;\n \t\t\t\tlen -= cp - name;\n \t\t\t\treturn get_short_oid(r,\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 3f6160d702b..9217bd0fa89 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -725,4 +725,26 @@ test_expect_success '--exact-match does not show --always fallback' '\n \ttest_must_fail git describe --exact-match --always\n '\n \n+test_expect_success 'avoid being fooled by describe-like filename' '\n+\ttest_when_finished rm out &&\n+\n+\tgit rev-parse --short HEAD >out &&\n+\tFILENAME=filename-g$(cat out) &&\n+\ttouch $FILENAME &&\n+\tgit add $FILENAME &&\n+\tgit commit -m \"Add $FILENAME\" &&\n+\n+\tgit cat-file -t HEAD:$FILENAME >actual &&\n+\n+\techo blob >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'do not be fooled by invalid describe format ' '\n+\ttest_when_finished rm out &&\n+\n+\tgit rev-parse --short HEAD >out &&\n+\ttest_must_fail git cat-file -t \"refs/tags/super-invalid/./../...../ ~^:/?*[////\\\\\\\\\\\\&}/busted.lock-42-g\"$(cat out)\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"509903","messageId":"xmqqa5c7nvdf.fsf@gitster.g","threadId":"62719","inReplyTo":"CABPp-BG47KNt5v3vEzPCCb+qRyz9473qPTfoOcTytb_Eis96SQ@mail.gmail.com","subject":"Re: [PATCH] object-name: fix resolution of object names containing curly braces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-04T02:52:28Z","receivedAt":"2025-01-04T02:52:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> Interesting. I wonder if this looseness was to ensure that we won't\n>> mistake a colon inside \"master^{/title with : a colon}\" as a start\n>> of a subpath, instead of asking for a commit with a title that\n>> happens to have a colon in it?\n>\n> Yeah, good catch, my changes would for example break parsing\n> master^{/object-name:}:t/t1006-cat-file.sh\n>\n> I'll fix that and add a testcase.\n\nI am not sure what the updated approach would be, but I kind of\nprefer if the parser does not have to be intimately familiar with\nthe fact that we know about '@' and '^' as possible characters that\ncan appear before the opening '{'. That same attitude of \"We know\nthat before a '{'\" '@' is valid thing to appear, so let's special\ncase '@'\" was what got us into this exchange in the first place, and\nI am not confident that we now are exhaustive, knowing about '@' and\n'^'.\n\nThanks.\n\n"},{"id":"509910","messageId":"xmqqed1i4pga.fsf@gitster.g","threadId":"62719","inReplyTo":"pull.1844.v2.git.1735949870.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] object-name: fix resolution of object names containing curly braces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-04T14:35:17Z","receivedAt":"2025-01-04T14:35:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> * Added a second patch for another bug discovered by the same reporter,\n> where branch:path/to/file/named/major-gaffed is interpreted as a request\n> for a commit (namely affed) rather than a blob. (At least, assuming\n> commit affed exists)\n>\n> The second patch has some backward compatibility concerns. People used to be\n> able to do e.g. git show ${garbage}-g${hash}. I tightened it to\n> ${valid_refname}-${number}-g${hash}, but do we want to allow e.g.\n> ${valid_refname}-g${hash} (allowing the count to be omitted) or maybe even\n> allow a subset of invalid refnames?\n\nMy take on it is that it is an absolute no-no if we require that\n${valid_refname} exists locally, and it is still iffy if we checked\n${valid_refname} with check_format() (because the definition of\nvalidity can change over time, and we would not know the rules that\nwere valid back when the reference to the commit was written).\n\nOtherwise a tightened rule would make \"${garbage}-g${hash}\" less\nuseful to copy-and-paste from a text file to command line.\n\nIn general what would we do if a string can be interpreted in\nmultiple ways in _different_ parts of the object-name codepaths. We\nall know that \"affed\" would trigger the \"ambiguous object name\"\nerror if there are more than one object whose object name begins\nwith \"affed\", but if \"${garbage}-gaffed\" can be interpreted as the\nname of an object whose object name begins with \"affed\" and also can\nbe interpreted as the name of another object that sits at a path\nthat ends with \"-gaffed\" in some tree object, regardless of how the\nleading part \"${garbage}\" looks like, it would be desirable if we\ndeclared such a string as \"ambiguous\" the same way.\n"},{"id":"509913","messageId":"CABPp-BEzKyXY7YZOzedwQzAFqLiE_9oWsD=GByiDhi7go2aXNw@mail.gmail.com","threadId":"62719","inReplyTo":"xmqqed1i4pga.fsf@gitster.g","subject":"Re: [PATCH v2 0/2] object-name: fix resolution of object names containing curly braces","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-01-04T15:55:27Z","receivedAt":"2025-01-04T15:55:39Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jan 4, 2025 at 6:35 AM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > * Added a second patch for another bug discovered by the same reporter,\n> > where branch:path/to/file/named/major-gaffed is interpreted as a request\n> > for a commit (namely affed) rather than a blob. (At least, assuming\n> > commit affed exists)\n> >\n> > The second patch has some backward compatibility concerns. People used to be\n> > able to do e.g. git show ${garbage}-g${hash}. I tightened it to\n> > ${valid_refname}-${number}-g${hash}, but do we want to allow e.g.\n> > ${valid_refname}-g${hash} (allowing the count to be omitted) or maybe even\n> > allow a subset of invalid refnames?\n>\n> My take on it is that it is an absolute no-no if we require that\n> ${valid_refname} exists locally, and it is still iffy if we checked\n> ${valid_refname} with check_format() (because the definition of\n> validity can change over time, and we would not know the rules that\n> were valid back when the reference to the commit was written).\n\nFair enough. However...\n\n> Otherwise a tightened rule would make \"${garbage}-g${hash}\" less\n> useful to copy-and-paste from a text file to command line.\n>\n> In general what would we do if a string can be interpreted in\n> multiple ways in _different_ parts of the object-name codepaths. We\n> all know that \"affed\" would trigger the \"ambiguous object name\"\n> error if there are more than one object whose object name begins\n> with \"affed\", but if \"${garbage}-gaffed\" can be interpreted as the\n> name of an object whose object name begins with \"affed\" and also can\n> be interpreted as the name of another object that sits at a path\n> that ends with \"-gaffed\" in some tree object, regardless of how the\n> leading part \"${garbage}\" looks like, it would be desirable if we\n> declared such a string as \"ambiguous\" the same way.\n\nHow would that be desirable? There's no possible way to disambiguate.\nWhile abbreviated revisions can just be modified to be less\nabbreviated, paths cannot be spelled any other way. How would you\nspell\n master:path/to/who-gabbed\nin a \"less ambiguous\" way to differentiate it from commit abbed? As\nfar as I can tell, this proposal just leaves the user stuck with an\nerror with no way to get the path they want.\n\nIf you don't like check_format() being called on the leading part of\nthe string, can we at least enforce that there is no ':', so that we\ncan successfully request explicit paths of given revisions and know\nthat we'll get them? (That'd disallow e.g. next^{/doc:}-12-gabbed,\nbut that clearly was never a valid describe output anyway.)\n"},{"id":"509915","messageId":"xmqq34hy4hii.fsf@gitster.g","threadId":"62719","inReplyTo":"13f68bebe90549ba19452f12abb6fea41c2517fb.1735949870.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] object-name: fix resolution of object names containing curly braces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-04T17:26:45Z","receivedAt":"2025-01-04T17:26:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> \tfor (cp = name, bracket_depth = 0; *cp; cp++) {\n> -\t\tif (*cp == '{')\n> +\t\tif (*(cp+1) == '{' && (*cp == '@' || *cp == '^')) {\n> +\t\t\tcp++;\n> \t\t\tbracket_depth++;\n\nChecking cp[1] before even knowing if cp[0] is the end of the string\n(hence cp[1] is an out of bounds access) smells fishy. If it were\nsomething like ...\n\n\tif (cp[0] && strchr(\"@^\", cp[0]) && cp[1] == '{')\n\n... it may be a bit more palatable, perhaps? At least writing it\nthis way we can easily scale when we find the third character we\nneed to special case, hopefully, but again, I do prefer if we can\nfind a solution that does not have such an intimate knowledge about\n\"@^\", which I just failed to do here X-<.\n\nThanks.\n\n"},{"id":"509916","messageId":"xmqqy0zq31s3.fsf@gitster.g","threadId":"62719","inReplyTo":"CABPp-BEzKyXY7YZOzedwQzAFqLiE_9oWsD=GByiDhi7go2aXNw@mail.gmail.com","subject":"Re: [PATCH v2 0/2] object-name: fix resolution of object names containing curly braces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-04T17:51:56Z","receivedAt":"2025-01-04T17:51:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> In general what would we do if a string can be interpreted in\n>> multiple ways in _different_ parts of the object-name codepaths. We\n>> all know that \"affed\" would trigger the \"ambiguous object name\"\n>> error if there are more than one object whose object name begins\n>> with \"affed\", but if \"${garbage}-gaffed\" can be interpreted as the\n>> name of an object whose object name begins with \"affed\" and also can\n>> be interpreted as the name of another object that sits at a path\n>> that ends with \"-gaffed\" in some tree object, regardless of how the\n>> leading part \"${garbage}\" looks like, it would be desirable if we\n>> declared such a string as \"ambiguous\" the same way.\n>\n> How would that be desirable?\n\nIn \"a:b/c-0-gabcde\", *if* \"a:b/c-0\" *were* a valid way to spell a\nvalid refname, then the whole thing is an ambiguous object name,\ni.e. it could be \"something reachable from object 'a:b/c' whose\nobject name begins with abcde\", or it could be \"object at the path\nb/c-0-gabcde in a tree-ish a\", and in such a case our code should be\nset up to allow us to give a \"that's ambiguous\" error, instead of\nyielding the first possible interpretation (i.e. if we happen to\nhave checked the describe name first and \"$garbage-0-gabcde\", we\nyield \"abcde\" before even checking if $garbage part gives a possible\nleading part of a tree-ish; but if a future refactoring of the code\nflips the order of checking, we may end up yielding 'an object at a\npath, which ends with -0-gabcde, sitting in a tree-ish', without\nchecking if that could be a valid describe name).\n\nOf course we should make sure that the syntax cannot be ambiguous\nwhen we introduce a new syntax to represent a new feature ;-)\n\nNow, I think \":\" has always been a byte that is invalid as a part of\nany refname, so \"${garbage}-gabcde\" with a colon in ${garbage}\ncannot be a describe name. So in the above about \"a:b/c-0\" is an\nimpossible example, but I was wondering more about the general\nprinciple we should follow.\n\n"},{"id":"509918","messageId":"CABPp-BHACrtkMJAKpOsPw5bw8Fvnhe_wFLMNntsPifmg8yomfw@mail.gmail.com","threadId":"62719","inReplyTo":"xmqq34hy4hii.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] object-name: fix resolution of object names containing curly braces","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-01-04T18:54:44Z","receivedAt":"2025-01-04T18:54:56Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jan 4, 2025 at 9:26 AM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > for (cp = name, bracket_depth = 0; *cp; cp++) {\n> > - if (*cp == '{')\n> > + if (*(cp+1) == '{' && (*cp == '@' || *cp == '^')) {\n> > + cp++;\n> > bracket_depth++;\n>\n> Checking cp[1] before even knowing if cp[0] is the end of the string\n> (hence cp[1] is an out of bounds access) smells fishy.\n\nWe checked *cp in the loop already, so we know cp[0] != '\\0'.\nCombined with the fact that this is a NUL-terminated string, we\ntherefore also know that cp[1] is not an out-of-bounds access.\n\n> If it were\n> something like ...\n>\n> if (cp[0] && strchr(\"@^\", cp[0]) && cp[1] == '{')\n\nSince we know cp[0] != '\\0' already, couldn't this be simplified to\n\n if (strchr(\"@^\", *cp) && cp[1] == '{')\n\n?\n\nI do like this form better though, yes.\n\n> ... it may be a bit more palatable, perhaps? At least writing it\n> this way we can easily scale when we find the third character we\n> need to special case, hopefully, but again, I do prefer if we can\n> find a solution that does not have such an intimate knowledge about\n> \"@^\", which I just failed to do here X-<.\n\nYeah, I have failed to come up with an alternative as well. If I and\nothers can't come up with something better in a few days, I'll\nresubmit with the above change and a comment in the commit message\nthat we'd prefer something better but were unable to come up with\nanything.\n"},{"id":"509919","messageId":"CABPp-BF5Sw=DY3rahmoS8NXg3GZX+nAKC0v0Y49vgq1i38DR3g@mail.gmail.com","threadId":"62719","inReplyTo":"xmqqy0zq31s3.fsf@gitster.g","subject":"Re: [PATCH v2 0/2] object-name: fix resolution of object names containing curly braces","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-01-04T18:55:42Z","receivedAt":"2025-01-04T18:55:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jan 4, 2025 at 9:51 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> >> In general what would we do if a string can be interpreted in\n> >> multiple ways in _different_ parts of the object-name codepaths. We\n> >> all know that \"affed\" would trigger the \"ambiguous object name\"\n> >> error if there are more than one object whose object name begins\n> >> with \"affed\", but if \"${garbage}-gaffed\" can be interpreted as the\n> >> name of an object whose object name begins with \"affed\" and also can\n> >> be interpreted as the name of another object that sits at a path\n> >> that ends with \"-gaffed\" in some tree object, regardless of how the\n> >> leading part \"${garbage}\" looks like, it would be desirable if we\n> >> declared such a string as \"ambiguous\" the same way.\n> >\n> > How would that be desirable?\n>\n> In \"a:b/c-0-gabcde\", *if* \"a:b/c-0\" *were* a valid way to spell a\n> valid refname, then the whole thing is an ambiguous object name,\n> i.e. it could be \"something reachable from object 'a:b/c' whose\n> object name begins with abcde\", or it could be \"object at the path\n> b/c-0-gabcde in a tree-ish a\", and in such a case our code should be\n> set up to allow us to give a \"that's ambiguous\" error, instead of\n> yielding the first possible interpretation (i.e. if we happen to\n> have checked the describe name first and \"$garbage-0-gabcde\", we\n> yield \"abcde\" before even checking if $garbage part gives a possible\n> leading part of a tree-ish; but if a future refactoring of the code\n> flips the order of checking, we may end up yielding 'an object at a\n> path, which ends with -0-gabcde, sitting in a tree-ish', without\n> checking if that could be a valid describe name).\n>\n> Of course we should make sure that the syntax cannot be ambiguous\n> when we introduce a new syntax to represent a new feature ;-)\n>\n> Now, I think \":\" has always been a byte that is invalid as a part of\n> any refname, so \"${garbage}-gabcde\" with a colon in ${garbage}\n> cannot be a describe name. So in the above about \"a:b/c-0\" is an\n> impossible example, but I was wondering more about the general\n> principle we should follow.\n\nAre you only interested in the general principle for the \"possible\nexamples\"? What about the general principle for the \"impossible\nexamples\"? Things like \"master:path/to/who-gabbed\" are unambiguously\na reference to a path within a revision that cannot be spelled any\nalternate way, but the code currently gives the user a commit instead.\nWhat's the right way to fix these \"impossible examples\"? I've given\nthree proposals and implemented the first of them:\n - ${POSSIBLY_VALID_REFNAME}-${INTEGER}-g${HASH}\n - ${POSSIBLY_VALID_REFNAME}-g${HASH}\n - ${ANYTHING_WITHOUT_A_COLON}-g${HASH}\n\nYou said you don't like the first two because check_refname() rules\nmight change, and not commented on the third.\n\nAlso, as far as I can tell, the set of \"possible examples\" you are\nfocusing on is currently the empty set. A change of syntax might in\nthe future expand that to a non-empty-set, and then bring us backward\ncompatibility headaches because we have been allowing\n\"${garbage}-g${hash}\" to mean a reference to ${hash} and we'd then\nhave to deal with it becoming ambiguous (and potentially also having\nno way to disambiguate those cases, similar to how if colon is allowed\nin garbage then we have no way to disambiguate paths). If we want to\nallow future object naming extensions, it seems like we should lock\ndown and rule out as many existing forms of known ${garbage} as we\ncan, but that'd push us towards the\n${POSSIBLY_VALID_REFNAME}-${INTEGER}-g${HASH} solution I implemented\nthat you don't seem to like. Is there a middle ground that you do\nlike?\n"},{"id":"509941","messageId":"xmqqmsg52q7l.fsf@gitster.g","threadId":"62719","inReplyTo":"13f68bebe90549ba19452f12abb6fea41c2517fb.1735949870.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] object-name: fix resolution of object names containing curly braces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-05T16:14:06Z","receivedAt":"2025-01-05T16:14:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> Given a branch name of 'foo{bar', commands like\n>\n> git cat-file -p foo{bar:README.md\n>\n> should succeed (assuming that branch had a README.md file, of course).\n> However, the change in cce91a2caef9 (Change 'master@noon' syntax to\n> 'master@{noon}'., 2006-05-19) presumed that curly braces would always\n> come after an '@' or '^' and be paired, causing e.g. 'foo{bar:README.md'\n> to entirely miss the ':' and assume there's no object being referenced.\n> In short, git would report:\n>\n> fatal: Not a valid object name foo{bar:README.md\n\nNaïvely, it seems that a solution is to parse from left to right,\ni.e., (1) notice there is a colon, (2) see if everything before that\ncolon resolves to a treeish, and (3) see if everything after it is a\npath that appears in the treeish.\n\n - When we are given foo@{some:thing}, if we did that, we realize\n that \"foo@{some\" is not a valid tree-ish object name (since \"@{\"\n cannot appear in a refname) and then can backtrack by realizing\n \"foo\" is a ref, and @{...} could be a reflog reference (most\n likely a way to spell some sort of timestamp), and try that.\n\n - Similarly, for foo:path-gaffed, we would notice \"foo\" is a valid\n tree-ish object name, and if path-gaffed is a path in it, we'd be\n happy. Or foo may not be a tree-ish, or path-gaffed may not\n exist in that tree-ish. In which case, we can backtrack and see\n foo:path-g is an allowed prefix in a desribe name.\n\nNow in the above description, I have assumed that an alternative\ninterpretation kicks in only as a fallback when we backtrack, but\nwe could make sure we try all possibilities and notice ambiguity if\nwe wanted to.\n\nIn any case, such an updated structure of the parsing code paths\n(whether alternative interpretations are treated as fallbacks or\nequally plausible candidates subject to disambiguation) would be\na vast departure from what we currently have, so a targeted \"fix\"\nlike these two patches attempt would be more appropriate as an\ninitial approach, I think.\n\nThanks, will queue, but probably we'd look at in any seriousness\nafter the 2.48 final gets tagged.\n\n\n"},{"id":"510003","messageId":"xmqqbjwjzw9b.fsf@gitster.g","threadId":"62719","inReplyTo":"pull.1844.v2.git.1735949870.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] object-name: fix resolution of object names containing curly braces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-06T17:29:20Z","receivedAt":"2025-01-06T17:29:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> Maintainer note: these bugs both date back to 2006; neither is a regression\n> in this cycle.\n\nWhile I was preparing today's -rc2 release, I noticed that this\nchange broke some of my release scripts with\n\n $ git rev-parse --verify v2.48.0-rc2-161-g6c2274cdbc^0\n fatal: Needed a single revision\n\nwhich is the construct that has been there almost forever. Its\nexpected output is\n\n $ git rev-parse --verify v2.48.0-rc2-161-g6c2274cdbc^0\n 6c2274cdbca14b7eb70fb182ffac80bf6950e137\n\nThe series seems to need a bit more work.\n\nThanks.\n"},{"id":"510012","messageId":"CABPp-BHyWgu47mCDH_f_MkS4CMGh_OtX4K-YGFsB-fZvvaNJPQ@mail.gmail.com","threadId":"62719","inReplyTo":"xmqqbjwjzw9b.fsf@gitster.g","subject":"Re: [PATCH v2 0/2] object-name: fix resolution of object names containing curly braces","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-01-06T19:26:48Z","receivedAt":"2025-01-06T19:26:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jan 6, 2025 at 9:29 AM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > Maintainer note: these bugs both date back to 2006; neither is a regression\n> > in this cycle.\n>\n> While I was preparing today's -rc2 release, I noticed that this\n> change broke some of my release scripts with\n>\n> $ git rev-parse --verify v2.48.0-rc2-161-g6c2274cdbc^0\n> fatal: Needed a single revision\n>\n> which is the construct that has been there almost forever. Its\n> expected output is\n>\n> $ git rev-parse --verify v2.48.0-rc2-161-g6c2274cdbc^0\n> 6c2274cdbca14b7eb70fb182ffac80bf6950e137\n>\n> The series seems to need a bit more work.\n\nGah, I made sure to copy the object name into a string buf, so I could\noperate on just the relevant part, but then ignored that and operated\non the full thing.\n\nThis fixes it:\n\ndiff --git a/object-name.c b/object-name.c\nindex 614520954c7..cb96a0e6161 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1318,7 +1318,7 @@ static int ref_and_count_parts_valid(const char\n*name, int len)\n len = cp - name;\n strbuf_init(&sb, len);\n strbuf_add(&sb, name, len);\n- ret = !check_refname_format(name, flags);\n+ ret = !check_refname_format(sb.buf, flags);\n strbuf_release(&sb);\n return ret;\n }\n\nI'll include it with all my other fixes in a reroll, which I'll\nprobably send out after 2.48 to avoid distracting from the release.\n"},{"id":"510016","messageId":"xmqq1pxfy8xk.fsf@gitster.g","threadId":"62719","inReplyTo":"CABPp-BHyWgu47mCDH_f_MkS4CMGh_OtX4K-YGFsB-fZvvaNJPQ@mail.gmail.com","subject":"Re: [PATCH v2 0/2] object-name: fix resolution of object names containing curly braces","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-06T20:38:31Z","receivedAt":"2025-01-06T20:38:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Mon, Jan 6, 2025 at 9:29 AM Junio C Hamano wrote:\n>>\n>> \"Elijah Newren via GitGitGadget\" writes:\n>>\n>> > Maintainer note: these bugs both date back to 2006; neither is a regression\n>> > in this cycle.\n>>\n>> While I was preparing today's -rc2 release, I noticed that this\n>> change broke some of my release scripts with\n>>\n>> $ git rev-parse --verify v2.48.0-rc2-161-g6c2274cdbc^0\n>> fatal: Needed a single revision\n>>\n>> which is the construct that has been there almost forever. Its\n>> expected output is\n>>\n>> $ git rev-parse --verify v2.48.0-rc2-161-g6c2274cdbc^0\n>> 6c2274cdbca14b7eb70fb182ffac80bf6950e137\n>>\n>> The series seems to need a bit more work.\n>\n> Gah, I made sure to copy the object name into a string buf, so I could\n> operate on just the relevant part, but then ignored that and operated\n> on the full thing.\n>\n> This fixes it:\n>\n> diff --git a/object-name.c b/object-name.c\n> index 614520954c7..cb96a0e6161 100644\n> --- a/object-name.c\n> +++ b/object-name.c\n> @@ -1318,7 +1318,7 @@ static int ref_and_count_parts_valid(const char\n> *name, int len)\n> len = cp - name;\n> strbuf_init(&sb, len);\n> strbuf_add(&sb, name, len);\n> - ret = !check_refname_format(name, flags);\n> + ret = !check_refname_format(sb.buf, flags);\n> strbuf_release(&sb);\n> return ret;\n> }\n>\n> I'll include it with all my other fixes in a reroll, which I'll\n> probably send out after 2.48 to avoid distracting from the release.\n\nIn existing tests, we seem to be lacking coverage to notice this\nbreakage, so let's make sure we add one or two.\n\nThanks.\n"},{"id":"510435","messageId":"pull.1844.v3.git.1736788417.gitgitgadget@gmail.com","threadId":"62719","inReplyTo":"pull.1844.v2.git.1735949870.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] object-name: fix a pair of object name resolution issues","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-01-13T17:13:35Z","receivedAt":"2025-01-13T17:13:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Changes since v2:\n\n * Readability improvement to the first patch, which fixes object name\n resolution with refs containing a curly brace\n * Fixed the second patch for cases like OBJ-COUNT-gHASH~13 and added a\n couple test cases for that. Also, extended the commit message a bit to\n discuss the cases brought up on the list.\n\nFor the second patch, if folks want some open source examples where it could\nbe triggered, I found two examples:\n\n * lore.git: git cat-file -t master:random/path/major-gaffed\n * git.git: git cat-file -t super-invalid~///\\\\.....@.lock-gfd0bba94e\n\nElijah Newren (2):\n object-name: fix resolution of object names containing curly braces\n object-name: be more strict in parsing describe-like output\n\n object-name.c | 63 ++++++++++++++++++++++++++++++++++++++++++---\n t/t1006-cat-file.sh | 31 +++++++++++++++++++++-\n t/t6120-describe.sh | 24 +++++++++++++++++\n 3 files changed, 113 insertions(+), 5 deletions(-)\n\n\nbase-commit: 063bcebf0c917140ca0e705cbe0fdea127e90086\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1844%2Fnewren%2Fobject-name-fix-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1844/newren/object-name-fix-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1844\n\nRange-diff vs v2:\n\n 1: 13f68bebe90 ! 1: b21b8fc5554 object-name: fix resolution of object names containing curly braces\n @@ Commit message\n * 'foo@@{...}'\n * 'foo^{/${SEARCH_TEXT_WITH_COLON}}:${PATH}'\n \n + Note that we'd prefer duplicating the special logic for \"@^\" characters\n + here, because if get_oid_basic() or interpret_nth_prior_checkout() or\n + get_oid_basic() or similar gain extra methods of using curly braces,\n + then the logic in get_oid_with_context_1() would need to be updated as\n + well. But it's not clear how to refactor all of these to have a simple\n + common callpoint with the specialized logic.\n +\n Reported-by: Gabriel Amaral \n Helped-by: Michael Haggerty \n Signed-off-by: Elijah Newren \n @@ object-name.c: static enum get_oid_result get_oid_with_context_1(struct reposito\n \t}\n \tfor (cp = name, bracket_depth = 0; *cp; cp++) {\n -\t\tif (*cp == '{')\n -+\t\tif (*(cp+1) == '{' && (*cp == '@' || *cp == '^')) {\n ++\t\tif (strchr(\"@^\", *cp) && cp[1] == '{') {\n +\t\t\tcp++;\n \t\t\tbracket_depth++;\n -\t\telse if (bracket_depth && *cp == '}')\n 2: 31f1c37b31a ! 2: 19f84dfc9cc object-name: be more strict in parsing describe-like output\n @@ Commit message\n 'g', and an abbreviated object name.\n which means that output of the format\n ${REFNAME}-${INTEGER}-g${HASH}\n - should parse to fully expand ${HASH}. This is fine. However, we\n + should parse to fully expanded ${HASH}. This is fine. However, we\n currently don't validate any of ${REFNAME}-${INTEGER}, we only parse\n -g${HASH} and assume the rest is valid. That is problematic, since it\n breaks things like\n \n git cat-file -p branchname:path/to/file/named/i-gaffed\n \n - which, when commit affed exists, will not return us information about a\n - file we are looking for but will instead tell us about commit affed.\n + which, when commit (or tree or blob) affed exists, will not return us\n + information about the file we are looking for but will instead\n + erroneously tell us about object affed.\n \n - Similarly, we should probably not treat\n - refs/tags/invalid/./../...../// ~^:/?*\\\\&[}/busted.lock-g049e0ef6\n - as a request for commit 050e0ef6 either.\n + A few additional notes:\n + - This is a slight backward incompatibility break, because we used\n + to allow ${GARBAGE}-g${HASH} as a way to spell ${HASH}. However,\n + a backward incompatible break is necessary, because there is no\n + other way for someone to be more specific and disambiguate that they\n + want the blob master:path/to/who-gabbed instead of the object abbed.\n + - There is a possibility that check_refname_format() rules change in\n + the future. However, we can only realistically loosen the rules\n + for what that function accepts rather than tighten. If we were to\n + tighten the rules, some real world repositories may already have\n + refnames that suddenly become unacceptable and we break those\n + repositories. As such, any describe-like syntax of the form\n + ${VALID_FOR_A_REFNAME}-${INTEGER}-g${HASH} that is valid with the\n + changes in this commit will remain valid in the future.\n + - The fact that check_refname_format() rules could loosen in the\n + future is probably also an important reason to make this change. If\n + the rules loosen, there might be additional cases within\n + ${GARBAGE}-g${HASH} that become ambiguous in the future. While\n + abbreviated hashes can be disambiguated by abbreviating less, it may\n + well be that these alternative object names have no way of being\n + disambiguated (much like pathnames cannot be). Accepting all random\n + ${GARBAGE} thus makes it difficult for us to allow future\n + extensions to object naming.\n \n - Tighten up the parsing to make sure ${REFNAME} and ${INTEGER} are\n - present and valid.\n + So, tighten up the parsing to make sure ${REFNAME} and ${INTEGER} are\n + present in the string, and would be considered a valid ref and\n + non-negative integer.\n +\n + Also, add a few tests for git describe using object names of the form\n + ${REVISION_NAME}${MODIFIERS}\n + since an early version of this patch failed on constructs like\n + git describe v2.48.0-rc2-161-g6c2274cdbc^0\n \n Reported-by: Gabriel Amaral \n Signed-off-by: Elijah Newren \n @@ object-name.c: static int peel_onion(struct repository *r, const char *name, int\n +\tlen = cp - name;\n +\tstrbuf_init(&sb, len);\n +\tstrbuf_add(&sb, name, len);\n -+\tret = !check_refname_format(name, flags);\n ++\tret = !check_refname_format(sb.buf, flags);\n +\tstrbuf_release(&sb);\n +\treturn ret;\n +}\n @@ object-name.c: static int get_describe_name(struct repository *r,\n \t\t\t\treturn get_short_oid(r,\n \n ## t/t6120-describe.sh ##\n +@@ t/t6120-describe.sh: check_describe R-2-gHASH HEAD^^\n + check_describe A-3-gHASH HEAD^^2\n + check_describe B HEAD^^2^\n + check_describe R-1-gHASH HEAD^^^\n ++check_describe R-1-gHASH R-1-g$(git rev-parse --short HEAD^^)~1\n + \n + check_describe c-7-gHASH --tags HEAD\n + check_describe c-6-gHASH --tags HEAD^\n + check_describe e-1-gHASH --tags HEAD^^\n + check_describe c-2-gHASH --tags HEAD^^2\n ++check_describe c-2-gHASH --tags c-2-g$(git rev-parse --short HEAD^^2)^0\n + check_describe B --tags HEAD^^2^\n + check_describe e --tags HEAD^^^\n + check_describe e --tags --exact-match HEAD^^^\n @@ t/t6120-describe.sh: test_expect_success '--exact-match does not show --always fallback' '\n \ttest_must_fail git describe --exact-match --always\n '\n\n-- \ngitgitgadget\n"},{"id":"510436","messageId":"b21b8fc555414789d835f454a0dce3bf685fdb4c.1736788417.git.gitgitgadget@gmail.com","threadId":"62719","inReplyTo":"pull.1844.v3.git.1736788417.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] object-name: fix resolution of object names containing curly braces","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-01-13T17:13:36Z","receivedAt":"2025-01-13T17:13:41Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nGiven a branch name of 'foo{bar', commands like\n\n git cat-file -p foo{bar:README.md\n\nshould succeed (assuming that branch had a README.md file, of course).\nHowever, the change in cce91a2caef9 (Change 'master@noon' syntax to\n'master@{noon}'., 2006-05-19) presumed that curly braces would always\ncome after an '@' or '^' and be paired, causing e.g. 'foo{bar:README.md'\nto entirely miss the ':' and assume there's no object being referenced.\nIn short, git would report:\n\n fatal: Not a valid object name foo{bar:README.md\n\nChange the parsing to only make the assumption of paired curly braces\nimmediately after either a '@' or '^' character appears.\n\nAdd tests for this, as well as for a few other test cases that initial\nversions of this patch broke:\n * 'foo@@{...}'\n * 'foo^{/${SEARCH_TEXT_WITH_COLON}}:${PATH}'\n\nNote that we'd prefer duplicating the special logic for \"@^\" characters\nhere, because if get_oid_basic() or interpret_nth_prior_checkout() or\nget_oid_basic() or similar gain extra methods of using curly braces,\nthen the logic in get_oid_with_context_1() would need to be updated as\nwell. But it's not clear how to refactor all of these to have a simple\ncommon callpoint with the specialized logic.\n\nReported-by: Gabriel Amaral \nHelped-by: Michael Haggerty \nSigned-off-by: Elijah Newren \n---\n object-name.c | 8 +++++---\n t/t1006-cat-file.sh | 31 ++++++++++++++++++++++++++++++-\n 2 files changed, 35 insertions(+), 4 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex a563635a8cb..8e80841acd3 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -2051,12 +2051,14 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n \t\treturn -1;\n \t}\n \tfor (cp = name, bracket_depth = 0; *cp; cp++) {\n-\t\tif (*cp == '{')\n+\t\tif (strchr(\"@^\", *cp) && cp[1] == '{') {\n+\t\t\tcp++;\n \t\t\tbracket_depth++;\n-\t\telse if (bracket_depth && *cp == '}')\n+\t\t} else if (bracket_depth && *cp == '}') {\n \t\t\tbracket_depth--;\n-\t\telse if (!bracket_depth && *cp == ':')\n+\t\t} else if (!bracket_depth && *cp == ':') {\n \t\t\tbreak;\n+\t\t}\n \t}\n \tif (*cp == ':') {\n \t\tstruct object_id tree_oid;\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex ff9bf213aa2..398865d6ebe 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -240,7 +240,8 @@ test_expect_success \"setup\" '\n \tgit config extensions.objectformat $test_hash_algo &&\n \tgit config extensions.compatobjectformat $test_compat_hash_algo &&\n \techo_without_newline \"$hello_content\" > hello &&\n-\tgit update-index --add hello\n+\tgit update-index --add hello &&\n+\tgit commit -m \"add hello file\"\n '\n \n run_blob_tests () {\n@@ -602,6 +603,34 @@ test_expect_success FUNNYNAMES '--batch-check, -Z with newline in input' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup with curly braches in input' '\n+\tgit branch \"foo{bar\" HEAD &&\n+\tgit branch \"foo@\" HEAD\n+'\n+\n+test_expect_success 'object reference with curly brace' '\n+\tgit cat-file -p \"foo{bar:hello\" >actual &&\n+\tgit cat-file -p HEAD:hello >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'object reference with at-sign' '\n+\tgit cat-file -p \"foo@@{0}:hello\" >actual &&\n+\tgit cat-file -p HEAD:hello >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'setup with commit with colon' '\n+\tgit commit-tree -m \"testing: just a bunch of junk\" HEAD^{tree} >out &&\n+\tgit branch other $(cat out)\n+'\n+\n+test_expect_success 'object reference via commit text search' '\n+\tgit cat-file -p \"other^{/testing:}:hello\" >actual &&\n+\tgit cat-file -p HEAD:hello >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'setup blobs which are likely to delta' '\n \ttest-tool genrandom foo 10240 >foo &&\n \t{ cat foo && echo plus; } >foo-plus &&\n-- \ngitgitgadget\n\n"},{"id":"510437","messageId":"19f84dfc9cc33616f3720d740c449b29de8d492e.1736788417.git.gitgitgadget@gmail.com","threadId":"62719","inReplyTo":"pull.1844.v3.git.1736788417.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] object-name: be more strict in parsing describe-like output","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-01-13T17:13:37Z","receivedAt":"2025-01-13T17:13:42Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nFrom Documentation/revisions.txt:\n '', e.g. 'v1.7.4.2-679-g3bee7fb'::\n Output from `git describe`; i.e. a closest tag, optionally\n followed by a dash and a number of commits, followed by a dash, a\n 'g', and an abbreviated object name.\nwhich means that output of the format\n ${REFNAME}-${INTEGER}-g${HASH}\nshould parse to fully expanded ${HASH}. This is fine. However, we\ncurrently don't validate any of ${REFNAME}-${INTEGER}, we only parse\n-g${HASH} and assume the rest is valid. That is problematic, since it\nbreaks things like\n\n git cat-file -p branchname:path/to/file/named/i-gaffed\n\nwhich, when commit (or tree or blob) affed exists, will not return us\ninformation about the file we are looking for but will instead\nerroneously tell us about object affed.\n\nA few additional notes:\n - This is a slight backward incompatibility break, because we used\n to allow ${GARBAGE}-g${HASH} as a way to spell ${HASH}. However,\n a backward incompatible break is necessary, because there is no\n other way for someone to be more specific and disambiguate that they\n want the blob master:path/to/who-gabbed instead of the object abbed.\n - There is a possibility that check_refname_format() rules change in\n the future. However, we can only realistically loosen the rules\n for what that function accepts rather than tighten. If we were to\n tighten the rules, some real world repositories may already have\n refnames that suddenly become unacceptable and we break those\n repositories. As such, any describe-like syntax of the form\n ${VALID_FOR_A_REFNAME}-${INTEGER}-g${HASH} that is valid with the\n changes in this commit will remain valid in the future.\n - The fact that check_refname_format() rules could loosen in the\n future is probably also an important reason to make this change. If\n the rules loosen, there might be additional cases within\n ${GARBAGE}-g${HASH} that become ambiguous in the future. While\n abbreviated hashes can be disambiguated by abbreviating less, it may\n well be that these alternative object names have no way of being\n disambiguated (much like pathnames cannot be). Accepting all random\n ${GARBAGE} thus makes it difficult for us to allow future\n extensions to object naming.\n\nSo, tighten up the parsing to make sure ${REFNAME} and ${INTEGER} are\npresent in the string, and would be considered a valid ref and\nnon-negative integer.\n\nAlso, add a few tests for git describe using object names of the form\n ${REVISION_NAME}${MODIFIERS}\nsince an early version of this patch failed on constructs like\n git describe v2.48.0-rc2-161-g6c2274cdbc^0\n\nReported-by: Gabriel Amaral \nSigned-off-by: Elijah Newren \n---\n object-name.c | 55 ++++++++++++++++++++++++++++++++++++++++++++-\n t/t6120-describe.sh | 24 ++++++++++++++++++++\n 2 files changed, 78 insertions(+), 1 deletion(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex 8e80841acd3..cb96a0e6161 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1271,6 +1271,58 @@ static int peel_onion(struct repository *r, const char *name, int len,\n \treturn 0;\n }\n \n+/*\n+ * Documentation/revisions.txt says:\n+ * '', e.g. 'v1.7.4.2-679-g3bee7fb'::\n+ * Output from `git describe`; i.e. a closest tag, optionally\n+ * followed by a dash and a number of commits, followed by a dash, a\n+ * 'g', and an abbreviated object name.\n+ *\n+ * which means that the stuff before '-g${HASH}' needs to be a valid\n+ * refname, a dash, and a non-negative integer. This function verifies\n+ * that.\n+ *\n+ * In particular, we do not want to treat\n+ * branchname:path/to/file/named/i-gaffed\n+ * as a request for commit affed.\n+ *\n+ * More generally, we should probably not treat\n+ * 'refs/heads/./../.../ ~^:/?*[////\\\\\\&}/busted.lock-g050e0ef6ead'\n+ * as a request for object 050e0ef6ead either.\n+ *\n+ * We are called with name[len] == '-' and name[len+1] == 'g', i.e.\n+ * we are verifying ${REFNAME}-{INTEGER} part of the name.\n+ */\n+static int ref_and_count_parts_valid(const char *name, int len)\n+{\n+\tstruct strbuf sb;\n+\tconst char *cp;\n+\tint flags = REFNAME_ALLOW_ONELEVEL;\n+\tint ret = 1;\n+\n+\t/* Ensure we have at least one digit */\n+\tif (!isxdigit(name[len-1]))\n+\t\treturn 0;\n+\n+\t/* Skip over digits backwards until we get to the dash */\n+\tfor (cp = name + len - 2; name < cp; cp--) {\n+\t\tif (*cp == '-')\n+\t\t\tbreak;\n+\t\tif (!isxdigit(*cp))\n+\t\t\treturn 0;\n+\t}\n+\t/* Ensure we found the leading dash */\n+\tif (*cp != '-')\n+\t\treturn 0;\n+\n+\tlen = cp - name;\n+\tstrbuf_init(&sb, len);\n+\tstrbuf_add(&sb, name, len);\n+\tret = !check_refname_format(sb.buf, flags);\n+\tstrbuf_release(&sb);\n+\treturn ret;\n+}\n+\n static int get_describe_name(struct repository *r,\n \t\t\t const char *name, int len,\n \t\t\t struct object_id *oid)\n@@ -1284,7 +1336,8 @@ static int get_describe_name(struct repository *r,\n \t\t\t/* We must be looking at g in \"SOMETHING-g\"\n \t\t\t * for it to be describe output.\n \t\t\t */\n-\t\t\tif (ch == 'g' && cp[-1] == '-') {\n+\t\t\tif (ch == 'g' && cp[-1] == '-' &&\n+\t\t\t ref_and_count_parts_valid(name, cp - 1 - name)) {\n \t\t\t\tcp++;\n \t\t\t\tlen -= cp - name;\n \t\t\t\treturn get_short_oid(r,\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 3f6160d702b..76843a61691 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -82,11 +82,13 @@ check_describe R-2-gHASH HEAD^^\n check_describe A-3-gHASH HEAD^^2\n check_describe B HEAD^^2^\n check_describe R-1-gHASH HEAD^^^\n+check_describe R-1-gHASH R-1-g$(git rev-parse --short HEAD^^)~1\n \n check_describe c-7-gHASH --tags HEAD\n check_describe c-6-gHASH --tags HEAD^\n check_describe e-1-gHASH --tags HEAD^^\n check_describe c-2-gHASH --tags HEAD^^2\n+check_describe c-2-gHASH --tags c-2-g$(git rev-parse --short HEAD^^2)^0\n check_describe B --tags HEAD^^2^\n check_describe e --tags HEAD^^^\n check_describe e --tags --exact-match HEAD^^^\n@@ -725,4 +727,26 @@ test_expect_success '--exact-match does not show --always fallback' '\n \ttest_must_fail git describe --exact-match --always\n '\n \n+test_expect_success 'avoid being fooled by describe-like filename' '\n+\ttest_when_finished rm out &&\n+\n+\tgit rev-parse --short HEAD >out &&\n+\tFILENAME=filename-g$(cat out) &&\n+\ttouch $FILENAME &&\n+\tgit add $FILENAME &&\n+\tgit commit -m \"Add $FILENAME\" &&\n+\n+\tgit cat-file -t HEAD:$FILENAME >actual &&\n+\n+\techo blob >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'do not be fooled by invalid describe format ' '\n+\ttest_when_finished rm out &&\n+\n+\tgit rev-parse --short HEAD >out &&\n+\ttest_must_fail git cat-file -t \"refs/tags/super-invalid/./../...../ ~^:/?*[////\\\\\\\\\\\\&}/busted.lock-42-g\"$(cat out)\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"510446","messageId":"xmqqjzay8trm.fsf@gitster.g","threadId":"62719","inReplyTo":"pull.1844.v3.git.1736788417.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/2] object-name: fix a pair of object name resolution issues","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-13T18:15:41Z","receivedAt":"2025-01-13T18:15:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> Changes since v2:\n>\n> * Readability improvement to the first patch, which fixes object name\n> resolution with refs containing a curly brace\n> * Fixed the second patch for cases like OBJ-COUNT-gHASH~13 and added a\n> couple test cases for that. Also, extended the commit message a bit to\n> discuss the cases brought up on the list.\n>\n> For the second patch, if folks want some open source examples where it could\n> be triggered, I found two examples:\n>\n> * lore.git: git cat-file -t master:random/path/major-gaffed\n> * git.git: git cat-file -t super-invalid~///\\\\.....@.lock-gfd0bba94e\n>\n> Elijah Newren (2):\n> object-name: fix resolution of object names containing curly braces\n> object-name: be more strict in parsing describe-like output\n>\n> object-name.c | 63 ++++++++++++++++++++++++++++++++++++++++++---\n> t/t1006-cat-file.sh | 31 +++++++++++++++++++++-\n> t/t6120-describe.sh | 24 +++++++++++++++++\n> 3 files changed, 113 insertions(+), 5 deletions(-)\n>\n\nAlthough ...\n\n> + Note that we'd prefer duplicating the special logic for \"@^\" characters\n> + here, because if get_oid_basic() or interpret_nth_prior_checkout() or\n\n... I suspect that you meant \"we'd prefer not duplicating\" here,\nboth patches look very good to me.\n\nThanks, will replace.\n"},{"id":"510448","messageId":"CABPp-BG=Yu8DRb_xPOE0PC8HPRSBzYn7FW_8rN=rRsg4t_hE2Q@mail.gmail.com","threadId":"62719","inReplyTo":"xmqqjzay8trm.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] object-name: fix a pair of object name resolution issues","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-01-13T19:26:42Z","receivedAt":"2025-01-13T19:26:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jan 13, 2025 at 10:15 AM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > Changes since v2:\n> >\n> > * Readability improvement to the first patch, which fixes object name\n> > resolution with refs containing a curly brace\n> > * Fixed the second patch for cases like OBJ-COUNT-gHASH~13 and added a\n> > couple test cases for that. Also, extended the commit message a bit to\n> > discuss the cases brought up on the list.\n> >\n> > For the second patch, if folks want some open source examples where it could\n> > be triggered, I found two examples:\n> >\n> > * lore.git: git cat-file -t master:random/path/major-gaffed\n> > * git.git: git cat-file -t super-invalid~///\\\\.....@.lock-gfd0bba94e\n> >\n> > Elijah Newren (2):\n> > object-name: fix resolution of object names containing curly braces\n> > object-name: be more strict in parsing describe-like output\n> >\n> > object-name.c | 63 ++++++++++++++++++++++++++++++++++++++++++---\n> > t/t1006-cat-file.sh | 31 +++++++++++++++++++++-\n> > t/t6120-describe.sh | 24 +++++++++++++++++\n> > 3 files changed, 113 insertions(+), 5 deletions(-)\n> >\n>\n> Although ...\n>\n> > + Note that we'd prefer duplicating the special logic for \"@^\" characters\n> > + here, because if get_oid_basic() or interpret_nth_prior_checkout() or\n>\n> ... I suspect that you meant \"we'd prefer not duplicating\" here,\n> both patches look very good to me.\n\nOops, indeed.\n\n> Thanks, will replace.\n\nThanks.\n"}]}