{"thread":{"id":"56458","subject":"[PATCH 0/3] Fix D/F issues in stash","startedAt":"2021-09-08T01:43:54Z","lastAt":"2021-09-10T10:30:04Z","messageCount":14,"participants":["Elijah Newren via GitGitGadget","Johannes Schindelin","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"435001","messageId":"pull.1086.git.git.1631065426.gitgitgadget@gmail.com","threadId":"56458","inReplyTo":null,"subject":"[PATCH 0/3] Fix D/F issues in stash","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T01:43:43Z","receivedAt":"2021-09-08T01:43:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series fixes a few D/F issues in the stash command. These were some\nissues I found while working on unintentional removal of untracked\nfiles/directories and the current working directory, and I'm just submitting\nthem separately.\n\nElijah Newren (3):\n  t3903: document a pair of directory/file bugs\n  stash: avoid feeding directories to update-index\n  stash: restore untracked files AFTER restoring tracked files\n\n builtin/stash.c  | 15 ++++++++++++---\n t/t3903-stash.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 50 insertions(+), 3 deletions(-)\n\n\nbase-commit: e0a2f5cbc585657e757385ad918f167f519cfb96\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1086%2Fnewren%2Fstash-df-fixes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1086/newren/stash-df-fixes-v1\nPull-Request: https://github.com/git/git/pull/1086\n-- \ngitgitgadget\n"},{"id":"435002","messageId":"bc66a6ae75d03858f48634612bb60062cc78f5eb.1631065427.git.gitgitgadget@gmail.com","threadId":"56458","inReplyTo":"pull.1086.git.git.1631065426.gitgitgadget@gmail.com","subject":"[PATCH 1/3] t3903: document a pair of directory/file bugs","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T01:43:44Z","receivedAt":"2021-09-08T01:43:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t3903-stash.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 38 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 873aa56e359..0727a494aa4 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1307,4 +1307,42 @@ test_expect_success 'stash -c stash.useBuiltin=false warning ' '\n \ttest_must_be_empty err\n '\n \n+test_expect_failure 'git stash succeeds despite directory/file change' '\n+\ttest_create_repo directory_file_switch_v1 &&\n+\t(\n+\t\tcd directory_file_switch_v1 &&\n+\t\ttest_commit init &&\n+\n+\t\ttest_write_lines this file has some words >filler &&\n+\t\tgit add filler &&\n+\t\tgit commit -m filler &&\n+\n+\t\tgit rm filler &&\n+\t\tmkdir filler &&\n+\t\techo contents >filler/file &&\n+\t\tcp filler/file expect &&\n+\t\tgit stash push\n+\t)\n+'\n+\n+test_expect_failure 'git stash can pop directory/file saved changes' '\n+\ttest_create_repo directory_file_switch_v2 &&\n+\t(\n+\t\tcd directory_file_switch_v2 &&\n+\t\ttest_commit init &&\n+\n+\t\ttest_write_lines this file has some words >filler &&\n+\t\tgit add filler &&\n+\t\tgit commit -m filler &&\n+\n+\t\tgit rm filler &&\n+\t\tmkdir filler &&\n+\t\techo contents >filler/file &&\n+\t\tcp filler/file expect &&\n+\t\tgit stash push --include-untracked &&\n+\t\tgit stash apply --index &&\n+\t\ttest_cmp expect filler/file\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"435003","messageId":"c7f5ae66a9204a4190b21bcb1338fd80fb907f33.1631065427.git.gitgitgadget@gmail.com","threadId":"56458","inReplyTo":"pull.1086.git.git.1631065426.gitgitgadget@gmail.com","subject":"[PATCH 2/3] stash: avoid feeding directories to update-index","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T01:43:45Z","receivedAt":"2021-09-08T01:43:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nWhen a file is removed from the cache, but there is a file of the same\nname present in the working directory, we would normally treat that file\nin the working directory as untracked.  However, in the case of stash,\ndoing that would prevent a simple 'git stash push', because the untracked\nfile would be in the way of restoring the deleted file.\n\ngit stash, however, blindly assumes that whatever is in the working\ndirectory for a deleted file is wanted and passes that path along to\nupdate-index.  That causes problems when the working directory contains\na directory with the same name as the deleted file.  Add some code for\nthis special case that will avoid passing directory names to\nupdate-index.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/stash.c  | 9 +++++++++\n t/t3903-stash.sh | 2 +-\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 8f42360ca91..b85cf9d267e 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -313,6 +313,12 @@ static int reset_head(void)\n \treturn run_command(&cp);\n }\n \n+static int is_path_a_directory(const char *path)\n+{\n+\tstruct stat st;\n+\treturn (!lstat(path, &st) && S_ISDIR(st.st_mode));\n+}\n+\n static void add_diff_to_buf(struct diff_queue_struct *q,\n \t\t\t    struct diff_options *options,\n \t\t\t    void *data)\n@@ -320,6 +326,9 @@ static void add_diff_to_buf(struct diff_queue_struct *q,\n \tint i;\n \n \tfor (i = 0; i < q->nr; i++) {\n+\t\tif (is_path_a_directory(q->queue[i]->one->path))\n+\t\t\tcontinue;\n+\n \t\tstrbuf_addstr(data, q->queue[i]->one->path);\n \n \t\t/* NUL-terminate: will be fed to update-index -z */\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 0727a494aa4..fc74918ccc0 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1307,7 +1307,7 @@ test_expect_success 'stash -c stash.useBuiltin=false warning ' '\n \ttest_must_be_empty err\n '\n \n-test_expect_failure 'git stash succeeds despite directory/file change' '\n+test_expect_success 'git stash succeeds despite directory/file change' '\n \ttest_create_repo directory_file_switch_v1 &&\n \t(\n \t\tcd directory_file_switch_v1 &&\n-- \ngitgitgadget\n\n"},{"id":"435004","messageId":"ac8ca07481d2ed4156036c2441d10712a1b92b0e.1631065427.git.gitgitgadget@gmail.com","threadId":"56458","inReplyTo":"pull.1086.git.git.1631065426.gitgitgadget@gmail.com","subject":"[PATCH 3/3] stash: restore untracked files AFTER restoring tracked files","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-08T01:43:46Z","receivedAt":"2021-09-08T01:43:56Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nIf a user deletes a file and places a directory of untracked files\nthere, then stashes all these changes, the untracked directory of files\ncannot be restored until after the corresponding file in the way is\nremoved.  So, restore changes to tracked files before restoring\nuntracked files.\n\nThere is no similar problem to worry about in the opposite directory,\nbecause untracked files are always additive.  Said another way, there's\nno way to \"stash a removal of an untracked file\" because if an untracked\nfile is removed, git simply doesn't know about it.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/stash.c  | 6 +++---\n t/t3903-stash.sh | 2 +-\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex b85cf9d267e..9a1f6a5a854 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -530,9 +530,6 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\t}\n \t}\n \n-\tif (info->has_u && restore_untracked(&info->u_tree))\n-\t\treturn error(_(\"could not restore untracked files from stash\"));\n-\n \tinit_merge_options(&o, the_repository);\n \n \to.branch1 = \"Updated upstream\";\n@@ -567,6 +564,9 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\tunstage_changes_unless_new(&c_tree);\n \t}\n \n+\tif (info->has_u && restore_untracked(&info->u_tree))\n+\t\treturn error(_(\"could not restore untracked files from stash\"));\n+\n \tif (!quiet) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex fc74918ccc0..bfd3158a502 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1325,7 +1325,7 @@ test_expect_success 'git stash succeeds despite directory/file change' '\n \t)\n '\n \n-test_expect_failure 'git stash can pop directory/file saved changes' '\n+test_expect_success 'git stash can pop directory/file saved changes' '\n \ttest_create_repo directory_file_switch_v2 &&\n \t(\n \t\tcd directory_file_switch_v2 &&\n-- \ngitgitgadget\n"},{"id":"435040","messageId":"nycvar.QRO.7.76.6.2109080959020.55@tvgsbejvaqbjf.bet","threadId":"56458","inReplyTo":"c7f5ae66a9204a4190b21bcb1338fd80fb907f33.1631065427.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] stash: avoid feeding directories to update-index","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-08T08:02:32Z","receivedAt":"2021-09-08T08:02:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Wed, 8 Sep 2021, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren <newren@gmail.com>\n>\n> When a file is removed from the cache, but there is a file of the same\n> name present in the working directory, we would normally treat that file\n> in the working directory as untracked.  However, in the case of stash,\n> doing that would prevent a simple 'git stash push', because the untracked\n> file would be in the way of restoring the deleted file.\n>\n> git stash, however, blindly assumes that whatever is in the working\n> directory for a deleted file is wanted and passes that path along to\n> update-index.  That causes problems when the working directory contains\n> a directory with the same name as the deleted file.  Add some code for\n> this special case that will avoid passing directory names to\n> update-index.\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>  builtin/stash.c  | 9 +++++++++\n>  t/t3903-stash.sh | 2 +-\n>  2 files changed, 10 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 8f42360ca91..b85cf9d267e 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -313,6 +313,12 @@ static int reset_head(void)\n>  \treturn run_command(&cp);\n>  }\n>\n> +static int is_path_a_directory(const char *path)\n> +{\n> +\tstruct stat st;\n> +\treturn (!lstat(path, &st) && S_ISDIR(st.st_mode));\n> +}\n\nGit's API is unnecessarily confusing (because grown organically), so it is\neasy to get confused by that `is_directory()` function that is declared in\n`cache.h` and defined in `abspath.c`:\n\n\t/*\n\t * Do not use this for inspecting *tracked* content.  When path is a\n\t * symlink to a directory, we do not want to say it is a directory when\n\t * dealing with tracked content in the working tree.\n\t */\n\tint is_directory(const char *path)\n\t{\n\t\tstruct stat st;\n\t\treturn (!stat(path, &st) && S_ISDIR(st.st_mode));\n\t}\n\nThe difference I see is that you use an `lstat()`, which is kind of\nimportant here.\n\nMaybe you could add a paragraph pointing out that we cannot use\n`is_directory()` here because it would follow symbolic links, which we\nneed to avoid here?\n\n> +\n>  static void add_diff_to_buf(struct diff_queue_struct *q,\n>  \t\t\t    struct diff_options *options,\n>  \t\t\t    void *data)\n> @@ -320,6 +326,9 @@ static void add_diff_to_buf(struct diff_queue_struct *q,\n>  \tint i;\n>\n>  \tfor (i = 0; i < q->nr; i++) {\n> +\t\tif (is_path_a_directory(q->queue[i]->one->path))\n> +\t\t\tcontinue;\n> +\n>  \t\tstrbuf_addstr(data, q->queue[i]->one->path);\n>\n>  \t\t/* NUL-terminate: will be fed to update-index -z */\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 0727a494aa4..fc74918ccc0 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -1307,7 +1307,7 @@ test_expect_success 'stash -c stash.useBuiltin=false warning ' '\n>  \ttest_must_be_empty err\n>  '\n>\n> -test_expect_failure 'git stash succeeds despite directory/file change' '\n> +test_expect_success 'git stash succeeds despite directory/file change' '\n\nExcellent!\n\nThanks,\nDscho\n\n>  \ttest_create_repo directory_file_switch_v1 &&\n>  \t(\n>  \t\tcd directory_file_switch_v1 &&\n> --\n> gitgitgadget\n>\n>\n"},{"id":"435041","messageId":"nycvar.QRO.7.76.6.2109081003270.55@tvgsbejvaqbjf.bet","threadId":"56458","inReplyTo":"pull.1086.git.git.1631065426.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Fix D/F issues in stash","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-09-08T08:04:32Z","receivedAt":"2021-09-08T08:04:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Wed, 8 Sep 2021, Elijah Newren via GitGitGadget wrote:\n\n> This series fixes a few D/F issues in the stash command. These were some\n> issues I found while working on unintentional removal of untracked\n> files/directories and the current working directory, and I'm just submitting\n> them separately.\n\nAwesome work! Apart from asking for an additional clarification in the\ncommit message of the second patch, I have nothing else to offer but my\nsincere thanks for working on the `stash` code.\n\nThank you,\nDscho\n"},{"id":"435081","messageId":"53a16340-561b-dc67-5562-31d16460da84@gmail.com","threadId":"56458","inReplyTo":"ac8ca07481d2ed4156036c2441d10712a1b92b0e.1631065427.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] stash: restore untracked files AFTER restoring tracked files","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-09-08T13:15:15Z","receivedAt":"2021-09-08T13:15:26Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/7/2021 9:43 PM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren <newren@gmail.com>\n> \n> If a user deletes a file and places a directory of untracked files\n> there, then stashes all these changes, the untracked directory of files\n> cannot be restored until after the corresponding file in the way is\n> removed.  So, restore changes to tracked files before restoring\n> untracked files.\n> \n> There is no similar problem to worry about in the opposite directory,\n\ns/directory/direction/ ?\n\n> because untracked files are always additive.  Said another way, there's\n> no way to \"stash a removal of an untracked file\" because if an untracked\n> file is removed, git simply doesn't know about it.\n\nMakes sense.\n\nThanks,\n-Stolee\n"},{"id":"435082","messageId":"c39e1b86-479f-2a07-9e9b-9da2bf1c9493@gmail.com","threadId":"56458","inReplyTo":"nycvar.QRO.7.76.6.2109081003270.55@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 0/3] Fix D/F issues in stash","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-09-08T13:15:57Z","receivedAt":"2021-09-08T13:16:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/8/2021 4:04 AM, Johannes Schindelin wrote:\n> Hi Elijah,\n> \n> On Wed, 8 Sep 2021, Elijah Newren via GitGitGadget wrote:\n> \n>> This series fixes a few D/F issues in the stash command. These were some\n>> issues I found while working on unintentional removal of untracked\n>> files/directories and the current working directory, and I'm just submitting\n>> them separately.\n> \n> Awesome work! Apart from asking for an additional clarification in the\n> commit message of the second patch, I have nothing else to offer but my\n> sincere thanks for working on the `stash` code.\n\nI found what is probably a typo in a commit message, but otherwise this\nlooks great.\n\nThanks,\n-Stolee\n"},{"id":"435113","messageId":"xmqqfsuf59bp.fsf@gitster.g","threadId":"56458","inReplyTo":"bc66a6ae75d03858f48634612bb60062cc78f5eb.1631065427.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] t3903: document a pair of directory/file bugs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-08T16:19:22Z","receivedAt":"2021-09-08T16:19:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Elijah Newren <newren@gmail.com>\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>  t/t3903-stash.sh | 38 ++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 38 insertions(+)\n>\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 873aa56e359..0727a494aa4 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -1307,4 +1307,42 @@ test_expect_success 'stash -c stash.useBuiltin=false warning ' '\n>  \ttest_must_be_empty err\n>  '\n>  \n> +test_expect_failure 'git stash succeeds despite directory/file change' '\n> +\ttest_create_repo directory_file_switch_v1 &&\n> +\t(\n> +\t\tcd directory_file_switch_v1 &&\n> +\t\ttest_commit init &&\n> +\n> +\t\ttest_write_lines this file has some words >filler &&\n> +\t\tgit add filler &&\n> +\t\tgit commit -m filler &&\n> +\n> +\t\tgit rm filler &&\n> +\t\tmkdir filler &&\n> +\t\techo contents >filler/file &&\n> +\t\tcp filler/file expect &&\n> +\t\tgit stash push\n\nCreating 'expect' that is not used---perhaps this side also wants to\napply the created stash and check the result, like the other side\ndoes, or something?\n\nOf course, an obvious alternative is to stop making a needless\n'expect'.\n\n> +\t)\n> +'\n> +\n> +test_expect_failure 'git stash can pop directory/file saved changes' '\n> +\ttest_create_repo directory_file_switch_v2 &&\n> +\t(\n> +\t\tcd directory_file_switch_v2 &&\n> +\t\ttest_commit init &&\n> +\n> +\t\ttest_write_lines this file has some words >filler &&\n> +\t\tgit add filler &&\n> +\t\tgit commit -m filler &&\n> +\n> +\t\tgit rm filler &&\n> +\t\tmkdir filler &&\n> +\t\techo contents >filler/file &&\n> +\t\tcp filler/file expect &&\n> +\t\tgit stash push --include-untracked &&\n> +\t\tgit stash apply --index &&\n> +\t\ttest_cmp expect filler/file\n> +\t)\n> +'\n> +\n>  test_done\n"},{"id":"435115","messageId":"xmqq5yvb58sr.fsf@gitster.g","threadId":"56458","inReplyTo":"ac8ca07481d2ed4156036c2441d10712a1b92b0e.1631065427.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] stash: restore untracked files AFTER restoring tracked files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-08T16:30:44Z","receivedAt":"2021-09-08T16:30:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Elijah Newren <newren@gmail.com>\n>\n> If a user deletes a file and places a directory of untracked files\n> there, then stashes all these changes, the untracked directory of files\n> cannot be restored until after the corresponding file in the way is\n> removed.  So, restore changes to tracked files before restoring\n> untracked files.\n>\n> There is no similar problem to worry about in the opposite directory,\n\ndirection, I think.\n\nIf a user deletes a directory with tracked files in it and places an\nuntracked file at the path the directory used to be, and then stash\nall these changes, that would be the opposite direction, I would\npresume?  The untracked file would be restorable only after applying\nthe removal of the tracked files that occupied the directory.\n\nWhich would be fine if we apply the changes to the tracked ones first.\n\nOK.\n\nAfter creating a stash, we clear the working tree.  How do we do\nthat exactly, and would we have a similar problem there?  We need to\nfirst remove the untracked file that currently occupies where the\ndirectory used to be in HEAD, and then create the directory and\nresurrect the tracked files in the directory from HEAD, in my\nmodified example above.  In the file-becomes-directory scenario, is\nthe story the same?\n\nThanks.\n"},{"id":"435375","messageId":"pull.1086.v2.git.git.1631269796.gitgitgadget@gmail.com","threadId":"56458","inReplyTo":"pull.1086.git.git.1631065426.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Fix D/F issues in stash","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-10T10:29:53Z","receivedAt":"2021-09-10T10:30:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series fixes a few D/F issues in the stash command. These were some\nissues I found while working on unintentional removal of untracked\nfiles/directories and the current working directory, and I'm just submitting\nthem separately.\n\nChanges since v1:\n\n * Fix accidental creation of file named 'expect' (copy-paste problem...)\n * Documented the reason for adding is_path_a_directory() and not using\n   is_directory()\n * Removed typo, fixed up confusing wording, and added a companion test to\n   show that F->D and D->F have the same fix.\n\nElijah Newren (3):\n  t3903: document a pair of directory/file bugs\n  stash: avoid feeding directories to update-index\n  stash: restore untracked files AFTER restoring tracked files\n\n builtin/stash.c  | 20 ++++++++++++++---\n t/t3903-stash.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 75 insertions(+), 3 deletions(-)\n\n\nbase-commit: e0a2f5cbc585657e757385ad918f167f519cfb96\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1086%2Fnewren%2Fstash-df-fixes-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1086/newren/stash-df-fixes-v2\nPull-Request: https://github.com/git/git/pull/1086\n\nRange-diff vs v1:\n\n 1:  bc66a6ae75d ! 1:  5ddb70d332b t3903: document a pair of directory/file bugs\n     @@ Metadata\n       ## Commit message ##\n          t3903: document a pair of directory/file bugs\n      \n     +    There are three tests here, because the second bug is documented with\n     +    two tests: a file -> directory change and a directory -> file change.\n     +    The reason for the two tests is just to verify that both are indeed\n     +    broken but that both will be fixed by the same simple change (which will\n     +    be provided in a subsequent patch).\n     +\n          Signed-off-by: Elijah Newren <newren@gmail.com>\n      \n       ## t/t3903-stash.sh ##\n     @@ t/t3903-stash.sh: test_expect_success 'stash -c stash.useBuiltin=false warning '\n      +\t\tgit rm filler &&\n      +\t\tmkdir filler &&\n      +\t\techo contents >filler/file &&\n     -+\t\tcp filler/file expect &&\n      +\t\tgit stash push\n      +\t)\n      +'\n      +\n     -+test_expect_failure 'git stash can pop directory/file saved changes' '\n     ++test_expect_failure 'git stash can pop file -> directory saved changes' '\n      +\ttest_create_repo directory_file_switch_v2 &&\n      +\t(\n      +\t\tcd directory_file_switch_v2 &&\n     @@ t/t3903-stash.sh: test_expect_success 'stash -c stash.useBuiltin=false warning '\n      +\t\ttest_cmp expect filler/file\n      +\t)\n      +'\n     ++\n     ++test_expect_failure 'git stash can pop directory -> file saved changes' '\n     ++\ttest_create_repo directory_file_switch_v3 &&\n     ++\t(\n     ++\t\tcd directory_file_switch_v3 &&\n     ++\t\ttest_commit init &&\n     ++\n     ++\t\tmkdir filler &&\n     ++\t\ttest_write_lines some words >filler/file1 &&\n     ++\t\ttest_write_lines and stuff >filler/file2 &&\n     ++\t\tgit add filler &&\n     ++\t\tgit commit -m filler &&\n     ++\n     ++\t\tgit rm -rf filler &&\n     ++\t\techo contents >filler &&\n     ++\t\tcp filler expect &&\n     ++\t\tgit stash push --include-untracked &&\n     ++\t\tgit stash apply --index &&\n     ++\t\ttest_cmp expect filler\n     ++\t)\n     ++'\n      +\n       test_done\n 2:  c7f5ae66a92 ! 2:  31e38c6c33c stash: avoid feeding directories to update-index\n     @@ builtin/stash.c: static int reset_head(void)\n       \n      +static int is_path_a_directory(const char *path)\n      +{\n     ++\t/*\n     ++\t * This function differs from abspath.c:is_directory() in that\n     ++\t * here we use lstat() instead of stat(); we do not want to\n     ++\t * follow symbolic links here.\n     ++\t */\n      +\tstruct stat st;\n      +\treturn (!lstat(path, &st) && S_ISDIR(st.st_mode));\n      +}\n 3:  ac8ca07481d ! 3:  6254938948c stash: restore untracked files AFTER restoring tracked files\n     @@ Commit message\n          removed.  So, restore changes to tracked files before restoring\n          untracked files.\n      \n     -    There is no similar problem to worry about in the opposite directory,\n     -    because untracked files are always additive.  Said another way, there's\n     -    no way to \"stash a removal of an untracked file\" because if an untracked\n     -    file is removed, git simply doesn't know about it.\n     +    There is no counterpart problem to worry about with the user deleting an\n     +    untracked file and then add a tracked one in its place.  Git does not\n     +    track untracked files, and so will not know the untracked file was\n     +    deleted, and thus won't be able to stash the removal of that file.\n      \n          Signed-off-by: Elijah Newren <newren@gmail.com>\n      \n     @@ t/t3903-stash.sh: test_expect_success 'git stash succeeds despite directory/file\n       \t)\n       '\n       \n     --test_expect_failure 'git stash can pop directory/file saved changes' '\n     -+test_expect_success 'git stash can pop directory/file saved changes' '\n     +-test_expect_failure 'git stash can pop file -> directory saved changes' '\n     ++test_expect_success 'git stash can pop file -> directory saved changes' '\n       \ttest_create_repo directory_file_switch_v2 &&\n       \t(\n       \t\tcd directory_file_switch_v2 &&\n     +@@ t/t3903-stash.sh: test_expect_failure 'git stash can pop file -> directory saved changes' '\n     + \t)\n     + '\n     + \n     +-test_expect_failure 'git stash can pop directory -> file saved changes' '\n     ++test_expect_success 'git stash can pop directory -> file saved changes' '\n     + \ttest_create_repo directory_file_switch_v3 &&\n     + \t(\n     + \t\tcd directory_file_switch_v3 &&\n\n-- \ngitgitgadget\n"},{"id":"435376","messageId":"5ddb70d332b73be88393ac3050cfc3fabdf3db95.1631269796.git.gitgitgadget@gmail.com","threadId":"56458","inReplyTo":"pull.1086.v2.git.git.1631269796.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] t3903: document a pair of directory/file bugs","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-10T10:29:54Z","receivedAt":"2021-09-10T10:30:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nThere are three tests here, because the second bug is documented with\ntwo tests: a file -> directory change and a directory -> file change.\nThe reason for the two tests is just to verify that both are indeed\nbroken but that both will be fixed by the same simple change (which will\nbe provided in a subsequent patch).\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n t/t3903-stash.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 58 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 873aa56e359..7346f8d1037 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1307,4 +1307,62 @@ test_expect_success 'stash -c stash.useBuiltin=false warning ' '\n \ttest_must_be_empty err\n '\n \n+test_expect_failure 'git stash succeeds despite directory/file change' '\n+\ttest_create_repo directory_file_switch_v1 &&\n+\t(\n+\t\tcd directory_file_switch_v1 &&\n+\t\ttest_commit init &&\n+\n+\t\ttest_write_lines this file has some words >filler &&\n+\t\tgit add filler &&\n+\t\tgit commit -m filler &&\n+\n+\t\tgit rm filler &&\n+\t\tmkdir filler &&\n+\t\techo contents >filler/file &&\n+\t\tgit stash push\n+\t)\n+'\n+\n+test_expect_failure 'git stash can pop file -> directory saved changes' '\n+\ttest_create_repo directory_file_switch_v2 &&\n+\t(\n+\t\tcd directory_file_switch_v2 &&\n+\t\ttest_commit init &&\n+\n+\t\ttest_write_lines this file has some words >filler &&\n+\t\tgit add filler &&\n+\t\tgit commit -m filler &&\n+\n+\t\tgit rm filler &&\n+\t\tmkdir filler &&\n+\t\techo contents >filler/file &&\n+\t\tcp filler/file expect &&\n+\t\tgit stash push --include-untracked &&\n+\t\tgit stash apply --index &&\n+\t\ttest_cmp expect filler/file\n+\t)\n+'\n+\n+test_expect_failure 'git stash can pop directory -> file saved changes' '\n+\ttest_create_repo directory_file_switch_v3 &&\n+\t(\n+\t\tcd directory_file_switch_v3 &&\n+\t\ttest_commit init &&\n+\n+\t\tmkdir filler &&\n+\t\ttest_write_lines some words >filler/file1 &&\n+\t\ttest_write_lines and stuff >filler/file2 &&\n+\t\tgit add filler &&\n+\t\tgit commit -m filler &&\n+\n+\t\tgit rm -rf filler &&\n+\t\techo contents >filler &&\n+\t\tcp filler expect &&\n+\t\tgit stash push --include-untracked &&\n+\t\tgit stash apply --index &&\n+\t\ttest_cmp expect filler\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"435377","messageId":"31e38c6c33cea596cb926a2df13cf0df34766a7a.1631269796.git.gitgitgadget@gmail.com","threadId":"56458","inReplyTo":"pull.1086.v2.git.git.1631269796.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] stash: avoid feeding directories to update-index","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-10T10:29:55Z","receivedAt":"2021-09-10T10:30:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nWhen a file is removed from the cache, but there is a file of the same\nname present in the working directory, we would normally treat that file\nin the working directory as untracked.  However, in the case of stash,\ndoing that would prevent a simple 'git stash push', because the untracked\nfile would be in the way of restoring the deleted file.\n\ngit stash, however, blindly assumes that whatever is in the working\ndirectory for a deleted file is wanted and passes that path along to\nupdate-index.  That causes problems when the working directory contains\na directory with the same name as the deleted file.  Add some code for\nthis special case that will avoid passing directory names to\nupdate-index.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/stash.c  | 14 ++++++++++++++\n t/t3903-stash.sh |  2 +-\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 8f42360ca91..9ad2940f87a 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -313,6 +313,17 @@ static int reset_head(void)\n \treturn run_command(&cp);\n }\n \n+static int is_path_a_directory(const char *path)\n+{\n+\t/*\n+\t * This function differs from abspath.c:is_directory() in that\n+\t * here we use lstat() instead of stat(); we do not want to\n+\t * follow symbolic links here.\n+\t */\n+\tstruct stat st;\n+\treturn (!lstat(path, &st) && S_ISDIR(st.st_mode));\n+}\n+\n static void add_diff_to_buf(struct diff_queue_struct *q,\n \t\t\t    struct diff_options *options,\n \t\t\t    void *data)\n@@ -320,6 +331,9 @@ static void add_diff_to_buf(struct diff_queue_struct *q,\n \tint i;\n \n \tfor (i = 0; i < q->nr; i++) {\n+\t\tif (is_path_a_directory(q->queue[i]->one->path))\n+\t\t\tcontinue;\n+\n \t\tstrbuf_addstr(data, q->queue[i]->one->path);\n \n \t\t/* NUL-terminate: will be fed to update-index -z */\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 7346f8d1037..34d1ad9837f 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1307,7 +1307,7 @@ test_expect_success 'stash -c stash.useBuiltin=false warning ' '\n \ttest_must_be_empty err\n '\n \n-test_expect_failure 'git stash succeeds despite directory/file change' '\n+test_expect_success 'git stash succeeds despite directory/file change' '\n \ttest_create_repo directory_file_switch_v1 &&\n \t(\n \t\tcd directory_file_switch_v1 &&\n-- \ngitgitgadget\n\n"},{"id":"435378","messageId":"6254938948c0db856ffeb4f1c4629c79c6d72197.1631269796.git.gitgitgadget@gmail.com","threadId":"56458","inReplyTo":"pull.1086.v2.git.git.1631269796.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] stash: restore untracked files AFTER restoring tracked files","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-10T10:29:56Z","receivedAt":"2021-09-10T10:30:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nIf a user deletes a file and places a directory of untracked files\nthere, then stashes all these changes, the untracked directory of files\ncannot be restored until after the corresponding file in the way is\nremoved.  So, restore changes to tracked files before restoring\nuntracked files.\n\nThere is no counterpart problem to worry about with the user deleting an\nuntracked file and then add a tracked one in its place.  Git does not\ntrack untracked files, and so will not know the untracked file was\ndeleted, and thus won't be able to stash the removal of that file.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n builtin/stash.c  | 6 +++---\n t/t3903-stash.sh | 4 ++--\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 9ad2940f87a..5512f4942cd 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -535,9 +535,6 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\t}\n \t}\n \n-\tif (info->has_u && restore_untracked(&info->u_tree))\n-\t\treturn error(_(\"could not restore untracked files from stash\"));\n-\n \tinit_merge_options(&o, the_repository);\n \n \to.branch1 = \"Updated upstream\";\n@@ -572,6 +569,9 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\tunstage_changes_unless_new(&c_tree);\n \t}\n \n+\tif (info->has_u && restore_untracked(&info->u_tree))\n+\t\treturn error(_(\"could not restore untracked files from stash\"));\n+\n \tif (!quiet) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 34d1ad9837f..f0a82be9de7 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1324,7 +1324,7 @@ test_expect_success 'git stash succeeds despite directory/file change' '\n \t)\n '\n \n-test_expect_failure 'git stash can pop file -> directory saved changes' '\n+test_expect_success 'git stash can pop file -> directory saved changes' '\n \ttest_create_repo directory_file_switch_v2 &&\n \t(\n \t\tcd directory_file_switch_v2 &&\n@@ -1344,7 +1344,7 @@ test_expect_failure 'git stash can pop file -> directory saved changes' '\n \t)\n '\n \n-test_expect_failure 'git stash can pop directory -> file saved changes' '\n+test_expect_success 'git stash can pop directory -> file saved changes' '\n \ttest_create_repo directory_file_switch_v3 &&\n \t(\n \t\tcd directory_file_switch_v3 &&\n-- \ngitgitgadget\n"}]}