{"thread":{"id":"57692","subject":"[PATCH] stash: disable literal treatment when passing top pathspec","startedAt":"2022-04-08T03:12:51Z","lastAt":"2022-04-11T17:55:39Z","messageCount":7,"participants":["Kyle Meyer","Bagas Sanjaya","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"453309","messageId":"20220408031228.782547-1-kyle@kyleam.com","threadId":"57692","inReplyTo":null,"subject":"[PATCH] stash: disable literal treatment when passing top pathspec","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2022-04-08T03:12:28Z","receivedAt":"2022-04-08T03:12:51Z","isPatch":true,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"do_push_stash() passes \":/\" as the pathspec to two subprocess calls.\nWhen pathspecs are interpreted literally for the main process, these\nsubprocess calls do not behave as intended:\n\n * the 'git clean' call, triggered by --include-untracked, does not\n   remove untracked files from the working tree\n\n * the 'git checkout' call, triggered by --keep-index, fails with a\n   message about \":/\" not matching any known files, and the main\n   command exits with a non-zero status\n\nFix both of these spots by passing --no-literal-pathspecs to the\nsubprocess commands.\n\nSigned-off-by: Kyle Meyer <kyle@kyleam.com>\n---\n builtin/stash.c                    | 5 ++++-\n t/t3903-stash.sh                   | 5 +++++\n t/t3905-stash-include-untracked.sh | 5 +++++\n 3 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 0c7b6a9588..afc8400c5d 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1529,7 +1529,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \t\t\t\t\t     GIT_WORK_TREE_ENVIRONMENT,\n \t\t\t\t\t     the_repository->worktree);\n \t\t\t}\n-\t\t\tstrvec_pushl(&cp.args, \"clean\", \"--force\",\n+\t\t\tstrvec_pushl(&cp.args, \"--no-literal-pathspecs\",\n+\t\t\t\t     \"clean\", \"--force\",\n \t\t\t\t     \"--quiet\", \"-d\", \":/\", NULL);\n \t\t\tif (include_untracked == INCLUDE_ALL_FILES)\n \t\t\t\tstrvec_push(&cp.args, \"-x\");\n@@ -1592,6 +1593,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \n \t\t\tcp.git_cmd = 1;\n+\t\t\tif (!ps->nr)\n+\t\t\t\tstrvec_push(&cp.args, \"--no-literal-pathspecs\");\n \t\t\tstrvec_pushl(&cp.args, \"checkout\", \"--no-overlay\",\n \t\t\t\t     oid_to_hex(&info.i_tree), \"--\", NULL);\n \t\t\tif (!ps->nr)\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 4abbc8fcca..f85c3a06cb 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1383,6 +1383,11 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu\n \ttest_path_is_missing to-remove\n '\n \n+test_expect_success 'stash --keep-index succeeds with --literal-pathspecs' '\n+\techo modified >file &&\n+\tgit --literal-pathspecs stash --keep-index\n+'\n+\n test_expect_success 'stash apply should succeed with unmodified file' '\n \techo base >file &&\n \tgit add file &&\ndiff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh\nindex 5390eec4e3..2f216274b2 100755\n--- a/t/t3905-stash-include-untracked.sh\n+++ b/t/t3905-stash-include-untracked.sh\n@@ -427,5 +427,10 @@ test_expect_success 'stash -u ignores sub-repository' '\n \tgit init sub-repo &&\n \tgit stash -u\n '\n+test_expect_success 'stash -u works with --literal-pathspecs' '\n+\t>untracked &&\n+\tgit --literal-pathspecs stash -u &&\n+\ttest_path_is_missing untracked\n+'\n \n test_done\n\nbase-commit: bf23fe5c37d62f37267d31d4afa1a1444f70cdac\n-- \n2.34.0\n\n"},{"id":"453313","messageId":"e93e9bb1-8bd1-a70f-f671-ca322c14c7a1@gmail.com","threadId":"57692","inReplyTo":"20220408031228.782547-1-kyle@kyleam.com","subject":"Re: [PATCH] stash: disable literal treatment when passing top pathspec","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2022-04-08T06:33:18Z","receivedAt":"2022-04-08T06:33:28Z","isPatch":true,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 08/04/22 10.12, Kyle Meyer wrote:\n> +test_expect_success 'stash -u works with --literal-pathspecs' '\n> +\t>untracked &&\n> +\tgit --literal-pathspecs stash -u &&\n> +\ttest_path_is_missing untracked\n> +'\n\nWhy not \"touch untracked\" instead?\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"453320","messageId":"220408.868rsgc67o.gmgdl@evledraar.gmail.com","threadId":"57692","inReplyTo":"e93e9bb1-8bd1-a70f-f671-ca322c14c7a1@gmail.com","subject":"Re: [PATCH] stash: disable literal treatment when passing top pathspec","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-08T08:46:38Z","receivedAt":"2022-04-08T08:47:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Apr 08 2022, Bagas Sanjaya wrote:\n\n> On 08/04/22 10.12, Kyle Meyer wrote:\n>> +test_expect_success 'stash -u works with --literal-pathspecs' '\n>> +\t>untracked &&\n>> +\tgit --literal-pathspecs stash -u &&\n>> +\ttest_path_is_missing untracked\n>> +'\n>\n> Why not \"touch untracked\" instead?\n\nThe \">\" form is correct here. We use \"touch\" when updating the timestamp\nto something in particular is important, but here we're just creating an\nempty file.\n"},{"id":"453346","messageId":"xmqqa6cvmmzn.fsf@gitster.g","threadId":"57692","inReplyTo":"20220408031228.782547-1-kyle@kyleam.com","subject":"Re: [PATCH] stash: disable literal treatment when passing top pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-08T18:46:52Z","receivedAt":"2022-04-08T18:47:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Meyer <kyle@kyleam.com> writes:\n\n> do_push_stash() passes \":/\" as the pathspec to two subprocess calls.\n> When pathspecs are interpreted literally for the main process, these\n> subprocess calls do not behave as intended:\n>\n>  * the 'git clean' call, triggered by --include-untracked, does not\n>    remove untracked files from the working tree\n>\n>  * the 'git checkout' call, triggered by --keep-index, fails with a\n>    message about \":/\" not matching any known files, and the main\n>    command exits with a non-zero status\n>\n> Fix both of these spots by passing --no-literal-pathspecs to the\n> subprocess commands.\n\nYuck (to the original problem, not to the proposed solution).\n\nI wonder if stopping to use \":/\" (or using \".\" instead, if we need\nto give _some_ pathspec) is a better approach.  Don't we move to the\ntop of the working tree by the time cmd_stash() is called and whatever\nsubprocess we spawn via run_command() interface will start at the\ntop anyway, no?\n\n> Signed-off-by: Kyle Meyer <kyle@kyleam.com>\n> ---\n>  builtin/stash.c                    | 5 ++++-\n>  t/t3903-stash.sh                   | 5 +++++\n>  t/t3905-stash-include-untracked.sh | 5 +++++\n>  3 files changed, 14 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 0c7b6a9588..afc8400c5d 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -1529,7 +1529,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n>  \t\t\t\t\t     GIT_WORK_TREE_ENVIRONMENT,\n>  \t\t\t\t\t     the_repository->worktree);\n>  \t\t\t}\n> -\t\t\tstrvec_pushl(&cp.args, \"clean\", \"--force\",\n> +\t\t\tstrvec_pushl(&cp.args, \"--no-literal-pathspecs\",\n> +\t\t\t\t     \"clean\", \"--force\",\n>  \t\t\t\t     \"--quiet\", \"-d\", \":/\", NULL);\n>  \t\t\tif (include_untracked == INCLUDE_ALL_FILES)\n>  \t\t\t\tstrvec_push(&cp.args, \"-x\");\n> @@ -1592,6 +1593,8 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n>  \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n>  \n>  \t\t\tcp.git_cmd = 1;\n> +\t\t\tif (!ps->nr)\n> +\t\t\t\tstrvec_push(&cp.args, \"--no-literal-pathspecs\");\n>  \t\t\tstrvec_pushl(&cp.args, \"checkout\", \"--no-overlay\",\n>  \t\t\t\t     oid_to_hex(&info.i_tree), \"--\", NULL);\n>  \t\t\tif (!ps->nr)\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 4abbc8fcca..f85c3a06cb 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -1383,6 +1383,11 @@ test_expect_success 'stash --keep-index with file deleted in index does not resu\n>  \ttest_path_is_missing to-remove\n>  '\n>  \n> +test_expect_success 'stash --keep-index succeeds with --literal-pathspecs' '\n> +\techo modified >file &&\n> +\tgit --literal-pathspecs stash --keep-index\n> +'\n> +\n>  test_expect_success 'stash apply should succeed with unmodified file' '\n>  \techo base >file &&\n>  \tgit add file &&\n> diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh\n> index 5390eec4e3..2f216274b2 100755\n> --- a/t/t3905-stash-include-untracked.sh\n> +++ b/t/t3905-stash-include-untracked.sh\n> @@ -427,5 +427,10 @@ test_expect_success 'stash -u ignores sub-repository' '\n>  \tgit init sub-repo &&\n>  \tgit stash -u\n>  '\n> +test_expect_success 'stash -u works with --literal-pathspecs' '\n> +\t>untracked &&\n> +\tgit --literal-pathspecs stash -u &&\n> +\ttest_path_is_missing untracked\n> +'\n>  \n>  test_done\n>\n> base-commit: bf23fe5c37d62f37267d31d4afa1a1444f70cdac\n"},{"id":"453347","messageId":"xmqq5ynjmmy1.fsf@gitster.g","threadId":"57692","inReplyTo":"220408.868rsgc67o.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] stash: disable literal treatment when passing top pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-08T18:47:50Z","receivedAt":"2022-04-08T18:47:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Fri, Apr 08 2022, Bagas Sanjaya wrote:\n>\n>> On 08/04/22 10.12, Kyle Meyer wrote:\n>>> +test_expect_success 'stash -u works with --literal-pathspecs' '\n>>> +\t>untracked &&\n>>> +\tgit --literal-pathspecs stash -u &&\n>>> +\ttest_path_is_missing untracked\n>>> +'\n>>\n>> Why not \"touch untracked\" instead?\n>\n> The \">\" form is correct here. We use \"touch\" when updating the timestamp\n> to something in particular is important, but here we're just creating an\n> empty file.\n\nMaybe it is better to have it in CodingGuidelines or t/README?\n\nThanks.\n"},{"id":"453362","messageId":"877d7y3nif.fsf@kyleam.com","threadId":"57692","inReplyTo":"xmqqa6cvmmzn.fsf@gitster.g","subject":"Re: [PATCH] stash: disable literal treatment when passing top pathspec","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2022-04-09T04:10:32Z","receivedAt":"2022-04-09T04:10:40Z","isPatch":true,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Junio C Hamano writes:\n\n> Kyle Meyer <kyle@kyleam.com> writes:\n[...]\n>>  * the 'git clean' call, triggered by --include-untracked, does not\n>>    remove untracked files from the working tree\n>>\n>>  * the 'git checkout' call, triggered by --keep-index, fails with a\n>>    message about \":/\" not matching any known files, and the main\n>>    command exits with a non-zero status\n>>\n>> Fix both of these spots by passing --no-literal-pathspecs to the\n>> subprocess commands.\n>\n> Yuck (to the original problem, not to the proposed solution).\n>\n> I wonder if stopping to use \":/\" (or using \".\" instead, if we need\n> to give _some_ pathspec) is a better approach.  Don't we move to the\n> top of the working tree by the time cmd_stash() is called and whatever\n> subprocess we spawn via run_command() interface will start at the\n> top anyway, no?\n\nFor the --keep-index/checkout case, yes, it looks like the command\nstarts from the top-level.  Passing \".\" as the pathspec to checkout\nworks fine, as far as I can tell.\n\nHowever, for --include-untracked/clean case, the subprocess directory is\nset to startup_info->original_cwd since 0fce211ccc (stash: do not\nattempt to remove startup_info->original_cwd, 2021-12-09).\n"},{"id":"453416","messageId":"xmqq8rsbjyic.fsf@gitster.g","threadId":"57692","inReplyTo":"877d7y3nif.fsf@kyleam.com","subject":"Re: [PATCH] stash: disable literal treatment when passing top pathspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-11T17:55:23Z","receivedAt":"2022-04-11T17:55:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kyle Meyer <kyle@kyleam.com> writes:\n\n> However, for --include-untracked/clean case, the subprocess directory is\n> set to startup_info->original_cwd since 0fce211ccc (stash: do not\n> attempt to remove startup_info->original_cwd, 2021-12-09).\n\nInteresting.  I find the logic there a bit convoluted.  IIUC, it\ngoes like this:\n\n - we do not want to lose the directory our process was originally\n   in, which is recorded in startup_info->original_cmd.\n\n - we have gone up to the root of the working tree, and running\n   \"clean\" from there is what we want---even if we started \"git\n   stash\" from a subdirectory, we want to make the entire working\n   tree clean, not just inside our subdirectory.\n\n - but we came up with a hack that allows us to skip removing the\n   directory the Git process started at.  To take advantage of the\n   mechanism, we'd need to start from that original_cmd.\n\n - but then \"clean\" run from that subdirectory normally cleans only\n   that subdirectory, which is not what we want to do.  To work it\n   around, we'd need to pass :/ pathspec to say that we are cleaning\n   from the top.\n\nIt makes me suspect that \"we protect current directory\" is a too\nspecialized way that didn't really consider the possibility that we\nsometimes spawn a subcommand.  Even \"we protect this directory\" may\nnot be sufficient and we may need a \"we protect these directories\",\nI suspect.  When the user originally starts \"git foo\" in one\ndirectory, which may have to run \"git bar\" in another directory, and\n\"git bar\" would want to protect the directory it starts in and also\nwhere \"git foo\" started from, no?  It almost makes me suspect that\nwe'd want some \"git\" wide option that allows us to pass a list of\npaths not to rmdir, whose default value is [\".\"], or something.\n\nElijah, thoughts?\n\nBut as a short-term fix, I think \"--no-literal-pathspecs\" is fine for\nthis code path.\n\nThanks.\n\n\n\n"}]}