Re: [PATCH v2 3/4] t: test failed "stash apply --index"

From: D. Ben Knoble <ben.knoble@gmail.com>
Date: Sep 25, 2026, 13:36 UTC
Message-ID: <CALnO6CDTaunaBby+Gy4B5vxiHES3DHpybv8Eq2JPvQ1cteGzrw@mail.gmail.com>
In-Reply-To: <232f2bf6-04d8-4a54-b4e9-51b5ee79799f@gmail.com>

Hi Phillip,

On Thu, Sep 24, 2026 at 5:42 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 34 quoted lines

>
> Hi Ben
>
> On 23/09/2026 13:58, D. Ben Knoble wrote:
> > The next commit will refactor index handling for applied stashes, so
> > let's make sure we cover conflicted index merging, too.
> >
> > Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com>
> > ---
> >   t/t3903-stash.sh | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> >
> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> > index 721158606f..3958ab3c8d 100755
> > --- a/t/t3903-stash.sh
> > +++ b/t/t3903-stash.sh
> > @@ -374,6 +374,24 @@ setup_stash() {
> >       test_cmp expect actual
> >   '
> >
> > +test_expect_success 'stash apply --index leaves everything untouched on failure' '
> > +     git reset --hard &&
> > +     echo test >other-file &&
> > +     git add other-file &&
> > +     git stash &&
> > +     echo unrelated >file &&
> > +     echo unrelated >another-file &&
> > +     git add another-file &&
> > +     git diff-files >expect &&
>
> diff-files shows the worktree blobs as null object ids, so comparing
> this before and after stashing only tells us that the same set of files
> have unstaged changes, not that the unstaged changes are the same.
> Adding "-p" would check the worktree files are unchanged.

I confess I played with diff-files and diff-index manually before trying to construct this test case, and I still don't totally understand how they're being used in the test just prior…

Anway, it looks to me like "diff-files -p" is the same as "diff -p" (albeit without some niceties like color-moved applying automatically from config), so that would make the test quite a bit more complicated, no? (The "index $sha1..$sha2" line would change…)

Since we know what the expected contents are, perhaps we can simply assert on those.

Hm. I spent some time with test_pause in the previous test, and I think my concerns about that line changing are moot. But, asserting on the contents is also a bit silly (as that's what the blob IDs are doing for us in the output).

Show 9 quoted lines

> > +     echo conflict >other-file &&
> > +     git add other-file &&
>
> I wonder if we should to add "git diff-index --cached HEAD
>  >expect-index" here so we can check the index is unchanged as well. For
> the paths that have unstaged changes we're already checking the index
> object ids via "diff-files", but I think in theory it would be possible
> to have an identical change in the index and worktree that is not picked
> up by that.

So, this test sets up an intermediate state prior to attempting to unstash where

- another-file is new in the index & working tree (content: "unrelated")
- other-file is modified in the index & working tree (content: from
"6" to "conflict")
- file is modified in the working tree (content: from "bar" to "unrelated")

And we should still be there when finished. (I wonder if, like the previous test, we should have a file that differs from itself in the index and working tree?)

So overall, I'm thinking

- (old) diff-files only shows file is changed
- diff-files -p shows us changes for file, better (and won't show the
other 2 files unless they've become unstaged)
- diff-index --cached HEAD helps us check all the index changes

Phew! Thanks for reading my rambling thinking aloud :)

-- 
D. Ben Knoble