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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Mon, Sep 22, 2025 at 12:36:22PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > 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.
>
>> 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.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/stash.c  | 3 ++-
>  t/t3903-stash.sh | 4 ++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index f5ddee5c7f..e5ab3c4cf5 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -957,6 +957,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op
>  static int show_stash(int argc, const char **argv, const char *prefix,
>  		      struct repository *repo UNUSED)
>  {
> +	struct setup_revision_opt opt = { .free_removed_argv_elements = 1 };
>  	int i;
>  	int ret = -1;
>  	struct stash_info info = STASH_INFO_INIT;
> @@ -1015,7 +1016,7 @@ static int show_stash(int argc, const char **argv, const char *prefix,
>  		}
>  	}
>  
> -	argc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL);
> +	argc = setup_revisions(revision_args.nr, revision_args.v, &rev, &opt);
>  	if (argc > 1)
>  		goto usage;
>  	if (!rev.diffopt.output_format) {
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 0bb4648e36..daf96aa931 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1741,4 +1741,8 @@ test_expect_success 'submodules does not affect the branch recorded in stash mes
>  	)
>  '
>  
> +test_expect_success SANITIZE_LEAK 'stash show handles -- without leaking' '
> +	git stash show --
> +'
> +
>  test_done