[PATCH] object-name: fix reversed ordering with magic pathspecs
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Dec 6, 2024, 09:51 UTC
- Message-ID
- <20241206-pks-rev-parse-fix-reversed-list-v1-1-95a96564a4d7@pks.im>
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(-)
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 = © 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_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