Re: [PATCH v3 0/2] stash: fix and improve "git stash -p <pathspec>"

From: Martin Ågren <martin.agren@gmail.com>
Date: Jun 7, 2025, 12:56 UTC
Message-ID: <CAN0heSotWpNmqd905aknVTfk6WEcYifAwbXBKYfAWkhzxua3ZA@mail.gmail.com>
In-Reply-To: <cover.1749289514.git.phillip.wood@dunelm.org.uk>

Hi Phillip,

On Sat, 7 Jun 2025 at 11:45, Phillip Wood <phillip.wood123@gmail.com> wrote:

> Range-diff against v2:

Show 13 quoted lines

>      +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 "add 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 &&

This range-diff matches what I'd expect. Now this test makes sure we really pick up the `-p`. On that note ... I just realized that all of these would keep the test passing:

 test_write_lines s y n | git stash -p file # what you have
 test_write_lines s y n | git stash -p file otherfile
 test_write_lines s y n | git stash -p .
 test_write_lines s y n | git stash -p

So the implementation under test could bungle the pathspec, query the user for both `file` and `otherfile` (in that order!), get EOF from stdin while handling `otherfile`, leave it out of the stash, and end up passing the test. We could try to protect against this by providing another "y": if git wants to read something after our "s y n" sequence, we'll give it a "y" in the hopes that it will trip things up. We do want to test the handling of pathspecs here, so maybe tighten this?

>     ++  git checkout HEAD -- file &&

This is better than what I had in my "maybe something like this". This explicitly restores the file.

Martin