Re: [PATCH] stash: allow "git stash -p <pathspec>" to assume push again

[Phillip Wood <phillip.wood123@gmail.com>]

On 16/05/2025 20:10, Junio C Hamano wrote:
> 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.

I'll fix that
>> 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

>> 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.