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

From: Jeff King <peff@peff.net>
Date: Sep 22, 2025, 19:05 UTC
Message-ID: <20250922190555.GA2205919@coredump.intra.peff.net>
In-Reply-To: <xmqq1pnywkwv.fsf@gitster.g>

On Mon, Sep 22, 2025 at 08:45:36AM -0700, Junio C Hamano wrote:

> "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.

Oof, that is certainly unexpected. Ironically I had done all of the manual testing with "-p --", because the point of this topic was looking at how we passed arguments to setup_revisions(). And then I simplified it when I added the test, because it seemed that the "-p" would just be noise there.

So it is surprising that the test passes, but you explained that clearly below.

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

I think the difference is probably a bug, but one that is out of scope for what this patch is trying to fix. So even if we wanted to fix it, I'd prefer not to deal with it here.

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

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.

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

I'm glad you did. I would rather find out about it now while it is fresh in my mind, then 3 years from now when we all wonder what the heck is going on.

Show 11 quoted lines

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

Yeah, I agree with both points (that the current behavior is nonsense, but people may accidentally have been relying on it). I wouldn't feel _too_ bad about saying "you were relying on nonsense, and we have fixed the bug" in this case. But I think it should be a separate topic (and possibly one that makes "git stash show -- foo" do something sensible).

-Peff