{"thread":{"id":"65949","subject":"[PATCH] merge --abort: don't delete autostash before reset succeeds","startedAt":"2026-07-08T01:51:21Z","lastAt":"2026-07-08T17:46:37Z","messageCount":3,"participants":["Kris Point","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"547423","messageId":"SI1PPF1BAF45F0FA46A6EED57B732BB04D7ABFF2@SI1PPF1BAF45F0F.apcprd02.prod.outlook.com","threadId":"65949","inReplyTo":null,"subject":"[PATCH] merge --abort: don't delete autostash before reset succeeds","fromName":"Kris Point","fromEmail":"krispointcsgo@outlook.com","sentAt":"2026-07-08T01:51:18Z","receivedAt":"2026-07-08T01:51:21Z","isPatch":true,"body":"From bf4b12438a83d81f2c8df6e39f6114ddd5002430 Mon Sep 17 00:00:00 2001\nFrom: KrisPointCSGO <KrisPointCSGO@outlook.com>\nDate: Tue, 7 Jul 2026 20:10:00 +0800\nSubject: [PATCH] merge --abort: don't delete autostash before reset succeeds\nTo: git@vger.kernel.org\nCc: gitster@pobox.com\n\nIn cmd_merge()'s --abort path, MERGE_AUTOSTASH was deleted before\ncmd_reset() was called. If cmd_reset() failed (e.g. due to a locked\nindex), the autostash was permanently lost.\n\nInstead, read the MERGE_AUTOSTASH OID without deleting the ref, run\ncmd_reset() (which itself calls remove_branch_state() ->\nsave_autostash_ref() to persist the stash), and only apply the\nautostash on success.\n\nReported-by: KrisPoint\nSigned-off-by: KrisPoint <KrisPointCSGO@outlook.com>\n---\n builtin/merge.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 5b46a596f0..5d9a242027 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1427,15 +1427,14 @@ int cmd_merge(int argc,\n \t\tif (!file_exists(git_path_merge_head(the_repository)))\n \t\t\tdie(_(\"There is no merge to abort (MERGE_HEAD missing).\"));\n \n-\t\tif (!refs_read_ref(get_main_ref_store(the_repository), \"MERGE_AUTOSTASH\", &stash_oid))\n-\t\t\trefs_delete_ref(get_main_ref_store(the_repository),\n-\t\t\t\t\t\"\", \"MERGE_AUTOSTASH\", &stash_oid,\n-\t\t\t\t\tREF_NO_DEREF);\n+\t\trefs_read_ref(get_main_ref_store(the_repository), \"MERGE_AUTOSTASH\", &stash_oid);\n \n-\t\t/* Invoke 'git reset --merge' */\n+\t\t/* Invoke 'git reset --merge' (which also cleans up merge state,\n+\t\t * including saving the autostash to the stash list).\n+\t\t */\n \t\tret = cmd_reset(nargc, nargv, prefix, the_repository);\n \n-\t\tif (!is_null_oid(&stash_oid)) {\n+\t\tif (!ret && !is_null_oid(&stash_oid)) {\n \t\t\toid_to_hex_r(stash_oid_hex, &stash_oid);\n \t\t\tapply_autostash_oid(stash_oid_hex);\n \t\t}\n-- \n2.53.0\n\nI've already changed the format to plain text. I don't think I did anything wrong."},{"id":"547491","messageId":"0b7e6d74-0287-4be5-a19f-ed8c5fbc9217@gmail.com","threadId":"65949","inReplyTo":"SI1PPF1BAF45F0FA46A6EED57B732BB04D7ABFF2@SI1PPF1BAF45F0F.apcprd02.prod.outlook.com","subject":"Re: [PATCH] merge --abort: don't delete autostash before reset succeeds","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-07-08T13:35:32Z","receivedAt":"2026-07-08T13:35:37Z","isPatch":true,"body":"Hi Kris\n\nOn 08/07/2026 02:51, Kris Point wrote:\n>  From bf4b12438a83d81f2c8df6e39f6114ddd5002430 Mon Sep 17 00:00:00 2001\n> From: KrisPointCSGO <KrisPointCSGO@outlook.com>\n> Date: Tue, 7 Jul 2026 20:10:00 +0800\n> Subject: [PATCH] merge --abort: don't delete autostash before reset succeeds\n> To: git@vger.kernel.org\n> Cc: gitster@pobox.com\n> \n> In cmd_merge()'s --abort path, MERGE_AUTOSTASH was deleted before\n> cmd_reset() was called. If cmd_reset() failed (e.g. due to a locked\n> index), the autostash was permanently lost.\n\nThat's bad\n> Instead, read the MERGE_AUTOSTASH OID without deleting the ref, run\n> cmd_reset() (which itself calls remove_branch_state() ->\n> save_autostash_ref() to persist the stash), and only apply the\n> autostash on success.\n\nI'm afraid I don't think this is the right solution. We only want to \nsave the stash if there are conflicts when we apply it - that is why \nMERGE_AUTOSTASH is deleted before we do the reset - we want to prevent \nremove_branch_state() from saving it. If the stash applies cleanly then \nwe should not save it. If the reset fails then we should keep \nMERGE_AUTOSTASH along with the other merge state files rather than \nsaving the stash (which is actually what happens after this patch \nbecause cmd_reset() dies before it calls remove_branch_state()).\n\nI think the solution is probably to stop calling \nbuiltin/reset.c:cmd_reset() and instead extend \nreset.c:reset_working_tree()[1] to do a \"merge\" reset by adding a \n\"RESET_WORKING_TREE_MERGE\" flag (or possibly we want to remove \nRESET_WORKTING_TREE_HARD from the flags and add a reset_mode member). \nThen we can call\n\n\tstruct reset_working_tree opts = {\n\t\t.flags = RESET_WORKING_TREE_MERGE;\n\t};\n\tif (reset_working_tree(the_repository, &opts))\n\t\tdie(_(\"could not reset index and working tree\")); \napply_autostash_ref(...); /* apply the stash */ \nremove_branch_state(...); /* remove merge state */\n\nSo we only delete MERGE_AUTOSTASH after a successful reset and we only \nsave the stash if it applies with conflicts. That's all a bit more \ninvolved than the patch here - please do give me a shout if you want \nsome more information.\n\nThanks\n\nPhillip\n\n[1] Note that in the master branch this function is called reset_head(),\n     you should base the fix on top of the \"ps/history-drop\" branch which\n     is in \"seen\" (currently the tip is d11b348f784 (builtin/history:\n     implement \"drop\" subcommand, 2026-07-01) but that might change when\n     Junio rebuilds \"seen\".\n\n\n> Reported-by: KrisPoint\n> Signed-off-by: KrisPoint <KrisPointCSGO@outlook.com>\n> ---\n>   builtin/merge.c | 11 +++++------\n>   1 file changed, 5 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 5b46a596f0..5d9a242027 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -1427,15 +1427,14 @@ int cmd_merge(int argc,\n>   \t\tif (!file_exists(git_path_merge_head(the_repository)))\n>   \t\t\tdie(_(\"There is no merge to abort (MERGE_HEAD missing).\"));\n>   \n> -\t\tif (!refs_read_ref(get_main_ref_store(the_repository), \"MERGE_AUTOSTASH\", &stash_oid))\n> -\t\t\trefs_delete_ref(get_main_ref_store(the_repository),\n> -\t\t\t\t\t\"\", \"MERGE_AUTOSTASH\", &stash_oid,\n> -\t\t\t\t\tREF_NO_DEREF);\n> +\t\trefs_read_ref(get_main_ref_store(the_repository), \"MERGE_AUTOSTASH\", &stash_oid);\n>   \n> -\t\t/* Invoke 'git reset --merge' */\n> +\t\t/* Invoke 'git reset --merge' (which also cleans up merge state,\n> +\t\t * including saving the autostash to the stash list).\n> +\t\t */\n>   \t\tret = cmd_reset(nargc, nargv, prefix, the_repository);\n>   \n> -\t\tif (!is_null_oid(&stash_oid)) {\n> +\t\tif (!ret && !is_null_oid(&stash_oid)) {\n>   \t\t\toid_to_hex_r(stash_oid_hex, &stash_oid);\n>   \t\t\tapply_autostash_oid(stash_oid_hex);\n>   \t\t}\n\n"},{"id":"547516","messageId":"xmqq8q7lv0g5.fsf@gitster.g","threadId":"65949","inReplyTo":"0b7e6d74-0287-4be5-a19f-ed8c5fbc9217@gmail.com","subject":"Re: [PATCH] merge --abort: don't delete autostash before reset succeeds","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-08T17:46:34Z","receivedAt":"2026-07-08T17:46:37Z","isPatch":true,"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> I'm afraid I don't think this is the right solution. We only want to \n> save the stash if there are conflicts when we apply it - that is why \n> MERGE_AUTOSTASH is deleted before we do the reset - we want to prevent \n> remove_branch_state() from saving it. If the stash applies cleanly then \n> we should not save it. If the reset fails then we should keep \n> MERGE_AUTOSTASH along with the other merge state files rather than \n> saving the stash (which is actually what happens after this patch \n> because cmd_reset() dies before it calls remove_branch_state()).\n\nThanks for pointing it out that reset calls remove_branch_state(),\nwhich in turn calls remove_merge_branch_state(), which in turn calls\nsave_autostash_ref().  We end up (when cmd_reset() is successful)\napplying the autostash (which is good) but also saving a new stash.\n\n> I think the solution is probably to stop calling \n> builtin/reset.c:cmd_reset() and instead ...\n\nGreat.  In general, it is a bad pattern we should find and fix for\ncmd_A() to call cmd_B() in its implementation as a subroutine.  To\nclean any such instance is a great thing to do.\n\n> ...\n> So we only delete MERGE_AUTOSTASH after a successful reset and we only \n> save the stash if it applies with conflicts. That's all a bit more \n> involved than the patch here - please do give me a shout if you want \n> some more information.\n>\n> Thanks\n>\n> Phillip\n\nThanks.\n"}]}