[PATCH] merge --abort: don't delete autostash before reset succeeds

[PATCH] merge --abort: don't delete autostash before reset succeeds

[Kris Point]

From bf4b12438a83d81f2c8df6e39f6114ddd5002430 Mon Sep 17 00:00:00 2001
From: KrisPointCSGO <KrisPointCSGO@outlook.com>
Date: Tue, 7 Jul 2026 20:10:00 +0800
Subject: [PATCH] merge --abort: don't delete autostash before reset succeeds
To: git@vger.kernel.org
Cc: gitster@pobox.com

In cmd_merge()'s --abort path, MERGE_AUTOSTASH was deleted before
cmd_reset() was called. If cmd_reset() failed (e.g. due to a locked
index), the autostash was permanently lost.

Instead, read the MERGE_AUTOSTASH OID without deleting the ref, run
cmd_reset() (which itself calls remove_branch_state() ->
save_autostash_ref() to persist the stash), and only apply the
autostash on success.

Reported-by: KrisPoint
Signed-off-by: KrisPoint <KrisPointCSGO@outlook.com>
---
 builtin/merge.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 5b46a596f0..5d9a242027 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1427,15 +1427,14 @@ int cmd_merge(int argc,
 		if (!file_exists(git_path_merge_head(the_repository)))
 			die(_("There is no merge to abort (MERGE_HEAD missing)."));
 
-		if (!refs_read_ref(get_main_ref_store(the_repository), "MERGE_AUTOSTASH", &stash_oid))
-			refs_delete_ref(get_main_ref_store(the_repository),
-					"", "MERGE_AUTOSTASH", &stash_oid,
-					REF_NO_DEREF);
+		refs_read_ref(get_main_ref_store(the_repository), "MERGE_AUTOSTASH", &stash_oid);
 
-		/* Invoke 'git reset --merge' */
+		/* Invoke 'git reset --merge' (which also cleans up merge state,
+		 * including saving the autostash to the stash list).
+		 */
 		ret = cmd_reset(nargc, nargv, prefix, the_repository);
 
-		if (!is_null_oid(&stash_oid)) {
+		if (!ret && !is_null_oid(&stash_oid)) {
 			oid_to_hex_r(stash_oid_hex, &stash_oid);
 			apply_autostash_oid(stash_oid_hex);
 		}
-- 
2.53.0

I've already changed the format to plain text. I don't think I did anything wrong.

Re: [PATCH] merge --abort: don't delete autostash before reset succeeds

[Phillip Wood]

Hi Kris

On 08/07/2026 02:51, Kris Point wrote:
>  From bf4b12438a83d81f2c8df6e39f6114ddd5002430 Mon Sep 17 00:00:00 2001
> From: KrisPointCSGO <KrisPointCSGO@outlook.com>
> Date: Tue, 7 Jul 2026 20:10:00 +0800
> Subject: [PATCH] merge --abort: don't delete autostash before reset succeeds
> To: git@vger.kernel.org
> Cc: gitster@pobox.com
> 
> In cmd_merge()'s --abort path, MERGE_AUTOSTASH was deleted before
> cmd_reset() was called. If cmd_reset() failed (e.g. due to a locked
> index), the autostash was permanently lost.

That's bad
> Instead, read the MERGE_AUTOSTASH OID without deleting the ref, run
> cmd_reset() (which itself calls remove_branch_state() ->
> save_autostash_ref() to persist the stash), and only apply the
> autostash on success.

I'm afraid I don't think this is the right solution. We only want to 
save the stash if there are conflicts when we apply it - that is why 
MERGE_AUTOSTASH is deleted before we do the reset - we want to prevent 
remove_branch_state() from saving it. If the stash applies cleanly then 
we should not save it. If the reset fails then we should keep 
MERGE_AUTOSTASH along with the other merge state files rather than 
saving the stash (which is actually what happens after this patch 
because cmd_reset() dies before it calls remove_branch_state()).

I think the solution is probably to stop calling 
builtin/reset.c:cmd_reset() and instead extend 
reset.c:reset_working_tree()[1] to do a "merge" reset by adding a 
"RESET_WORKING_TREE_MERGE" flag (or possibly we want to remove 
RESET_WORKTING_TREE_HARD from the flags and add a reset_mode member). 
Then we can call

	struct reset_working_tree opts = {
		.flags = RESET_WORKING_TREE_MERGE;
	};
	if (reset_working_tree(the_repository, &opts))
		die(_("could not reset index and working tree")); 
apply_autostash_ref(...); /* apply the stash */ 
remove_branch_state(...); /* remove merge state */

So we only delete MERGE_AUTOSTASH after a successful reset and we only 
save the stash if it applies with conflicts. That's all a bit more 
involved than the patch here - please do give me a shout if you want 
some more information.

Thanks

Phillip

[1] Note that in the master branch this function is called reset_head(),
     you should base the fix on top of the "ps/history-drop" branch which
     is in "seen" (currently the tip is d11b348f784 (builtin/history:
     implement "drop" subcommand, 2026-07-01) but that might change when
     Junio rebuilds "seen".


> Reported-by: KrisPoint
> Signed-off-by: KrisPoint <KrisPointCSGO@outlook.com>
> ---
>   builtin/merge.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 5b46a596f0..5d9a242027 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1427,15 +1427,14 @@ int cmd_merge(int argc,
>   		if (!file_exists(git_path_merge_head(the_repository)))
>   			die(_("There is no merge to abort (MERGE_HEAD missing)."));
>   
> -		if (!refs_read_ref(get_main_ref_store(the_repository), "MERGE_AUTOSTASH", &stash_oid))
> -			refs_delete_ref(get_main_ref_store(the_repository),
> -					"", "MERGE_AUTOSTASH", &stash_oid,
> -					REF_NO_DEREF);
> +		refs_read_ref(get_main_ref_store(the_repository), "MERGE_AUTOSTASH", &stash_oid);
>   
> -		/* Invoke 'git reset --merge' */
> +		/* Invoke 'git reset --merge' (which also cleans up merge state,
> +		 * including saving the autostash to the stash list).
> +		 */
>   		ret = cmd_reset(nargc, nargv, prefix, the_repository);
>   
> -		if (!is_null_oid(&stash_oid)) {
> +		if (!ret && !is_null_oid(&stash_oid)) {
>   			oid_to_hex_r(stash_oid_hex, &stash_oid);
>   			apply_autostash_oid(stash_oid_hex);
>   		}

Re: [PATCH] merge --abort: don't delete autostash before reset succeeds

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I'm afraid I don't think this is the right solution. We only want to 
> save the stash if there are conflicts when we apply it - that is why 
> MERGE_AUTOSTASH is deleted before we do the reset - we want to prevent 
> remove_branch_state() from saving it. If the stash applies cleanly then 
> we should not save it. If the reset fails then we should keep 
> MERGE_AUTOSTASH along with the other merge state files rather than 
> saving the stash (which is actually what happens after this patch 
> because cmd_reset() dies before it calls remove_branch_state()).

Thanks for pointing it out that reset calls remove_branch_state(),
which in turn calls remove_merge_branch_state(), which in turn calls
save_autostash_ref().  We end up (when cmd_reset() is successful)
applying the autostash (which is good) but also saving a new stash.

> I think the solution is probably to stop calling 
> builtin/reset.c:cmd_reset() and instead ...

Great.  In general, it is a bad pattern we should find and fix for
cmd_A() to call cmd_B() in its implementation as a subroutine.  To
clean any such instance is a great thing to do.

> ...
> So we only delete MERGE_AUTOSTASH after a successful reset and we only 
> save the stash if it applies with conflicts. That's all a bit more 
> involved than the patch here - please do give me a shout if you want 
> some more information.
>
> Thanks
>
> Phillip

Thanks.