{"thread":{"id":"62608","subject":"[PATCH] object-name: fix reversed ordering with magic pathspecs","startedAt":"2024-12-06T09:51:35Z","lastAt":"2024-12-09T11:53:09Z","messageCount":16,"participants":["Patrick Steinhardt","Kristoffer Haugsbakk","Junio C Hamano","Justin Tobler","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"508716","messageId":"20241206-pks-rev-parse-fix-reversed-list-v1-1-95a96564a4d7@pks.im","threadId":"62608","inReplyTo":null,"subject":"[PATCH] object-name: fix reversed ordering with magic pathspecs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-06T09:51:05Z","receivedAt":"2024-12-06T09:51:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It was reported that magic pathspecs started to return results in\nreverse recency order starting with Git v2.47.0. This behaviour bisects\nto 57fb139b5e (object-name: fix leaking commit list items, 2024-08-01),\nwhich has refactored `get_oid_oneline()` to plug a couple of memory\nleaks.\n\nAs part of that refactoring we have started to create a copy of the\npassed-in list of commits and work on that list instead. The intent of\nthis was to stop modifying the caller-provided commit list such that the\ncaller can properly free all commit list items, including those that we\nmight potentially drop.\n\nWe already knew to create such a copy beforehand with the `backup` list,\nwhich was used to clear the `ONELINE_SEEN` commit mark after we were\ndone. So the refactoring simply renamed that list to `copy` and started\nto operate on that list instead. There is a gotcha though: the backup\nlist, and thus now also the copied list, is always being prepended to,\nso the resulting list is in reverse order! The end result is that we\npop commits from the wrong end of the commit list, returning commits in\nreverse recency order.\n\nFix the bug by appending to the list instead.\n\nReported-by: Aarni Koskela \nSigned-off-by: Patrick Steinhardt \n---\nThis patch applies on top of v2.47.0, which is the first version which\nhad this regression.\n---\n object-name.c | 4 ++--\n t/t4208-log-magic-pathspec.sh | 13 +++++++++++++\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex c892fbe80aa7173dfcc1995de5a75bc322c6adb7..34433d2a01d6a23ce6b4ca19b85c53b7b82fd0e5 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1401,7 +1401,7 @@ static int get_oid_oneline(struct repository *r,\n \t\t\t const char *prefix, struct object_id *oid,\n \t\t\t const struct commit_list *list)\n {\n-\tstruct commit_list *copy = NULL;\n+\tstruct commit_list *copy = NULL, **copy_tail = ©\n \tconst struct commit_list *l;\n \tint found = 0;\n \tint negative = 0;\n@@ -1423,7 +1423,7 @@ static int get_oid_oneline(struct repository *r,\n \n \tfor (l = list; l; l = l->next) {\n \t\tl->item->object.flags |= ONELINE_SEEN;\n-\t\tcommit_list_insert(l->item, ©);\n+\t\tcopy_tail = &commit_list_insert(l->item, copy_tail)->next;\n \t}\n \twhile (copy) {\n \t\tconst char *p, *buf;\ndiff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh\nindex 2a46eb6bedbe283a08fd77917b7fb17da155b027..2d80b9044bcf9ec8e2f11b6afd2b0fe8e2fcabfd 100755\n--- a/t/t4208-log-magic-pathspec.sh\n+++ b/t/t4208-log-magic-pathspec.sh\n@@ -58,6 +58,19 @@ test_expect_success '\"git log :/any/path/\" should not segfault' '\n \ttest_must_fail git log :/any/path/\n '\n \n+test_expect_success ':/ favors more recent matching commits' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit common-old &&\n+\t\ttest_commit --no-tag common-new &&\n+\t\tgit rev-parse HEAD >expect &&\n+\t\tgit rev-parse :/common >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n # This differs from the \":/a\" check above in that :/in looks like a pathspec,\n # but doesn't match an actual file.\n test_expect_success '\"git log :/in\" should not be ambiguous' '\n\n---\nbase-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\nchange-id: 20241206-pks-rev-parse-fix-reversed-list-0f94a20a6165\n\n"},{"id":"508740","messageId":"9c7b9090-8cf2-4095-8e86-2c85b6fe407f@app.fastmail.com","threadId":"62608","inReplyTo":"20241206-pks-rev-parse-fix-reversed-list-v1-1-95a96564a4d7@pks.im","subject":"Re: [PATCH] object-name: fix reversed ordering with magic pathspecs","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-12-06T11:20:45Z","receivedAt":"2024-12-06T11:21:07Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"Hi\n\nOn Fri, Dec 6, 2024, at 10:51, Patrick Steinhardt wrote:\n> It was reported\n\nIt would be nice with a hyperlink since this email is not part of the\nemail thread for the report.\n\nhttps://lore.kernel.org/git/Z1LJSADiStlFicTL@pks.im/T/#mae906ec74d5657e702165e29b90927f730280c29\n\n> It was reported that magic pathspecs started to return results in\n\nI’m not used to this being called “magic pathspecs” as a user.\ngitglossary(7) talks about (magic) pathspecs as filepaths.\n\n(c.f. gitrevisions(7) where this revision selection syntax\nis discussed.)\n\n> reverse recency order starting with Git v2.47.0. This behaviour bisects\n> to 57fb139b5e (object-name: fix leaking commit list items, 2024-08-01),\n\nNit: Can it be simply be asserted that it is caused by that commit?\nBecause that would make for a simpler/more assertive narrative.\n\n“This bisects to” can be a nice phrase when it is followed by a “but”\nsubclause,[1] i.e. when the straightforward troubleshooting can turn up\nmisleading leads.\n\n† 1: 615d2de3b45 (config.c: avoid segfault with --fixed-value and valueless\n config, 2024-08-01)\n\n> which has refactored `get_oid_oneline()` to plug a couple of memory\n> leaks.\n>\n> As part of that refactoring we have started to create a copy of the\n> passed-in list of commits and work on that list instead. The intent of\n> this was to stop modifying the caller-provided commit list such that the\n\nNit: s/The intent of this was/The intent was/\n\n> caller can properly free all commit list items, including those that we\n> might potentially drop.\n>\n> We already knew to create such a copy beforehand with the `backup` list,\n> which was used to clear the `ONELINE_SEEN` commit mark after we were\n> done. So the refactoring simply renamed that list to `copy` and started\n> to operate on that list instead. There is a gotcha though: the backup\n> list, and thus now also the copied list, is always being prepended to,\n> so the resulting list is in reverse order! The end result is that we\n> pop commits from the wrong end of the commit list, returning commits in\n> reverse recency order.\n\nNice explanation.\n\n>\n> Fix the bug by appending to the list instead.\n>\n> Reported-by: Aarni Koskela \n> Signed-off-by: Patrick Steinhardt \n> [snip]\n> diff --git a/t/t4208-log-magic-pathspec.sh\n> b/t/t4208-log-magic-pathspec.sh\n\nYes, so here is that magic pathspec name. But this test file has a lot\nof tests that test positional argument ambiguity. Which seems very\nrelevant to pathspecs in particular. And revision selection syntax\nseems to be used to test how things are interpreted. Not really how\nthings are ultimately processed (that seems secondary).\n\nThe tests involving `:/` in particular seem to only be about\nambiguity testing.\n\nIs this the correct test file?\n\n> index\n> 2a46eb6bedbe283a08fd77917b7fb17da155b027..2d80b9044bcf9ec8e2f11b6afd2b0fe8e2fcabfd\n> 100755\n> --- a/t/t4208-log-magic-pathspec.sh\n> +++ b/t/t4208-log-magic-pathspec.sh\n> @@ -58,6 +58,19 @@ test_expect_success '\"git log :/any/path/\" should\n> not segfault' '\n> \ttest_must_fail git log :/any/path/\n> '\n>\n> +test_expect_success ':/ favors more recent matching commits' '\n\nThis wasn’t mentioned in the report but `HEAD^{/}` is a similar syntax.\nThat one is more controllable since you provide a ref yourself\n(`:/` returns the youngest commit from any ref).\n\nI have indeed noticed that `HEAD^{/}` returns a sensible thing while\n`:/` does something strange like finding the root commit. (Then I\nshrugged and half-assumed that I hadn’t read some fine print)\n\ngitrevisions(7) calls out the relation between these two. It could be\nnice for a regression test to assert that these two syntaxes return the\nsame commit. I.e. when you have just made a commit, `:/` and\n`HEAD^{/}` return the same commit, and that commit is\nthe youngest.\n\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_commit common-old &&\n> +\t\ttest_commit --no-tag common-new &&\n> +\t\tgit rev-parse HEAD >expect &&\n> +\t\tgit rev-parse :/common >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> # This differs from the \":/a\" check above in that :/in looks like a pathspec,\n> # but doesn't match an actual file.\n> test_expect_success '\"git log :/in\" should not be ambiguous' '\n>\n> ---\n> base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\n> change-id: 20241206-pks-rev-parse-fix-reversed-list-0f94a20a6165\n"},{"id":"508748","messageId":"xmqqcyi5vxbj.fsf@gitster.g","threadId":"62608","inReplyTo":"9c7b9090-8cf2-4095-8e86-2c85b6fe407f@app.fastmail.com","subject":"Re: [PATCH] object-name: fix reversed ordering with magic pathspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-06T11:57:04Z","receivedAt":"2024-12-06T11:57:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> On Fri, Dec 6, 2024, at 10:51, Patrick Steinhardt wrote:\n>> It was reported\n>\n> It would be nice with a hyperlink since this email is not part of the\n> email thread for the report.\n>\n> https://lore.kernel.org/git/Z1LJSADiStlFicTL@pks.im/T/#mae906ec74d5657e702165e29b90927f730280c29\n>\n>> It was reported that magic pathspecs started to return results in\n>\n> I’m not used to this being called “magic pathspecs” as a user.\n> gitglossary(7) talks about (magic) pathspecs as filepaths.\n\nThanks for catching the mistaken phrasing. It would have caused\nunnecessary confusion later to \"git log\" readers.\n\nThe syntax to say that the following path is from the top-level of\nthe working tree even when you are running the command from a\nsubdirectory, e.g.\n\n cd Documentation\n git log :/t\n\nis described in gitglossary(7):\n\n A pathspec that begins with a colon `:` has special meaning. In the\n short form, the leading colon `:` is followed by zero or more \"magic\n signature\" letters (which optionally is terminated by another colon `:`),\n and the remainder is the pattern to match against the path.\n The \"magic signature\" consists of ASCII symbols that are neither\n alphanumeric, glob, regex special characters nor colon.\n The optional colon that terminates the \"magic signature\" can be\n omitted if the pattern begins with a character that does not belong to\n \"magic signature\" symbol set and is not a colon.\n\nand even though the word \"magic\" signature is used, there is no\ndefinition given for the entire construct, i.e. the pathspec that\nuses a special meaning introduced by the use of \"colon followed by\none or more magic signature letters\". We probably should add a\nsentence to officially define it, if only to reduce the need for\nexchanges like this ;-)\n\n ... and is not a colon. Such a pathspec that uses these \"magic\"\n meaning is called \"magic pathspec\".\n\nBut more importantly, the syntax that triggered this topic in\n\n\n\nis *NOT* a magic pathspec at all. It is a revision syntax to name\nthe first commit that is reachable from the current HEAD with a\ncommit log message, that matches the given patterh, i.e.\n\n git show \":/my message\"\n\nwhich is a (rather lousy) short-hand for a more general\n\n git show \"HEAD^{/my message}\"\n\ni.e. ^{/}. There is no specific name for\nthis syntax.\n\nI suspect that \"filepath\" you mentioned may be something some folks\n(but not me or any other long timers) would call yet another syntax,\nwhich is :, that names the object sitting at in the\nindex. Because \":/myMessage\" look so similar to \":myFile\", yet\nthey mean so different things, as I said, \":/myMessage\" is a rather\nlousy short-hand for the more general \"^{/}\" suffix that\nis less ambiguous.\n\nPatrick, let's not use a wrong word. This is not about pathspec at\nall, and is a revision syntax. As there is no specific jargon for\nthe syntax, here is what I would write, if I were explaining the\nproblem being solved:\n\n Recently it was reported [*1*] that \"look for the youngest\n reachable commit with log message that match the given pattern\"\n syntax (e.g. :/myMessagePattern, or HEAD^{/myMessagePattern})\n started to show older commit.\n\n [Footnote]\n *1* \n\nThanks.\n"},{"id":"508751","messageId":"Z1LohIVUyAOJvjW7@pks.im","threadId":"62608","inReplyTo":"xmqqcyi5vxbj.fsf@gitster.g","subject":"Re: [PATCH] object-name: fix reversed ordering with magic pathspecs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-06T12:05:24Z","receivedAt":"2024-12-06T12:05:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Dec 06, 2024 at 08:57:04PM +0900, Junio C Hamano wrote:\n> \"Kristoffer Haugsbakk\" writes:\n> \n> > On Fri, Dec 6, 2024, at 10:51, Patrick Steinhardt wrote:\n> >> It was reported\n> >\n> > It would be nice with a hyperlink since this email is not part of the\n> > email thread for the report.\n> >\n> > https://lore.kernel.org/git/Z1LJSADiStlFicTL@pks.im/T/#mae906ec74d5657e702165e29b90927f730280c29\n> >\n> >> It was reported that magic pathspecs started to return results in\n> >\n> > I’m not used to this being called “magic pathspecs” as a user.\n> > gitglossary(7) talks about (magic) pathspecs as filepaths.\n> \n> Thanks for catching the mistaken phrasing. It would have caused\n> unnecessary confusion later to \"git log\" readers.\n> \n> The syntax to say that the following path is from the top-level of\n> the working tree even when you are running the command from a\n> subdirectory, e.g.\n> \n> cd Documentation\n> git log :/t\n> \n> is described in gitglossary(7):\n> \n> A pathspec that begins with a colon `:` has special meaning. In the\n> short form, the leading colon `:` is followed by zero or more \"magic\n> signature\" letters (which optionally is terminated by another colon `:`),\n> and the remainder is the pattern to match against the path.\n> The \"magic signature\" consists of ASCII symbols that are neither\n> alphanumeric, glob, regex special characters nor colon.\n> The optional colon that terminates the \"magic signature\" can be\n> omitted if the pattern begins with a character that does not belong to\n> \"magic signature\" symbol set and is not a colon.\n> \n> and even though the word \"magic\" signature is used, there is no\n> definition given for the entire construct, i.e. the pathspec that\n> uses a special meaning introduced by the use of \"colon followed by\n> one or more magic signature letters\". We probably should add a\n> sentence to officially define it, if only to reduce the need for\n> exchanges like this ;-)\n> \n> ... and is not a colon. Such a pathspec that uses these \"magic\"\n> meaning is called \"magic pathspec\".\n> \n> But more importantly, the syntax that triggered this topic in\n> \n> \n> \n> is *NOT* a magic pathspec at all. It is a revision syntax to name\n> the first commit that is reachable from the current HEAD with a\n> commit log message, that matches the given patterh, i.e.\n> \n> git show \":/my message\"\n> \n> which is a (rather lousy) short-hand for a more general\n> \n> git show \"HEAD^{/my message}\"\n> \n> i.e. ^{/}. There is no specific name for\n> this syntax.\n\nHm, yeah. I think I read pathspec somewhere and just went with it\nwithout thinking.\n\n> I suspect that \"filepath\" you mentioned may be something some folks\n> (but not me or any other long timers) would call yet another syntax,\n> which is :, that names the object sitting at in the\n> index. Because \":/myMessage\" look so similar to \":myFile\", yet\n> they mean so different things, as I said, \":/myMessage\" is a rather\n> lousy short-hand for the more general \"^{/}\" suffix that\n> is less ambiguous.\n> \n> Patrick, let's not use a wrong word. This is not about pathspec at\n> all, and is a revision syntax. As there is no specific jargon for\n> the syntax, here is what I would write, if I were explaining the\n> problem being solved:\n> \n> Recently it was reported [*1*] that \"look for the youngest\n> reachable commit with log message that match the given pattern\"\n> syntax (e.g. :/myMessagePattern, or HEAD^{/myMessagePattern})\n> started to show older commit.\n> \n> [Footnote]\n> *1* \n\nWill update in v2. Thanks!\n\nPatrick\n"},{"id":"508752","messageId":"cfc7adaa-ccf3-4f27-807c-2eed70e56aa8@app.fastmail.com","threadId":"62608","inReplyTo":"xmqqcyi5vxbj.fsf@gitster.g","subject":"Re: [PATCH] object-name: fix reversed ordering with magic pathspecs","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-12-06T12:25:49Z","receivedAt":"2024-12-06T12:26:11Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Fri, Dec 6, 2024, at 12:57, Junio C Hamano wrote:\n> [snip]\n> Recently it was reported [*1*] that \"look for the youngest\n> reachable commit with log message that match the given pattern\"\n> syntax (e.g. :/myMessagePattern, or HEAD^{/myMessagePattern})\n\nBut these are slightly different since `:/` searches in all refs.\nThey are only equivalent (i.e. with Patrick’s fix) when you have\njust made a commit (like in Patrick’s test).\n\nAnd the longer syntax doesn’t seem to be affected by any regressions.\nIn this repo:\n\n $ # wrong\n $ git log --format=reference -1 :/Merge\n 3857284f7b8 (Merge refs/heads/master from ., 2005-08-24)\n $ # correct (looks correct)\n $ git log --format=reference -1 HEAD^{/Merge}\n 4a611ee7ebd (Merge branch 'kn/ref-transaction-hook-with-reflog', 2024-11-27)\n\n> started to show older commit.\n"},{"id":"508753","messageId":"Z1LtS-8f8WZyobz3@pks.im","threadId":"62608","inReplyTo":"9c7b9090-8cf2-4095-8e86-2c85b6fe407f@app.fastmail.com","subject":"Re: [PATCH] object-name: fix reversed ordering with magic pathspecs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-06T12:25:54Z","receivedAt":"2024-12-06T12:26:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Dec 06, 2024 at 12:20:45PM +0100, Kristoffer Haugsbakk wrote:\n> > diff --git a/t/t4208-log-magic-pathspec.sh\n> > b/t/t4208-log-magic-pathspec.sh\n> \n> Yes, so here is that magic pathspec name. But this test file has a lot\n> of tests that test positional argument ambiguity. Which seems very\n> relevant to pathspecs in particular. And revision selection syntax\n> seems to be used to test how things are interpreted. Not really how\n> things are ultimately processed (that seems secondary).\n> \n> The tests involving `:/` in particular seem to only be about\n> ambiguity testing.\n> \n> Is this the correct test file?\n\nProbably not. I couldn't really find any cases where we explicitly\nverify this syntax, which explains why the regression was not found.\nI've added it to t1500 now, which is at least better than t4208.\n\nThanks!\n\nPatrick\n"},{"id":"508754","messageId":"20241206-pks-rev-parse-fix-reversed-list-v2-1-190514278ead@pks.im","threadId":"62608","inReplyTo":"20241206-pks-rev-parse-fix-reversed-list-v1-1-95a96564a4d7@pks.im","subject":"[PATCH v2] object-name: fix reversed ordering with \":/\" revisions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-06T12:28:54Z","receivedAt":"2024-12-06T12:29:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Recently it was reported [1] that \"look for the youngest reachable\ncommit with log message that match the given pattern\" syntax (e.g.\n':/' or 'HEAD^{/}') started to return results in\nreverse recency order. This regression was introduced in Git v2.47.0 and\nis caused by a memory leak fix done in 57fb139b5e (object-name: fix\nleaking commit list items, 2024-08-01).\n\nThe intent of the identified commit is to stop modifying the commit list\nprovided by the caller such that the caller can properly free all commit\nlist items, including those that the called function might potentially\nremove from the list. This was done by creating a copy of the passed-in\ncommit list and modifying this copy instead of the caller-provided list.\n\nWe already knew to create such a copy beforehand with the `backup` list,\nwhich was used to clear the `ONELINE_SEEN` commit mark after we were\ndone. So the refactoring simply renamed that list to `copy` and started\nto operate on that list instead. There is a gotcha though: the backup\nlist, and thus now also the copied list, is always being prepended to,\nso the resulting list is in reverse order! The end result is that we\npop commits from the wrong end of the commit list, returning commits in\nreverse recency order.\n\nFix the bug by appending to the list instead.\n\n[1]: \n\nReported-by: Aarni Koskela \nSigned-off-by: Patrick Steinhardt \n---\nThis patch applies on top of v2.47.0, which is the first version which\nhad this regression.\n\nChanges in v2:\n\n - Include the message ID of the report in the commit message.\n\n - Fix terminology used by the commit message.\n\n - Move the test from t4208 to t1500.\n\n - Link to v1: https://lore.kernel.org/r/20241206-pks-rev-parse-fix-reversed-list-v1-1-95a96564a4d7@pks.im\n\nThanks!\n\nPatrick\n---\n object-name.c | 4 ++--\n t/t1500-rev-parse.sh | 15 +++++++++++++++\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex c892fbe80aa7173dfcc1995de5a75bc322c6adb7..34433d2a01d6a23ce6b4ca19b85c53b7b82fd0e5 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1401,7 +1401,7 @@ static int get_oid_oneline(struct repository *r,\n \t\t\t const char *prefix, struct object_id *oid,\n \t\t\t const struct commit_list *list)\n {\n-\tstruct commit_list *copy = NULL;\n+\tstruct commit_list *copy = NULL, **copy_tail = ©\n \tconst struct commit_list *l;\n \tint found = 0;\n \tint negative = 0;\n@@ -1423,7 +1423,7 @@ static int get_oid_oneline(struct repository *r,\n \n \tfor (l = list; l; l = l->next) {\n \t\tl->item->object.flags |= ONELINE_SEEN;\n-\t\tcommit_list_insert(l->item, ©);\n+\t\tcopy_tail = &commit_list_insert(l->item, copy_tail)->next;\n \t}\n \twhile (copy) {\n \t\tconst char *p, *buf;\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 30c31918fde6539d52800e18dfbb3423b5b73491..42c4a63cb95eed781ed7d3029c4ff5e600e6f8b8 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -310,4 +310,19 @@ test_expect_success '--short= truncates to the actual hash length' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success ':/ and HEAD^{/} favor more recent matching commits' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit common-old &&\n+\t\ttest_commit --no-tag common-new &&\n+\t\tgit rev-parse HEAD >expect &&\n+\t\tgit rev-parse :/common >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit rev-parse HEAD^{/common} >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n\n---\nbase-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\nchange-id: 20241206-pks-rev-parse-fix-reversed-list-0f94a20a6165\n\n"},{"id":"508791","messageId":"4fc44fe4-cf34-4b6b-a073-8ad5b02fe663@app.fastmail.com","threadId":"62608","inReplyTo":"20241206-pks-rev-parse-fix-reversed-list-v2-1-190514278ead@pks.im","subject":"Re: [PATCH v2] object-name: fix reversed ordering with \":/\" revisions","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-12-06T14:33:52Z","receivedAt":"2024-12-06T14:34:14Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Fri, Dec 6, 2024, at 13:28, Patrick Steinhardt wrote:\n> Recently it was reported [1] that \"look for the youngest reachable\n> commit with log message that match the given pattern\" syntax (e.g.\n> ':/' or 'HEAD^{/}') started to return results in\n\nBut the regression is only for `:/`. Not for `HEAD^{/}`. I’m sorry\nthat I wasn’t clear in my previous message[1] since I didn’t establish\nthe context properly:\n\n I have indeed noticed that `HEAD^{/}` returns a sensible thing while\n `:/` does something strange like finding the root commit.\n\nWhat I had noticed myself for a little while was that `HEAD^{/}` on\nGit 2.47.0 did something that I wanted (and which is documented) while\n`:/` behaved (behaves) weirdly. I just shrugged that off since the\nsecond syntax is more useful anyway (like Junio said).\n\n🔗 1: https://lore.kernel.org/git/Z1LtS-8f8WZyobz3@pks.im/T/#m0b68bb083d5ec6fddbc2af2ec5886a7a884d27ad\n"},{"id":"508792","messageId":"Z1McGsD8nG5x43g9@pks.im","threadId":"62608","inReplyTo":"4fc44fe4-cf34-4b6b-a073-8ad5b02fe663@app.fastmail.com","subject":"Re: [PATCH v2] object-name: fix reversed ordering with \":/\" revisions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-06T15:45:30Z","receivedAt":"2024-12-06T15:44:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Dec 06, 2024 at 03:33:52PM +0100, Kristoffer Haugsbakk wrote:\n> On Fri, Dec 6, 2024, at 13:28, Patrick Steinhardt wrote:\n> > Recently it was reported [1] that \"look for the youngest reachable\n> > commit with log message that match the given pattern\" syntax (e.g.\n> > ':/' or 'HEAD^{/}') started to return results in\n> \n> But the regression is only for `:/`. Not for `HEAD^{/}`. I’m sorry\n> that I wasn’t clear in my previous message[1] since I didn’t establish\n> the context properly:\n> \n> I have indeed noticed that `HEAD^{/}` returns a sensible thing while\n> `:/` does something strange like finding the root commit.\n> \n> What I had noticed myself for a little while was that `HEAD^{/}` on\n> Git 2.47.0 did something that I wanted (and which is documented) while\n> `:/` behaved (behaves) weirdly. I just shrugged that off since the\n> second syntax is more useful anyway (like Junio said).\n> \n> 🔗 1: https://lore.kernel.org/git/Z1LtS-8f8WZyobz3@pks.im/T/#m0b68bb083d5ec6fddbc2af2ec5886a7a884d27ad\n\nFair, I should've double checked. Anyway, verifying the behaviour of\nboth in the added test is probably still sensible.\n\nThanks!\n\nPatrick\n"},{"id":"508793","messageId":"20241206-pks-rev-parse-fix-reversed-list-v3-1-d934c17db168@pks.im","threadId":"62608","inReplyTo":"20241206-pks-rev-parse-fix-reversed-list-v1-1-95a96564a4d7@pks.im","subject":"[PATCH v3] object-name: fix reversed ordering with \":/\" revisions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-06T15:45:13Z","receivedAt":"2024-12-06T15:45:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Recently it was reported [1] that \"look for the youngest reachable\ncommit with log message that match the given pattern\" syntax (e.g.\n':/') started to return results in reverse recency order. This\nregression was introduced in Git v2.47.0 and is caused by a memory leak\nfix done in 57fb139b5e (object-name: fix leaking commit list items,\n2024-08-01).\n\nThe intent of the identified commit is to stop modifying the commit list\nprovided by the caller such that the caller can properly free all commit\nlist items, including those that the called function might potentially\nremove from the list. This was done by creating a copy of the passed-in\ncommit list and modifying this copy instead of the caller-provided list.\n\nWe already knew to create such a copy beforehand with the `backup` list,\nwhich was used to clear the `ONELINE_SEEN` commit mark after we were\ndone. So the refactoring simply renamed that list to `copy` and started\nto operate on that list instead. There is a gotcha though: the backup\nlist, and thus now also the copied list, is always being prepended to,\nso the resulting list is in reverse order! The end result is that we\npop commits from the wrong end of the commit list, returning commits in\nreverse recency order.\n\nFix the bug by appending to the list instead.\n\n[1]: \n\nReported-by: Aarni Koskela \nSigned-off-by: Patrick Steinhardt \n---\nThis patch applies on top of v2.47.0, which is the first version which\nhad this regression.\n\nChanges in v2:\n\n - Include the message ID of the report in the commit message.\n\n - Fix terminology used by the commit message.\n\n - Move the test from t4208 to t1500.\n\n - Link to v1: https://lore.kernel.org/r/20241206-pks-rev-parse-fix-reversed-list-v1-1-95a96564a4d7@pks.im\n\nChanges in v3:\n\n - Only mention ':/' as having regressed, not 'HEAD^{/}'.\n\n - Link to v2: https://lore.kernel.org/r/20241206-pks-rev-parse-fix-reversed-list-v2-1-190514278ead@pks.im\n\nThanks!\n\nPatrick\n---\n object-name.c | 4 ++--\n t/t1500-rev-parse.sh | 15 +++++++++++++++\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex c892fbe80aa7173dfcc1995de5a75bc322c6adb7..34433d2a01d6a23ce6b4ca19b85c53b7b82fd0e5 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1401,7 +1401,7 @@ static int get_oid_oneline(struct repository *r,\n \t\t\t const char *prefix, struct object_id *oid,\n \t\t\t const struct commit_list *list)\n {\n-\tstruct commit_list *copy = NULL;\n+\tstruct commit_list *copy = NULL, **copy_tail = ©\n \tconst struct commit_list *l;\n \tint found = 0;\n \tint negative = 0;\n@@ -1423,7 +1423,7 @@ static int get_oid_oneline(struct repository *r,\n \n \tfor (l = list; l; l = l->next) {\n \t\tl->item->object.flags |= ONELINE_SEEN;\n-\t\tcommit_list_insert(l->item, ©);\n+\t\tcopy_tail = &commit_list_insert(l->item, copy_tail)->next;\n \t}\n \twhile (copy) {\n \t\tconst char *p, *buf;\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 30c31918fde6539d52800e18dfbb3423b5b73491..42c4a63cb95eed781ed7d3029c4ff5e600e6f8b8 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -310,4 +310,19 @@ test_expect_success '--short= truncates to the actual hash length' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success ':/ and HEAD^{/} favor more recent matching commits' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit common-old &&\n+\t\ttest_commit --no-tag common-new &&\n+\t\tgit rev-parse HEAD >expect &&\n+\t\tgit rev-parse :/common >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit rev-parse HEAD^{/common} >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n\n---\nbase-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2\nchange-id: 20241206-pks-rev-parse-fix-reversed-list-0f94a20a6165\n\n"},{"id":"508809","messageId":"xmqq8qsswi4b.fsf@gitster.g","threadId":"62608","inReplyTo":"cfc7adaa-ccf3-4f27-807c-2eed70e56aa8@app.fastmail.com","subject":"Re: [PATCH] object-name: fix reversed ordering with magic pathspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-06T22:40:04Z","receivedAt":"2024-12-06T22:40:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> And the longer syntax doesn’t seem to be affected by any regressions.\n\nThat is curious. I wonder if we have two redundant code paths and\nthe regression hit only one of them?\n\nThanks.\n"},{"id":"508810","messageId":"xmqq1pykwhsw.fsf@gitster.g","threadId":"62608","inReplyTo":"Z1McGsD8nG5x43g9@pks.im","subject":"Re: [PATCH v2] object-name: fix reversed ordering with \":/\" revisions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-06T22:46:55Z","receivedAt":"2024-12-06T22:46:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Fair, I should've double checked. Anyway, verifying the behaviour of\n> both in the added test is probably still sensible.\n\nI'll queue with this range-diff on top. :/ is explained in\n\"git help revisions\" as \"reachable from any ref\", and it is a good\nphrase to use there, I think.\n\nThanks, both, I should've double-checked, too.\n\n\n ## Commit message ##\n object-name: fix reversed ordering with \":/\" revisions\n \n - Recently it was reported [1] that \"look for the youngest reachable\n - commit with log message that match the given pattern\" syntax (e.g.\n - ':/' or 'HEAD^{/}') started to return results in\n + Recently it was reported [1] that \"look for the youngest commit\n + reachable from any ref with log message that match the given\n + pattern\" syntax (e.g. ':/') started to return results in\n reverse recency order. This regression was introduced in Git v2.47.0 and\n is caused by a memory leak fix done in 57fb139b5e (object-name: fix\n leaking commit list items, 2024-08-01).\n"},{"id":"508812","messageId":"rgy5ghibgbsblrtzn4ujzv3ggx6m4cx4uba56l74dcuhqyikuq@y4ljecsqm5ox","threadId":"62608","inReplyTo":"4fc44fe4-cf34-4b6b-a073-8ad5b02fe663@app.fastmail.com","subject":"Re: [PATCH v2] object-name: fix reversed ordering with \":/\" revisions","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-12-07T02:05:27Z","receivedAt":"2024-12-07T02:07:36Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/12/06 03:33PM, Kristoffer Haugsbakk wrote:\n> On Fri, Dec 6, 2024, at 13:28, Patrick Steinhardt wrote:\n> > Recently it was reported [1] that \"look for the youngest reachable\n> > commit with log message that match the given pattern\" syntax (e.g.\n> > ':/' or 'HEAD^{/}') started to return results in\n> \n> But the regression is only for `:/`. Not for `HEAD^{/}`. I’m sorry\n> that I wasn’t clear in my previous message[1] since I didn’t establish\n> the context properly:\n> \n> I have indeed noticed that `HEAD^{/}` returns a sensible thing while\n> `:/` does something strange like finding the root commit.\n> \n> What I had noticed myself for a little while was that `HEAD^{/}` on\n> Git 2.47.0 did something that I wanted (and which is documented) while\n> `:/` behaved (behaves) weirdly. I just shrugged that off since the\n> second syntax is more useful anyway (like Junio said).\n\nI was a bit curious why the regression only affected the `:/` syntax but\nnot `HEAD^{/}` as they both use `get_oid_oneline()`. The commit list\nthat was getting reversed only ever contain the reference commits\nthemselves and not the commits being traversed to find the matching\ncommit (that part comes a bit later). This means it is not a problem\nwhen there is only a single reference being traversed as in the case of\n`HEAD^{/}`. So this fix looks good. :)\n\n-Justin\n"},{"id":"508823","messageId":"3ef4f213-a49e-4d72-8f45-00a5fa030852@app.fastmail.com","threadId":"62608","inReplyTo":"20241206-pks-rev-parse-fix-reversed-list-v3-1-d934c17db168@pks.im","subject":"Re: [PATCH v3] object-name: fix reversed ordering with \":/\" revisions","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-12-07T15:51:39Z","receivedAt":"2024-12-07T15:52:00Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"Just bringing up this nitpick in case you decide to make a v4 based on\nJustin’s reply in this thread.\n\nOn Fri, Dec 6, 2024, at 16:45, Patrick Steinhardt wrote:\n> Recently it was reported [1] that \"look for the youngest reachable\n> commit with log message that match the given pattern\" syntax (e.g.\n> ':/') started to return results in reverse recency order. This\n\nThis isn’t “e.g. ':/'” any more since you are not listing\nexamples. “i.e.” perhaps?\n\nBut it’s clear what you mean from the context anyway.\n"},{"id":"508824","messageId":"xmqqcyi3t6ui.fsf@gitster.g","threadId":"62608","inReplyTo":"3ef4f213-a49e-4d72-8f45-00a5fa030852@app.fastmail.com","subject":"Re: [PATCH v3] object-name: fix reversed ordering with \":/\" revisions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-07T23:24:05Z","receivedAt":"2024-12-07T23:24:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n> This isn’t “e.g. ':/'” any more since you are not listing\n> examples. “i.e.” perhaps?\n\nWill locally amend (and with s/PATTERN/text/ to match the relevant\npart of the documentation). Thanks for carefully reading.\n\n"},{"id":"508864","messageId":"4c8604c2-4287-48dd-b1ec-78ba9c1401f0@web.de","threadId":"62608","inReplyTo":"20241206-pks-rev-parse-fix-reversed-list-v3-1-d934c17db168@pks.im","subject":"Re: [PATCH v3] object-name: fix reversed ordering with \":/\" revisions","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-09T11:47:39Z","receivedAt":"2024-12-09T11:53:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.12.24 um 16:45 schrieb Patrick Steinhardt:\n> @@ -1423,7 +1423,7 @@ static int get_oid_oneline(struct repository *r,\n>\n> \tfor (l = list; l; l = l->next) {\n> \t\tl->item->object.flags |= ONELINE_SEEN;\n> -\t\tcommit_list_insert(l->item, ©);\n> +\t\tcopy_tail = &commit_list_insert(l->item, copy_tail)->next;\n\nOK. The following does the same while being clearer:\n\n\t\tcopy_tail = commit_list_append(l->item, copy_tail);\n\nYou could get the idea to do that replacement across the whole source\ntree. That would be nice, but must not be done blindly (e.g. with\nCoccinelle), as the result will be different if the second argument\ncan point to somewhere in the middle of the list. Here it's OK because\nwe indeed are appending (invariant *copy_tail == NULL holds).\n\nRené\n\n"}]}