{"thread":{"id":"63470","subject":"[PATCH] stash: allow \"git stash -p \" to assume push again","startedAt":"2025-05-16T14:59:02Z","lastAt":"2025-06-10T09:56:24Z","messageCount":18,"participants":["Phillip Wood","Junio C Hamano","Martin Ågren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"518250","messageId":"6292feee7c4347efad31e9fb2a1763779b7df133.1747407473.git.phillip.wood@dunelm.org.uk","threadId":"63470","inReplyTo":null,"subject":"[PATCH] stash: allow \"git stash -p \" to assume push again","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-16T14:58:29Z","receivedAt":"2025-05-16T14:59:02Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nHistorically \"git stash []\" was assumed to mean \"git stash save\n[]\". Since 1ada5020b38 (stash: use stash_push for no verb form,\n2017-02-28) it is assumed to mean \"git stash push []\". As the\npush subcommand supports pathspecs 9e140909f61 (stash: allow pathspecs\nin the no verb form, 2017-02-28) allowed \"git stash -p \" to\nmean \"git stash push -p \". This was broken in 8c3713cede7\n(stash: eliminate crude option parsing, 2020-02-17) which failed to\naccount for \"push\" being added to the start of argv in cmd_stash()\nbefore it calls push_stash() and kept looking in argv[0] for \"-p\" after\nmoving the code to push_stash().\n\nThe support for assuming \"push\" when \"-p\" is given introduced in\n9e140909f61 is very narrow, neither \"git stash -m -p\n\" nor \"git stash --patch \" imply \"push\" and die\ninstead. Fix the regression introduced by 8c3713cede7 and relax the\nbehavior introduced in 9e140909f61 by passing\nPARSE_OPT_STOP_AT_NON_OPTION when push is being assumed and then setting\n\"force_assume\" if \"--patch\" was present. This means \"git stash\n -p\" still dies so do assume the user meant \"push\" if they\nmistype a subcommand name but \"git stash -m -p \"\nwill now succeed. Tests are added to prevent future regressions.\n\nSigned-off-by: Phillip Wood \n---\nBase-Commit: 1a8a4971cc6c179c4dd711f4a7f5d7178f4b3ab7\nPublished-As: https://github.com/phillipwood/git/releases/tag/pw%2Fstash-assume-push-with-dash-p%2Fv1\nView-Changes-At: https://github.com/phillipwood/git/compare/1a8a4971c...6292feee7\nFetch-It-Via: git fetch https://github.com/phillipwood/git pw/stash-assume-push-with-dash-p/v1\n\n builtin/stash.c | 10 +++++++---\n t/t3903-stash.sh | 19 +++++++++++++++++++\n 2 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex cfbd92852a6..b12fd6c40f1 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1789,11 +1789,15 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n \tint ret;\n \n \tif (argc) {\n-\t\tforce_assume = !strcmp(argv[0], \"-p\");\n+\t\tint flags = PARSE_OPT_KEEP_DASHDASH;\n+\n+\t\tif (push_assumed)\n+\t\t\tflags |= PARSE_OPT_STOP_AT_NON_OPTION;\n+\n \t\targc = parse_options(argc, argv, prefix, options,\n \t\t\t\t push_assumed ? git_stash_usage :\n-\t\t\t\t git_stash_push_usage,\n-\t\t\t\t PARSE_OPT_KEEP_DASHDASH);\n+\t\t\t\t git_stash_push_usage, flags);\n+\t\tforce_assume |= patch_mode;\n \t}\n \n \tif (argc) {\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 74666ff3e4b..295cb508a35 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1177,6 +1177,25 @@ test_expect_success 'stash -- stashes and restores the file' '\n \ttest_path_is_file bar\n '\n \n+test_expect_success 'stash --patch stash and restores the file' '\n+\tcat file >expect-file &&\n+\techo changed-file >file &&\n+\techo changed-other-file >other-file &&\n+\techo a | git stash -m \"stash bar\" --patch file &&\n+\ttest_cmp expect-file file &&\n+\techo changed-other-file >expect &&\n+\ttest_cmp expect other-file &&\n+\tgit stash pop &&\n+\ttest_cmp expect other-file &&\n+\techo changed-file >expect &&\n+\ttest_cmp expect file\n+'\n+\n+test_expect_success 'stash -p is rejected' '\n+\ttest_must_fail git stash file -p 2>err &&\n+\ttest_grep \"subcommand wasn${SQ}t specified; ${SQ}push${SQ} can${SQ}t be assumed due to unexpected token ${SQ}file${SQ}\" err\n+'\n+\n test_expect_success 'stash -- stashes in subdirectory' '\n \tmkdir sub &&\n \t>foo &&\n-- \n2.49.0.897.gfad3eb7d210\n\n"},{"id":"518304","messageId":"xmqqtt5ktlqm.fsf@gitster.g","threadId":"63470","inReplyTo":"6292feee7c4347efad31e9fb2a1763779b7df133.1747407473.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH] stash: allow \"git stash -p \" to assume push again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-16T19:10:41Z","receivedAt":"2025-05-16T19:10:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> From: Phillip Wood \n>\n> Historically \"git stash []\" was assumed to mean \"git stash save\n> []\". Since 1ada5020b38 (stash: use stash_push for no verb form,\n> 2017-02-28) it is assumed to mean \"git stash push []\". As the\n> push subcommand supports pathspecs 9e140909f61 (stash: allow pathspecs\n\nCan I safely do \"pathspecs\" -> \"pathspecs,\" here? I found this sentence\nhard to read without a comma.\n\n> in the no verb form, 2017-02-28) allowed \"git stash -p \" to\n> mean \"git stash push -p \". This was broken in 8c3713cede7\n> (stash: eliminate crude option parsing, 2020-02-17) which failed to\n> account for \"push\" being added to the start of argv in cmd_stash()\n> before it calls push_stash() and kept looking in argv[0] for \"-p\" after\n> moving the code to push_stash().\n>\n> The support for assuming \"push\" when \"-p\" is given introduced in\n> 9e140909f61 is very narrow, neither \"git stash -m -p\n> \" nor \"git stash --patch \" imply \"push\" and die\n> instead. Fix the regression introduced by 8c3713cede7 and relax the\n> behavior introduced in 9e140909f61 by passing\n\nHmph, is it too much work to have a patch that only fixes the\nregression and another that extends the feature on top as a separate\npatch? Not that I am opposed by the new feature, though.\n\n> PARSE_OPT_STOP_AT_NON_OPTION when push is being assumed and then setting\n> \"force_assume\" if \"--patch\" was present. This means \"git stash\n> -p\" still dies so do assume the user meant \"push\" if they\n> mistype a subcommand name but \"git stash -m -p \"\n> will now succeed.\n\n> Tests are added to prevent future regressions.\n\nNice.\n\n> +test_expect_success 'stash --patch stash and restores the file' '\n> +\tcat file >expect-file &&\n> +\techo changed-file >file &&\n> +\techo changed-other-file >other-file &&\n> +\techo a | git stash -m \"stash bar\" --patch file &&\n> +\ttest_cmp expect-file file &&\n> +\techo changed-other-file >expect &&\n> +\ttest_cmp expect other-file &&\n> +\tgit stash pop &&\n> +\ttest_cmp expect other-file &&\n> +\techo changed-file >expect &&\n> +\ttest_cmp expect file\n> +'\n\nOK.\n\n> +test_expect_success 'stash -p is rejected' '\n> +\ttest_must_fail git stash file -p 2>err &&\n> +\ttest_grep \"subcommand wasn${SQ}t specified; ${SQ}push${SQ} can${SQ}t be assumed due to unexpected token ${SQ}file${SQ}\" err\n> +'\n\nGood thing to test.\n"},{"id":"518473","messageId":"0ca879cf-303c-406f-8040-cc0c7e9b0964@gmail.com","threadId":"63470","inReplyTo":"xmqqtt5ktlqm.fsf@gitster.g","subject":"Re: [PATCH] stash: allow \"git stash -p \" to assume push again","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-20T09:21:17Z","receivedAt":"2025-05-20T09:21:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 16/05/2025 20:10, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> From: Phillip Wood \n>>\n>> Historically \"git stash []\" was assumed to mean \"git stash save\n>> []\". Since 1ada5020b38 (stash: use stash_push for no verb form,\n>> 2017-02-28) it is assumed to mean \"git stash push []\". As the\n>> push subcommand supports pathspecs 9e140909f61 (stash: allow pathspecs\n> \n> Can I safely do \"pathspecs\" -> \"pathspecs,\" here? I found this sentence\n> hard to read without a comma.\n\nI'll fix that\n>> in the no verb form, 2017-02-28) allowed \"git stash -p \" to\n>> mean \"git stash push -p \". This was broken in 8c3713cede7\n>> (stash: eliminate crude option parsing, 2020-02-17) which failed to\n>> account for \"push\" being added to the start of argv in cmd_stash()\n>> before it calls push_stash() and kept looking in argv[0] for \"-p\" after\n>> moving the code to push_stash().\n>>\n>> The support for assuming \"push\" when \"-p\" is given introduced in\n>> 9e140909f61 is very narrow, neither \"git stash -m -p\n>> \" nor \"git stash --patch \" imply \"push\" and die\n>> instead. Fix the regression introduced by 8c3713cede7 and relax the\n>> behavior introduced in 9e140909f61 by passing\n> \n> Hmph, is it too much work to have a patch that only fixes the\n> regression and another that extends the feature on top as a separate\n> patch? Not that I am opposed by the new feature, though.\n\nI can do that, I was just being lazy skipping the separate regression fix\n\nThanks\n\nPhillip\n\n>> PARSE_OPT_STOP_AT_NON_OPTION when push is being assumed and then setting\n>> \"force_assume\" if \"--patch\" was present. This means \"git stash\n>> -p\" still dies so do assume the user meant \"push\" if they\n>> mistype a subcommand name but \"git stash -m -p \"\n>> will now succeed.\n> \n>> Tests are added to prevent future regressions.\n> \n> Nice.\n> \n>> +test_expect_success 'stash --patch stash and restores the file' '\n>> +\tcat file >expect-file &&\n>> +\techo changed-file >file &&\n>> +\techo changed-other-file >other-file &&\n>> +\techo a | git stash -m \"stash bar\" --patch file &&\n>> +\ttest_cmp expect-file file &&\n>> +\techo changed-other-file >expect &&\n>> +\ttest_cmp expect other-file &&\n>> +\tgit stash pop &&\n>> +\ttest_cmp expect other-file &&\n>> +\techo changed-file >expect &&\n>> +\ttest_cmp expect file\n>> +'\n> \n> OK.\n> \n>> +test_expect_success 'stash -p is rejected' '\n>> +\ttest_must_fail git stash file -p 2>err &&\n>> +\ttest_grep \"subcommand wasn${SQ}t specified; ${SQ}push${SQ} can${SQ}t be assumed due to unexpected token ${SQ}file${SQ}\" err\n>> +'\n> \n> Good thing to test.\n\n"},{"id":"518478","messageId":"cover.1747733203.git.phillip.wood@dunelm.org.uk","threadId":"63470","inReplyTo":"6292feee7c4347efad31e9fb2a1763779b7df133.1747407473.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 0/2] stash: fix and improve \"git stash -p \"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-20T09:26:58Z","receivedAt":"2025-05-20T09:27:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n\"git stash -p \" should imply \"git stash push -p \"\nbut that was broken by a code cleanup in c3713cede7 (stash: eliminate\ncrude option parsing, 2020-02-17). This regression is fixed in the\nfirst patch. Although \"-p\" implies the \"push\" subcommand \"--patch\"\nhas never implied \"push\". That is fixed in the second patch.\n\nThanks to Junio for his comments on V1.\n\nChanges since V1:\n - Split out the regression fix into its own patch\n\nBase-Commit: 1a8a4971cc6c179c4dd711f4a7f5d7178f4b3ab7\nPublished-As: https://github.com/phillipwood/git/releases/tag/pw%2Fstash-assume-push-with-dash-p%2Fv2\nView-Changes-At: https://github.com/phillipwood/git/compare/1a8a4971c...98ad3de97\nFetch-It-Via: git fetch https://github.com/phillipwood/git pw/stash-assume-push-with-dash-p/v2\n\n\nPhillip Wood (2):\n stash: allow \"git stash -p \" to assume push again\n stash: allow \"git stash [] --patch \" to assume push\n\n builtin/stash.c | 10 +++++++---\n t/t3903-stash.sh | 19 +++++++++++++++++++\n 2 files changed, 26 insertions(+), 3 deletions(-)\n\nRange-diff against v1:\n1: 6292feee7c4 ! 1: 2cd67f5cd85 stash: allow \"git stash -p \" to assume push again\n @@ Commit message\n Historically \"git stash []\" was assumed to mean \"git stash save\n []\". Since 1ada5020b38 (stash: use stash_push for no verb form,\n 2017-02-28) it is assumed to mean \"git stash push []\". As the\n - push subcommand supports pathspecs 9e140909f61 (stash: allow pathspecs\n + push subcommand supports pathspecs, 9e140909f61 (stash: allow pathspecs\n in the no verb form, 2017-02-28) allowed \"git stash -p \" to\n mean \"git stash push -p \". This was broken in 8c3713cede7\n (stash: eliminate crude option parsing, 2020-02-17) which failed to\n account for \"push\" being added to the start of argv in cmd_stash()\n before it calls push_stash() and kept looking in argv[0] for \"-p\" after\n moving the code to push_stash().\n \n - The support for assuming \"push\" when \"-p\" is given introduced in\n - 9e140909f61 is very narrow, neither \"git stash -m -p\n - \" nor \"git stash --patch \" imply \"push\" and die\n - instead. Fix the regression introduced by 8c3713cede7 and relax the\n - behavior introduced in 9e140909f61 by passing\n - PARSE_OPT_STOP_AT_NON_OPTION when push is being assumed and then setting\n - \"force_assume\" if \"--patch\" was present. This means \"git stash\n - -p\" still dies so do assume the user meant \"push\" if they\n - mistype a subcommand name but \"git stash -m -p \"\n - will now succeed. Tests are added to prevent future regressions.\n + Fix this by regression by checking argv[1] instead of argv[0] and add a\n + couple of tests to prevent future regressions.\n \n Signed-off-by: Phillip Wood \n \n @@ builtin/stash.c: static int push_stash(int argc, const char **argv, const char *\n \n \tif (argc) {\n -\t\tforce_assume = !strcmp(argv[0], \"-p\");\n -+\t\tint flags = PARSE_OPT_KEEP_DASHDASH;\n -+\n -+\t\tif (push_assumed)\n -+\t\t\tflags |= PARSE_OPT_STOP_AT_NON_OPTION;\n -+\n ++\t\tforce_assume = argc > 1 && !strcmp(argv[1], \"-p\");\n \t\targc = parse_options(argc, argv, prefix, options,\n \t\t\t\t push_assumed ? git_stash_usage :\n --\t\t\t\t git_stash_push_usage,\n --\t\t\t\t PARSE_OPT_KEEP_DASHDASH);\n -+\t\t\t\t git_stash_push_usage, flags);\n -+\t\tforce_assume |= patch_mode;\n - \t}\n - \n - \tif (argc) {\n + \t\t\t\t git_stash_push_usage,\n \n ## t/t3903-stash.sh ##\n @@ t/t3903-stash.sh: test_expect_success 'stash -- stashes and restores the file' '\n \ttest_path_is_file bar\n '\n \n -+test_expect_success 'stash --patch stash and restores the file' '\n ++test_expect_success 'stash -p stash and restores the file' '\n +\tcat file >expect-file &&\n +\techo changed-file >file &&\n +\techo changed-other-file >other-file &&\n -+\techo a | git stash -m \"stash bar\" --patch file &&\n ++\techo a | git stash -p file &&\n +\ttest_cmp expect-file file &&\n +\techo changed-other-file >expect &&\n +\ttest_cmp expect other-file &&\n-: ----------- > 2: 98ad3de9770 stash: allow \"git stash [] --patch \" to assume push\n-- \n2.49.0.897.gfad3eb7d210\n\n"},{"id":"518479","messageId":"2cd67f5cd85af03ae99a2760a76e9df5a7edfd95.1747733203.git.phillip.wood@dunelm.org.uk","threadId":"63470","inReplyTo":"cover.1747733203.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 1/2] stash: allow \"git stash -p \" to assume push again","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-20T09:26:59Z","receivedAt":"2025-05-20T09:27:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nHistorically \"git stash []\" was assumed to mean \"git stash save\n[]\". Since 1ada5020b38 (stash: use stash_push for no verb form,\n2017-02-28) it is assumed to mean \"git stash push []\". As the\npush subcommand supports pathspecs, 9e140909f61 (stash: allow pathspecs\nin the no verb form, 2017-02-28) allowed \"git stash -p \" to\nmean \"git stash push -p \". This was broken in 8c3713cede7\n(stash: eliminate crude option parsing, 2020-02-17) which failed to\naccount for \"push\" being added to the start of argv in cmd_stash()\nbefore it calls push_stash() and kept looking in argv[0] for \"-p\" after\nmoving the code to push_stash().\n\nFix this by regression by checking argv[1] instead of argv[0] and add a\ncouple of tests to prevent future regressions.\n\nSigned-off-by: Phillip Wood \n---\n builtin/stash.c | 2 +-\n t/t3903-stash.sh | 19 +++++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex cfbd92852a6..bc2c34fa048 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1789,7 +1789,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n \tint ret;\n \n \tif (argc) {\n-\t\tforce_assume = !strcmp(argv[0], \"-p\");\n+\t\tforce_assume = argc > 1 && !strcmp(argv[1], \"-p\");\n \t\targc = parse_options(argc, argv, prefix, options,\n \t\t\t\t push_assumed ? git_stash_usage :\n \t\t\t\t git_stash_push_usage,\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 74666ff3e4b..d24559a328d 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1177,6 +1177,25 @@ test_expect_success 'stash -- stashes and restores the file' '\n \ttest_path_is_file bar\n '\n \n+test_expect_success 'stash -p stash and restores the file' '\n+\tcat file >expect-file &&\n+\techo changed-file >file &&\n+\techo changed-other-file >other-file &&\n+\techo a | git stash -p file &&\n+\ttest_cmp expect-file file &&\n+\techo changed-other-file >expect &&\n+\ttest_cmp expect other-file &&\n+\tgit stash pop &&\n+\ttest_cmp expect other-file &&\n+\techo changed-file >expect &&\n+\ttest_cmp expect file\n+'\n+\n+test_expect_success 'stash -p is rejected' '\n+\ttest_must_fail git stash file -p 2>err &&\n+\ttest_grep \"subcommand wasn${SQ}t specified; ${SQ}push${SQ} can${SQ}t be assumed due to unexpected token ${SQ}file${SQ}\" err\n+'\n+\n test_expect_success 'stash -- stashes in subdirectory' '\n \tmkdir sub &&\n \t>foo &&\n-- \n2.49.0.897.gfad3eb7d210\n\n"},{"id":"518480","messageId":"98ad3de977090a793408b25ca880b65f058ea44e.1747733203.git.phillip.wood@dunelm.org.uk","threadId":"63470","inReplyTo":"cover.1747733203.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v2 2/2] stash: allow \"git stash [] --patch \" to assume push","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-05-20T09:27:00Z","receivedAt":"2025-05-20T09:27:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe support for assuming \"push\" when \"-p\" is given introduced in\n9e140909f61 (stash: allow pathspecs in the no verb form, 2017-02-28) is\nvery narrow, neither \"git stash -m -p \" nor \"git\nstash --patch \" imply \"push\" and die instead. Relax this by\npassing PARSE_OPT_STOP_AT_NON_OPTION when push is being assumed and then\nsetting \"force_assume\" if \"--patch\" was present. This means \"git stash\n -p\" still dies so that it does not assume the user meant\n\"push\" if they mistype a subcommand name but \"git stash -m -p\n\" will now succeed. The test added in the last commit is\nadjusted to check that push is still assumed when \"--patch\" comes after\nother options on the command-line.\n\nSigned-off-by: Phillip Wood \n---\n builtin/stash.c | 10 +++++++---\n t/t3903-stash.sh | 4 ++--\n 2 files changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex bc2c34fa048..b12fd6c40f1 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1789,11 +1789,15 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n \tint ret;\n \n \tif (argc) {\n-\t\tforce_assume = argc > 1 && !strcmp(argv[1], \"-p\");\n+\t\tint flags = PARSE_OPT_KEEP_DASHDASH;\n+\n+\t\tif (push_assumed)\n+\t\t\tflags |= PARSE_OPT_STOP_AT_NON_OPTION;\n+\n \t\targc = parse_options(argc, argv, prefix, options,\n \t\t\t\t push_assumed ? git_stash_usage :\n-\t\t\t\t git_stash_push_usage,\n-\t\t\t\t PARSE_OPT_KEEP_DASHDASH);\n+\t\t\t\t git_stash_push_usage, flags);\n+\t\tforce_assume |= patch_mode;\n \t}\n \n \tif (argc) {\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex d24559a328d..295cb508a35 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1177,11 +1177,11 @@ test_expect_success 'stash -- stashes and restores the file' '\n \ttest_path_is_file bar\n '\n \n-test_expect_success 'stash -p stash and restores the file' '\n+test_expect_success 'stash --patch stash and restores the file' '\n \tcat file >expect-file &&\n \techo changed-file >file &&\n \techo changed-other-file >other-file &&\n-\techo a | git stash -p file &&\n+\techo a | git stash -m \"stash bar\" --patch file &&\n \ttest_cmp expect-file file &&\n \techo changed-other-file >expect &&\n \ttest_cmp expect other-file &&\n-- \n2.49.0.897.gfad3eb7d210\n\n"},{"id":"518560","messageId":"xmqqy0uqdsj2.fsf@gitster.g","threadId":"63470","inReplyTo":"cover.1747733203.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 0/2] stash: fix and improve \"git stash -p \"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-21T13:04:17Z","receivedAt":"2025-05-21T13:04:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Phillip Wood (2):\n> stash: allow \"git stash -p \" to assume push again\n> stash: allow \"git stash [] --patch \" to assume push\n\nThanks, queued.\n\n"},{"id":"519600","messageId":"xmqqcybkh3wg.fsf@gitster.g","threadId":"63470","inReplyTo":"cover.1747733203.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 0/2] stash: fix and improve \"git stash -p \"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-06-03T22:11:11Z","receivedAt":"2025-06-03T22:11:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> From: Phillip Wood \n>\n> \"git stash -p \" should imply \"git stash push -p \"\n> but that was broken by a code cleanup in c3713cede7 (stash: eliminate\n> crude option parsing, 2020-02-17). This regression is fixed in the\n> first patch. Although \"-p\" implies the \"push\" subcommand \"--patch\"\n> has never implied \"push\". That is fixed in the second patch.\n>\n> Thanks to Junio for his comments on V1.\n>\n> Changes since V1:\n> - Split out the regression fix into its own patch\n>\n> Base-Commit: 1a8a4971cc6c179c4dd711f4a7f5d7178f4b3ab7\n> Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fstash-assume-push-with-dash-p%2Fv2\n> View-Changes-At: https://github.com/phillipwood/git/compare/1a8a4971c...98ad3de97\n> Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/stash-assume-push-with-dash-p/v2\n>\n>\n> Phillip Wood (2):\n> stash: allow \"git stash -p \" to assume push again\n> stash: allow \"git stash [] --patch \" to assume push\n\nAre other people interested in this work? I haven't seen any\ncomments other than a few nitpicky one form mine, and want to (1)\ngauge the interest in the fix, and (2) see how well reviewed it is\n(and my review or reading over the patches again would not count all\nthat much here).\n\nThanks.\n"},{"id":"519840","messageId":"CAN0heSpGtLW8B-wtoMgW7gunMMeVTL1jhk8xN1LBbeeG4f1Fxw@mail.gmail.com","threadId":"63470","inReplyTo":"2cd67f5cd85af03ae99a2760a76e9df5a7edfd95.1747733203.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 1/2] stash: allow \"git stash -p \" to assume push again","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-06-06T11:31:03Z","receivedAt":"2025-06-06T11:31:16Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 20 May 2025 at 11:27, Phillip Wood wrote:\n>\n> Fix this by regression by checking argv[1] instead of argv[0] and add a\n> couple of tests to prevent future regressions.\n\n> if (argc) {\n> - force_assume = !strcmp(argv[0], \"-p\");\n> + force_assume = argc > 1 && !strcmp(argv[1], \"-p\");\n> argc = parse_options(argc, argv, prefix, options,\n> push_assumed ? git_stash_usage :\n> git_stash_push_usage,\n\nAfter reading up on 8c3713cede (stash: eliminate crude option parsing,\n2020-02-17), I share your analysis. This fix looks correct to me.\n\n> +test_expect_success 'stash -p stash and restores the file' '\n> + cat file >expect-file &&\n> + echo changed-file >file &&\n> + echo changed-other-file >other-file &&\n> + echo a | git stash -p file &&\n> + test_cmp expect-file file &&\n> + echo changed-other-file >expect &&\n> + test_cmp expect other-file &&\n> + git stash pop &&\n> + test_cmp expect other-file &&\n> + echo changed-file >expect &&\n> + test_cmp expect file\n> +'\n\nThis only exercises the patch machinery fairly trivially: all hunks are\nadded. The implementation under test could miss `-p` completely and\nbehave as `git stash push -- file` or some variant of it, and this test\nwould continue to pass. (Confirmed by editing the test to not use `-p`\nand seeing it run successfully.)\n\nIt might be worthwhile to set up some more elaborate scenario where you\npick only some hunks, e.g., this (whitespace-damaged) diff:\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex d24559a328..3b28504126 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1178,16 +1178,19 @@ test_expect_success 'stash -- \nstashes and restores the file' '\n '\n\ntest_expect_success 'stash -p stash and restores the file' '\n- cat file >expect-file &&\n- echo changed-file >file &&\n+ test_write_lines b c >file &&\n+ git commit -m \"a few lines\" -- file &&\n+ test_write_lines a b c d >file &&\n+ test_write_lines b c d >expect-file &&\n echo changed-other-file >other-file &&\n- echo a | git stash -p file &&\n+ test_write_lines s y n | git stash -p file &&\n test_cmp expect-file file &&\n echo changed-other-file >expect &&\n test_cmp expect other-file &&\n+ test_write_lines b c >file &&\n git stash pop &&\n test_cmp expect other-file &&\n- echo changed-file >expect &&\n+ test_write_lines a b c >expect &&\n test_cmp expect file\n '\n\n\nMartin\n"},{"id":"519841","messageId":"CAN0heSq56q5nQnrd0YBOWEvj7uXEFkWG3DH6Ms6JVkFNnuUmBA@mail.gmail.com","threadId":"63470","inReplyTo":"98ad3de977090a793408b25ca880b65f058ea44e.1747733203.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v2 2/2] stash: allow \"git stash [] --patch \" to assume push","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-06-06T11:32:58Z","receivedAt":"2025-06-06T11:33:12Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 20 May 2025 at 11:27, Phillip Wood wrote:\n>\n> The support for assuming \"push\" when \"-p\" is given introduced in\n> 9e140909f61 (stash: allow pathspecs in the no verb form, 2017-02-28) is\n> very narrow, neither \"git stash -m -p \" nor \"git\n> stash --patch \" imply \"push\" and die instead. Relax this by\n> passing PARSE_OPT_STOP_AT_NON_OPTION when push is being assumed and then\n> setting \"force_assume\" if \"--patch\" was present. This means \"git stash\n> -p\" still dies so that it does not assume the user meant\n> \"push\" if they mistype a subcommand name but \"git stash -m -p\n> \" will now succeed. The test added in the last commit is\n> adjusted to check that push is still assumed when \"--patch\" comes after\n> other options on the command-line.\n\nAll makes sense to me.\n\n> if (argc) {\n> - force_assume = argc > 1 && !strcmp(argv[1], \"-p\");\n\nThis is where we drop the very specific approach of \"let's look for -p\".\n\n> + int flags = PARSE_OPT_KEEP_DASHDASH;\n\nThis is the flag we've always been using.\n\n> + if (push_assumed)\n> + flags |= PARSE_OPT_STOP_AT_NON_OPTION;\n\nNow we use this, too, if we've assumed \"push\". Makes sense even without\nthe specific context of this patch: we've assumed an implicit \"push\", so\nlet's be a bit less aggressive in parsing the remainder.\n\n> argc = parse_options(argc, argv, prefix, options,\n> push_assumed ? git_stash_usage :\n> - git_stash_push_usage,\n> - PARSE_OPT_KEEP_DASHDASH);\n> + git_stash_push_usage, flags);\n> + force_assume |= patch_mode;\n\nRather than looking for \"-p\" in a fixed place, we see if option parsing\nspotted it. Makes perfect sense. Although, why `|=` here? We initialize\n`force_assume` to 0 at the top and this is the only other time we write\nto it. Why not just `force_assume = patch_mode`? Future-proofing?\n\n> -test_expect_success 'stash -p stash and restores the file' '\n> +test_expect_success 'stash --patch stash and restores the file' '\n> cat file >expect-file &&\n> echo changed-file >file &&\n> echo changed-other-file >other-file &&\n> - echo a | git stash -p file &&\n> + echo a | git stash -m \"stash bar\" --patch file &&\n> test_cmp expect-file file &&\n> echo changed-other-file >expect &&\n> test_cmp expect other-file &&\n\nWe lose the test of `-p` that we just added. Ok. We should be able to\ntrust our option parsing machinery to get this right. This s/-p/--patch/\ndemonstrates that your patch works, and as for running this as a\nregression test in the future, we'll be using one of the equivalent ways\nof spelling this option. Ok.\n\n\nMartin\n"},{"id":"519842","messageId":"CAN0heSpRWoiPh-c9y27unLgx18VNiHwJvnPiUERM_KSiP-39=g@mail.gmail.com","threadId":"63470","inReplyTo":"xmqqcybkh3wg.fsf@gitster.g","subject":"Re: [PATCH v2 0/2] stash: fix and improve \"git stash -p \"","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-06-06T11:39:31Z","receivedAt":"2025-06-06T11:39:45Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 4 Jun 2025 at 00:11, Junio C Hamano wrote:\n>\n> > Phillip Wood (2):\n> > stash: allow \"git stash -p \" to assume push again\n> > stash: allow \"git stash [] --patch \" to assume push\n>\n> Are other people interested in this work? I haven't seen any\n> comments other than a few nitpicky one form mine, and want to (1)\n> gauge the interest in the fix, and (2) see how well reviewed it is\n> (and my review or reading over the patches again would not count all\n> that much here).\n\nOn reading the patches, I realized that I have some interest in this. I\nleft some comments. Most of them amount to thinking out loud, but I do\nthink that the new test could do a bit better at proving that the\n(fixed/improved) implementation actually ends up picking up `-p` at all.\n\nA nice, pleasant read. The series has a well-defined focus.\n\nMartin\n"},{"id":"519852","messageId":"ffafdfe8-7754-4aa7-b2bc-ef85452f8afb@gmail.com","threadId":"63470","inReplyTo":"CAN0heSpGtLW8B-wtoMgW7gunMMeVTL1jhk8xN1LBbeeG4f1Fxw@mail.gmail.com","subject":"Re: [PATCH v2 1/2] stash: allow \"git stash -p \" to assume push again","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-06T15:26:47Z","receivedAt":"2025-06-06T15:26:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Martin\n\nOn 06/06/2025 12:31, Martin Ågren wrote:\n> On Tue, 20 May 2025 at 11:27, Phillip Wood wrote:\n>>\n>> +test_expect_success 'stash -p stash and restores the file' '\n>> + cat file >expect-file &&\n>> + echo changed-file >file &&\n>> + echo changed-other-file >other-file &&\n>> + echo a | git stash -p file &&\n>> + test_cmp expect-file file &&\n>> + echo changed-other-file >expect &&\n>> + test_cmp expect other-file &&\n>> + git stash pop &&\n>> + test_cmp expect other-file &&\n>> + echo changed-file >expect &&\n>> + test_cmp expect file\n>> +'\n> \n> This only exercises the patch machinery fairly trivially: all hunks are\n> added. The implementation under test could miss `-p` completely and\n> behave as `git stash push -- file` or some variant of it, and this test\n> would continue to pass. (Confirmed by editing the test to not use `-p`\n> and seeing it run successfully.)\n> \n> It might be worthwhile to set up some more elaborate scenario where you\n> pick only some hunks, e.g., this (whitespace-damaged) diff:\n\nI avoided doing this because we already have tests that check \"git stash \npush -p\" works correctly when staging a selection of hunks and so I \ndidn't think it was worth the extra complexity here when I was only \ninterested it whether we parsed '-p' correctly. However you're right \nthat the test passes if we ignore '-p' completely so I agree it is worth \nchanging it. I'll send a re-roll.\n\nThanks for your thoughtful review\n\nPhillip\n\n> \n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index d24559a328..3b28504126 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -1178,16 +1178,19 @@ test_expect_success 'stash -- \n> stashes and restores the file' '\n> '\n> \n> test_expect_success 'stash -p stash and restores the file' '\n> - cat file >expect-file &&\n> - echo changed-file >file &&\n> + test_write_lines b c >file &&\n> + git commit -m \"a few lines\" -- file &&\n> + test_write_lines a b c d >file &&\n> + test_write_lines b c d >expect-file &&\n> echo changed-other-file >other-file &&\n> - echo a | git stash -p file &&\n> + test_write_lines s y n | git stash -p file &&\n> test_cmp expect-file file &&\n> echo changed-other-file >expect &&\n> test_cmp expect other-file &&\n> + test_write_lines b c >file &&\n> git stash pop &&\n> test_cmp expect other-file &&\n> - echo changed-file >expect &&\n> + test_write_lines a b c >expect &&\n> test_cmp expect file\n> '\n> \n> \n> Martin\n\n"},{"id":"519886","messageId":"cover.1749289514.git.phillip.wood@dunelm.org.uk","threadId":"63470","inReplyTo":"6292feee7c4347efad31e9fb2a1763779b7df133.1747407473.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 0/2] stash: fix and improve \"git stash -p \"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-07T09:45:24Z","receivedAt":"2025-06-07T09:45:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n\"git stash -p \" should imply \"git stash push -p \"\nbut that was broken by a code cleanup in c3713cede7 (stash: eliminate\ncrude option parsing, 2020-02-17). This regression is fixed in the\nfirst patch. Although \"-p\" implies the \"push\" subcommand \"--patch\"\nhas never implied \"push\". That is fixed in the second patch.\n\nThanks to Martin for his comments on V2\n\nChanges since V2:\n - Made test stricter as suggested by Martin\n\nThanks to Junio for his comments on V1.\n\nChanges since V1:\n - Split out the regression fix into its own patch\n\nBase-Commit: 1a8a4971cc6c179c4dd711f4a7f5d7178f4b3ab7\nPublished-As: https://github.com/phillipwood/git/releases/tag/pw%2Fstash-assume-push-with-dash-p%2Fv3\nView-Changes-At: https://github.com/phillipwood/git/compare/1a8a4971c...d3a958430\nFetch-It-Via: git fetch https://github.com/phillipwood/git pw/stash-assume-push-with-dash-p/v3\n\n\nPhillip Wood (2):\n stash: allow \"git stash -p \" to assume push again\n stash: allow \"git stash [] --patch \" to assume push\n\n builtin/stash.c | 10 +++++++---\n t/t3903-stash.sh | 22 ++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 3 deletions(-)\n\nRange-diff against v2:\n1: 2cd67f5cd85 ! 1: c147eaf2eae stash: allow \"git stash -p \" to assume push again\n @@ Commit message\n Fix this by regression by checking argv[1] instead of argv[0] and add a\n couple of tests to prevent future regressions.\n \n + Helped-by: Martin Ågren \n Signed-off-by: Phillip Wood \n \n ## builtin/stash.c ##\n @@ t/t3903-stash.sh: test_expect_success 'stash -- stashes and restores\n '\n \n +test_expect_success 'stash -p stash and restores the file' '\n -+\tcat file >expect-file &&\n -+\techo changed-file >file &&\n ++\ttest_write_lines b c >file &&\n ++\tgit commit -m \"add a few lines\" file &&\n ++\ttest_write_lines a b c d >file &&\n ++\ttest_write_lines b c d >expect-file &&\n +\techo changed-other-file >other-file &&\n -+\techo a | git stash -p file &&\n ++\ttest_write_lines s y n | git stash -p file &&\n +\ttest_cmp expect-file file &&\n +\techo changed-other-file >expect &&\n +\ttest_cmp expect other-file &&\n ++\tgit checkout HEAD -- file &&\n +\tgit stash pop &&\n +\ttest_cmp expect other-file &&\n -+\techo changed-file >expect &&\n ++\ttest_write_lines a b c >expect &&\n +\ttest_cmp expect file\n +'\n +\n2: 98ad3de9770 ! 2: d3a95843055 stash: allow \"git stash [] --patch \" to assume push\n @@ t/t3903-stash.sh: test_expect_success 'stash -- stashes and restores\n \n -test_expect_success 'stash -p stash and restores the file' '\n +test_expect_success 'stash --patch stash and restores the file' '\n - \tcat file >expect-file &&\n - \techo changed-file >file &&\n + \ttest_write_lines b c >file &&\n + \tgit commit -m \"add a few lines\" file &&\n + \ttest_write_lines a b c d >file &&\n + \ttest_write_lines b c d >expect-file &&\n \techo changed-other-file >other-file &&\n --\techo a | git stash -p file &&\n -+\techo a | git stash -m \"stash bar\" --patch file &&\n +-\ttest_write_lines s y n | git stash -p file &&\n ++\ttest_write_lines s y n | git stash -m \"stash bar\" --patch file &&\n \ttest_cmp expect-file file &&\n \techo changed-other-file >expect &&\n \ttest_cmp expect other-file &&\n-- \n2.49.0.897.gfad3eb7d210\n\n"},{"id":"519888","messageId":"c147eaf2eaec6ed4e46f3f34bc864cbf8ecb8e45.1749289514.git.phillip.wood@dunelm.org.uk","threadId":"63470","inReplyTo":"cover.1749289514.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 1/2] stash: allow \"git stash -p \" to assume push again","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-07T09:45:25Z","receivedAt":"2025-06-07T09:45:41Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nHistorically \"git stash []\" was assumed to mean \"git stash save\n[]\". Since 1ada5020b38 (stash: use stash_push for no verb form,\n2017-02-28) it is assumed to mean \"git stash push []\". As the\npush subcommand supports pathspecs, 9e140909f61 (stash: allow pathspecs\nin the no verb form, 2017-02-28) allowed \"git stash -p \" to\nmean \"git stash push -p \". This was broken in 8c3713cede7\n(stash: eliminate crude option parsing, 2020-02-17) which failed to\naccount for \"push\" being added to the start of argv in cmd_stash()\nbefore it calls push_stash() and kept looking in argv[0] for \"-p\" after\nmoving the code to push_stash().\n\nFix this by regression by checking argv[1] instead of argv[0] and add a\ncouple of tests to prevent future regressions.\n\nHelped-by: Martin Ågren \nSigned-off-by: Phillip Wood \n---\n builtin/stash.c | 2 +-\n t/t3903-stash.sh | 22 ++++++++++++++++++++++\n 2 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex cfbd92852a6..bc2c34fa048 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1789,7 +1789,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n \tint ret;\n \n \tif (argc) {\n-\t\tforce_assume = !strcmp(argv[0], \"-p\");\n+\t\tforce_assume = argc > 1 && !strcmp(argv[1], \"-p\");\n \t\targc = parse_options(argc, argv, prefix, options,\n \t\t\t\t push_assumed ? git_stash_usage :\n \t\t\t\t git_stash_push_usage,\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 74666ff3e4b..a99a746221e 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1177,6 +1177,28 @@ test_expect_success 'stash -- stashes and restores the file' '\n \ttest_path_is_file bar\n '\n \n+test_expect_success 'stash -p stash and restores the file' '\n+\ttest_write_lines b c >file &&\n+\tgit commit -m \"add a few lines\" file &&\n+\ttest_write_lines a b c d >file &&\n+\ttest_write_lines b c d >expect-file &&\n+\techo changed-other-file >other-file &&\n+\ttest_write_lines s y n | git stash -p file &&\n+\ttest_cmp expect-file file &&\n+\techo changed-other-file >expect &&\n+\ttest_cmp expect other-file &&\n+\tgit checkout HEAD -- file &&\n+\tgit stash pop &&\n+\ttest_cmp expect other-file &&\n+\ttest_write_lines a b c >expect &&\n+\ttest_cmp expect file\n+'\n+\n+test_expect_success 'stash -p is rejected' '\n+\ttest_must_fail git stash file -p 2>err &&\n+\ttest_grep \"subcommand wasn${SQ}t specified; ${SQ}push${SQ} can${SQ}t be assumed due to unexpected token ${SQ}file${SQ}\" err\n+'\n+\n test_expect_success 'stash -- stashes in subdirectory' '\n \tmkdir sub &&\n \t>foo &&\n-- \n2.49.0.897.gfad3eb7d210\n\n"},{"id":"519887","messageId":"d3a958430554cd4db7ba6dc7fdc20bcd5a3cdcad.1749289514.git.phillip.wood@dunelm.org.uk","threadId":"63470","inReplyTo":"cover.1749289514.git.phillip.wood@dunelm.org.uk","subject":"[PATCH v3 2/2] stash: allow \"git stash [] --patch \" to assume push","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-07T09:45:26Z","receivedAt":"2025-06-07T09:45:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe support for assuming \"push\" when \"-p\" is given introduced in\n9e140909f61 (stash: allow pathspecs in the no verb form, 2017-02-28) is\nvery narrow, neither \"git stash -m -p \" nor \"git\nstash --patch \" imply \"push\" and die instead. Relax this by\npassing PARSE_OPT_STOP_AT_NON_OPTION when push is being assumed and then\nsetting \"force_assume\" if \"--patch\" was present. This means \"git stash\n -p\" still dies so that it does not assume the user meant\n\"push\" if they mistype a subcommand name but \"git stash -m -p\n\" will now succeed. The test added in the last commit is\nadjusted to check that push is still assumed when \"--patch\" comes after\nother options on the command-line.\n\nSigned-off-by: Phillip Wood \n---\n builtin/stash.c | 10 +++++++---\n t/t3903-stash.sh | 4 ++--\n 2 files changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex bc2c34fa048..b12fd6c40f1 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1789,11 +1789,15 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n \tint ret;\n \n \tif (argc) {\n-\t\tforce_assume = argc > 1 && !strcmp(argv[1], \"-p\");\n+\t\tint flags = PARSE_OPT_KEEP_DASHDASH;\n+\n+\t\tif (push_assumed)\n+\t\t\tflags |= PARSE_OPT_STOP_AT_NON_OPTION;\n+\n \t\targc = parse_options(argc, argv, prefix, options,\n \t\t\t\t push_assumed ? git_stash_usage :\n-\t\t\t\t git_stash_push_usage,\n-\t\t\t\t PARSE_OPT_KEEP_DASHDASH);\n+\t\t\t\t git_stash_push_usage, flags);\n+\t\tforce_assume |= patch_mode;\n \t}\n \n \tif (argc) {\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex a99a746221e..2bba3baa10f 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1177,13 +1177,13 @@ test_expect_success 'stash -- stashes and restores the file' '\n \ttest_path_is_file bar\n '\n \n-test_expect_success 'stash -p stash and restores the file' '\n+test_expect_success 'stash --patch stash and restores the file' '\n \ttest_write_lines b c >file &&\n \tgit commit -m \"add a few lines\" file &&\n \ttest_write_lines a b c d >file &&\n \ttest_write_lines b c d >expect-file &&\n \techo changed-other-file >other-file &&\n-\ttest_write_lines s y n | git stash -p file &&\n+\ttest_write_lines s y n | git stash -m \"stash bar\" --patch file &&\n \ttest_cmp expect-file file &&\n \techo changed-other-file >expect &&\n \ttest_cmp expect other-file &&\n-- \n2.49.0.897.gfad3eb7d210\n\n"},{"id":"519890","messageId":"CAN0heSotWpNmqd905aknVTfk6WEcYifAwbXBKYfAWkhzxua3ZA@mail.gmail.com","threadId":"63470","inReplyTo":"cover.1749289514.git.phillip.wood@dunelm.org.uk","subject":"Re: [PATCH v3 0/2] stash: fix and improve \"git stash -p \"","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-06-07T12:56:28Z","receivedAt":"2025-06-07T12:56:42Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Phillip,\n\nOn Sat, 7 Jun 2025 at 11:45, Phillip Wood wrote:\n> Range-diff against v2:\n\n> +test_expect_success 'stash -p stash and restores the file' '\n> -+ cat file >expect-file &&\n> -+ echo changed-file >file &&\n> ++ test_write_lines b c >file &&\n> ++ git commit -m \"add a few lines\" file &&\n> ++ test_write_lines a b c d >file &&\n> ++ test_write_lines b c d >expect-file &&\n> + echo changed-other-file >other-file &&\n> -+ echo a | git stash -p file &&\n> ++ test_write_lines s y n | git stash -p file &&\n> + test_cmp expect-file file &&\n> + echo changed-other-file >expect &&\n> + test_cmp expect other-file &&\n\nThis range-diff matches what I'd expect. Now this test makes sure we\nreally pick up the `-p`. On that note ... I just realized that all of\nthese would keep the test passing:\n\n test_write_lines s y n | git stash -p file # what you have\n test_write_lines s y n | git stash -p file otherfile\n test_write_lines s y n | git stash -p .\n test_write_lines s y n | git stash -p\n\nSo the implementation under test could bungle the pathspec, query the\nuser for both `file` and `otherfile` (in that order!), get EOF from\nstdin while handling `otherfile`, leave it out of the stash, and end up\npassing the test. We could try to protect against this by providing\nanother \"y\": if git wants to read something after our \"s y n\" sequence,\nwe'll give it a \"y\" in the hopes that it will trip things up. We do want\nto test the handling of pathspecs here, so maybe tighten this?\n\n> ++ git checkout HEAD -- file &&\n\nThis is better than what I had in my \"maybe something like this\". This\nexplicitly restores the file.\n\nMartin\n"},{"id":"519967","messageId":"a66483fb-5bc4-42b5-b361-c900a69015ed@gmail.com","threadId":"63470","inReplyTo":"CAN0heSotWpNmqd905aknVTfk6WEcYifAwbXBKYfAWkhzxua3ZA@mail.gmail.com","subject":"Re: [PATCH v3 0/2] stash: fix and improve \"git stash -p \"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-06-09T09:42:17Z","receivedAt":"2025-06-09T09:42:27Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Martin\n\nOn 07/06/2025 13:56, Martin Ågren wrote:\n> Hi Phillip,\n> \n> On Sat, 7 Jun 2025 at 11:45, Phillip Wood wrote:\n> [...]\n> This range-diff matches what I'd expect. Now this test makes sure we\n> really pick up the `-p`. On that note ... I just realized that all of\n> these would keep the test passing:\n> \n> test_write_lines s y n | git stash -p file # what you have\n> test_write_lines s y n | git stash -p file otherfile\n> test_write_lines s y n | git stash -p .\n> test_write_lines s y n | git stash -p\n> \n> So the implementation under test could bungle the pathspec, query the\n> user for both `file` and `otherfile` (in that order!), get EOF from\n> stdin while handling `otherfile`, leave it out of the stash, and end up\n> passing the test. We could try to protect against this by providing\n> another \"y\": if git wants to read something after our \"s y n\" sequence,\n> we'll give it a \"y\" in the hopes that it will trip things up. We do want\n> to test the handling of pathspecs here, so maybe tighten this?\n\nJunio has merged this to next now. I was hoping that we would already \nhave coverage for this with other tests but I couldn't see anything so \nI'll look at improving the coverage for \"git stash push -p \" \nin the next release cycle.\n\nThanks\n\nPhillip\n\n"},{"id":"520032","messageId":"CAN0heSrk4osiXTfxSZB9EN3o4NF+zLCBJscrSTa1Rsz+VjzjVg@mail.gmail.com","threadId":"63470","inReplyTo":"a66483fb-5bc4-42b5-b361-c900a69015ed@gmail.com","subject":"Re: [PATCH v3 0/2] stash: fix and improve \"git stash -p \"","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2025-06-10T09:56:10Z","receivedAt":"2025-06-10T09:56:24Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Phillip,\n\nOn Mon, 9 Jun 2025 at 11:42, Phillip Wood wrote:\n>\n> On 07/06/2025 13:56, Martin Ågren wrote:\n> >\n> > On Sat, 7 Jun 2025 at 11:45, Phillip Wood wrote:\n> > [...]\n> > So the implementation under test could bungle the pathspec, query the\n> > user for both `file` and `otherfile` (in that order!), get EOF from\n> > stdin while handling `otherfile`, leave it out of the stash, and end up\n> > passing the test. We could try to protect against this by providing\n> > another \"y\": if git wants to read something after our \"s y n\" sequence,\n> > we'll give it a \"y\" in the hopes that it will trip things up. We do want\n> > to test the handling of pathspecs here, so maybe tighten this?\n>\n> Junio has merged this to next now. I was hoping that we would already\n> have coverage for this with other tests but I couldn't see anything so\n> I'll look at improving the coverage for \"git stash push -p \"\n> in the next release cycle.\n\nOk, makes sense. Those would certainly be good regression tests to have.\nI did some manual testing when I wrote the above and feel confident,\nFWIW, that it works correctly as of now.\n\nThanks for these git-stash improvements.\n\nMartin\n"}]}