{"thread":{"id":"61948","subject":"[PATCH] rebase: apply and cleanup autostash when rebase fails to start","startedAt":"2024-08-14T13:22:31Z","lastAt":"2024-09-12T20:44:43Z","messageCount":10,"participants":["Phillip Wood via GitGitGadget","Patrick Steinhardt","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"500892","messageId":"pull.1772.git.1723641747309.gitgitgadget@gmail.com","threadId":"61948","inReplyTo":null,"subject":"[PATCH] rebase: apply and cleanup autostash when rebase fails to start","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-08-14T13:22:27Z","receivedAt":"2024-08-14T13:22:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIf \"git rebase\" fails to start after stashing the user's uncommitted\nchanges then it forgets to restore the stashed changes and remove state\ndirectory. To make matters worse running \"git rebase --abort\" to apply\nthe stashed changes and cleanup the state directory fails because the\nstate directory only contains the \"autostash\" file and is missing the\n\"head-name\" and \"onto\" required by read_basic_state().\n\nFix this by applying the autostash and removing the state directory if\nthe pre-rebase hook or initial checkout fail. New tests are added to\ncheck the autostash is applied and the state directory is removed if the\nrebase fails to start. Checks are also added to some existing tests in\norder to ensure there is no state directory left behind when a rebase\nfails to start and no autostash has been created.\n\nReported-by: Brian Lyles <brianmlyles@gmail.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    rebase: apply and cleanup autostash when rebase fails to start\n    \n    Thanks to Brian for reporting this. This patch is based on maint.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1772%2Fphillipwood%2Frebase-fix-autostash-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1772/phillipwood/rebase-fix-autostash-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1772\n\n builtin/rebase.c            | 39 +++++++++++++++++++++++++++++-------\n t/t3400-rebase.sh           |  8 ++++++--\n t/t3413-rebase-hook.sh      |  4 +++-\n t/t3420-rebase-autostash.sh | 40 +++++++++++++++++++++++++++++++++++++\n 4 files changed, 81 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex e3a8e74cfc2..ac23c4ddbb0 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -526,6 +526,23 @@ static int rebase_write_basic_state(struct rebase_options *opts)\n \treturn 0;\n }\n \n+static int cleanup_autostash(struct rebase_options *opts)\n+{\n+\tint ret;\n+\tstruct strbuf dir = STRBUF_INIT;\n+\tconst char *path = state_dir_path(\"autostash\", opts);\n+\n+\tif (!file_exists(path))\n+\t\treturn 0;\n+\tret = apply_autostash(path);\n+\tstrbuf_addstr(&dir, opts->state_dir);\n+\tif (remove_dir_recursively(&dir, 0))\n+\t\tret = error_errno(_(\"could not remove '%s'\"), opts->state_dir);\n+\tstrbuf_release(&dir);\n+\n+\treturn ret;\n+}\n+\n static int finish_rebase(struct rebase_options *opts)\n {\n \tstruct strbuf dir = STRBUF_INIT;\n@@ -1726,7 +1743,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (require_clean_work_tree(the_repository, \"rebase\",\n \t\t\t\t    _(\"Please commit or stash them.\"), 1, 1)) {\n \t\tret = -1;\n-\t\tgoto cleanup;\n+\t\tgoto cleanup_autostash;\n \t}\n \n \t/*\n@@ -1749,7 +1766,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tif (options.switch_to) {\n \t\t\t\tret = checkout_up_to_date(&options);\n \t\t\t\tif (ret)\n-\t\t\t\t\tgoto cleanup;\n+\t\t\t\t\tgoto cleanup_autostash;\n \t\t\t}\n \n \t\t\tif (!(options.flags & REBASE_NO_QUIET))\n@@ -1775,8 +1792,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t/* If a hook exists, give it a chance to interrupt*/\n \tif (!ok_to_skip_pre_rebase &&\n \t    run_hooks_l(\"pre-rebase\", options.upstream_arg,\n-\t\t\targc ? argv[0] : NULL, NULL))\n-\t\tdie(_(\"The pre-rebase hook refused to rebase.\"));\n+\t\t\targc ? argv[0] : NULL, NULL)) {\n+\t\tret = error(_(\"The pre-rebase hook refused to rebase.\"));\n+\t\tgoto cleanup_autostash;\n+\t}\n \n \tif (options.flags & REBASE_DIFFSTAT) {\n \t\tstruct diff_options opts;\n@@ -1821,9 +1840,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tRESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n \tropts.head_msg = msg.buf;\n \tropts.default_reflog_action = options.reflog_action;\n-\tif (reset_head(the_repository, &ropts))\n-\t\tdie(_(\"Could not detach HEAD\"));\n-\tstrbuf_release(&msg);\n+\tif (reset_head(the_repository, &ropts)) {\n+\t\tret = error(_(\"Could not detach HEAD\"));\n+\t\tgoto cleanup_autostash;\n+\t}\n \n \t/*\n \t * If the onto is a proper descendant of the tip of the branch, then\n@@ -1851,9 +1871,14 @@ run_rebase:\n \n cleanup:\n \tstrbuf_release(&buf);\n+\tstrbuf_release(&msg);\n \tstrbuf_release(&revisions);\n \trebase_options_release(&options);\n \tfree(squash_onto_name);\n \tfree(keep_base_onto_name);\n \treturn !!ret;\n+\n+cleanup_autostash:\n+\tret |= !!cleanup_autostash(&options);\n+\tgoto cleanup;\n }\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex ae34bfad607..972ea85febc 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -145,7 +145,9 @@ test_expect_success 'Show verbose error when HEAD could not be detached' '\n \ttest_when_finished \"rm -f B\" &&\n \ttest_must_fail git rebase topic 2>output.err >output.out &&\n \ttest_grep \"The following untracked working tree files would be overwritten by checkout:\" output.err &&\n-\ttest_grep B output.err\n+\ttest_grep B output.err &&\n+\ttest_must_fail git rebase --quit 2>err &&\n+\ttest_grep \"no rebase in progress\" err\n '\n \n test_expect_success 'fail when upstream arg is missing and not on branch' '\n@@ -422,7 +424,9 @@ test_expect_success 'refuse to switch to branch checked out elsewhere' '\n \tgit checkout main &&\n \tgit worktree add wt &&\n \ttest_must_fail git -C wt rebase main main 2>err &&\n-\ttest_grep \"already used by worktree at\" err\n+\ttest_grep \"already used by worktree at\" err &&\n+\ttest_must_fail git -C wt rebase --quit 2>err &&\n+\ttest_grep \"no rebase in progress\" err\n '\n \n test_expect_success 'rebase when inside worktree subdirectory' '\ndiff --git a/t/t3413-rebase-hook.sh b/t/t3413-rebase-hook.sh\nindex e8456831e8b..426ff098e1d 100755\n--- a/t/t3413-rebase-hook.sh\n+++ b/t/t3413-rebase-hook.sh\n@@ -110,7 +110,9 @@ test_expect_success 'pre-rebase hook stops rebase (1)' '\n \tgit reset --hard side &&\n \ttest_must_fail git rebase main &&\n \ttest \"z$(git symbolic-ref HEAD)\" = zrefs/heads/test &&\n-\ttest 0 = $(git rev-list HEAD...side | wc -l)\n+\ttest 0 = $(git rev-list HEAD...side | wc -l) &&\n+\ttest_must_fail git rebase --quit 2>err &&\n+\ttest_grep \"no rebase in progress\" err\n '\n \n test_expect_success 'pre-rebase hook stops rebase (2)' '\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex 63e400b89f2..b43046b3b0d 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -82,6 +82,46 @@ testrebase () {\n \ttype=$1\n \tdotest=$2\n \n+\ttest_expect_success \"rebase$type: restore autostash when pre-rebase hook fails\" '\n+\t\tgit checkout -f feature-branch &&\n+\t\ttest_hook pre-rebase <<-\\EOF &&\n+\t\texit 1\n+\t\tEOF\n+\n+\t\techo changed >file0 &&\n+\t\ttest_must_fail git rebase $type --autostash -f HEAD^ &&\n+\t\ttest_must_fail git rebase --quit 2>err &&\n+\t\ttest_grep \"no rebase in progress\" err &&\n+\t\techo changed >expect &&\n+\t\ttest_cmp expect file0\n+\t'\n+\n+\ttest_expect_success \"rebase$type: restore autostash when checkout onto fails\" '\n+\t\tgit checkout -f --detach feature-branch &&\n+\t\techo uncommitted-content >file0 &&\n+\t\techo untracked >file4 &&\n+\t\ttest_when_finished \"rm file4\" &&\n+\t\ttest_must_fail git rebase $type --autostash \\\n+\t\t\t\t\t\t\tunrelated-onto-branch &&\n+\t\ttest_must_fail git rebase --quit 2>err &&\n+\t\ttest_grep \"no rebase in progress\" err &&\n+\t\techo uncommitted-content >expect &&\n+\t\ttest_cmp expect file0\n+\t'\n+\n+\ttest_expect_success \"rebase$type: restore autostash when branch checkout fails\" '\n+\t\tgit checkout -f unrelated-onto-branch^ &&\n+\t\techo uncommitted-content >file0 &&\n+\t\techo untracked >file4 &&\n+\t\ttest_when_finished \"rm file4\" &&\n+\t\ttest_must_fail git rebase $type --autostash HEAD \\\n+\t\t\t\t\t\t\tunrelated-onto-branch &&\n+\t\ttest_must_fail git rebase --quit 2>err &&\n+\t\ttest_grep \"no rebase in progress\" err &&\n+\t\techo uncommitted-content >expect &&\n+\t\ttest_cmp expect file0\n+\t'\n+\n \ttest_expect_success \"rebase$type: dirty worktree, --no-autostash\" '\n \t\ttest_config rebase.autostash true &&\n \t\tgit reset --hard &&\n\nbase-commit: 39bf06adf96da25b87c9aa7d35a32ef3683eb4a4\n-- \ngitgitgadget\n"},{"id":"500913","messageId":"ZrzA0yp45w9NuTp2@tanuki","threadId":"61948","inReplyTo":"pull.1772.git.1723641747309.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase: apply and cleanup autostash when rebase fails to start","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-08-14T14:36:03Z","receivedAt":"2024-08-14T14:36:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 14, 2024 at 01:22:27PM +0000, Phillip Wood via GitGitGadget wrote:\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> \n> If \"git rebase\" fails to start after stashing the user's uncommitted\n> changes then it forgets to restore the stashed changes and remove state\n\ns/remove/& the/\n\n> directory. To make matters worse running \"git rebase --abort\" to apply\n\ns/worse/&,/\n\n> the stashed changes and cleanup the state directory fails because the\n\ns/cleanup/& of/\n\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index e3a8e74cfc2..ac23c4ddbb0 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -526,6 +526,23 @@ static int rebase_write_basic_state(struct rebase_options *opts)\n>  \treturn 0;\n>  }\n>  \n> +static int cleanup_autostash(struct rebase_options *opts)\n> +{\n> +\tint ret;\n> +\tstruct strbuf dir = STRBUF_INIT;\n> +\tconst char *path = state_dir_path(\"autostash\", opts);\n> +\n> +\tif (!file_exists(path))\n> +\t\treturn 0;\n> +\tret = apply_autostash(path);\n> +\tstrbuf_addstr(&dir, opts->state_dir);\n> +\tif (remove_dir_recursively(&dir, 0))\n> +\t\tret = error_errno(_(\"could not remove '%s'\"), opts->state_dir);\n\nThis seems somewhat dangerous to me though. Should we really delete the\nautostash directory in case applying the autostash has failed?\n\n> @@ -1851,9 +1871,14 @@ run_rebase:\n>  \n>  cleanup:\n>  \tstrbuf_release(&buf);\n> +\tstrbuf_release(&msg);\n>  \tstrbuf_release(&revisions);\n>  \trebase_options_release(&options);\n>  \tfree(squash_onto_name);\n>  \tfree(keep_base_onto_name);\n>  \treturn !!ret;\n> +\n> +cleanup_autostash:\n> +\tret |= !!cleanup_autostash(&options);\n> +\tgoto cleanup;\n>  }\n\nIt's somewhat curious of a code flow to jump backwards. It might be\neasier to reason about the flow if we kept track of a variable\n`autostash_needs_cleanup` that we set such that all jumps can go to the\n`cleanup` label instead.\n\nPatrick\n"},{"id":"500916","messageId":"b676bd17-1cc8-4639-acb7-675dde32a1ae@gmail.com","threadId":"61948","inReplyTo":"ZrzA0yp45w9NuTp2@tanuki","subject":"Re: [PATCH] rebase: apply and cleanup autostash when rebase fails to start","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-08-14T15:59:32Z","receivedAt":"2024-08-14T15:59:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Patrick\n\nOn 14/08/2024 15:36, Patrick Steinhardt wrote:\n> On Wed, Aug 14, 2024 at 01:22:27PM +0000, Phillip Wood via GitGitGadget wrote:\n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> If \"git rebase\" fails to start after stashing the user's uncommitted\n>> changes then it forgets to restore the stashed changes and remove state\n> \n> s/remove/& the/\n> \n>> directory. To make matters worse running \"git rebase --abort\" to apply\n> \n> s/worse/&,/\n> \n>> the stashed changes and cleanup the state directory fails because the\n> \n> s/cleanup/& of/\n\nThanks for catching those typos\n\n>> diff --git a/builtin/rebase.c b/builtin/rebase.c\n>> index e3a8e74cfc2..ac23c4ddbb0 100644\n>> --- a/builtin/rebase.c\n>> +++ b/builtin/rebase.c\n>> @@ -526,6 +526,23 @@ static int rebase_write_basic_state(struct rebase_options *opts)\n>>   \treturn 0;\n>>   }\n>>   \n>> +static int cleanup_autostash(struct rebase_options *opts)\n>> +{\n>> +\tint ret;\n>> +\tstruct strbuf dir = STRBUF_INIT;\n>> +\tconst char *path = state_dir_path(\"autostash\", opts);\n>> +\n>> +\tif (!file_exists(path))\n>> +\t\treturn 0;\n>> +\tret = apply_autostash(path);\n>> +\tstrbuf_addstr(&dir, opts->state_dir);\n>> +\tif (remove_dir_recursively(&dir, 0))\n>> +\t\tret = error_errno(_(\"could not remove '%s'\"), opts->state_dir);\n> \n> This seems somewhat dangerous to me though. Should we really delete the\n> autostash directory in case applying the autostash has failed?\n\nApplying the stash should not fail because the rebase has not started \nand so HEAD, the index and the worktree are unchanged since the stash \nwas created. If it does fail for some reason then apply_autostash() \ncreates a new entry under refs/stash. We definitely do want to remove \nthe directory otherwise we're left with the inconsistent state we're \ntying to fix.\n\n>> @@ -1851,9 +1871,14 @@ run_rebase:\n>>   \n>>   cleanup:\n>>   \tstrbuf_release(&buf);\n>> +\tstrbuf_release(&msg);\n>>   \tstrbuf_release(&revisions);\n>>   \trebase_options_release(&options);\n>>   \tfree(squash_onto_name);\n>>   \tfree(keep_base_onto_name);\n>>   \treturn !!ret;\n>> +\n>> +cleanup_autostash:\n>> +\tret |= !!cleanup_autostash(&options);\n>> +\tgoto cleanup;\n>>   }\n> \n> It's somewhat curious of a code flow to jump backwards. It might be\n> easier to reason about the flow if we kept track of a variable\n> `autostash_needs_cleanup` that we set such that all jumps can go to the\n> `cleanup` label instead.\n\nIt is slightly odd, but in the end I decided it was simpler to say \"goto \ncleanup_autostash\" than try and keep track of what needs cleaning up \nwhen saying \"goto cleanup\". There are a couple of similar examples in \nbuiltin/stash.c:show_stash() and \nconfig.c:git_config_set_multivar_in_file_gently()\n\nThanks for the review\n\nPhillip\n\n\n> Patrick\n> \n"},{"id":"500924","messageId":"xmqq8qwzypz2.fsf@gitster.g","threadId":"61948","inReplyTo":"b676bd17-1cc8-4639-acb7-675dde32a1ae@gmail.com","subject":"Re: [PATCH] rebase: apply and cleanup autostash when rebase fails to start","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-14T17:27:29Z","receivedAt":"2024-08-14T17:27:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> Applying the stash should not fail because the rebase has not started\n> and so HEAD, the index and the worktree are unchanged since the stash\n> was created. If it does fail for some reason then apply_autostash()\n> creates a new entry under refs/stash. We definitely do want to remove\n> the directory otherwise we're left with the inconsistent state we're\n> tying to fix.\n\nIf it is not expected to fail 99% of times, it feels more prudent to\nabort loudly without making further damage to lose information and\nask the user to check what happened in the working tree, rather than\nblindly removing the clue to understand what went wrong.  For\nexample, could the reason why applying the stash failed be because\nthe user forgot that the working tree was being used for rebasing\nand mucked with its contents from say another terminal?\n\nThanks.\n"},{"id":"500963","messageId":"0ca8ce79-6c9f-4836-99d6-b2a86dfe4109@gmail.com","threadId":"61948","inReplyTo":"xmqq8qwzypz2.fsf@gitster.g","subject":"Re: [PATCH] rebase: apply and cleanup autostash when rebase fails to start","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-08-15T09:47:59Z","receivedAt":"2024-08-15T09:48:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 14/08/2024 18:27, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>> Applying the stash should not fail because the rebase has not started\n>> and so HEAD, the index and the worktree are unchanged since the stash\n>> was created. If it does fail for some reason then apply_autostash()\n>> creates a new entry under refs/stash. We definitely do want to remove\n>> the directory otherwise we're left with the inconsistent state we're\n>> tying to fix.\n> \n> If it is not expected to fail 99% of times, it feels more prudent to\n> abort loudly without making further damage to lose information and\n> ask the user to check what happened in the working tree, rather than\n> blindly removing the clue to understand what went wrong.  For\n> example, could the reason why applying the stash failed be because\n> the user forgot that the working tree was being used for rebasing\n> and mucked with its contents from say another terminal?\n\nIf the working tree has changed then the stash will still apply but \npossibly with conflicts - the same thing can happen when the branch has \nbeen successfully rebased. If there are conflicts then the stash is \nsaved in refs/stash as well. This code is just doing what we do at the \nend of a successful rebase so I'm don't really understand what the issue \nis. Looking at finish_rebase() we don't even check the return value of \napply_autostash() when applying the stash at the end of a successful rebase.\n\nBest Wishes\n\nPhillip\n\n> Thanks.\n> \n"},{"id":"501990","messageId":"pull.1772.v2.git.1725289979450.gitgitgadget@gmail.com","threadId":"61948","inReplyTo":"pull.1772.git.1723641747309.gitgitgadget@gmail.com","subject":"[PATCH v2] rebase: apply and cleanup autostash when rebase fails to start","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-09-02T15:12:59Z","receivedAt":"2024-09-02T15:13:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIf \"git rebase\" fails to start after stashing the user's uncommitted\nchanges then it forgets to restore the stashed changes and remove the\nstate directory. To make matters worse, running \"git rebase --abort\" to\napply the stashed changes and cleanup the state directory fails because\nthe state directory only contains the \"autostash\" file and is missing\nthe \"head-name\" and \"onto\" files required by read_basic_state().\n\nFix this by applying the autostash and removing the state directory if\nthe pre-rebase hook or initial checkout fail. This matches what\nfinish_rebase() does at the end of a successful rebase. If the user\nmodifies any files after the autostash is created it is possible there\nwill be conflicts when the autostash is applied. In that case\napply_autostash() saves the stash in a new entry under refs/stash and so\nit is safe to remove the state directory containing the autostash file.\n\nNew tests are added to check the autostash is applied and the state\ndirectory is removed if the rebase fails to start. Checks are also added\nto some existing tests in order to ensure there is no state directory\nleft behind when a rebase fails to start and no autostash has been\ncreated.\n\nReported-by: Brian Lyles <brianmlyles@gmail.com>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n    rebase: apply and cleanup autostash when rebase fails to start\n    \n    Thanks to Junio and Patrick for their comments on V1. I've updated the\n    commit message to correct the typos found by Patrick and added an\n    explanation of why it is safe to remove the state directory.\n    \n    In his review Patrick suggested removing the backwards jump after\n    applying the autostash and cleaning up the state directory in favor of a\n    variable that tracks if we need to apply the autostash. I've decided not\n    to do that as I think having to set a variable before jumping to a\n    cleanup label is more complicated and error prone. There are similar\n    backward jumps in builtin/stash.c:show_stash() and\n    config.c:git_config_set_multivar_in_file_gently()\n    \n    Thanks to Brian for reporting this. This patch is based on maint, when\n    merging with master there is an easily resolved conflict with a change\n    from 'ps/config-wo-the-repository' which modifies an adjacent line.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1772%2Fphillipwood%2Frebase-fix-autostash-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1772/phillipwood/rebase-fix-autostash-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1772\n\nRange-diff vs v1:\n\n 1:  f5b46082047 ! 1:  c76c05092aa rebase: apply and cleanup autostash when rebase fails to start\n     @@ Commit message\n          rebase: apply and cleanup autostash when rebase fails to start\n      \n          If \"git rebase\" fails to start after stashing the user's uncommitted\n     -    changes then it forgets to restore the stashed changes and remove state\n     -    directory. To make matters worse running \"git rebase --abort\" to apply\n     -    the stashed changes and cleanup the state directory fails because the\n     -    state directory only contains the \"autostash\" file and is missing the\n     -    \"head-name\" and \"onto\" required by read_basic_state().\n     +    changes then it forgets to restore the stashed changes and remove the\n     +    state directory. To make matters worse, running \"git rebase --abort\" to\n     +    apply the stashed changes and cleanup the state directory fails because\n     +    the state directory only contains the \"autostash\" file and is missing\n     +    the \"head-name\" and \"onto\" files required by read_basic_state().\n      \n          Fix this by applying the autostash and removing the state directory if\n     -    the pre-rebase hook or initial checkout fail. New tests are added to\n     -    check the autostash is applied and the state directory is removed if the\n     -    rebase fails to start. Checks are also added to some existing tests in\n     -    order to ensure there is no state directory left behind when a rebase\n     -    fails to start and no autostash has been created.\n     +    the pre-rebase hook or initial checkout fail. This matches what\n     +    finish_rebase() does at the end of a successful rebase. If the user\n     +    modifies any files after the autostash is created it is possible there\n     +    will be conflicts when the autostash is applied. In that case\n     +    apply_autostash() saves the stash in a new entry under refs/stash and so\n     +    it is safe to remove the state directory containing the autostash file.\n     +\n     +    New tests are added to check the autostash is applied and the state\n     +    directory is removed if the rebase fails to start. Checks are also added\n     +    to some existing tests in order to ensure there is no state directory\n     +    left behind when a rebase fails to start and no autostash has been\n     +    created.\n      \n          Reported-by: Brian Lyles <brianmlyles@gmail.com>\n          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n\n\n builtin/rebase.c            | 39 +++++++++++++++++++++++++++++-------\n t/t3400-rebase.sh           |  8 ++++++--\n t/t3413-rebase-hook.sh      |  4 +++-\n t/t3420-rebase-autostash.sh | 40 +++++++++++++++++++++++++++++++++++++\n 4 files changed, 81 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex e3a8e74cfc2..ac23c4ddbb0 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -526,6 +526,23 @@ static int rebase_write_basic_state(struct rebase_options *opts)\n \treturn 0;\n }\n \n+static int cleanup_autostash(struct rebase_options *opts)\n+{\n+\tint ret;\n+\tstruct strbuf dir = STRBUF_INIT;\n+\tconst char *path = state_dir_path(\"autostash\", opts);\n+\n+\tif (!file_exists(path))\n+\t\treturn 0;\n+\tret = apply_autostash(path);\n+\tstrbuf_addstr(&dir, opts->state_dir);\n+\tif (remove_dir_recursively(&dir, 0))\n+\t\tret = error_errno(_(\"could not remove '%s'\"), opts->state_dir);\n+\tstrbuf_release(&dir);\n+\n+\treturn ret;\n+}\n+\n static int finish_rebase(struct rebase_options *opts)\n {\n \tstruct strbuf dir = STRBUF_INIT;\n@@ -1726,7 +1743,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (require_clean_work_tree(the_repository, \"rebase\",\n \t\t\t\t    _(\"Please commit or stash them.\"), 1, 1)) {\n \t\tret = -1;\n-\t\tgoto cleanup;\n+\t\tgoto cleanup_autostash;\n \t}\n \n \t/*\n@@ -1749,7 +1766,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tif (options.switch_to) {\n \t\t\t\tret = checkout_up_to_date(&options);\n \t\t\t\tif (ret)\n-\t\t\t\t\tgoto cleanup;\n+\t\t\t\t\tgoto cleanup_autostash;\n \t\t\t}\n \n \t\t\tif (!(options.flags & REBASE_NO_QUIET))\n@@ -1775,8 +1792,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t/* If a hook exists, give it a chance to interrupt*/\n \tif (!ok_to_skip_pre_rebase &&\n \t    run_hooks_l(\"pre-rebase\", options.upstream_arg,\n-\t\t\targc ? argv[0] : NULL, NULL))\n-\t\tdie(_(\"The pre-rebase hook refused to rebase.\"));\n+\t\t\targc ? argv[0] : NULL, NULL)) {\n+\t\tret = error(_(\"The pre-rebase hook refused to rebase.\"));\n+\t\tgoto cleanup_autostash;\n+\t}\n \n \tif (options.flags & REBASE_DIFFSTAT) {\n \t\tstruct diff_options opts;\n@@ -1821,9 +1840,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tRESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n \tropts.head_msg = msg.buf;\n \tropts.default_reflog_action = options.reflog_action;\n-\tif (reset_head(the_repository, &ropts))\n-\t\tdie(_(\"Could not detach HEAD\"));\n-\tstrbuf_release(&msg);\n+\tif (reset_head(the_repository, &ropts)) {\n+\t\tret = error(_(\"Could not detach HEAD\"));\n+\t\tgoto cleanup_autostash;\n+\t}\n \n \t/*\n \t * If the onto is a proper descendant of the tip of the branch, then\n@@ -1851,9 +1871,14 @@ run_rebase:\n \n cleanup:\n \tstrbuf_release(&buf);\n+\tstrbuf_release(&msg);\n \tstrbuf_release(&revisions);\n \trebase_options_release(&options);\n \tfree(squash_onto_name);\n \tfree(keep_base_onto_name);\n \treturn !!ret;\n+\n+cleanup_autostash:\n+\tret |= !!cleanup_autostash(&options);\n+\tgoto cleanup;\n }\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex ae34bfad607..972ea85febc 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -145,7 +145,9 @@ test_expect_success 'Show verbose error when HEAD could not be detached' '\n \ttest_when_finished \"rm -f B\" &&\n \ttest_must_fail git rebase topic 2>output.err >output.out &&\n \ttest_grep \"The following untracked working tree files would be overwritten by checkout:\" output.err &&\n-\ttest_grep B output.err\n+\ttest_grep B output.err &&\n+\ttest_must_fail git rebase --quit 2>err &&\n+\ttest_grep \"no rebase in progress\" err\n '\n \n test_expect_success 'fail when upstream arg is missing and not on branch' '\n@@ -422,7 +424,9 @@ test_expect_success 'refuse to switch to branch checked out elsewhere' '\n \tgit checkout main &&\n \tgit worktree add wt &&\n \ttest_must_fail git -C wt rebase main main 2>err &&\n-\ttest_grep \"already used by worktree at\" err\n+\ttest_grep \"already used by worktree at\" err &&\n+\ttest_must_fail git -C wt rebase --quit 2>err &&\n+\ttest_grep \"no rebase in progress\" err\n '\n \n test_expect_success 'rebase when inside worktree subdirectory' '\ndiff --git a/t/t3413-rebase-hook.sh b/t/t3413-rebase-hook.sh\nindex e8456831e8b..426ff098e1d 100755\n--- a/t/t3413-rebase-hook.sh\n+++ b/t/t3413-rebase-hook.sh\n@@ -110,7 +110,9 @@ test_expect_success 'pre-rebase hook stops rebase (1)' '\n \tgit reset --hard side &&\n \ttest_must_fail git rebase main &&\n \ttest \"z$(git symbolic-ref HEAD)\" = zrefs/heads/test &&\n-\ttest 0 = $(git rev-list HEAD...side | wc -l)\n+\ttest 0 = $(git rev-list HEAD...side | wc -l) &&\n+\ttest_must_fail git rebase --quit 2>err &&\n+\ttest_grep \"no rebase in progress\" err\n '\n \n test_expect_success 'pre-rebase hook stops rebase (2)' '\ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex 63e400b89f2..b43046b3b0d 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -82,6 +82,46 @@ testrebase () {\n \ttype=$1\n \tdotest=$2\n \n+\ttest_expect_success \"rebase$type: restore autostash when pre-rebase hook fails\" '\n+\t\tgit checkout -f feature-branch &&\n+\t\ttest_hook pre-rebase <<-\\EOF &&\n+\t\texit 1\n+\t\tEOF\n+\n+\t\techo changed >file0 &&\n+\t\ttest_must_fail git rebase $type --autostash -f HEAD^ &&\n+\t\ttest_must_fail git rebase --quit 2>err &&\n+\t\ttest_grep \"no rebase in progress\" err &&\n+\t\techo changed >expect &&\n+\t\ttest_cmp expect file0\n+\t'\n+\n+\ttest_expect_success \"rebase$type: restore autostash when checkout onto fails\" '\n+\t\tgit checkout -f --detach feature-branch &&\n+\t\techo uncommitted-content >file0 &&\n+\t\techo untracked >file4 &&\n+\t\ttest_when_finished \"rm file4\" &&\n+\t\ttest_must_fail git rebase $type --autostash \\\n+\t\t\t\t\t\t\tunrelated-onto-branch &&\n+\t\ttest_must_fail git rebase --quit 2>err &&\n+\t\ttest_grep \"no rebase in progress\" err &&\n+\t\techo uncommitted-content >expect &&\n+\t\ttest_cmp expect file0\n+\t'\n+\n+\ttest_expect_success \"rebase$type: restore autostash when branch checkout fails\" '\n+\t\tgit checkout -f unrelated-onto-branch^ &&\n+\t\techo uncommitted-content >file0 &&\n+\t\techo untracked >file4 &&\n+\t\ttest_when_finished \"rm file4\" &&\n+\t\ttest_must_fail git rebase $type --autostash HEAD \\\n+\t\t\t\t\t\t\tunrelated-onto-branch &&\n+\t\ttest_must_fail git rebase --quit 2>err &&\n+\t\ttest_grep \"no rebase in progress\" err &&\n+\t\techo uncommitted-content >expect &&\n+\t\ttest_cmp expect file0\n+\t'\n+\n \ttest_expect_success \"rebase$type: dirty worktree, --no-autostash\" '\n \t\ttest_config rebase.autostash true &&\n \t\tgit reset --hard &&\n\nbase-commit: 39bf06adf96da25b87c9aa7d35a32ef3683eb4a4\n-- \ngitgitgadget\n"},{"id":"502414","messageId":"xmqqwmjlvmkl.fsf@gitster.g","threadId":"61948","inReplyTo":"0ca8ce79-6c9f-4836-99d6-b2a86dfe4109@gmail.com","subject":"Re: [PATCH] rebase: apply and cleanup autostash when rebase fails to start","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-08T21:54:02Z","receivedAt":"2024-09-08T21:54:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> ... This code is just doing what we do at\n> the end of a successful rebase so I'm don't really understand what the\n> issue is. Looking at finish_rebase() we don't even check the return\n> value of apply_autostash() when applying the stash at the end of a\n> successful rebase.\n\nAt that point we give control back the user, so if things are left\nin conflicted or any other \"unexpected\" funny state, the user kill\nkeep the both halves.  As long as the user clearly understands why\nthe working tree is in such a funny state, we should be OK (and I\nwould imagine that we are giving messages like \"applying preexisting\nchanges stashed away before rebasing\" or something).\n\nThanks.\n\n\n"},{"id":"502415","messageId":"xmqqv7z5vmk3.fsf@gitster.g","threadId":"61948","inReplyTo":"0ca8ce79-6c9f-4836-99d6-b2a86dfe4109@gmail.com","subject":"Re: [PATCH] rebase: apply and cleanup autostash when rebase fails to start","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-08T21:54:20Z","receivedAt":"2024-09-08T21:54:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> ... This code is just doing what we do at\n> the end of a successful rebase so I'm don't really understand what the\n> issue is. Looking at finish_rebase() we don't even check the return\n> value of apply_autostash() when applying the stash at the end of a\n> successful rebase.\n\nAt that point we give control back the user, so if things are left\nin conflicted or any other \"unexpected\" funny state, the user kill\nkeep the both halves.  As long as the user clearly understands why\nthe working tree is in such a funny state, we should be OK (and I\nwould imagine that we are giving messages like \"applying preexisting\nchanges stashed away before rebasing\" or something).\n\nThanks.\n\n\n"},{"id":"502416","messageId":"xmqqr09tvmi5.fsf@gitster.g","threadId":"61948","inReplyTo":"pull.1772.v2.git.1725289979450.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] rebase: apply and cleanup autostash when rebase fails to start","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-08T21:55:30Z","receivedAt":"2024-09-08T21:55:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>     Thanks to Junio and Patrick for their comments on V1. I've updated the\n>     commit message to correct the typos found by Patrick and added an\n>     explanation of why it is safe to remove the state directory.\n\nAny other comments, or are we all happy with this iteration?\n\nThanks.\n"},{"id":"502717","messageId":"xmqqldzw38li.fsf@gitster.g","threadId":"61948","inReplyTo":"xmqqr09tvmi5.fsf@gitster.g","subject":"Re: [PATCH v2] rebase: apply and cleanup autostash when rebase fails to start","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-12T20:44:41Z","receivedAt":"2024-09-12T20:44:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n>>     Thanks to Junio and Patrick for their comments on V1. I've updated the\n>>     commit message to correct the typos found by Patrick and added an\n>>     explanation of why it is safe to remove the state directory.\n>\n> Any other comments, or are we all happy with this iteration?\n\nLet me mark the topic for 'next'.  Thanks for reporting, fixing and\nreviewing.\n"}]}