Re: [PATCH] stash: allow "git stash -p <pathspec>" to assume push again
On 16/05/2025 20:10, Junio C Hamano wrote:
Show 11 quoted lines
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Historically "git stash [<options>]" was assumed to mean "git stash save
>> [<options>]". Since 1ada5020b38 (stash: use stash_push for no verb form,
>> 2017-02-28) it is assumed to mean "git stash push [<options>]". As the
>> push subcommand supports pathspecs 9e140909f61 (stash: allow pathspecs
>
> Can I safely do "pathspecs" -> "pathspecs," here? I found this sentence
> hard to read without a comma.
Show 16 quoted lines
>> in the no verb form, 2017-02-28) allowed "git stash -p <pathspec>" to
>> mean "git stash push -p <pathspec>". This was broken in 8c3713cede7
>> (stash: eliminate crude option parsing, 2020-02-17) which failed to
>> account for "push" being added to the start of argv in cmd_stash()
>> before it calls push_stash() and kept looking in argv[0] for "-p" after
>> moving the code to push_stash().
>>
>> The support for assuming "push" when "-p" is given introduced in
>> 9e140909f61 is very narrow, neither "git stash -m <message> -p
>> <pathspec>" nor "git stash --patch <pathspec>" imply "push" and die
>> instead. Fix the regression introduced by 8c3713cede7 and relax the
>> behavior introduced in 9e140909f61 by passing
>
> Hmph, is it too much work to have a patch that only fixes the
> regression and another that extends the feature on top as a separate
> patch? Not that I am opposed by the new feature, though.
I can do that, I was just being lazy skipping the separate regression fix
Thanks
Phillip
Show 32 quoted lines
>> PARSE_OPT_STOP_AT_NON_OPTION when push is being assumed and then setting
>> "force_assume" if "--patch" was present. This means "git stash
>> <pathspec> -p" still dies so do assume the user meant "push" if they
>> mistype a subcommand name but "git stash -m <message> -p <pathspec>"
>> will now succeed.
>
>> Tests are added to prevent future regressions.
>
> Nice.
>
>> +test_expect_success 'stash --patch <pathspec> stash and restores the file' '
>> + cat file >expect-file &&
>> + echo changed-file >file &&
>> + echo changed-other-file >other-file &&
>> + echo a | git stash -m "stash bar" --patch 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
>> +'
>
> OK.
>
>> +test_expect_success 'stash <pathspec> -p is rejected' '
>> + test_must_fail git stash file -p 2>err &&
>> + test_grep "subcommand wasn${SQ}t specified; ${SQ}push${SQ} can${SQ}t be assumed due to unexpected token ${SQ}file${SQ}" err
>> +'
>
> Good thing to test.