Jeff King [off-list ref] writes:

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.

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 [off-list ref] 1112912473 -0700
    committer C O Mitter [off-list ref] 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).