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

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

[Phillip Wood]

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

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
Base-Commit: 1a8a4971cc6c179c4dd711f4a7f5d7178f4b3ab7
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fstash-assume-push-with-dash-p%2Fv1
View-Changes-At: https://github.com/phillipwood/git/compare/1a8a4971c...6292feee7
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/stash-assume-push-with-dash-p/v1

 builtin/stash.c  | 10 +++++++---
 t/t3903-stash.sh | 19 +++++++++++++++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index cfbd92852a6..b12fd6c40f1 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1789,11 +1789,15 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 	int ret;
 
 	if (argc) {
-		force_assume = !strcmp(argv[0], "-p");
+		int flags = PARSE_OPT_KEEP_DASHDASH;
+
+		if (push_assumed)
+			flags |= PARSE_OPT_STOP_AT_NON_OPTION;
+
 		argc = parse_options(argc, argv, prefix, options,
 				     push_assumed ? git_stash_usage :
-				     git_stash_push_usage,
-				     PARSE_OPT_KEEP_DASHDASH);
+				     git_stash_push_usage, flags);
+		force_assume |= patch_mode;
 	}
 
 	if (argc) {
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 74666ff3e4b..295cb508a35 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1177,6 +1177,25 @@ test_expect_success 'stash -- <pathspec> stashes and restores the file' '
 	test_path_is_file bar
 '
 
+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
+'
+
+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
+'
+
 test_expect_success 'stash -- <pathspec> stashes in subdirectory' '
 	mkdir sub &&
 	>foo &&
-- 
2.49.0.897.gfad3eb7d210

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

[Junio C Hamano]

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.

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

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

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

[Phillip Wood]

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.

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

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

"git stash -p <pathspec>" should imply "git stash push -p <pathspec>"
but that was broken by a code cleanup in c3713cede7 (stash: eliminate
crude option parsing, 2020-02-17). This regression is fixed in the
first patch. Although "-p" implies the "push" subcommand "--patch"
has never implied "push". That is fixed in the second patch.

Thanks to Junio for his comments on V1.

Changes since V1:
 - Split out the regression fix into its own patch

Base-Commit: 1a8a4971cc6c179c4dd711f4a7f5d7178f4b3ab7
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fstash-assume-push-with-dash-p%2Fv2
View-Changes-At: https://github.com/phillipwood/git/compare/1a8a4971c...98ad3de97
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/stash-assume-push-with-dash-p/v2


Phillip Wood (2):
  stash: allow "git stash -p <pathspec>" to assume push again
  stash: allow "git stash [<options>] --patch <pathspec>" to assume push

 builtin/stash.c  | 10 +++++++---
 t/t3903-stash.sh | 19 +++++++++++++++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

Range-diff against v1:
1:  6292feee7c4 ! 1:  2cd67f5cd85 stash: allow "git stash -p <pathspec>" to assume push again
    @@ Commit message
         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
    +    push subcommand supports pathspecs, 9e140909f61 (stash: allow pathspecs
         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
    -    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.
    +    Fix this by regression by checking argv[1] instead of argv[0] and add a
    +    couple of tests to prevent future regressions.
     
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
    @@ builtin/stash.c: static int push_stash(int argc, const char **argv, const char *
      
      	if (argc) {
     -		force_assume = !strcmp(argv[0], "-p");
    -+		int flags = PARSE_OPT_KEEP_DASHDASH;
    -+
    -+		if (push_assumed)
    -+			flags |= PARSE_OPT_STOP_AT_NON_OPTION;
    -+
    ++		force_assume = argc > 1 && !strcmp(argv[1], "-p");
      		argc = parse_options(argc, argv, prefix, options,
      				     push_assumed ? git_stash_usage :
    --				     git_stash_push_usage,
    --				     PARSE_OPT_KEEP_DASHDASH);
    -+				     git_stash_push_usage, flags);
    -+		force_assume |= patch_mode;
    - 	}
    - 
    - 	if (argc) {
    + 				     git_stash_push_usage,
     
      ## t/t3903-stash.sh ##
     @@ t/t3903-stash.sh: test_expect_success 'stash -- <pathspec> stashes and restores the file' '
      	test_path_is_file bar
      '
      
    -+test_expect_success 'stash --patch <pathspec> stash and restores the file' '
    ++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 -m "stash bar" --patch file &&
    ++	echo a | git stash -p file &&
     +	test_cmp expect-file file &&
     +	echo changed-other-file >expect &&
     +	test_cmp expect other-file &&
-:  ----------- > 2:  98ad3de9770 stash: allow "git stash [<options>] --patch <pathspec>" to assume push
-- 
2.49.0.897.gfad3eb7d210

[PATCH v2 1/2] stash: allow "git stash -p <pathspec>" to assume push again

[Phillip Wood]

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

Fix this by regression by checking argv[1] instead of argv[0] and add a
couple of tests to prevent future regressions.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/stash.c  |  2 +-
 t/t3903-stash.sh | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index cfbd92852a6..bc2c34fa048 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1789,7 +1789,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 	int ret;
 
 	if (argc) {
-		force_assume = !strcmp(argv[0], "-p");
+		force_assume = argc > 1 && !strcmp(argv[1], "-p");
 		argc = parse_options(argc, argv, prefix, options,
 				     push_assumed ? git_stash_usage :
 				     git_stash_push_usage,
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 74666ff3e4b..d24559a328d 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1177,6 +1177,25 @@ test_expect_success 'stash -- <pathspec> stashes and restores the file' '
 	test_path_is_file bar
 '
 
+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
+'
+
+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
+'
+
 test_expect_success 'stash -- <pathspec> stashes in subdirectory' '
 	mkdir sub &&
 	>foo &&
-- 
2.49.0.897.gfad3eb7d210

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

[Martin Ågren]

On Tue, 20 May 2025 at 11:27, Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Fix this by regression by checking argv[1] instead of argv[0] and add a
> couple of tests to prevent future regressions.

>         if (argc) {
> -               force_assume = !strcmp(argv[0], "-p");
> +               force_assume = argc > 1 && !strcmp(argv[1], "-p");
>                 argc = parse_options(argc, argv, prefix, options,
>                                      push_assumed ? git_stash_usage :
>                                      git_stash_push_usage,

After reading up on 8c3713cede (stash: eliminate crude option parsing,
2020-02-17), I share your analysis. This fix looks correct to me.

> +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:

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

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

[Phillip Wood]

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

[PATCH v2 2/2] stash: allow "git stash [<options>] --patch <pathspec>" to assume push

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The support for assuming "push" when "-p" is given introduced in
9e140909f61 (stash: allow pathspecs in the no verb form, 2017-02-28) is
very narrow, neither "git stash -m <message> -p <pathspec>" nor "git
stash --patch <pathspec>" imply "push" and die instead. Relax this by
passing 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 that it does not assume the user meant
"push" if they mistype a subcommand name but "git stash -m <message> -p
<pathspec>" will now succeed. The test added in the last commit is
adjusted to check that push is still assumed when "--patch" comes after
other options on the command-line.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/stash.c  | 10 +++++++---
 t/t3903-stash.sh |  4 ++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index bc2c34fa048..b12fd6c40f1 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1789,11 +1789,15 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 	int ret;
 
 	if (argc) {
-		force_assume = argc > 1 && !strcmp(argv[1], "-p");
+		int flags = PARSE_OPT_KEEP_DASHDASH;
+
+		if (push_assumed)
+			flags |= PARSE_OPT_STOP_AT_NON_OPTION;
+
 		argc = parse_options(argc, argv, prefix, options,
 				     push_assumed ? git_stash_usage :
-				     git_stash_push_usage,
-				     PARSE_OPT_KEEP_DASHDASH);
+				     git_stash_push_usage, flags);
+		force_assume |= patch_mode;
 	}
 
 	if (argc) {
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index d24559a328d..295cb508a35 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1177,11 +1177,11 @@ test_expect_success 'stash -- <pathspec> stashes and restores the file' '
 	test_path_is_file bar
 '
 
-test_expect_success 'stash -p <pathspec> stash and restores the file' '
+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 -p 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 &&
-- 
2.49.0.897.gfad3eb7d210

Re: [PATCH v2 2/2] stash: allow "git stash [<options>] --patch <pathspec>" to assume push

[Martin Ågren]

On Tue, 20 May 2025 at 11:27, Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> The support for assuming "push" when "-p" is given introduced in
> 9e140909f61 (stash: allow pathspecs in the no verb form, 2017-02-28) is
> very narrow, neither "git stash -m <message> -p <pathspec>" nor "git
> stash --patch <pathspec>" imply "push" and die instead. Relax this by
> passing 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 that it does not assume the user meant
> "push" if they mistype a subcommand name but "git stash -m <message> -p
> <pathspec>" will now succeed. The test added in the last commit is
> adjusted to check that push is still assumed when "--patch" comes after
> other options on the command-line.

All makes sense to me.

>         if (argc) {
> -               force_assume = argc > 1 && !strcmp(argv[1], "-p");

This is where we drop the very specific approach of "let's look for -p".

> +               int flags = PARSE_OPT_KEEP_DASHDASH;

This is the flag we've always been using.

> +               if (push_assumed)
> +                       flags |= PARSE_OPT_STOP_AT_NON_OPTION;

Now we use this, too, if we've assumed "push". Makes sense even without
the specific context of this patch: we've assumed an implicit "push", so
let's be a bit less aggressive in parsing the remainder.

>                 argc = parse_options(argc, argv, prefix, options,
>                                      push_assumed ? git_stash_usage :
> -                                    git_stash_push_usage,
> -                                    PARSE_OPT_KEEP_DASHDASH);
> +                                    git_stash_push_usage, flags);
> +               force_assume |= patch_mode;

Rather than looking for "-p" in a fixed place, we see if option parsing
spotted it. Makes perfect sense. Although, why `|=` here? We initialize
`force_assume` to 0 at the top and this is the only other time we write
to it. Why not just `force_assume = patch_mode`? Future-proofing?

> -test_expect_success 'stash -p <pathspec> stash and restores the file' '
> +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 -p 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 &&

We lose the test of `-p` that we just added. Ok. We should be able to
trust our option parsing machinery to get this right. This s/-p/--patch/
demonstrates that your patch works, and as for running this as a
regression test in the future, we'll be using one of the equivalent ways
of spelling this option. Ok.


Martin

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Phillip Wood (2):
>   stash: allow "git stash -p <pathspec>" to assume push again
>   stash: allow "git stash [<options>] --patch <pathspec>" to assume push

Thanks, queued.

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> "git stash -p <pathspec>" should imply "git stash push -p <pathspec>"
> but that was broken by a code cleanup in c3713cede7 (stash: eliminate
> crude option parsing, 2020-02-17). This regression is fixed in the
> first patch. Although "-p" implies the "push" subcommand "--patch"
> has never implied "push". That is fixed in the second patch.
>
> Thanks to Junio for his comments on V1.
>
> Changes since V1:
>  - Split out the regression fix into its own patch
>
> Base-Commit: 1a8a4971cc6c179c4dd711f4a7f5d7178f4b3ab7
> Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fstash-assume-push-with-dash-p%2Fv2
> View-Changes-At: https://github.com/phillipwood/git/compare/1a8a4971c...98ad3de97
> Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/stash-assume-push-with-dash-p/v2
>
>
> Phillip Wood (2):
>   stash: allow "git stash -p <pathspec>" to assume push again
>   stash: allow "git stash [<options>] --patch <pathspec>" to assume push

Are other people interested in this work?  I haven't seen any
comments other than a few nitpicky one form mine, and want to (1)
gauge the interest in the fix, and (2) see how well reviewed it is
(and my review or reading over the patches again would not count all
that much here).

Thanks.

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

[Martin Ågren]

On Wed, 4 Jun 2025 at 00:11, Junio C Hamano <gitster@pobox.com> wrote:
>
> > Phillip Wood (2):
> >   stash: allow "git stash -p <pathspec>" to assume push again
> >   stash: allow "git stash [<options>] --patch <pathspec>" to assume push
>
> Are other people interested in this work?  I haven't seen any
> comments other than a few nitpicky one form mine, and want to (1)
> gauge the interest in the fix, and (2) see how well reviewed it is
> (and my review or reading over the patches again would not count all
> that much here).

On reading the patches, I realized that I have some interest in this. I
left some comments. Most of them amount to thinking out loud, but I do
think that the new test could do a bit better at proving that the
(fixed/improved) implementation actually ends up picking up `-p` at all.

A nice, pleasant read. The series has a well-defined focus.

Martin

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

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

"git stash -p <pathspec>" should imply "git stash push -p <pathspec>"
but that was broken by a code cleanup in c3713cede7 (stash: eliminate
crude option parsing, 2020-02-17). This regression is fixed in the
first patch. Although "-p" implies the "push" subcommand "--patch"
has never implied "push". That is fixed in the second patch.

Thanks to Martin for his comments on V2

Changes since V2:
 - Made test stricter as suggested by Martin

Thanks to Junio for his comments on V1.

Changes since V1:
 - Split out the regression fix into its own patch

Base-Commit: 1a8a4971cc6c179c4dd711f4a7f5d7178f4b3ab7
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fstash-assume-push-with-dash-p%2Fv3
View-Changes-At: https://github.com/phillipwood/git/compare/1a8a4971c...d3a958430
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/stash-assume-push-with-dash-p/v3


Phillip Wood (2):
  stash: allow "git stash -p <pathspec>" to assume push again
  stash: allow "git stash [<options>] --patch <pathspec>" to assume push

 builtin/stash.c  | 10 +++++++---
 t/t3903-stash.sh | 22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

Range-diff against v2:
1:  2cd67f5cd85 ! 1:  c147eaf2eae stash: allow "git stash -p <pathspec>" to assume push again
    @@ Commit message
         Fix this by regression by checking argv[1] instead of argv[0] and add a
         couple of tests to prevent future regressions.
     
    +    Helped-by: Martin Ågren <martin.agren@gmail.com>
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## builtin/stash.c ##
    @@ t/t3903-stash.sh: test_expect_success 'stash -- <pathspec> stashes and restores
      '
      
     +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 &&
    ++	git checkout HEAD -- file &&
     +	git stash pop &&
     +	test_cmp expect other-file &&
    -+	echo changed-file >expect &&
    ++	test_write_lines a b c >expect &&
     +	test_cmp expect file
     +'
     +
2:  98ad3de9770 ! 2:  d3a95843055 stash: allow "git stash [<options>] --patch <pathspec>" to assume push
    @@ t/t3903-stash.sh: test_expect_success 'stash -- <pathspec> stashes and restores
      
     -test_expect_success 'stash -p <pathspec> stash and restores the file' '
     +test_expect_success 'stash --patch <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 &&
    -+	echo a | git stash -m "stash bar" --patch file &&
    +-	test_write_lines s y n | git stash -p file &&
    ++	test_write_lines s y n | git stash -m "stash bar" --patch file &&
      	test_cmp expect-file file &&
      	echo changed-other-file >expect &&
      	test_cmp expect other-file &&
-- 
2.49.0.897.gfad3eb7d210

[PATCH v3 1/2] stash: allow "git stash -p <pathspec>" to assume push again

[Phillip Wood]

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

Fix this by regression by checking argv[1] instead of argv[0] and add a
couple of tests to prevent future regressions.

Helped-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/stash.c  |  2 +-
 t/t3903-stash.sh | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index cfbd92852a6..bc2c34fa048 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1789,7 +1789,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 	int ret;
 
 	if (argc) {
-		force_assume = !strcmp(argv[0], "-p");
+		force_assume = argc > 1 && !strcmp(argv[1], "-p");
 		argc = parse_options(argc, argv, prefix, options,
 				     push_assumed ? git_stash_usage :
 				     git_stash_push_usage,
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 74666ff3e4b..a99a746221e 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1177,6 +1177,28 @@ test_expect_success 'stash -- <pathspec> stashes and restores the file' '
 	test_path_is_file bar
 '
 
+test_expect_success 'stash -p <pathspec> stash and restores the 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 &&
+	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 &&
+	git checkout HEAD -- file &&
+	git stash pop &&
+	test_cmp expect other-file &&
+	test_write_lines a b c >expect &&
+	test_cmp expect file
+'
+
+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
+'
+
 test_expect_success 'stash -- <pathspec> stashes in subdirectory' '
 	mkdir sub &&
 	>foo &&
-- 
2.49.0.897.gfad3eb7d210

[PATCH v3 2/2] stash: allow "git stash [<options>] --patch <pathspec>" to assume push

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The support for assuming "push" when "-p" is given introduced in
9e140909f61 (stash: allow pathspecs in the no verb form, 2017-02-28) is
very narrow, neither "git stash -m <message> -p <pathspec>" nor "git
stash --patch <pathspec>" imply "push" and die instead. Relax this by
passing 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 that it does not assume the user meant
"push" if they mistype a subcommand name but "git stash -m <message> -p
<pathspec>" will now succeed. The test added in the last commit is
adjusted to check that push is still assumed when "--patch" comes after
other options on the command-line.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/stash.c  | 10 +++++++---
 t/t3903-stash.sh |  4 ++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index bc2c34fa048..b12fd6c40f1 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1789,11 +1789,15 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 	int ret;
 
 	if (argc) {
-		force_assume = argc > 1 && !strcmp(argv[1], "-p");
+		int flags = PARSE_OPT_KEEP_DASHDASH;
+
+		if (push_assumed)
+			flags |= PARSE_OPT_STOP_AT_NON_OPTION;
+
 		argc = parse_options(argc, argv, prefix, options,
 				     push_assumed ? git_stash_usage :
-				     git_stash_push_usage,
-				     PARSE_OPT_KEEP_DASHDASH);
+				     git_stash_push_usage, flags);
+		force_assume |= patch_mode;
 	}
 
 	if (argc) {
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index a99a746221e..2bba3baa10f 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1177,13 +1177,13 @@ test_expect_success 'stash -- <pathspec> stashes and restores the file' '
 	test_path_is_file bar
 '
 
-test_expect_success 'stash -p <pathspec> stash and restores the file' '
+test_expect_success 'stash --patch <pathspec> stash and restores the 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 &&
-	test_write_lines s y n | git stash -p file &&
+	test_write_lines s y n | git stash -m "stash bar" --patch file &&
 	test_cmp expect-file file &&
 	echo changed-other-file >expect &&
 	test_cmp expect other-file &&
-- 
2.49.0.897.gfad3eb7d210

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

[Martin Ågren]

Hi Phillip,

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

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

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

[Phillip Wood]

Hi Martin

On 07/06/2025 13:56, Martin Ågren wrote:
> Hi Phillip,
> 
> On Sat, 7 Jun 2025 at 11:45, Phillip Wood <phillip.wood123@gmail.com> wrote:
> [...]
> 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?

Junio has merged this to next now. I was hoping that we would already 
have coverage for this with other tests but I couldn't see anything so 
I'll look at improving the coverage for "git stash push -p <pathspec>" 
in the next release cycle.

Thanks

Phillip

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

[Martin Ågren]

Hi Phillip,

On Mon, 9 Jun 2025 at 11:42, Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 07/06/2025 13:56, Martin Ågren wrote:
> >
> > On Sat, 7 Jun 2025 at 11:45, Phillip Wood <phillip.wood123@gmail.com> wrote:
> > [...]
> > 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?
>
> Junio has merged this to next now. I was hoping that we would already
> have coverage for this with other tests but I couldn't see anything so
> I'll look at improving the coverage for "git stash push -p <pathspec>"
> in the next release cycle.

Ok, makes sense. Those would certainly be good regression tests to have.
I did some manual testing when I wrote the above and feel confident,
FWIW, that it works correctly as of now.

Thanks for these git-stash improvements.

Martin