From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 06 Jun 2025 15:26:47 GMT
Subject: Re: [PATCH v2 1/2] stash: allow "git stash -p <pathspec>" to assume push again
Message-ID: <ffafdfe8-7754-4aa7-b2bc-ef85452f8afb@gmail.com>
In-Reply-To: <CAN0heSpGtLW8B-wtoMgW7gunMMeVTL1jhk8xN1LBbeeG4f1Fxw@mail.gmail.com>

Hi Martin

On 06/06/2025 12:31, Martin Ågren wrote:
> On Tue, 20 May 2025 at 11:27, Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> +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

> 
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index d24559a328..3b28504126 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1178,16 +1178,19 @@ test_expect_success 'stash -- <pathspec>
> stashes and restores the file' '
>   '
> 
> test_expect_success 'stash -p <pathspec> stash and restores the file' '
> -       cat file >expect-file &&
> -       echo changed-file >file &&
> +       test_write_lines b c >file &&
> +       git commit -m "a few lines" -- file &&
> +       test_write_lines a b c d >file &&
> +       test_write_lines b c d >expect-file &&
>         echo changed-other-file >other-file &&
> -       echo a | git stash -p file &&
> +       test_write_lines s y n | git stash -p file &&
>         test_cmp expect-file file &&
>         echo changed-other-file >expect &&
>         test_cmp expect other-file &&
> +       test_write_lines b c >file &&
>         git stash pop &&
>         test_cmp expect other-file &&
> -       echo changed-file >expect &&
> +       test_write_lines a b c >expect &&
>         test_cmp expect file
>   '
> 
> 
> Martin