Re: [PATCH 1/6] stash: tell setup_revisions() to free our allocated strings

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 22, 2025, 15:45 UTC
Message-ID: <xmqq1pnywkwv.fsf@gitster.g>
In-Reply-To: <20250919224027.GA594545@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 40 quoted lines

> In "git stash show", we do a first pass of parsing our command line
> options by splitting them into revision args and stash args. These are
> stored in strvecs, and we pass the revision args to setup_revisions().
>
> But setup_revisions() may modify the argv we pass it, causing us to leak
> some of the entries. In particular, if it sees a "--" string, that will
> be dropped from argv. This is the same as other cases addressed by
> f92dbdbc6a (revisions API: don't leak memory on argv elements that need
> free()-ing, 2022-08-02), and we should fix it the same way: by passing
> the free_removed_argv_elements option to setup_revisions().
>
> I've added a test here which fails when built with SANITIZE=leak because
> it calls "git stash show --". This by itself is not a very
> interesting invocation, because there is nothing after the "--", and
> thus the "--" is not really doing anything.
>
> But I think the current parsing in show_stash() is a little
> questionable. It splits the arguments into revision options and stash
> options solely based on the presence of a leading dash, with no regard
> to "--" at all. So:
>
>   git stash show -- foo
>
> will take "foo" as a stash option before we even pass anything to
> setup_revisions(). And something like:
>
>   git stash show -- 1
>
> will show stash@{1}. But I would expect anything after the "--" to be a
> pathspec. So in this example it would show only the part of the diff
> that touched "foo". And something like:
>
>   git stash show -p 1 -- foo
>
> would treat "1" as a stash and "foo" as a pathspec.
>
> That may be something we want to fix, but I want to focus here on the
> leak-fixing without changing behavior. So this test is a little odd, but
> does what we want without locking us in to any particular behavior (we
> only care that "--" by itself does not change the output nor leak).

"git stash show" gives a "git diff --stat stash~ stash" (i.e. the worktree relative to then-current-HEAD in diffstat format), and "git stash show --" (no other arguments) gives us "git diff -p" for the same, it seems; this is with or without your patch.

We may care "--" by itself does not change the output, but it has already been giving different output without your patch.

Show 15 quoted lines

> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 0bb4648e36..1c9e589bbe 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1741,4 +1741,10 @@ test_expect_success 'submodules does not affect the branch recorded in stash mes
>  	)
>  '
>  
> +test_expect_success 'stash show handles --' '
> +	git stash show >expect &&
> +	git stash show -- >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done

In other words, this test should not pass if the stash stores something reasonable. The only case is when both "git stash show" and "git stash show -p" would have been silent.

The commit object pointed at by refs/stash before this test runs has this curious thing.

    tree c5f56db0c5c4e40b68ffda4bee76b301c9cc3406
    parent a2a54b9cb0873c567fbab134c4b95020b9419f6c
    parent 182836fa1adbc25d81a137d4e7489cbd0bec744a
    parent f93620bcecd8173d3bd3ad4dff4e69e32ba6f278
    author A U Thor <author@example.com> 1112912473 -0700
    committer C O Mitter <committer@example.com> 1112912473 -0700

    WIP on orphan2: a2a54b9 A

The trees of these three parents reveal why this test passes.

    c5f56db0c5c4e40b68ffda4bee76b301c9cc3406
    c5f56db0c5c4e40b68ffda4bee76b301c9cc3406
    adc18b651de078499a4acf1454fadb65352ed961

In other words, "git diff refs/stash~$n refs/stash" for n == 1 & n == 2 are empty and this stash entry exists only to record the untracked cruft. Even "git show -c refs/stash" would stay silent for a "merge" like this, and both "git stash show" and "git stash show -p" would of course be empty.

I do not think we want to drop this test (we do want the "handles without leaking" part of the test), but we should not expect the output from these commands match.

Unless we are aspiring to introduce a breaking change, that is [*].

Perhaps create a new stash entry that does not show anything in this test and drop that entry with test_when_finished before leaving it? Or just run "git stash show --" without any check on its output, possibly with a prereq that we are running under leak checker?

I dunno.

I only discovered this while merging this and another topic that happen to touch the same t3903 into 'seen'.

Thanks.

[Footnote]

* I personally find the traditional behaviour nonsense and it may be
coming from the crappy command line parsing we have had forever, but
I am sure people who wrote

    git stash show --
    git stash show --end-of-options

out of "principle" in their scripts, and assumed that the patch output is the norm for the command even though it should have been giving diffstat, would be unhappy if we suddenly made them behave exactly like "git stash show" (nothing else on the command line).