{"thread":{"id":"57183","subject":"[PATCH] stash: do not return before restoring untracked files","startedAt":"2022-01-04T23:05:03Z","lastAt":"2022-01-04T23:46:31Z","messageCount":2,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"445488","messageId":"pull.1180.git.git.1641337498996.gitgitgadget@gmail.com","threadId":"57183","inReplyTo":null,"subject":"[PATCH] stash: do not return before restoring untracked files","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-04T23:04:58Z","receivedAt":"2022-01-04T23:05: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\nIn commit bee8691f19 (\"stash: restore untracked files AFTER restoring\ntracked files\", 2021-09-10), we correctly identified that we should\nrestore changes to tracked files before attempting to restore untracked\nfiles, and accordingly moved the code for restoring untracked files a\nfew lines down in do_apply_stash().  Unfortunately, the intervening\nlines had some early return statements meaning that we suddenly stopped\nrestoring untracked files in some cases.\n\nEven before the previous commit, there was another possible issue with\nthe current code -- a post-stash-apply 'git status' that was intended\nto be run after restoring the stash was skipped when we hit a conflict\n(or other error condition), which seems slightly inconsistent.\n\nFix both issues by saving the return status, and letting other\nfunctionality run before returning.\n\nReported-by: AJ Henderson\nTest-case-by: Randall S. Becker <randall.becker@nexbridge.ca>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    stash: do not return before restoring untracked files\n    \n    This fixes a regression in v2.34.1 relative to v2.33.0.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1180%2Fnewren%2Ffix-stash-restore-untracked-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1180/newren/fix-stash-restore-untracked-v1\nPull-Request: https://github.com/git/git/pull/1180\n\n builtin/stash.c  |  9 +++++----\n t/t3903-stash.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 18c812bbe03..397210168da 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -561,18 +561,19 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\tif (index)\n \t\t\tfprintf_ln(stderr, _(\"Index was not unstashed.\"));\n \n-\t\treturn ret;\n+\t\tgoto restore_untracked;\n \t}\n \n \tif (has_index) {\n \t\tif (reset_tree(&index_tree, 0, 0))\n-\t\t\treturn -1;\n+\t\t\tret = -1;\n \t} else {\n \t\tunstage_changes_unless_new(&c_tree);\n \t}\n \n+restore_untracked:\n \tif (info->has_u && restore_untracked(&info->u_tree))\n-\t\treturn error(_(\"could not restore untracked files from stash\"));\n+\t\tret = error(_(\"could not restore untracked files from stash\"));\n \n \tif (!quiet) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n@@ -592,7 +593,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\trun_command(&cp);\n \t}\n \n-\treturn 0;\n+\treturn ret;\n }\n \n static int apply_stash(int argc, const char **argv, const char *prefix)\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 2c66cfbc3b7..2a7d8e511db 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1376,4 +1376,28 @@ test_expect_success 'git stash can pop directory -> file saved changes' '\n \t)\n '\n \n+test_expect_success 'restore untracked files even when we hit conflicts' '\n+\tgit init restore_untracked_after_conflict &&\n+\t(\n+\t\tcd restore_untracked_after_conflict &&\n+\n+\t\techo hi >a &&\n+\t\techo there >b &&\n+\t\tgit add . &&\n+\t\tgit commit -m first &&\n+\t\techo hello >a &&\n+\t\techo something >c &&\n+\n+\t\tgit stash push --include-untracked &&\n+\n+\t\techo conflict >a &&\n+\t\tgit add a &&\n+\t\tgit commit -m second &&\n+\n+\t\ttest_must_fail git stash pop &&\n+\n+\t\ttest_path_is_file c\n+\t)\n+'\n+\n test_done\n\nbase-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907\n-- \ngitgitgadget\n"},{"id":"445494","messageId":"xmqqy23v83os.fsf@gitster.g","threadId":"57183","inReplyTo":"pull.1180.git.git.1641337498996.gitgitgadget@gmail.com","subject":"Re: [PATCH] stash: do not return before restoring untracked files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-04T23:46:27Z","receivedAt":"2022-01-04T23:46:31Z","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> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 18c812bbe03..397210168da 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -561,18 +561,19 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n>  \t\tif (index)\n>  \t\t\tfprintf_ln(stderr, _(\"Index was not unstashed.\"));\n>  \n> -\t\treturn ret;\n> +\t\tgoto restore_untracked;\n>  \t}\n>  \n>  \tif (has_index) {\n>  \t\tif (reset_tree(&index_tree, 0, 0))\n> -\t\t\treturn -1;\n> +\t\t\tret = -1;\n>  \t} else {\n>  \t\tunstage_changes_unless_new(&c_tree);\n>  \t}\n>  \n> +restore_untracked:\n>  \tif (info->has_u && restore_untracked(&info->u_tree))\n> -\t\treturn error(_(\"could not restore untracked files from stash\"));\n> +\t\tret = error(_(\"could not restore untracked files from stash\"));\n\nIn other words, instead of doing an early return, we try to\nresurrect untracked ones and always show the \"git status\" before\nreturning.\n\nI think that takes us in the right direction.  I looked at other\nearly returns in this function, and while there may be a room for\nimprovements when the --index stash does not apply well, I think it\ncan be left outside the topic of this patch.\n\nWill queue.\n\nThanks.\n\n>  \tif (!quiet) {\n>  \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> @@ -592,7 +593,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n>  \t\trun_command(&cp);\n>  \t}\n>  \n> -\treturn 0;\n> +\treturn ret;\n>  }\n>  \n>  static int apply_stash(int argc, const char **argv, const char *prefix)\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 2c66cfbc3b7..2a7d8e511db 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -1376,4 +1376,28 @@ test_expect_success 'git stash can pop directory -> file saved changes' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'restore untracked files even when we hit conflicts' '\n> +\tgit init restore_untracked_after_conflict &&\n> +\t(\n> +\t\tcd restore_untracked_after_conflict &&\n> +\n> +\t\techo hi >a &&\n> +\t\techo there >b &&\n> +\t\tgit add . &&\n> +\t\tgit commit -m first &&\n> +\t\techo hello >a &&\n> +\t\techo something >c &&\n> +\n> +\t\tgit stash push --include-untracked &&\n> +\n> +\t\techo conflict >a &&\n> +\t\tgit add a &&\n> +\t\tgit commit -m second &&\n> +\n> +\t\ttest_must_fail git stash pop &&\n> +\n> +\t\ttest_path_is_file c\n> +\t)\n> +'\n> +\n>  test_done\n>\n> base-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907\n"}]}