From: D. Ben Knoble Date: Fri, 25 Sep 2026 13:36:15 GMT Subject: Re: [PATCH v2 3/4] t: test failed "stash apply --index" Message-ID: In-Reply-To: <232f2bf6-04d8-4a54-b4e9-51b5ee79799f@gmail.com> Hi Phillip, On Thu, Sep 24, 2026 at 5:42 AM Phillip Wood wrote: > > 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 > > --- > > 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). > > + 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