Hi Martin

On 06/06/2025 12:31, Martin Ågren wrote:

On Tue, 20 May 2025 at 11:27, Phillip Wood [off-list ref] wrote:

quoted

+test_expect_success 'stash -p <pathspec> stash and restores the file' '
+       cat file >expect-file &&
+       echo changed-file >file &&
+       echo changed-other-file >other-file &&
+       echo a | git stash -p file &&
+       test_cmp expect-file file &&
+       echo changed-other-file >expect &&
+       test_cmp expect other-file &&
+       git stash pop &&
+       test_cmp expect other-file &&
+       echo changed-file >expect &&
+       test_cmp expect file
+'

This only exercises the patch machinery fairly trivially: all hunks are
added. The implementation under test could miss `-p` completely and
behave as `git stash push -- file` or some variant of it, and this test
would continue to pass. (Confirmed by editing the test to not use `-p`
and seeing it run successfully.)

It might be worthwhile to set up some more elaborate scenario where you
pick only some hunks, e.g., this (whitespace-damaged) diff:

I avoided doing this because we already have tests that check "git stash 
push -p" works correctly when staging a selection of hunks and so I 
didn't think it was worth the extra complexity here when I was only 
interested it whether we parsed '-p' correctly. However you're right 
that the test passes if we ignore '-p' completely so I agree it is worth 
changing it. I'll send a re-roll.

Thanks for your thoughtful review

Phillip