Jeff King [off-list ref] writes:

On Mon, Sep 22, 2025 at 12:36:22PM -0700, Junio C Hamano wrote:

quoted

Jeff King [off-list ref] writes:

quoted

Yeah, I had originally written just:

  test_expect_success 'stash show -- does not leak' '
	git stash show --
  '

but it felt funny, since the test is doing nothing in a build without
SANITIZE=leak. If we are OK with that funniness, I can switch back to
that.

We have a prerequisite for that.  Very nice vehicle for
documentation purposes, even though we do not care about a single
"stash show" invocation for correctness or performance reasons.

Ah, nice. It crossed my mind that we could try marking it as such, but I
forgot (or never knew) that we already had this prereq available. I
guess it was mainly used as "!SANITIZE_LEAK" before for suppressing
leaks.

quoted

Perhaps I can squash the following in, unless you have other changes
in mind.

Thanks, that looks perfect for the code change. But we probably need to
update the discussion of the test in the commit message. Here's what I
came up with (replacing patch 1):

Thanks, will replace then.

-- >8 --
Subject: [PATCH] stash: tell setup_revisions() to free our allocated strings

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

The added test here is run only with SANITIZE=leak, without checking its
output, because the behavior of stash with "--" is a little odd:

  1. Running "git stash show" will show --stat output. But running "git
     stash show --" will show --patch.

  2. I'd expect a non-option after "--" to be treated as a pathspec, so:

       git stash show -p 1 -- foo

     would look treat "1" as a stash (a synonym for stash@{1}) and
     restrict the resulting diff to "foo". But it doesn't. We split the
     revision/stash args without any regard to "--". So in the example
     above both "1" and "foo" are stashes. Which is an error, but also:

       git stash show -- foo

     treats "foo" as a stash, not a pathspec.

All good.  I do regret that I didn't resist harder against the "1 is
the same as stash@{1}" or insisted that the parser be written more
robustly if we wanted to support such syntax, but that also is way
too late to change now.

These are both oddities that we may want to address (or may not, if we
want to retain historical quirks). But they are well outside the scope
of this patch. So for now we'll just let the tests confirm we aren't
leaking without otherwise expecting any behavior. If we later address
either of those points and end up with another test that covers "stash
show --", we can drop this leak-only test.

OK.  

By the way this also solves the original "git stash show -p --invalid";
do we want to credit the bug reporter and mention that issue as well?

Thanks.