{"thread":{"id":"60021","subject":"Lost files after git stash && git stash pop","startedAt":"2023-07-21T17:32:15Z","lastAt":"2023-08-15T18:04:59Z","messageCount":15,"participants":["Till Friebe","Torsten Bögershausen","Phillip Wood","tboegi@web.de","Eric Sunshine","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"479758","messageId":"5260C6A0-C53C-4F6D-B899-6AD8601F8458@gmail.com","threadId":"60021","inReplyTo":null,"subject":"Lost files after git stash && git stash pop","fromName":"Till Friebe","fromEmail":"friebetill@gmail.com","sentAt":"2023-07-21T17:31:53Z","receivedAt":"2023-07-21T17:32:15Z","isPatch":false,"sender":{"key":"friebetill@gmail.com","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n```\ngit init\nmkdir README\ntouch README/README\ngit add .\ngit commit -m \"Init project\"\necho \"Test\" > README/README\nmv README/README README2\nrmdir README\nmv README2 README\ngit stash \ngit stash pop\n```\n\nWhat did you expect to happen? (Expected behavior)\nI expected that after the `git stash pop` the README file would be back.\n\nWhat happened instead? (Actual behavior)\nThis README with \"Test\" file was deleted and I lost 5 hours of work.\n\nWhat's different between what you expected and what actually happened?\nThe file doesn't exist anymore and I can't recover it.\n\nAnything else you want to add:\nThis is just a reproducible example.\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.39.2 (Apple Git-143)\ncpu: arm64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nfeature: fsmonitor--daemon\nuname: Darwin 22.5.0 Darwin Kernel Version 22.5.0: Thu Jun 8 22:22:20 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T6000 arm64\ncompiler info: clang: 14.0.3 (clang-1403.0.22.14.1)\nlibc info: no libc information available\n$SHELL (typically, interactive shell): /bin/zsh\n\n\n[Enabled Hooks]\n\n"},{"id":"479779","messageId":"20230722214433.3xfoebf7my5wsihf@tb-raspi4","threadId":"60021","inReplyTo":"5260C6A0-C53C-4F6D-B899-6AD8601F8458@gmail.com","subject":"Re: Lost files after git stash && git stash pop","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2023-07-22T21:44:33Z","receivedAt":"2023-07-22T21:44:43Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Fri, Jul 21, 2023 at 07:31:53PM +0200, Till Friebe wrote:\n> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n>\n> What did you do before the bug happened? (Steps to reproduce your issue)\n> ```\n> git init\n> mkdir README\n> touch README/README\n> git add .\n> git commit -m \"Init project\"\n> echo \"Test\" > README/README\n> mv README/README README2\n> rmdir README\n> mv README2 README\n> git stash\n> git stash pop\n> ```\n>\n> What did you expect to happen? (Expected behavior)\n> I expected that after the `git stash pop` the README file would be back.\n>\n> What happened instead? (Actual behavior)\n> This README with \"Test\" file was deleted and I lost 5 hours of work.\n\nThat is always sad to hear, when work is lost.\n\nHowever, I personally wonder if this is a bug or not.\nFirst, Git is told to track a file called README/README\nThen the file is removed, without telling Git.\nAnd a new, unkown file appers on disk (which collides with the name\nof the directory)\n\nUsing this sequence could have told Git, what is going on:\ngit mv README/README README2\nrmdir README\ngit mv README2 README\n\n(a temporary branch may be checked out, with the option\n to merge-squash the final result)\n\n\nAn other alternative could be to tell `git stash` to care\nabout untracked file(s):\n\ngit stash -u\ngit stash pop\n\nWhich will refuse to apply the stash.\n\nA third alternative could be to keep the file inside an\neditor, to have the content still available.\n\nHowever, it would/could be nice, if files are not simply deleted,\nbut saved into a \"lost+found\" folder, or a wastebasket kind of thing.\n\nBut which files ?\nThose that are untracked ?\nThey may be important (local config files, passwords, help scripts, ...)\nor not (.o files from a C compiler).\n\nIn some older discussions they had been named \"precious\" files.\nBut, as far as I remember, there was no easy solution.\nIn that sense I don't have a better answer.\nOthers may have.\n\nThanks for reporting, it make me read [1] and come to the conclusion\nthat it is sometimes safer to checkout out a temporary branch, commit\neverything and clean up later, rather than relying too much on\n`git stash`\n\n\n\n>\n> What's different between what you expected and what actually happened?\n> The file doesn't exist anymore and I can't recover it.\n>\n> Anything else you want to add:\n> This is just a reproducible example.\n>\n"},{"id":"479780","messageId":"a373a659-a232-77cb-a177-a517b1f228f4@gmail.com","threadId":"60021","inReplyTo":"20230722214433.3xfoebf7my5wsihf@tb-raspi4","subject":"Re: Lost files after git stash && git stash pop","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-23T10:01:29Z","receivedAt":"2023-07-23T10:01:40Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 22/07/2023 22:44, Torsten Bögershausen wrote:\n> On Fri, Jul 21, 2023 at 07:31:53PM +0200, Till Friebe wrote:\n>> Thank you for filling out a Git bug report!\n>> Please answer the following questions to help us understand your issue.\n>>\n>> What did you do before the bug happened? (Steps to reproduce your issue)\n>> ```\n>> git init\n>> mkdir README\n>> touch README/README\n>> git add .\n>> git commit -m \"Init project\"\n>> echo \"Test\" > README/README\n>> mv README/README README2\n>> rmdir README\n>> mv README2 README\n>> git stash\n>> git stash pop\n>> ```\n>>\n>> What did you expect to happen? (Expected behavior)\n>> I expected that after the `git stash pop` the README file would be back.\n>>\n>> What happened instead? (Actual behavior)\n>> This README with \"Test\" file was deleted and I lost 5 hours of work.\n> \n> That is always sad to hear, when work is lost.\n\nIndeed it is. Thanks Till for providing an easy reproducer.\n\n> However, I personally wonder if this is a bug or not.\n\nI think whenever git overwrites an untracked file without the user \npassing some option indicating that they want to do so it is a bug. For \nexample \"git checkout\" refuses to overwrite untracked files by default. \nSadly this seems to be a known bug in do_push_stash() where we are using \n\"git reset --hard\" to remove the stashed changes from the working copy. \nThis was documented in 94b7f1563a (Comment important codepaths regarding \nnuking untracked files/dirs, 2021-09-27). The stash implementation does \na lot of necessary forking of subprocesses, in this case I think it \nwould be better to call unpack_trees() directly with \nUNPACK_RESET_PROTECT_UNTRACKED.\n\nBest Wishes\n\nPhillip\n\n> First, Git is told to track a file called README/README\n> Then the file is removed, without telling Git.\n> And a new, unkown file appers on disk (which collides with the name\n> of the directory)\n> \n> Using this sequence could have told Git, what is going on:\n> git mv README/README README2\n> rmdir README\n> git mv README2 README\n> \n> (a temporary branch may be checked out, with the option\n> to merge-squash the final result)\n> \n> \n> An other alternative could be to tell `git stash` to care\n> about untracked file(s):\n> \n> git stash -u\n> git stash pop\n> \n> Which will refuse to apply the stash.\n> \n> A third alternative could be to keep the file inside an\n> editor, to have the content still available.\n> \n> However, it would/could be nice, if files are not simply deleted,\n> but saved into a \"lost+found\" folder, or a wastebasket kind of thing.\n> \n> But which files ?\n> Those that are untracked ?\n> They may be important (local config files, passwords, help scripts, ...)\n> or not (.o files from a C compiler).\n> \n> In some older discussions they had been named \"precious\" files.\n> But, as far as I remember, there was no easy solution.\n> In that sense I don't have a better answer.\n> Others may have.\n> \n> Thanks for reporting, it make me read [1] and come to the conclusion\n> that it is sometimes safer to checkout out a temporary branch, commit\n> everything and clean up later, rather than relying too much on\n> `git stash`\n> \n> \n> \n>>\n>> What's different between what you expected and what actually happened?\n>> The file doesn't exist anymore and I can't recover it.\n>>\n>> Anything else you want to add:\n>> This is just a reproducible example.\n>>\n"},{"id":"479787","messageId":"20230723205239.5snlakmd5ocy67q2@tb-raspi4","threadId":"60021","inReplyTo":"a373a659-a232-77cb-a177-a517b1f228f4@gmail.com","subject":"Re: Lost files after git stash && git stash pop","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2023-07-23T20:52:39Z","receivedAt":"2023-07-23T20:52:51Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Sun, Jul 23, 2023 at 11:01:29AM +0100, Phillip Wood wrote:\n> On 22/07/2023 22:44, Torsten Bögershausen wrote:\n> > On Fri, Jul 21, 2023 at 07:31:53PM +0200, Till Friebe wrote:\n> > > Thank you for filling out a Git bug report!\n> > > Please answer the following questions to help us understand your issue.\n> > >\n> > > What did you do before the bug happened? (Steps to reproduce your issue)\n> > > ```\n> > > git init\n> > > mkdir README\n> > > touch README/README\n> > > git add .\n> > > git commit -m \"Init project\"\n> > > echo \"Test\" > README/README\n> > > mv README/README README2\n> > > rmdir README\n> > > mv README2 README\n> > > git stash\n> > > git stash pop\n> > > ```\n> > >\n> > > What did you expect to happen? (Expected behavior)\n> > > I expected that after the `git stash pop` the README file would be back.\n> > >\n> > > What happened instead? (Actual behavior)\n> > > This README with \"Test\" file was deleted and I lost 5 hours of work.\n> >\n> > That is always sad to hear, when work is lost.\n>\n> Indeed it is. Thanks Till for providing an easy reproducer.\n>\n> > However, I personally wonder if this is a bug or not.\n>\n> I think whenever git overwrites an untracked file without the user passing\n> some option indicating that they want to do so it is a bug.\n\nOK, agreed after reading the next sentence.\n\n> For example \"git\n> checkout\" refuses to overwrite untracked files by default. Sadly this seems\n> to be a known bug in do_push_stash() where we are using \"git reset --hard\"\n> to remove the stashed changes from the working copy. This was documented in\n> 94b7f1563a (Comment important codepaths regarding nuking untracked\n> files/dirs, 2021-09-27). The stash implementation does a lot of necessary\n> forking of subprocesses, in this case I think it would be better to call\n> unpack_trees() directly with UNPACK_RESET_PROTECT_UNTRACKED.\n\nThanks for the fast response.\n\nThis is not an area of Git, where I have much understanding of the code.\nBut is seems as if pop_stash() in builtin/stash.c\n(and the called functions) seems to be the problem here ?\n\n"},{"id":"479801","messageId":"6dbd02ca-9587-f797-f2d3-035fc2d9efc0@gmail.com","threadId":"60021","inReplyTo":"20230723205239.5snlakmd5ocy67q2@tb-raspi4","subject":"Re: Lost files after git stash && git stash pop","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-07-24T09:59:51Z","receivedAt":"2023-07-24T10:08:48Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Torsten\n\nOn 23/07/2023 21:52, Torsten Bögershausen wrote:\n>> I think whenever git overwrites an untracked file without the user passing\n>> some option indicating that they want to do so it is a bug.\n> \n> OK, agreed after reading the next sentence.\n> \n>> For example \"git\n>> checkout\" refuses to overwrite untracked files by default. Sadly this seems\n>> to be a known bug in do_push_stash() where we are using \"git reset --hard\"\n>> to remove the stashed changes from the working copy. This was documented in\n>> 94b7f1563a (Comment important codepaths regarding nuking untracked\n>> files/dirs, 2021-09-27). The stash implementation does a lot of necessary\n>> forking of subprocesses, in this case I think it would be better to call\n>> unpack_trees() directly with UNPACK_RESET_PROTECT_UNTRACKED.\n> \n> Thanks for the fast response.\n> \n> This is not an area of Git, where I have much understanding of the code.\n> But is seems as if pop_stash() in builtin/stash.c\n> (and the called functions) seems to be the problem here ?\n\nConfusingly it is creating the stash that deletes the untracked file because\nit recreates README/README. do_push_stash() in builtin/stash.c is the culprit\nI think. I had hoped the diff below would fix the problem but it does not\nseem to and breaks half a dozen test cases that seem to rely on removing\nuntracked files. Unfortunately I don't really have time to dig any\nfurther at the moment.\n\nBest Wishes\n\nPhillip\n\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex fe64cde9ce3..c8bbfe56d26 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -29,6 +29,8 @@\n #include \"exec-cmd.h\"\n #include \"reflog.h\"\n #include \"add-interactive.h\"\n+#include \"reset.h\"\n+#include \"submodule.h\"\n \n #define INCLUDE_ALL_FILES 2\n \n@@ -336,7 +338,7 @@ static int apply_cached(struct strbuf *out)\n return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);\n }\n \n-static int reset_head(void)\n+static int stash_reset_head(void)\n {\n struct child_process cp = CHILD_PROCESS_INIT;\n \n@@ -569,7 +571,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n get_index_file(), 0, NULL))\n return error(_(\"could not save index tree\"));\n \n- reset_head();\n+ stash_reset_head();\n discard_index(&the_index);\n repo_read_index(the_repository);\n }\n@@ -1649,12 +1651,14 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n goto done;\n }\n } else {\n- struct child_process cp = CHILD_PROCESS_INIT;\n- cp.git_cmd = 1;\n- /* BUG: this nukes untracked files in the way */\n- strvec_pushl(&cp.args, \"reset\", \"--hard\", \"-q\",\n- \"--no-recurse-submodules\", NULL);\n- if (run_command(&cp)) {\n+ struct reset_head_opts opts = {\n+ .flags = RESET_HEAD_HARD,\n+ };\n+\n+ if (should_update_submodules())\n+ BUG(\"stash should not update submodules\");\n+\n+ if (reset_head(the_repository, &opts)) {\n ret = -1;\n goto done;\n }\n\n"},{"id":"480306","messageId":"20230808172624.14205-1-tboegi@web.de","threadId":"60021","inReplyTo":"5260C6A0-C53C-4F6D-B899-6AD8601F8458@gmail.com","subject":"[PATCH v1 1/1] git stash needing mkdir deletes untracked file","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2023-08-08T17:26:24Z","receivedAt":"2023-08-08T19:00:32Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nThe following sequence leads to loss of work:\n git init\n mkdir README\n touch README/README\n git add .\n git commit -m \"Init project\"\n echo \"Test\" > README/README\n mv README/README README2\n rmdir README\n mv README2 README\n git stash\n git stash pop\n\nThe problem is, that `git stash` needs to create the directory README/\nand to be able to do this, the file README needs to be removed.\nAnd this is, where the work was lost.\nThere are different possibilities preventing this loss of work:\na)\n `git stash` does refuse the removel of the untracked file,\n when a directory with the same name needs to be created\n There is a small problem here:\n In the ideal world, the stash would do nothing at all,\n and not do anything but complain.\n The current code makes this hard to achieve\n An other solution could be to do as much stash work as possible,\n but stop when the file/directory conflict is detected.\n This would create some inconsistent state.\n\nb) Create the directory as needed, but rename the file before doing that.\n This would let the `git stash` proceed as usual and create a \"new\" file,\n which may be surprising for some worlflows.\n\nThis change goes for b), as it seems the most intuitive solution for\nGit users.\n\nIntrodue a new function rename_to_untracked_or_warn() and use it\nin create_directories() in entry.c\n\nReported-by: Till Friebe \nSigned-off-by: Torsten Bögershausen \n---\n entry.c | 25 ++++++++++++++++++++++++-\n t/t3903-stash.sh | 23 +++++++++++++++++++++++\n 2 files changed, 47 insertions(+), 1 deletion(-)\n\ndiff --git a/entry.c b/entry.c\nindex 43767f9043..76d8a0762d 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -15,6 +15,28 @@\n #include \"entry.h\"\n #include \"parallel-checkout.h\"\n\n+static int rename_to_untracked_or_warn(const char *file)\n+{\n+\tconst size_t file_name_len = strlen(file);\n+\tconst static char *dot_untracked = \".untracked\";\n+\tconst size_t dot_un_len = strlen(dot_untracked);\n+\tstruct strbuf sb;\n+\tint ret;\n+\n+\tstrbuf_init(&sb, file_name_len + dot_un_len);\n+\tstrbuf_add(&sb, file, file_name_len);\n+\tstrbuf_add(&sb, dot_untracked, dot_un_len);\n+\tret = rename(file, sb.buf);\n+\n+\tif (ret) {\n+\t\tint saved_errno = errno;\n+\t\twarning_errno(_(\"unable rename '%s' into '%s'\"), file, sb.buf);\n+\t\terrno = saved_errno;\n+\t}\n+\tstrbuf_release(&sb);\n+\treturn ret;\n+}\n+\n static void create_directories(const char *path, int path_len,\n \t\t\t const struct checkout *state)\n {\n@@ -48,7 +70,8 @@ static void create_directories(const char *path, int path_len,\n \t\t */\n \t\tif (mkdir(buf, 0777)) {\n \t\t\tif (errno == EEXIST && state->force &&\n-\t\t\t !unlink_or_warn(buf) && !mkdir(buf, 0777))\n+\t\t\t !rename_to_untracked_or_warn(buf) &&\n+\t\t\t !mkdir(buf, 0777))\n \t\t\t\tcontinue;\n \t\t\tdie_errno(\"cannot create directory at '%s'\", buf);\n \t\t}\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 0b3dfeaea2..1a210f8a5a 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1512,4 +1512,27 @@ test_expect_success 'restore untracked files even when we hit conflicts' '\n \t)\n '\n\n+test_expect_success 'stash mkdir README needed - README.untracked created' '\n+\tgit init mkdir_needed_file_untracked &&\n+\t(\n+\t\tcd mkdir_needed_file_untracked &&\n+\t\tmkdir README &&\n+\t\ttouch README/README &&\n+\t\tgit add . &&\n+\t\tgit commit -m \"Add README/README\" &&\n+\t\techo Version2 > README/README &&\n+\t\tmv README/README README2 &&\n+\t\trmdir README &&\n+\t\tmv README2 README &&\n+\t\tgit stash &&\n+\t\ttest_path_is_file README.untracked &&\n+\t\techo Version2 >expect &&\n+\t\ttest_cmp expect README.untracked &&\n+\t\trm expect &&\n+\t\tgit stash pop &&\n+\t\ttest_path_is_file README.untracked &&\n+\t\techo Version2 >expect &&\n+\t\ttest_cmp expect README.untracked\n+\t)\n+'\n test_done\n--\n2.41.0.394.ge43f4fd0bd\n\n"},{"id":"480318","messageId":"20230808180318.bu5nlbrnndpfkuxt@tb-raspi4","threadId":"60021","inReplyTo":"20230808172624.14205-1-tboegi@web.de","subject":"Re: [PATCH v1 1/1] git stash needing mkdir deletes untracked file","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2023-08-08T18:03:18Z","receivedAt":"2023-08-08T19:53:38Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Aug 08, 2023 at 07:26:24PM +0200, tboegi@web.de wrote:\n> From: Torsten Bögershausen \n> .\n\n... The following sequence leads to loss of work:\n\nI just realized that this breaks other tests:\nt1092-sparse-checkout-compatibility.sh not ok 17 - diff with renames and conflicts\nt1092-sparse-checkout-compatibility.sh not ok 18 - diff with directory/file conflicts\n\nHowever, comments are welcome:\nIs it a good idea to create file called \"filename.untracked\" ?\n"},{"id":"480326","messageId":"CAPig+cSLoKc16AJkrZkVgQGd5deg+LLSaQYo29d8VCxPTsAO7g@mail.gmail.com","threadId":"60021","inReplyTo":"20230808172624.14205-1-tboegi@web.de","subject":"Re: [PATCH v1 1/1] git stash needing mkdir deletes untracked file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-08-08T19:28:11Z","receivedAt":"2023-08-08T20:21:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 8, 2023 at 3:15 PM wrote:\n> The following sequence leads to loss of work:\n> git init\n> mkdir README\n> touch README/README\n> git add .\n> git commit -m \"Init project\"\n> echo \"Test\" > README/README\n> mv README/README README2\n> rmdir README\n> mv README2 README\n> git stash\n> git stash pop\n>\n> The problem is, that `git stash` needs to create the directory README/\n> and to be able to do this, the file README needs to be removed.\n> And this is, where the work was lost.\n> There are different possibilities preventing this loss of work:\n> a)\n> `git stash` does refuse the removel of the untracked file,\n\ns/removel/removal/\n\n> when a directory with the same name needs to be created\n\ns/$/./\n\n> There is a small problem here:\n> In the ideal world, the stash would do nothing at all,\n> and not do anything but complain.\n> The current code makes this hard to achieve\n\ns/$/./\n\n> An other solution could be to do as much stash work as possible,\n\ns/An other/Another/\n\n> but stop when the file/directory conflict is detected.\n> This would create some inconsistent state.\n>\n> b) Create the directory as needed, but rename the file before doing that.\n> This would let the `git stash` proceed as usual and create a \"new\" file,\n> which may be surprising for some worlflows.\n\ns/worlflows/workflows/\n\n> This change goes for b), as it seems the most intuitive solution for\n> Git users.\n>\n> Introdue a new function rename_to_untracked_or_warn() and use it\n\ns/Introdue/Introduce/\n\n> in create_directories() in entry.c\n>\n> Reported-by: Till Friebe \n> Signed-off-by: Torsten Bögershausen \n> ---\n> diff --git a/entry.c b/entry.c\n> @@ -15,6 +15,28 @@\n> +static int rename_to_untracked_or_warn(const char *file)\n> +{\n> + const size_t file_name_len = strlen(file);\n> + const static char *dot_untracked = \".untracked\";\n> + const size_t dot_un_len = strlen(dot_untracked);\n> + struct strbuf sb;\n> + int ret;\n> +\n> + strbuf_init(&sb, file_name_len + dot_un_len);\n> + strbuf_add(&sb, file, file_name_len);\n> + strbuf_add(&sb, dot_untracked, dot_un_len);\n> + ret = rename(file, sb.buf);\n\nThis could probably all be simplified to:\n\n char *to = xstrfmt(\"%s.untracked\", file);\n ret = rename(...);\n ...\n free(to);\n\nIf there is already a file named \"foo.untracked\", then this will\noverwrite it, thus potentially losing work, right? I wonder if it\nmakes sense to be a bit more careful.\n\n> + if (ret) {\n> + int saved_errno = errno;\n> + warning_errno(_(\"unable rename '%s' into '%s'\"), file, sb.buf);\n> + errno = saved_errno;\n> + }\n> + strbuf_release(&sb);\n> + return ret;\n> +}\n\nDo we want to give the user some warning/notification that their file,\nas a safety precaution, got renamed to \"foo.untracked\"?\n\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> @@ -1512,4 +1512,27 @@ test_expect_success 'restore untracked files even when we hit conflicts' '\n> +test_expect_success 'stash mkdir README needed - README.untracked created' '\n> + git init mkdir_needed_file_untracked &&\n> + (\n> + cd mkdir_needed_file_untracked &&\n> + mkdir README &&\n> + touch README/README &&\n\ns/touch/>/\n\n> + git add . &&\n> + git commit -m \"Add README/README\" &&\n> + echo Version2 > README/README &&\n\ns/> R/>R/\n\n> + mv README/README README2 &&\n> + rmdir README &&\n> + mv README2 README &&\n> + git stash &&\n> + test_path_is_file README.untracked &&\n> + echo Version2 >expect &&\n> + test_cmp expect README.untracked &&\n> + rm expect &&\n> + git stash pop &&\n> + test_path_is_file README.untracked &&\n> + echo Version2 >expect &&\n> + test_cmp expect README.untracked\n> + )\n> +'\n"},{"id":"480360","messageId":"6e40eb0b-2331-1e39-bee0-c9720c24d1c8@gmail.com","threadId":"60021","inReplyTo":"20230808172624.14205-1-tboegi@web.de","subject":"Re: [PATCH v1 1/1] git stash needing mkdir deletes untracked file","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-09T13:15:28Z","receivedAt":"2023-08-09T13:15:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Torsten\n\nThanks for working on this. I've cc'd Junio for his unpack_trees() \nknowledge.\n\nOn 08/08/2023 18:26, tboegi@web.de wrote:\n> From: Torsten Bögershausen \n> \n> The following sequence leads to loss of work:\n> git init\n> mkdir README\n> touch README/README\n> git add .\n> git commit -m \"Init project\"\n> echo \"Test\" > README/README\n> mv README/README README2\n> rmdir README\n> mv README2 README\n> git stash\n> git stash pop\n> \n> The problem is, that `git stash` needs to create the directory README/\n> and to be able to do this, the file README needs to be removed.\n> And this is, where the work was lost.\n> There are different possibilities preventing this loss of work:\n> a)\n> `git stash` does refuse the removel of the untracked file,\n> when a directory with the same name needs to be created\n> There is a small problem here:\n> In the ideal world, the stash would do nothing at all,\n> and not do anything but complain.\n> The current code makes this hard to achieve\n> An other solution could be to do as much stash work as possible,\n> but stop when the file/directory conflict is detected.\n> This would create some inconsistent state.\n> \n> b) Create the directory as needed, but rename the file before doing that.\n> This would let the `git stash` proceed as usual and create a \"new\" file,\n> which may be surprising for some worlflows.\n> \n> This change goes for b), as it seems the most intuitive solution for\n> Git users.\n> \n> Introdue a new function rename_to_untracked_or_warn() and use it\n> in create_directories() in entry.c\n\nAlthough this change is framed in terms of changes to \"git stash push\" I \nthink the underlying issue and this patch actually affects all users of \nunpack_trees(). For example if \"README\" is untracked then\n\n\tgit checkout README\n\nwill currently fail if :README is a blob but will succeed and \nremove the untracked file if :README is a tree.\n\nI'm far from an expert in this area but I think we might want to \nunderstand why unpack_trees() sets state->force when it calls \ncheckout_entry() before making any changes.\n\nBest Wishes\n\nPhillip\n\n> Reported-by: Till Friebe \n> Signed-off-by: Torsten Bögershausen \n> ---\n> entry.c | 25 ++++++++++++++++++++++++-\n> t/t3903-stash.sh | 23 +++++++++++++++++++++++\n> 2 files changed, 47 insertions(+), 1 deletion(-)\n> \n> diff --git a/entry.c b/entry.c\n> index 43767f9043..76d8a0762d 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -15,6 +15,28 @@\n> #include \"entry.h\"\n> #include \"parallel-checkout.h\"\n> \n> +static int rename_to_untracked_or_warn(const char *file)\n> +{\n> +\tconst size_t file_name_len = strlen(file);\n> +\tconst static char *dot_untracked = \".untracked\";\n> +\tconst size_t dot_un_len = strlen(dot_untracked);\n> +\tstruct strbuf sb;\n> +\tint ret;\n> +\n> +\tstrbuf_init(&sb, file_name_len + dot_un_len);\n> +\tstrbuf_add(&sb, file, file_name_len);\n> +\tstrbuf_add(&sb, dot_untracked, dot_un_len);\n> +\tret = rename(file, sb.buf);\n> +\n> +\tif (ret) {\n> +\t\tint saved_errno = errno;\n> +\t\twarning_errno(_(\"unable rename '%s' into '%s'\"), file, sb.buf);\n> +\t\terrno = saved_errno;\n> +\t}\n> +\tstrbuf_release(&sb);\n> +\treturn ret;\n> +}\n> +\n> static void create_directories(const char *path, int path_len,\n> \t\t\t const struct checkout *state)\n> {\n> @@ -48,7 +70,8 @@ static void create_directories(const char *path, int path_len,\n> \t\t */\n> \t\tif (mkdir(buf, 0777)) {\n> \t\t\tif (errno == EEXIST && state->force &&\n> -\t\t\t !unlink_or_warn(buf) && !mkdir(buf, 0777))\n> +\t\t\t !rename_to_untracked_or_warn(buf) &&\n> +\t\t\t !mkdir(buf, 0777))\n> \t\t\t\tcontinue;\n> \t\t\tdie_errno(\"cannot create directory at '%s'\", buf);\n> \t\t}\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 0b3dfeaea2..1a210f8a5a 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -1512,4 +1512,27 @@ test_expect_success 'restore untracked files even when we hit conflicts' '\n> \t)\n> '\n> \n> +test_expect_success 'stash mkdir README needed - README.untracked created' '\n> +\tgit init mkdir_needed_file_untracked &&\n> +\t(\n> +\t\tcd mkdir_needed_file_untracked &&\n> +\t\tmkdir README &&\n> +\t\ttouch README/README &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m \"Add README/README\" &&\n> +\t\techo Version2 > README/README &&\n> +\t\tmv README/README README2 &&\n> +\t\trmdir README &&\n> +\t\tmv README2 README &&\n> +\t\tgit stash &&\n> +\t\ttest_path_is_file README.untracked &&\n> +\t\techo Version2 >expect &&\n> +\t\ttest_cmp expect README.untracked &&\n> +\t\trm expect &&\n> +\t\tgit stash pop &&\n> +\t\ttest_path_is_file README.untracked &&\n> +\t\techo Version2 >expect &&\n> +\t\ttest_cmp expect README.untracked\n> +\t)\n> +'\n> test_done\n> --\n> 2.41.0.394.ge43f4fd0bd\n> \n\n"},{"id":"480385","messageId":"20230809184751.ffwolkvjwoptnmen@tb-raspi4","threadId":"60021","inReplyTo":"6e40eb0b-2331-1e39-bee0-c9720c24d1c8@gmail.com","subject":"Re: [PATCH v1 1/1] git stash needing mkdir deletes untracked file","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2023-08-09T18:47:52Z","receivedAt":"2023-08-09T18:48:11Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Wed, Aug 09, 2023 at 02:15:28PM +0100, Phillip Wood wrote:\n> Hi Torsten\n>\n> Thanks for working on this. I've cc'd Junio for his unpack_trees()\n> knowledge.\n\nThanks Eric for the review.\n\nHej Phillip,\nI have been playing around with the whole thing some time.\nAt the end I had a version, which did fiddle the information\nthat we are doing a `git stash` (and not any other operation)\ninto entry.c, and all test cases passed.\nSo in principle I can dig out all changes, polish them\nand send them out, after doing cleanups of course.\n\n(And that could take a couple of days, or weeks ;-)\n\nMy main question is still open:\nIs it a good idea, to create a \"helper file\" ?\nThe naming can be discussed, we may stick the date/time\ninto the filename to make it really unique, or so.\n\nReading the different reports and including own experience,\nI still think that a directory called \".deleted-by-user\"\nor \".wastebin\" or something in that style is a good idea.\n\nWhat do others think ?\n\n\n\n\n"},{"id":"480401","messageId":"xmqqo7jgkk7s.fsf@gitster.g","threadId":"60021","inReplyTo":"6e40eb0b-2331-1e39-bee0-c9720c24d1c8@gmail.com","subject":"Re: [PATCH v1 1/1] git stash needing mkdir deletes untracked file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-09T20:57:11Z","receivedAt":"2023-08-09T20:57:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Although this change is framed in terms of changes to \"git stash push\"\n> I think the underlying issue and this patch actually affects all users\n> of unpack_trees(). For example if \"README\" is untracked then\n>\n> \tgit checkout README\n>\n> will currently fail if :README is a blob but will succeed and\n> remove the untracked file if :README is a tree.\n\nVery true, and with an .untracked file nobody asked Git to create,\npresumably? I am not sure if the updated behaviour is better than\nthe current behaviour. \n\nIf \"silent and unconditional removal\" bothers us, I wonder if it is\na lot better approach to error out and have the user sort out the\nmess, which is what we usually do when it gets tempting to \"move it\naway with an arbitrary rename\" like this patch tries to do. I\ndunno.\n\nThanks.\n\n\n\n"},{"id":"480670","messageId":"9f76de24-d337-ed41-fb81-888dba0b1656@gmail.com","threadId":"60021","inReplyTo":"20230809184751.ffwolkvjwoptnmen@tb-raspi4","subject":"Re: [PATCH v1 1/1] git stash needing mkdir deletes untracked file","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-15T09:15:37Z","receivedAt":"2023-08-15T09:16:46Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Torsten\n\nSorry for the slow reply\n\nOn 09/08/2023 19:47, Torsten Bögershausen wrote:\n> On Wed, Aug 09, 2023 at 02:15:28PM +0100, Phillip Wood wrote:\n>> Hi Torsten\n>>\n>> Thanks for working on this. I've cc'd Junio for his unpack_trees()\n>> knowledge.\n> \n> Thanks Eric for the review.\n> \n> Hej Phillip,\n> I have been playing around with the whole thing some time.\n> At the end I had a version, which did fiddle the information\n> that we are doing a `git stash` (and not any other operation)\n> into entry.c, and all test cases passed.\n> So in principle I can dig out all changes, polish them\n> and send them out, after doing cleanups of course.\n\nI don't think we should be treating \"git stash\" as a special case here - \ncommands like \"git checkout\" should not be removing untracked files \nunprompted either.\n\n> (And that could take a couple of days, or weeks ;-)\n> \n> My main question is still open:\n> Is it a good idea, to create a \"helper file\" ?\n> The naming can be discussed, we may stick the date/time\n> into the filename to make it really unique, or so.\n\nI think stopping and telling the user that the file would be overwritten \nas we do in other cases would be better.\n\n> Reading the different reports and including own experience,\n> I still think that a directory called \".deleted-by-user\"\n> or \".wastebin\" or something in that style is a good idea.\n\nI can see an argument for being able to opt-in to that for \"git restore\" \nand \"git reset --hard\" but that is a different problem to the one here.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"480671","messageId":"8adfe4d7-f3ac-6aae-6193-4cd0ac18aaba@gmail.com","threadId":"60021","inReplyTo":"xmqqo7jgkk7s.fsf@gitster.g","subject":"Re: [PATCH v1 1/1] git stash needing mkdir deletes untracked file","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-08-15T09:16:39Z","receivedAt":"2023-08-15T09:18:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 09/08/2023 21:57, Junio C Hamano wrote:\n> Phillip Wood writes:\n>\n> If \"silent and unconditional removal\" bothers us, I wonder if it is\n> a lot better approach to error out and have the user sort out the\n> mess, which is what we usually \n\nYes I think that would be a better approach.\n\nBest Wishes\n\nPhillip\n\n> do when it gets tempting to \"move it\n> away with an arbitrary rename\" like this patch tries to do. I\n> dunno.\n> \n> Thanks.\n> \n> \n> \n\n"},{"id":"480676","messageId":"20230815152511.suipgnzr2wgolmsx@tb-raspi4","threadId":"60021","inReplyTo":"9f76de24-d337-ed41-fb81-888dba0b1656@gmail.com","subject":"Re: [PATCH v1 1/1] git stash needing mkdir deletes untracked file","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2023-08-15T15:25:33Z","receivedAt":"2023-08-15T15:26:30Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Aug 15, 2023 at 10:15:37AM +0100, Phillip Wood wrote:\n> Hi Torsten\n>\n> Sorry for the slow reply\n\nNo problem.\nThanks for the response, I think that we have an\nagreement not to overwrite an untracked file, when a directory\nwith the same name needs to be created.\n\nI try to come up with a patch series -\nstarting with the stash operation.\n\n>\n> On 09/08/2023 19:47, Torsten Bögershausen wrote:\n> > On Wed, Aug 09, 2023 at 02:15:28PM +0100, Phillip Wood wrote:\n> > > Hi Torsten\n> > >\n> > > Thanks for working on this. I've cc'd Junio for his unpack_trees()\n> > > knowledge.\n> >\n> > Thanks Eric for the review.\n> >\n> > Hej Phillip,\n> > I have been playing around with the whole thing some time.\n> > At the end I had a version, which did fiddle the information\n> > that we are doing a `git stash` (and not any other operation)\n> > into entry.c, and all test cases passed.\n> > So in principle I can dig out all changes, polish them\n> > and send them out, after doing cleanups of course.\n>\n> I don't think we should be treating \"git stash\" as a special case here -\n> commands like \"git checkout\" should not be removing untracked files\n> unprompted either.\n>\n> > (And that could take a couple of days, or weeks ;-)\n> >\n> > My main question is still open:\n> > Is it a good idea, to create a \"helper file\" ?\n> > The naming can be discussed, we may stick the date/time\n> > into the filename to make it really unique, or so.\n>\n> I think stopping and telling the user that the file would be overwritten as\n> we do in other cases would be better.\n>\n> > Reading the different reports and including own experience,\n> > I still think that a directory called \".deleted-by-user\"\n> > or \".wastebin\" or something in that style is a good idea.\n>\n> I can see an argument for being able to opt-in to that for \"git restore\" and\n> \"git reset --hard\" but that is a different problem to the one here.\n>\n> Best Wishes\n>\n> Phillip\n>\n"},{"id":"480679","messageId":"xmqqcyzoji7u.fsf@gitster.g","threadId":"60021","inReplyTo":"9f76de24-d337-ed41-fb81-888dba0b1656@gmail.com","subject":"Re: [PATCH v1 1/1] git stash needing mkdir deletes untracked file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-15T18:03:49Z","receivedAt":"2023-08-15T18:04:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> I don't think we should be treating \"git stash\" as a special case here\n> - commands like \"git checkout\" should not be removing untracked files\n> unprompted either.\n\nYeah, I tend to agree. \"git checkout branch path\" should overwrite\na leftover \"path\" in the working tree in response to such an\nexplicit request, and that should equally apply for a request with\npathspec e.g. \"git checkout branch .\", as the latter is also an\nexplicit \"please check out all paths out of the tree-ish of the\nbranch\".\n\nBut \"git checkout branch\" in a working tree with untracked \"path\"\nshould not lose it if \"branch\" has it as a tracked file.\n\n> I think stopping and telling the user that the file would be\n> overwritten as we do in other cases would be better.\n\nYup, that is what we have done and probably one of the design\nchoices that made us successful.\n\n>> Reading the different reports and including own experience,\n>> I still think that a directory called \".deleted-by-user\"\n>> or \".wastebin\" or something in that style is a good idea.\n>\n> I can see an argument for being able to opt-in to that for \"git\n> restore\" and \"git reset --hard\" but that is a different problem to the\n> one here.\n\nYeah, I tend to agree. If anything, such a trash directory should\nbe kept out-of-line, not inside the working tree. Perhaps in $HOME\nor somewhere, and not necessarily tied to the use of Git, as the way\na file gets \"deleted by user\" is not necessarily limited to the use\nof Git.\n\n"}]}