]> git.ipfire.org Git - thirdparty/git.git/commit
stash: tell setup_revisions() to free our allocated strings
authorJeff King <peff@peff.net>
Mon, 22 Sep 2025 20:25:09 +0000 (16:25 -0400)
committerJunio C Hamano <gitster@pobox.com>
Mon, 22 Sep 2025 21:24:52 +0000 (14:24 -0700)
commit3ea35c64b0e6c86450ebadda22400295103cda64
treeb1822841dee275059dd67396416f8ee0ee1451d0
parentc44beea485f0f2feaf460e2ac87fdd5608d63cf0
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.

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.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/stash.c
t/t3903-stash.sh