threads / patch / 62608

patchobject-name: fix reversed ordering with magic pathspecs

Subject: [PATCH] object-name: fix reversed ordering with magic pathspecs

## tl;dr

16 messages between Dec 6, 2024 and Dec 9, 2024. Diffs are folded; open one to read it.

replies: 15people: 5as markdown or json

Patrick Steinhardt· Dec 6, 2024, 09:51 UTC · lore

It was reported that magic pathspecs started to return results in reverse recency order starting with Git v2.47.0. This behaviour bisects to 57fb139b5e (object-name: fix leaking commit list items, 2024-08-01), which has refactored `get_oid_oneline()` to plug a couple of memory leaks.

As part of that refactoring we have started to create a copy of the passed-in list of commits and work on that list instead. The intent of this was to stop modifying the caller-provided commit list such that the caller can properly free all commit list items, including those that we might potentially drop.

We already knew to create such a copy beforehand with the `backup` list, which was used to clear the `ONELINE_SEEN` commit mark after we were done. So the refactoring simply renamed that list to `copy` and started to operate on that list instead. There is a gotcha though: the backup list, and thus now also the copied list, is always being prepended to, so the resulting list is in reverse order! The end result is that we pop commits from the wrong end of the commit list, returning commits in reverse recency order.

Fix the bug by appending to the list instead.
Reported-by: Aarni Koskela <aarni@valohai.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
This patch applies on top of v2.47.0, which is the first version which
had this regression.
---
 object-name.c                 |  4 ++--
 t/t4208-log-magic-pathspec.sh | 13 +++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)
Show changes to 2 files +15 −2

object-name.c, t/t4208-log-magic-pathspec.sh

diff --git a/object-name.c b/object-name.c
index c892fbe80aa7173dfcc1995de5a75bc322c6adb7..34433d2a01d6a23ce6b4ca19b85c53b7b82fd0e5 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1401,7 +1401,7 @@ static int get_oid_oneline(struct repository *r,
 			   const char *prefix, struct object_id *oid,
 			   const struct commit_list *list)
 {
-	struct commit_list *copy = NULL;
+	struct commit_list *copy = NULL, **copy_tail = &copy;
 	const struct commit_list *l;
 	int found = 0;
 	int negative = 0;
@@ -1423,7 +1423,7 @@ static int get_oid_oneline(struct repository *r,
 
 	for (l = list; l; l = l->next) {
 		l->item->object.flags |= ONELINE_SEEN;
-		commit_list_insert(l->item, &copy);
+		copy_tail = &commit_list_insert(l->item, copy_tail)->next;
 	}
 	while (copy) {
 		const char *p, *buf;
diff --git a/t/t4208-log-magic-pathspec.sh b/t/t4208-log-magic-pathspec.sh
index 2a46eb6bedbe283a08fd77917b7fb17da155b027..2d80b9044bcf9ec8e2f11b6afd2b0fe8e2fcabfd 100755
--- a/t/t4208-log-magic-pathspec.sh
+++ b/t/t4208-log-magic-pathspec.sh
@@ -58,6 +58,19 @@ test_expect_success '"git log :/any/path/" should not segfault' '
 	test_must_fail git log :/any/path/
 '
 
+test_expect_success ':/ favors more recent matching commits' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit common-old &&
+		test_commit --no-tag common-new &&
+		git rev-parse HEAD >expect &&
+		git rev-parse :/common >actual &&
+		test_cmp expect actual
+	)
+'
+
 # This differs from the ":/a" check above in that :/in looks like a pathspec,
 # but doesn't match an actual file.
 test_expect_success '"git log :/in" should not be ambiguous' '

---
base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2
change-id: 20241206-pks-rev-parse-fix-reversed-list-0f94a20a6165
Kristoffer Haugsbakk· Dec 6, 2024, 11:20 UTC · re: Patrick Steinhardt · lore

Re: [PATCH] object-name: fix reversed ordering with magic pathspecs

Hi
On Fri, Dec 6, 2024, at 10:51, Patrick Steinhardt wrote:
> It was reported

It would be nice with a hyperlink since this email is not part of the email thread for the report.

https://lore.kernel.org/git/Z1LJSADiStlFicTL@pks.im/T/#mae906ec74d5657e702165e29b90927f730280c29
> It was reported that magic pathspecs started to return results in

I’m not used to this being called “magic pathspecs” as a user. gitglossary(7) talks about (magic) pathspecs as filepaths.

(c.f. gitrevisions(7) where this revision selection syntax is discussed.)

> reverse recency order starting with Git v2.47.0. This behaviour bisects
> to 57fb139b5e (object-name: fix leaking commit list items, 2024-08-01),
Nit: Can it be simply be asserted that it is caused by that commit?
Because that would make for a simpler/more assertive narrative.

“This bisects to” can be a nice phrase when it is followed by a “but” subclause,[1] i.e. when the straightforward troubleshooting can turn up misleading leads.

† 1: 615d2de3b45 (config.c: avoid segfault with --fixed-value and valueless
    config, 2024-08-01)
Show 6 quoted lines
> which has refactored `get_oid_oneline()` to plug a couple of memory
> leaks.
>
> As part of that refactoring we have started to create a copy of the
> passed-in list of commits and work on that list instead. The intent of
> this was to stop modifying the caller-provided commit list such that the
Nit: s/The intent of this was/The intent was/
Show 11 quoted lines
> caller can properly free all commit list items, including those that we
> might potentially drop.
>
> We already knew to create such a copy beforehand with the `backup` list,
> which was used to clear the `ONELINE_SEEN` commit mark after we were
> done. So the refactoring simply renamed that list to `copy` and started
> to operate on that list instead. There is a gotcha though: the backup
> list, and thus now also the copied list, is always being prepended to,
> so the resulting list is in reverse order! The end result is that we
> pop commits from the wrong end of the commit list, returning commits in
> reverse recency order.
Nice explanation.
Show 8 quoted lines
>
> Fix the bug by appending to the list instead.
>
> Reported-by: Aarni Koskela <aarni@valohai.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> [snip]
> diff --git a/t/t4208-log-magic-pathspec.sh
> b/t/t4208-log-magic-pathspec.sh

Yes, so here is that magic pathspec name. But this test file has a lot of tests that test positional argument ambiguity. Which seems very relevant to pathspecs in particular. And revision selection syntax seems to be used to test how things are interpreted. Not really how things are ultimately processed (that seems secondary).

The tests involving `:/` in particular seem to only be about ambiguity testing.

Is this the correct test file?
Show 11 quoted lines
> index
> 2a46eb6bedbe283a08fd77917b7fb17da155b027..2d80b9044bcf9ec8e2f11b6afd2b0fe8e2fcabfd
> 100755
> --- a/t/t4208-log-magic-pathspec.sh
> +++ b/t/t4208-log-magic-pathspec.sh
> @@ -58,6 +58,19 @@ test_expect_success '"git log :/any/path/" should
> not segfault' '
>  	test_must_fail git log :/any/path/
>  '
>
> +test_expect_success ':/ favors more recent matching commits' '

This wasn’t mentioned in the report but `HEAD^{/}` is a similar syntax. That one is more controllable since you provide a ref yourself (`:/` returns the youngest commit from any ref).

I have indeed noticed that `HEAD^{/}` returns a sensible thing while `:/` does something strange like finding the root commit. (Then I shrugged and half-assumed that I hadn’t read some fine print)

gitrevisions(7) calls out the relation between these two. It could be nice for a regression test to assert that these two syntaxes return the same commit. I.e. when you have just made a commit, `:/<search>` and `HEAD^{/<search>}` return the same commit, and that commit is the youngest.

Show 19 quoted lines
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit common-old &&
> +		test_commit --no-tag common-new &&
> +		git rev-parse HEAD >expect &&
> +		git rev-parse :/common >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
>  # This differs from the ":/a" check above in that :/in looks like a pathspec,
>  # but doesn't match an actual file.
>  test_expect_success '"git log :/in" should not be ambiguous' '
>
> ---
> base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2
> change-id: 20241206-pks-rev-parse-fix-reversed-list-0f94a20a6165
Junio C Hamano· Dec 6, 2024, 11:57 UTC · re: Kristoffer Haugsbakk · lore

Re: [PATCH] object-name: fix reversed ordering with magic pathspecs

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:
Show 12 quoted lines
> On Fri, Dec 6, 2024, at 10:51, Patrick Steinhardt wrote:
>> It was reported
>
> It would be nice with a hyperlink since this email is not part of the
> email thread for the report.
>
> https://lore.kernel.org/git/Z1LJSADiStlFicTL@pks.im/T/#mae906ec74d5657e702165e29b90927f730280c29
>
>> It was reported that magic pathspecs started to return results in
>
> I’m not used to this being called “magic pathspecs” as a user.
> gitglossary(7) talks about (magic) pathspecs as filepaths.

Thanks for catching the mistaken phrasing. It would have caused unnecessary confusion later to "git log" readers.

The syntax to say that the following path is from the top-level of the working tree even when you are running the command from a subdirectory, e.g.

    cd Documentation
    git log :/t
is described in gitglossary(7):
    A pathspec that begins with a colon `:` has special meaning.  In the
    short form, the leading colon `:` is followed by zero or more "magic
    signature" letters (which optionally is terminated by another colon `:`),
    and the remainder is the pattern to match against the path.
    The "magic signature" consists of ASCII symbols that are neither
    alphanumeric, glob, regex special characters nor colon.
    The optional colon that terminates the "magic signature" can be
    omitted if the pattern begins with a character that does not belong to
    "magic signature" symbol set and is not a colon.

and even though the word "magic" signature is used, there is no definition given for the entire construct, i.e. the pathspec that uses a special meaning introduced by the use of "colon followed by one or more magic signature letters". We probably should add a sentence to officially define it, if only to reduce the need for exchanges like this ;-)

    ... and is not a colon.  Such a pathspec that uses these "magic"
    meaning is called "magic pathspec".
But more importantly, the syntax that triggered this topic in
<CAKOEJdcPYn3O01p29rVa+xv=Qr504FQyKJeSB-Moze04ViCGGg@mail.gmail.com>

is *NOT* a magic pathspec at all. It is a revision syntax to name the first commit that is reachable from the current HEAD with a commit log message, that matches the given patterh, i.e.

    git show ":/my message"
which is a (rather lousy) short-hand for a more general
    git show "HEAD^{/my message}"

i.e. <startingRev>^{/<pattern>}. There is no specific name for this syntax.

I suspect that "filepath" you mentioned may be something some folks (but not me or any other long timers) would call yet another syntax, which is :<path>, that names the object sitting at <path> in the index. Because ":/myMessage" look so similar to ":myFile", yet they mean so different things, as I said, ":/myMessage" is a rather lousy short-hand for the more general "^{/<pattern>}" suffix that is less ambiguous.

Patrick, let's not use a wrong word. This is not about pathspec at all, and is a revision syntax. As there is no specific jargon for the syntax, here is what I would write, if I were explaining the problem being solved:

    Recently it was reported [*1*] that "look for the youngest
    reachable commit with log message that match the given pattern"
    syntax (e.g. :/myMessagePattern, or HEAD^{/myMessagePattern})
    started to show older commit.
    [Footnote]
    *1* <CAKOEJdcPYn3O01p29rVa+xv=Qr504FQyKJeSB-Moze04ViCGGg@mail.gmail.com>
Thanks.
Patrick Steinhardt· Dec 6, 2024, 12:05 UTC · re: Junio C Hamano · lore

Re: [PATCH] object-name: fix reversed ordering with magic pathspecs

On Fri, Dec 06, 2024 at 08:57:04PM +0900, Junio C Hamano wrote:
Show 63 quoted lines
> "Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:
> 
> > On Fri, Dec 6, 2024, at 10:51, Patrick Steinhardt wrote:
> >> It was reported
> >
> > It would be nice with a hyperlink since this email is not part of the
> > email thread for the report.
> >
> > https://lore.kernel.org/git/Z1LJSADiStlFicTL@pks.im/T/#mae906ec74d5657e702165e29b90927f730280c29
> >
> >> It was reported that magic pathspecs started to return results in
> >
> > I’m not used to this being called “magic pathspecs” as a user.
> > gitglossary(7) talks about (magic) pathspecs as filepaths.
> 
> Thanks for catching the mistaken phrasing.  It would have caused
> unnecessary confusion later to "git log" readers.
> 
> The syntax to say that the following path is from the top-level of
> the working tree even when you are running the command from a
> subdirectory, e.g.
> 
>     cd Documentation
>     git log :/t
> 
> is described in gitglossary(7):
> 
>     A pathspec that begins with a colon `:` has special meaning.  In the
>     short form, the leading colon `:` is followed by zero or more "magic
>     signature" letters (which optionally is terminated by another colon `:`),
>     and the remainder is the pattern to match against the path.
>     The "magic signature" consists of ASCII symbols that are neither
>     alphanumeric, glob, regex special characters nor colon.
>     The optional colon that terminates the "magic signature" can be
>     omitted if the pattern begins with a character that does not belong to
>     "magic signature" symbol set and is not a colon.
> 
> and even though the word "magic" signature is used, there is no
> definition given for the entire construct, i.e. the pathspec that
> uses a special meaning introduced by the use of "colon followed by
> one or more magic signature letters".  We probably should add a
> sentence to officially define it, if only to reduce the need for
> exchanges like this ;-)
> 
>     ... and is not a colon.  Such a pathspec that uses these "magic"
>     meaning is called "magic pathspec".
> 
> But more importantly, the syntax that triggered this topic in
> 
> <CAKOEJdcPYn3O01p29rVa+xv=Qr504FQyKJeSB-Moze04ViCGGg@mail.gmail.com>
> 
> is *NOT* a magic pathspec at all.  It is a revision syntax to name
> the first commit that is reachable from the current HEAD with a
> commit log message, that matches the given patterh, i.e.
> 
>     git show ":/my message"
> 
> which is a (rather lousy) short-hand for a more general
> 
>     git show "HEAD^{/my message}"
> 
> i.e. <startingRev>^{/<pattern>}.  There is no specific name for
> this syntax.

Hm, yeah. I think I read pathspec somewhere and just went with it without thinking.

Show 20 quoted lines
> I suspect that "filepath" you mentioned may be something some folks
> (but not me or any other long timers) would call yet another syntax,
> which is :<path>, that names the object sitting at <path> in the
> index.  Because ":/myMessage" look so similar to ":myFile", yet
> they mean so different things, as I said, ":/myMessage" is a rather
> lousy short-hand for the more general "^{/<pattern>}" suffix that
> is less ambiguous.
> 
> Patrick, let's not use a wrong word.  This is not about pathspec at
> all, and is a revision syntax.  As there is no specific jargon for
> the syntax, here is what I would write, if I were explaining the
> problem being solved:
> 
>     Recently it was reported [*1*] that "look for the youngest
>     reachable commit with log message that match the given pattern"
>     syntax (e.g. :/myMessagePattern, or HEAD^{/myMessagePattern})
>     started to show older commit.
> 
>     [Footnote]
>     *1* <CAKOEJdcPYn3O01p29rVa+xv=Qr504FQyKJeSB-Moze04ViCGGg@mail.gmail.com>
Will update in v2. Thanks!
Patrick
Kristoffer Haugsbakk· Dec 6, 2024, 12:25 UTC · re: Junio C Hamano · lore

Re: [PATCH] object-name: fix reversed ordering with magic pathspecs

On Fri, Dec 6, 2024, at 12:57, Junio C Hamano wrote:
> [snip]
>     Recently it was reported [*1*] that "look for the youngest
>     reachable commit with log message that match the given pattern"
>     syntax (e.g. :/myMessagePattern, or HEAD^{/myMessagePattern})

But these are slightly different since `:/` searches in all refs. They are only equivalent (i.e. with Patrick’s fix) when you have just made a commit (like in Patrick’s test).

And the longer syntax doesn’t seem to be affected by any regressions. In this repo:

    $ # wrong
    $ git log --format=reference -1 :/Merge
    3857284f7b8 (Merge refs/heads/master from ., 2005-08-24)
    $ # correct (looks correct)
    $ git log --format=reference -1 HEAD^{/Merge}
    4a611ee7ebd (Merge branch 'kn/ref-transaction-hook-with-reflog', 2024-11-27)
>     started to show older commit.
Junio C Hamano· Dec 6, 2024, 22:40 UTC · re: Kristoffer Haugsbakk · lore

Re: [PATCH] object-name: fix reversed ordering with magic pathspecs

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:
> And the longer syntax doesn’t seem to be affected by any regressions.

That is curious. I wonder if we have two redundant code paths and the regression hit only one of them?

Thanks.
Patrick Steinhardt· Dec 6, 2024, 12:25 UTC · re: Kristoffer Haugsbakk · lore

Re: [PATCH] object-name: fix reversed ordering with magic pathspecs

On Fri, Dec 06, 2024 at 12:20:45PM +0100, Kristoffer Haugsbakk wrote:
Show 13 quoted lines
> > diff --git a/t/t4208-log-magic-pathspec.sh
> > b/t/t4208-log-magic-pathspec.sh
> 
> Yes, so here is that magic pathspec name.  But this test file has a lot
> of tests that test positional argument ambiguity.  Which seems very
> relevant to pathspecs in particular.  And revision selection syntax
> seems to be used to test how things are interpreted.  Not really how
> things are ultimately processed (that seems secondary).
> 
> The tests involving `:/` in particular seem to only be about
> ambiguity testing.
> 
> Is this the correct test file?

Probably not. I couldn't really find any cases where we explicitly verify this syntax, which explains why the regression was not found. I've added it to t1500 now, which is at least better than t4208.

Thanks!
Patrick
Patrick Steinhardt· Dec 6, 2024, 12:28 UTC · re: Patrick Steinhardt · lore

[PATCH v2] object-name: fix reversed ordering with ":/<PATTERN>" revisions

Recently it was reported [1] that "look for the youngest reachable commit with log message that match the given pattern" syntax (e.g. ':/<PATTERN>' or 'HEAD^{/<PATTERN>}') started to return results in reverse recency order. This regression was introduced in Git v2.47.0 and is caused by a memory leak fix done in 57fb139b5e (object-name: fix leaking commit list items, 2024-08-01).

The intent of the identified commit is to stop modifying the commit list provided by the caller such that the caller can properly free all commit list items, including those that the called function might potentially remove from the list. This was done by creating a copy of the passed-in commit list and modifying this copy instead of the caller-provided list.

We already knew to create such a copy beforehand with the `backup` list, which was used to clear the `ONELINE_SEEN` commit mark after we were done. So the refactoring simply renamed that list to `copy` and started to operate on that list instead. There is a gotcha though: the backup list, and thus now also the copied list, is always being prepended to, so the resulting list is in reverse order! The end result is that we pop commits from the wrong end of the commit list, returning commits in reverse recency order.

Fix the bug by appending to the list instead.
[1]: <CAKOEJdcPYn3O01p29rVa+xv=Qr504FQyKJeSB-Moze04ViCGGg@mail.gmail.com>
Reported-by: Aarni Koskela <aarni@valohai.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
This patch applies on top of v2.47.0, which is the first version which
had this regression.
Changes in v2:
  - Include the message ID of the report in the commit message.
  - Fix terminology used by the commit message.
  - Move the test from t4208 to t1500.
  - Link to v1: https://lore.kernel.org/r/20241206-pks-rev-parse-fix-reversed-list-v1-1-95a96564a4d7@pks.im
Thanks!
Patrick
---
 object-name.c        |  4 ++--
 t/t1500-rev-parse.sh | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)
Show changes to 2 files +17 −2

object-name.c, t/t1500-rev-parse.sh

diff --git a/object-name.c b/object-name.c
index c892fbe80aa7173dfcc1995de5a75bc322c6adb7..34433d2a01d6a23ce6b4ca19b85c53b7b82fd0e5 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1401,7 +1401,7 @@ static int get_oid_oneline(struct repository *r,
 			   const char *prefix, struct object_id *oid,
 			   const struct commit_list *list)
 {
-	struct commit_list *copy = NULL;
+	struct commit_list *copy = NULL, **copy_tail = &copy;
 	const struct commit_list *l;
 	int found = 0;
 	int negative = 0;
@@ -1423,7 +1423,7 @@ static int get_oid_oneline(struct repository *r,
 
 	for (l = list; l; l = l->next) {
 		l->item->object.flags |= ONELINE_SEEN;
-		commit_list_insert(l->item, &copy);
+		copy_tail = &commit_list_insert(l->item, copy_tail)->next;
 	}
 	while (copy) {
 		const char *p, *buf;
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 30c31918fde6539d52800e18dfbb3423b5b73491..42c4a63cb95eed781ed7d3029c4ff5e600e6f8b8 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -310,4 +310,19 @@ test_expect_success '--short= truncates to the actual hash length' '
 	test_cmp expect actual
 '
 
+test_expect_success ':/ and HEAD^{/} favor more recent matching commits' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit common-old &&
+		test_commit --no-tag common-new &&
+		git rev-parse HEAD >expect &&
+		git rev-parse :/common >actual &&
+		test_cmp expect actual &&
+		git rev-parse HEAD^{/common} >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done

---
base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2
change-id: 20241206-pks-rev-parse-fix-reversed-list-0f94a20a6165
Kristoffer Haugsbakk· Dec 6, 2024, 14:33 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2] object-name: fix reversed ordering with ":/<PATTERN>" revisions

On Fri, Dec 6, 2024, at 13:28, Patrick Steinhardt wrote:
> Recently it was reported [1] that "look for the youngest reachable
> commit with log message that match the given pattern" syntax (e.g.
> ':/<PATTERN>' or 'HEAD^{/<PATTERN>}') started to return results in

But the regression is only for `:/`. Not for `HEAD^{/}`. I’m sorry that I wasn’t clear in my previous message[1] since I didn’t establish the context properly:

    I have indeed noticed that `HEAD^{/}` returns a sensible thing while
    `:/` does something strange like finding the root commit.

What I had noticed myself for a little while was that `HEAD^{/}` on Git 2.47.0 did something that I wanted (and which is documented) while `:/` behaved (behaves) weirdly. I just shrugged that off since the second syntax is more useful anyway (like Junio said).

🔗 1: https://lore.kernel.org/git/Z1LtS-8f8WZyobz3@pks.im/T/#m0b68bb083d5ec6fddbc2af2ec5886a7a884d27ad
Patrick Steinhardt· Dec 6, 2024, 15:45 UTC · re: Kristoffer Haugsbakk · lore

Re: [PATCH v2] object-name: fix reversed ordering with ":/<PATTERN>" revisions

On Fri, Dec 06, 2024 at 03:33:52PM +0100, Kristoffer Haugsbakk wrote:
Show 18 quoted lines
> On Fri, Dec 6, 2024, at 13:28, Patrick Steinhardt wrote:
> > Recently it was reported [1] that "look for the youngest reachable
> > commit with log message that match the given pattern" syntax (e.g.
> > ':/<PATTERN>' or 'HEAD^{/<PATTERN>}') started to return results in
> 
> But the regression is only for `:/`.  Not for `HEAD^{/}`.  I’m sorry
> that I wasn’t clear in my previous message[1] since I didn’t establish
> the context properly:
> 
>     I have indeed noticed that `HEAD^{/}` returns a sensible thing while
>     `:/` does something strange like finding the root commit.
> 
> What I had noticed myself for a little while was that `HEAD^{/}` on
> Git 2.47.0 did something that I wanted (and which is documented) while
> `:/` behaved (behaves) weirdly.  I just shrugged that off since the
> second syntax is more useful anyway (like Junio said).
> 
> 🔗 1: https://lore.kernel.org/git/Z1LtS-8f8WZyobz3@pks.im/T/#m0b68bb083d5ec6fddbc2af2ec5886a7a884d27ad

Fair, I should've double checked. Anyway, verifying the behaviour of both in the added test is probably still sensible.

Thanks!
Patrick
Junio C Hamano· Dec 6, 2024, 22:46 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v2] object-name: fix reversed ordering with ":/<PATTERN>" revisions

Patrick Steinhardt <ps@pks.im> writes:
> Fair, I should've double checked. Anyway, verifying the behaviour of
> both in the added test is probably still sensible.

I'll queue with this range-diff on top. :/<text> is explained in "git help revisions" as "reachable from any ref", and it is a good phrase to use there, I think.

Thanks, both, I should've double-checked, too.
      ## Commit message ##
         object-name: fix reversed ordering with ":/<PATTERN>" revisions
     
    -    Recently it was reported [1] that "look for the youngest reachable
    -    commit with log message that match the given pattern" syntax (e.g.
    -    ':/<PATTERN>' or 'HEAD^{/<PATTERN>}') started to return results in
    +    Recently it was reported [1] that "look for the youngest commit
    +    reachable from any ref with log message that match the given
    +    pattern" syntax (e.g. ':/<PATTERN>') started to return results in
         reverse recency order. This regression was introduced in Git v2.47.0 and
         is caused by a memory leak fix done in 57fb139b5e (object-name: fix
         leaking commit list items, 2024-08-01).
Justin Tobler· Dec 7, 2024, 02:05 UTC · re: Kristoffer Haugsbakk · lore

Re: [PATCH v2] object-name: fix reversed ordering with ":/<PATTERN>" revisions

On 24/12/06 03:33PM, Kristoffer Haugsbakk wrote:
Show 16 quoted lines
> On Fri, Dec 6, 2024, at 13:28, Patrick Steinhardt wrote:
> > Recently it was reported [1] that "look for the youngest reachable
> > commit with log message that match the given pattern" syntax (e.g.
> > ':/<PATTERN>' or 'HEAD^{/<PATTERN>}') started to return results in
> 
> But the regression is only for `:/`.  Not for `HEAD^{/}`.  I’m sorry
> that I wasn’t clear in my previous message[1] since I didn’t establish
> the context properly:
> 
>     I have indeed noticed that `HEAD^{/}` returns a sensible thing while
>     `:/` does something strange like finding the root commit.
> 
> What I had noticed myself for a little while was that `HEAD^{/}` on
> Git 2.47.0 did something that I wanted (and which is documented) while
> `:/` behaved (behaves) weirdly.  I just shrugged that off since the
> second syntax is more useful anyway (like Junio said).

I was a bit curious why the regression only affected the `:/` syntax but not `HEAD^{/}` as they both use `get_oid_oneline()`. The commit list that was getting reversed only ever contain the reference commits themselves and not the commits being traversed to find the matching commit (that part comes a bit later). This means it is not a problem when there is only a single reference being traversed as in the case of `HEAD^{/}`. So this fix looks good. :)

-Justin
Patrick Steinhardt· Dec 6, 2024, 15:45 UTC · re: Patrick Steinhardt · lore

[PATCH v3] object-name: fix reversed ordering with ":/<PATTERN>" revisions

Recently it was reported [1] that "look for the youngest reachable commit with log message that match the given pattern" syntax (e.g. ':/<PATTERN>') started to return results in reverse recency order. This regression was introduced in Git v2.47.0 and is caused by a memory leak fix done in 57fb139b5e (object-name: fix leaking commit list items, 2024-08-01).

The intent of the identified commit is to stop modifying the commit list provided by the caller such that the caller can properly free all commit list items, including those that the called function might potentially remove from the list. This was done by creating a copy of the passed-in commit list and modifying this copy instead of the caller-provided list.

We already knew to create such a copy beforehand with the `backup` list, which was used to clear the `ONELINE_SEEN` commit mark after we were done. So the refactoring simply renamed that list to `copy` and started to operate on that list instead. There is a gotcha though: the backup list, and thus now also the copied list, is always being prepended to, so the resulting list is in reverse order! The end result is that we pop commits from the wrong end of the commit list, returning commits in reverse recency order.

Fix the bug by appending to the list instead.
[1]: <CAKOEJdcPYn3O01p29rVa+xv=Qr504FQyKJeSB-Moze04ViCGGg@mail.gmail.com>
Reported-by: Aarni Koskela <aarni@valohai.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
This patch applies on top of v2.47.0, which is the first version which
had this regression.
Changes in v2:
  - Include the message ID of the report in the commit message.
  - Fix terminology used by the commit message.
  - Move the test from t4208 to t1500.
  - Link to v1: https://lore.kernel.org/r/20241206-pks-rev-parse-fix-reversed-list-v1-1-95a96564a4d7@pks.im
Changes in v3:
  - Only mention ':/' as having regressed, not 'HEAD^{/}'.
  - Link to v2: https://lore.kernel.org/r/20241206-pks-rev-parse-fix-reversed-list-v2-1-190514278ead@pks.im
Thanks!
Patrick
---
 object-name.c        |  4 ++--
 t/t1500-rev-parse.sh | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)
Show changes to 2 files +17 −2

object-name.c, t/t1500-rev-parse.sh

diff --git a/object-name.c b/object-name.c
index c892fbe80aa7173dfcc1995de5a75bc322c6adb7..34433d2a01d6a23ce6b4ca19b85c53b7b82fd0e5 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1401,7 +1401,7 @@ static int get_oid_oneline(struct repository *r,
 			   const char *prefix, struct object_id *oid,
 			   const struct commit_list *list)
 {
-	struct commit_list *copy = NULL;
+	struct commit_list *copy = NULL, **copy_tail = &copy;
 	const struct commit_list *l;
 	int found = 0;
 	int negative = 0;
@@ -1423,7 +1423,7 @@ static int get_oid_oneline(struct repository *r,
 
 	for (l = list; l; l = l->next) {
 		l->item->object.flags |= ONELINE_SEEN;
-		commit_list_insert(l->item, &copy);
+		copy_tail = &commit_list_insert(l->item, copy_tail)->next;
 	}
 	while (copy) {
 		const char *p, *buf;
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 30c31918fde6539d52800e18dfbb3423b5b73491..42c4a63cb95eed781ed7d3029c4ff5e600e6f8b8 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -310,4 +310,19 @@ test_expect_success '--short= truncates to the actual hash length' '
 	test_cmp expect actual
 '
 
+test_expect_success ':/ and HEAD^{/} favor more recent matching commits' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	(
+		cd repo &&
+		test_commit common-old &&
+		test_commit --no-tag common-new &&
+		git rev-parse HEAD >expect &&
+		git rev-parse :/common >actual &&
+		test_cmp expect actual &&
+		git rev-parse HEAD^{/common} >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done

---
base-commit: 777489f9e09c8d0dd6b12f9d90de6376330577a2
change-id: 20241206-pks-rev-parse-fix-reversed-list-0f94a20a6165
Kristoffer Haugsbakk· Dec 7, 2024, 15:51 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v3] object-name: fix reversed ordering with ":/<PATTERN>" revisions

Just bringing up this nitpick in case you decide to make a v4 based on Justin’s reply in this thread.

On Fri, Dec 6, 2024, at 16:45, Patrick Steinhardt wrote:
> Recently it was reported [1] that "look for the youngest reachable
> commit with log message that match the given pattern" syntax (e.g.
> ':/<PATTERN>') started to return results in reverse recency order. This

This isn’t “e.g. ':/<PATTERN>'” any more since you are not listing examples. “i.e.” perhaps?

But it’s clear what you mean from the context anyway.
Junio C Hamano· Dec 7, 2024, 23:24 UTC · re: Kristoffer Haugsbakk · lore

Re: [PATCH v3] object-name: fix reversed ordering with ":/<PATTERN>" revisions

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:
> This isn’t “e.g. ':/<PATTERN>'” any more since you are not listing
> examples.  “i.e.” perhaps?

Will locally amend (and with s/PATTERN/text/ to match the relevant part of the documentation). Thanks for carefully reading.

René Scharfe· Dec 9, 2024, 11:47 UTC · re: Patrick Steinhardt · lore

Re: [PATCH v3] object-name: fix reversed ordering with ":/<PATTERN>" revisions

Am 06.12.24 um 16:45 schrieb Patrick Steinhardt:
Show 6 quoted lines
> @@ -1423,7 +1423,7 @@ static int get_oid_oneline(struct repository *r,
>
>  	for (l = list; l; l = l->next) {
>  		l->item->object.flags |= ONELINE_SEEN;
> -		commit_list_insert(l->item, &copy);
> +		copy_tail = &commit_list_insert(l->item, copy_tail)->next;
OK.  The following does the same while being clearer:
		copy_tail = commit_list_append(l->item, copy_tail);

You could get the idea to do that replacement across the whole source tree. That would be nice, but must not be done blindly (e.g. with Coccinelle), as the result will be different if the second argument can point to somewhere in the middle of the list. Here it's OK because we indeed are appending (invariant *copy_tail == NULL holds).

René

← back to recent threads