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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxx/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)

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

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

>
> Fix the bug by appending to the list instead.
>
> Reported-by: Aarni Koskela <aarni@xxxxxxxxxxx>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> [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?

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

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





[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux