Fix merge restore state

From: Elijah Newren <[email protected]>

restore_state() had a local variable sb that is not used, and in fact, was never used even in the original commit that introduced it, 1c7b76be7d ("Build in merge", 2008-07-07). Remove it.

---
 builtin/merge.c | 2 --
 1 file changed, 2 deletions(-)

index f178f5a3ee1..00de224a2da 100644
@@ -375,7 +375,6 @@ static void reset_hard(const struct object_id *oid, int verbose)
 static void restore_state(const struct object_id *head,
 			  const struct object_id *stash)
 {
-	struct strbuf sb = STRBUF_INIT;
 	const char *args[] = { "stash", "apply", NULL, NULL };
 
 	if (is_null_oid(stash))

@@ -391,7 +390,6 @@ static void restore_state(const struct object_id *head,
 	 */
 	run_command_v_opt(args, RUN_GIT_CMD);
 
-	strbuf_release(&sb);
 	refresh_cache(REFRESH_QUIET);
 }
 

gitgitgadget

A simple series to fix restore_state() in builtin/merge.c, fixing the issue reported by ZheNing Hu over here:
https://lore.kernel.org/git/CAOLTT8R7QmpvaFPTRs3xTpxr7eiuxF-ZWtvUUSC0-JOo9Y+SqA@mail.gmail.com/

Elijah Newren (2):
merge: remove unused variable
merge: make restore_state() do as its name says

 builtin/merge.c        | 12 ++++++------
 t/t7607-merge-state.sh | 25 +++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 6 deletions(-)
 create mode 100755 t/t7607-merge-state.sh

base-commit: 6cd33dceed60949e2dbc32e3f0f5e67c4c882e1e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1231%2Fnewren%2Ffix-merge-restore-state-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1231/newren/fix-merge-restore-state-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1231

gitgitgadget

From: Elijah Newren <[email protected]>

Previously, if the user:

* Had no local changes before starting the merge
* A merge strategy makes changes to the working tree/index but returns with exit status 2

Then we'd call restore_state() to clean up the changes and either let the next merge strategy run (if there is one), or exit telling the user that no merge strategy could handle the merge. Unfortunately, restore_state() did not clean up the changes as expected; that function was a no-op if the stash was a null, and the stash would be null if there were no local changes before starting the merge. So, instead of "Rewinding the tree to pristine..." as the code claimed, restore_state() would leave garbage around in the index and working tree (possibly including conflicts) for either the next merge strategy or for the user after aborting the merge. And in the case of aborting the merge, the user would be unable to run "git merge --abort" to get rid of the unintended leftover conflicts, because the merge control files were not written as it was presumed that we had restored to a clean state already.

Fix the main problem by making sure that restore_state() only skips the stash application if the stash is null rather than skipping the whole function.

However, there is a secondary problem -- since merge.c forks subprocesses to do the cleanup, the in-memory index is left out-of-sync. While there was a refresh_cache(REFRESH_QUIET) call that attempted to correct that, that function would not handle cases where the previous merge strategy added conflicted entries. We need to drop the index and re-read it to handle such cases.

(Alternatively, we could stop forking subprocesses and instead call some appropriate function to do the work which would update the in-memory index automatically. For now, just do the simple fix.)

---
 builtin/merge.c        | 10 ++++++----
 t/t7607-merge-state.sh | 25 +++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 4 deletions(-)
 create mode 100755 t/t7607-merge-state.sh

index 00de224a2da..ae3ee3a996b 100644
@@ -377,11 +377,11 @@ static void restore_state(const struct object_id *head,
 {
 	const char *args[] = { "stash", "apply", NULL, NULL };
 
-	if (is_null_oid(stash))
-		return;
-
 	reset_hard(head, 1);
 
+	if (is_null_oid(stash))
+		goto refresh_cache;
+
 	args[2] = oid_to_hex(stash);
 
 	/*

@@ -390,7 +390,9 @@ static void restore_state(const struct object_id *head,
 	 */
 	run_command_v_opt(args, RUN_GIT_CMD);
 
-	refresh_cache(REFRESH_QUIET);
+refresh_cache:
+	if (discard_cache() < 0 || read_cache() < 0)
+		die(_("could not read index"));
 }
 
 /* This is called when no merge was necessary. */

new file mode 100755
index 00000000000..655478cd0b3
@@ -0,0 +1,25 @@
+#!/bin/sh
+
+test_description="Test that merge state is as expected after failed merge"
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+. ./test-lib.sh
+
+test_expect_success 'set up custom strategy' '
+	test_commit --no-tag "Initial" base base &&
+git show-ref &&
+
+	for b in branch1 branch2 branch3
+	do
+		git checkout -b $b main &&
+		test_commit --no-tag "Change on $b" base $b
+	done &&
+
+	git checkout branch1 &&
+	test_must_fail git merge branch2 branch3 &&
+	git diff --exit-code --name-status &&
+	test_path_is_missing .git/MERGE_HEAD
+'
+
+test_done

gitgitgadget

Nice. Thanks.

We do not need a new test to demonstrate the breakage in the proposed patch, I think. Here is one place I found that we already test that merging in a dirty working tree fails. We only need to make sure that we do so without losing local changes.

 t/t6424-merge-unrelated-index-changes.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 89dd544f38..88e0b541a0 100755
@@ -171,7 +171,8 @@ test_expect_success 'octopus, unrelated file touched' '
 	touch random_file && git add random_file &&
 
 	test_must_fail git merge C^0 D^0 &&
-	test_path_is_missing .git/MERGE_HEAD
+	test_path_is_missing .git/MERGE_HEAD &&
+	test_path_exists random_file
 '
 
 test_expect_success 'octopus, related file removed' '

I updated this series a few weeks ago, I believe addressing all the feedback. But I hesitated to type "/submit" over at
https://github.com/gitgitgadget/git/pull/1231. I've done virtually no development on "interesting" (to me) projects for quite some time; I'm only responding to email questions and review requests on the list. And the review process takes quite a bit of work at times, and on the chance there was more feedback to address, I just didn't have it in me to submit a new round even though it _might_ be complete and certainly fixes some known issues. I haven't even gotten to review Dscho's updates of my patches this whole week (spent the time I had mostly on the merge-ort issue reported).

Does anyone else want to take this topic over? There are updated patches and even an updated cover letter over at
https://github.com/gitgitgadget/git/pull/1231. It might involve nothing more than submitting those patches; they might be good enough already. (Alternatively, I can type "/submit" to send them in...if someone else agrees to respond to any feedback.)

Sorry for missing this patch before. I have tried this patch, which can perfectly
solve my origin problem. Now it can reset to origin state (Even when there are some files in the work-tree here that have changed)

Yes, I think you can submit it, thanks very much!

--
ZheNing Hu

MAINTAINER NOTE: Rebased on latest main/master. (In particular, needs vd/sparse-stash; otherwise, the changes made here regress a ensure-full-index testcase in t1092). Also, this fixes issues that predate the v2.37 cycle, so this series can wait until v2.38 opens up.

Other note: If this rounds needs updates, ZheNing Hu may be the one to respond and make any necessary updates, as per [1].

----------------------------------------------------------------------------

This is a simple series to fix restore_state() in builtin/merge.c, fixing the issue reported by ZheNing Hu over here:
https://lore.kernel.org/git/CAOLTT8R7QmpvaFPTRs3xTpxr7eiuxF-ZWtvUUSC0-JOo9Y+SqA@mail.gmail.com/

Changes since v1:

* Rebased
* Included Junio's patch providing more testcases from
https://lore.kernel.org/git/[email protected]/
* Added three new patches to fix issues highlighted by Junio's testcases, in particular to (a) fix stashing with racy-dirty files present, (b) fix restoring staged state in restore_state(), and (c) ensure we can restore pre-merge state. All three were long-standing issues that we just hadn't noticed yet and thus are useful fixes on their own. However, my fix from v1 (which still remains as the final patch) does make it easier to notice these issues, and in particular that combined with Junio's new testcases unearthed those problems.

[1]
https://lore.kernel.org/git/CAOLTT8RpGGioOyaMw5tkeWXmHpOaBW9UH8JghUvBRQ50ZcDdYQ@mail.gmail.com/

Elijah Newren (5):
merge: remove unused variable
merge: fix save_state() to work when there are racy-dirty files
merge: make restore_state() restore staged state too
merge: ensure we can actually restore pre-merge state
merge: do not exit restore_state() prematurely

Junio C Hamano (1):
t6424: make sure a failed merge preserves local changes

 builtin/merge.c                          | 32 ++++++++++++++----------
 t/t6424-merge-unrelated-index-changes.sh | 32 ++++++++++++++++++++++--
 t/t7607-merge-state.sh                   | 25 ++++++++++++++++++
 3 files changed, 74 insertions(+), 15 deletions(-)
 create mode 100755 t/t7607-merge-state.sh

base-commit: 8ddf593a250e07d388059f7e3f471078e1d2ed5c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1231%2Fnewren%2Ffix-merge-restore-state-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1231/newren/fix-merge-restore-state-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1231

Range-diff vs v1:

 -:  ----------- > 1:  6147e72c309 t6424: make sure a failed merge preserves local changes
 1:  042d624b815 = 2:  230d84f09c8 merge: remove unused variable
 -:  ----------- > 3:  89e5e633241 merge: fix save_state() to work when there are racy-dirty files
 -:  ----------- > 4:  4a8b7c9e06d merge: make restore_state() restore staged state too
 -:  ----------- > 5:  a03075167c1 merge: ensure we can actually restore pre-merge state
 2:  88bdca72a78 ! 6:  0783b48c121 merge: make restore_state() do as its name says

@@ Metadata
Author: Elijah Newren <[email protected]>

## Commit message ##
- merge: make restore_state() do as its name says
+ merge: do not exit restore_state() prematurely

Previously, if the user:

@@ Commit message
## builtin/merge.c ##
@@ builtin/merge.c: static void restore_state(const struct object_id *head,
{
- const char *args[] = { "stash", "apply", NULL, NULL };
+ const char *args[] = { "stash", "apply", "--index", NULL, NULL };

- if (is_null_oid(stash))
- return;
@@ builtin/merge.c: static void restore_state(const struct object_id *head,
+ if (is_null_oid(stash))
+ goto refresh_cache;
+
- args[2] = oid_to_hex(stash);
+ args[3] = oid_to_hex(stash);

/*
@@ builtin/merge.c: static void restore_state(const struct object_id *head,

gitgitgadget

From: Junio C Hamano <[email protected]>

We do make sure that an attempt to merge with various forms of local changes will "fail", but the point of stopping the merge is so that we refrain from discarding uncommitted local changes that could be precious. Add a few more checks for each case to make sure the local changes are left intact.

---
 t/t6424-merge-unrelated-index-changes.sh | 32 ++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

index 89dd544f388..b6e424a427b 100755
@@ -71,7 +71,9 @@ test_expect_success 'ff update' '
 	git merge E^0 &&
 
 	test_must_fail git rev-parse HEAD:random_file &&
-	test "$(git diff --name-only --cached E)" = "random_file"
+	test "$(git diff --name-only --cached E)" = "random_file" &&
+	test_path_is_file random_file &&
+	git rev-parse --verify :random_file
 '
 
 test_expect_success 'ff update, important file modified' '

@@ -83,6 +85,8 @@ test_expect_success 'ff update, important file modified' '
 	git add subdir/e &&
 
 	test_must_fail git merge E^0 &&
+	test_path_is_file subdir/e &&
+	git rev-parse --verify :subdir/e &&
 	test_path_is_missing .git/MERGE_HEAD
 '
 

@@ -93,6 +97,8 @@ test_expect_success 'resolve, trivial' '
 	touch random_file && git add random_file &&
 
 	test_must_fail git merge -s resolve C^0 &&
+	test_path_is_file random_file &&
+	git rev-parse --verify :random_file &&
 	test_path_is_missing .git/MERGE_HEAD
 '
 

@@ -103,6 +109,8 @@ test_expect_success 'resolve, non-trivial' '
 	touch random_file && git add random_file &&
 
 	test_must_fail git merge -s resolve D^0 &&
+	test_path_is_file random_file &&
+	git rev-parse --verify :random_file &&
 	test_path_is_missing .git/MERGE_HEAD
 '
 

@@ -113,6 +121,8 @@ test_expect_success 'recursive' '
 	touch random_file && git add random_file &&
 
 	test_must_fail git merge -s recursive C^0 &&
+	test_path_is_file random_file &&
+	git rev-parse --verify :random_file &&
 	test_path_is_missing .git/MERGE_HEAD
 '
 

@@ -145,9 +155,12 @@ test_expect_success 'recursive, when file has staged changes not matching HEAD n
 	mkdir subdir &&
 	test_seq 1 10 >subdir/a &&
 	git add subdir/a &&
+	git rev-parse --verify :subdir/a >expect &&
 
 	# We have staged changes; merge should error out
 	test_must_fail git merge -s recursive E^0 2>err &&
+	git rev-parse --verify :subdir/a >actual &&
+	test_cmp expect actual &&
 	test_i18ngrep "changes to the following files would be overwritten" err
 '
 

@@ -158,9 +171,12 @@ test_expect_success 'recursive, when file has staged changes matching what a mer
 	mkdir subdir &&
 	test_seq 1 11 >subdir/a &&
 	git add subdir/a &&
+	git rev-parse --verify :subdir/a >expect &&
 
 	# We have staged changes; merge should error out
 	test_must_fail git merge -s recursive E^0 2>err &&
+	git rev-parse --verify :subdir/a >actual &&
+	test_cmp expect actual &&
 	test_i18ngrep "changes to the following files would be overwritten" err
 '
 

@@ -171,7 +187,9 @@ test_expect_success 'octopus, unrelated file touched' '
 	touch random_file && git add random_file &&
 
 	test_must_fail git merge C^0 D^0 &&
-	test_path_is_missing .git/MERGE_HEAD
+	test_path_is_missing .git/MERGE_HEAD &&
+	git rev-parse --verify :random_file &&
+	test_path_exists random_file
 '
 
 test_expect_success 'octopus, related file removed' '

@@ -181,6 +199,8 @@ test_expect_success 'octopus, related file removed' '
 	git rm b &&
 
 	test_must_fail git merge C^0 D^0 &&
+	test_path_is_missing b &&
+	test_must_fail git rev-parse --verify :b &&
 	test_path_is_missing .git/MERGE_HEAD
 '
 

@@ -189,8 +209,12 @@ test_expect_success 'octopus, related file modified' '
 	git checkout B^0 &&
 
 	echo 12 >>a && git add a &&
+	git rev-parse --verify :a >expect &&
 
 	test_must_fail git merge C^0 D^0 &&
+	test_path_is_file a &&
+	git rev-parse --verify :a >actual &&
+	test_cmp expect actual &&
 	test_path_is_missing .git/MERGE_HEAD
 '
 

@@ -201,6 +225,8 @@ test_expect_success 'ours' '
 	touch random_file && git add random_file &&
 
 	test_must_fail git merge -s ours C^0 &&
+	test_path_is_file random_file &&
+	git rev-parse --verify :random_file &&
 	test_path_is_missing .git/MERGE_HEAD
 '
 

@@ -211,6 +237,8 @@ test_expect_success 'subtree' '
 	touch random_file && git add random_file &&
 
 	test_must_fail git merge -s subtree E^0 &&
+	test_path_is_file random_file &&
+	git rev-parse --verify :random_file &&
 	test_path_is_missing .git/MERGE_HEAD
 '
 

gitgitgadget

From: Elijah Newren <[email protected]>

restore_state() had a local variable sb that is not used, and in fact, was never used even in the original commit that introduced it, 1c7b76be7d ("Build in merge", 2008-07-07). Remove it.

---
 builtin/merge.c | 2 --
 1 file changed, 2 deletions(-)

index f178f5a3ee1..00de224a2da 100644
@@ -375,7 +375,6 @@ static void reset_hard(const struct object_id *oid, int verbose)
 static void restore_state(const struct object_id *head,
 			  const struct object_id *stash)
 {
-	struct strbuf sb = STRBUF_INIT;
 	const char *args[] = { "stash", "apply", NULL, NULL };
 
 	if (is_null_oid(stash))

@@ -391,7 +390,6 @@ static void restore_state(const struct object_id *head,
 	 */
 	run_command_v_opt(args, RUN_GIT_CMD);
 
-	strbuf_release(&sb);
 	refresh_cache(REFRESH_QUIET);
 }
 

gitgitgadget

From: Elijah Newren <[email protected]>

When there are racy-dirty files, but no files are modified, git stash create exits with unsuccessful status. This causes merge to fail. Refresh the index first to avoid this problem.

---
 builtin/merge.c | 8 ++++++++
 1 file changed, 8 insertions(+)

index 00de224a2da..8ce4336dd3f 100644
@@ -313,8 +313,16 @@ static int save_state(struct object_id *stash)
 	int len;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buffer = STRBUF_INIT;
+	struct lock_file lock_file = LOCK_INIT;
+	int fd;
 	int rc = -1;
 
+	fd = repo_hold_locked_index(the_repository, &lock_file, 0);
+	refresh_cache(REFRESH_QUIET);
+	if (0 <= fd)
+		repo_update_index_if_able(the_repository, &lock_file);
+	rollback_lock_file(&lock_file);
+
 	strvec_pushl(&cp.args, "stash", "create", NULL);
 	cp.out = -1;
 	cp.git_cmd = 1;

gitgitgadget

From: Elijah Newren <[email protected]>

merge can be invoked with uncommitted changes, including staged changes. merge is responsible for restoring this state if some of the merge strategies make changes. However, it was not restoring staged changes due to the lack of the "--index" option to "git stash apply". Add the option to fix this shortcoming.

---
 builtin/merge.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 8ce4336dd3f..2dc56fab70b 100644
@@ -383,14 +383,14 @@ static void reset_hard(const struct object_id *oid, int verbose)
 static void restore_state(const struct object_id *head,
 			  const struct object_id *stash)
 {
-	const char *args[] = { "stash", "apply", NULL, NULL };
+	const char *args[] = { "stash", "apply", "--index", NULL, NULL };
 
 	if (is_null_oid(stash))
 		return;
 
 	reset_hard(head, 1);
 
-	args[2] = oid_to_hex(stash);
+	args[3] = oid_to_hex(stash);
 
 	/*
 	 * It is OK to ignore error here, for example when there was

gitgitgadget

From: Elijah Newren <[email protected]>

Previously, if the user:

* Had no local changes before starting the merge
* A merge strategy makes changes to the working tree/index but returns with exit status 2

Then we'd call restore_state() to clean up the changes and either let the next merge strategy run (if there is one), or exit telling the user that no merge strategy could handle the merge. Unfortunately, restore_state() did not clean up the changes as expected; that function was a no-op if the stash was a null, and the stash would be null if there were no local changes before starting the merge. So, instead of "Rewinding the tree to pristine..." as the code claimed, restore_state() would leave garbage around in the index and working tree (possibly including conflicts) for either the next merge strategy or for the user after aborting the merge. And in the case of aborting the merge, the user would be unable to run "git merge --abort" to get rid of the unintended leftover conflicts, because the merge control files were not written as it was presumed that we had restored to a clean state already.

Fix the main problem by making sure that restore_state() only skips the stash application if the stash is null rather than skipping the whole function.

However, there is a secondary problem -- since merge.c forks subprocesses to do the cleanup, the in-memory index is left out-of-sync. While there was a refresh_cache(REFRESH_QUIET) call that attempted to correct that, that function would not handle cases where the previous merge strategy added conflicted entries. We need to drop the index and re-read it to handle such cases.

(Alternatively, we could stop forking subprocesses and instead call some appropriate function to do the work which would update the in-memory index automatically. For now, just do the simple fix.)

---
 builtin/merge.c        | 10 ++++++----
 t/t7607-merge-state.sh | 25 +++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 4 deletions(-)
 create mode 100755 t/t7607-merge-state.sh

index aaee8f6a553..a21dece1b55 100644
@@ -385,11 +385,11 @@ static void restore_state(const struct object_id *head,
 {
 	const char *args[] = { "stash", "apply", "--index", NULL, NULL };
 
-	if (is_null_oid(stash))
-		return;
-
 	reset_hard(head, 1);
 
+	if (is_null_oid(stash))
+		goto refresh_cache;
+
 	args[3] = oid_to_hex(stash);
 
 	/*

@@ -398,7 +398,9 @@ static void restore_state(const struct object_id *head,
 	 */
 	run_command_v_opt(args, RUN_GIT_CMD);
 
-	refresh_cache(REFRESH_QUIET);
+refresh_cache:
+	if (discard_cache() < 0 || read_cache() < 0)
+		die(_("could not read index"));
 }
 
 /* This is called when no merge was necessary. */

new file mode 100755
index 00000000000..655478cd0b3
@@ -0,0 +1,25 @@
+#!/bin/sh
+
+test_description="Test that merge state is as expected after failed merge"
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+. ./test-lib.sh
+
+test_expect_success 'set up custom strategy' '
+	test_commit --no-tag "Initial" base base &&
+git show-ref &&
+
+	for b in branch1 branch2 branch3
+	do
+		git checkout -b $b main &&
+		test_commit --no-tag "Change on $b" base $b
+	done &&
+
+	git checkout branch1 &&
+	test_must_fail git merge branch2 branch3 &&
+	git diff --exit-code --name-status &&
+	test_path_is_missing .git/MERGE_HEAD
+'
+
+test_done

gitgitgadget

From: Elijah Newren <[email protected]>

Merge strategies can fail -- not just have conflicts, but give up and say that they are unable to handle the current type of merge. However, they can also make changes to the index and working tree before giving up; merge-octopus does this, for example. Currently, we do not expect the individual strategies to clean up after themselves, but instead expect builtin/merge.c to do so. For it to be able to, it needs to save the state before trying the merge strategy so it can have something to restore to. Therefore, remove the shortcut bypassing the save_state() call.

---
 builtin/merge.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

index 2dc56fab70b..aaee8f6a553 100644
@@ -1663,12 +1663,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * tree in the index -- this means that the index must be in
 	 * sync with the head commit.  The strategies are responsible
 	 * to ensure this.
+	 *
+	 * Stash away the local changes so that we can try more than one.
 	 */
-	if (use_strategies_nr == 1 ||
-	    /*
-	     * Stash away the local changes so that we can try more than one.
-	     */
-	    save_state(&stash))
+	if (save_state(&stash))
 		oidclr(&stash);
 
 	for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {

gitgitgadget

I just want to show what sence will meet this errors:

1. touch file
2. git add file
3. git stash push (user may do it before git merge)
4. touch file (update file but not update its content)
5. git merge (call git stash create and return 1)

So I have knew about what's the meaning of this patch:

If user do the git stash manually and update file timestamp before git merge, it may make next git stash failed. And refresh index can make index entry sync with the work tree file status.

Thanks.

ZheNing Hu

Now git merge (all strategies) can learn to restore origin index state. LGTM.

ZheNing Hu

So now we will not make "stash" empty even if we are using only one merge strategy (e.g. octopus), so we can reset to the original state correctly.

Good.

ZheNing Hu

Racily dirty? Or just being stat-dirty is sufficient to cause the "stash create" to fail?

I think, from the above reproduction recipe, that the breakage does not depend on racily-clean index entries (i.e. file touched within the same timestamp as the last write of the index without changing their size). So s/racy-dirty/stat-dirty/ (both on the title and the body) would be a sufficient fix.

Thanks.

OK.

Absolutely right.

Shouldn't this be testable?

I actually take this part (which implied that the change is a good idea) back. I think we have clearly documented for the past 17 years that you can have local changes but your index must match the HEAD before you start your merge.

If "stash apply" vs "stash apply --index" makes any difference, there is something wrong. We should be aborting the "git merge" even before we even start mucking with the working tree and the index with strategies, no? I think it is the bug, if this change makes any difference, to be fixed---we shouldn't be proceeding to even create a stash with index changes to begin with.

[replying to Junio's email since I don't have the original available...]

Let's break out of the loop with || return 1 if something in the loop body fails.

    for b in branch1 branch2 branch3
    do
        git checkout -b $b main &&
        test_commit --no-tag "Change on $b" base $b || return 1
    done &&

Yep, stat-dirty. I'll fix up the title and body; thanks.

Yes, I will add a test.

Actually, we don't enforce that the index must match HEAD in all cases, as noted in commit 55f39cf755 ("merge: fix misleading pre-merge check documentation", 2018-06-30). That commit also pointed out how the documentation was a bit unclear in this area.

We also apparently fail to enforce the condition in at least two cases that weren't a valid exception, which I just found while working on a testcase for this patch. (Thus, we have one more sordid tale to add to the saga in commit 9822175d2b ("Ensure index matches head before invoking merge machinery, round N", 2019-08-17))

However, the failed enforcement and the "valid" special exceptions aren't too relevant here, so...

I agree with you that generally if the index does not match HEAD, then
(A) we should abort the merge, and (B) the working tree and index need to be left intact when the merge aborts.

But I don't think your conclusion follows from those two items, because of the last sentence of this comment:

/*
* At this point, we need a real merge. No matter what strategy
* we use, it would operate on the index, possibly affecting the
* working tree, and when resolved cleanly, have the desired
* tree in the index -- this means that the index must be in
* sync with the head commit. The strategies are responsible
* to ensure this.
*/

Due to this requirement, if a user has staged changes before starting the merge, builtin/merge.c will:

* stash the changes
* try all the merge strategies in turn, each of which report they
cannot function due to index not matching HEAD
* restore the changes via "git stash apply"

This sequence has the net effect of not quite cleanly aborting the merge -- it also unstashes the user's changes.

One way to fix this problem is the simple patch I proposed. An alternative fix would be to rip out the extra code from all the merge strategies that enforces the index matches HEAD requirement, and then adding enforcement of that condition early in builtin/merge.c. That alternative fix probably would have saved us from a lot of the headache detailed in commit 9822175d2b above, but it may also make recursive and ort a bit slower (which had relied on unpack-trees to do some of this checking, and thus they'd have some redundant checks).

I should probably change it to "Stash away the local changes so that we can try more than one and/or recover from merge strategies bailing".

In regards to the "good idea" side, I don't really like it either, but:

1. Merge strategies are allowed to make whatever changes they want
to the index and working tree. They have no requirement to clean up after themselves.
2. Merge strategies are allowed to bail and say, "Nevermind, not
only can I not successfully merge this, I can't even leave conflicts for the users to resolve; I just can't handle it at all. Try some other strategy." (See the "exit 2" codepath of commit 98efc8f3d8 ("octopus: allow manual resolve on the last round.", 2006-01-13), for example)
3. Merge strategies can bail with the "try some other strategy"
response _after_ mucking with the index and working tree. octopus does (again, see the commit mentioned above)
4. builtin/merge.c previously would _only_ cleanup for merge
strategies if there was more than 1.

This combination is clearly broken. We need to fix either item 1 or item 4 (or maybe item 3?). Since 1 might run into issues with user-custom merge strategies which may have been written to mimic octopus and thus rely on the assurances in items 1-3 above, I figured fixing item 4 was the easiest.

Okay, will do.

I don't think so; the situation for [3/6] is different. The basic timeline is as follows:
1. <User does lots of stuff over weeks and months>
2. User decides to merge one or more branches
3. merge does save_state() [i.e. "git stash create"]
4. The first strategy fails
5. We restore the state before trying the next strategy

The current code is dealing with step 5. Patch [3/6] was to prevent failures in step 3 from users creating stat-dirty files in step 1. Once step 3 runs, the only way to become stat-dirty again is if the user simultaneously messes with their checkout while the "git merge" command is running. Attempting to preventatively handle users modifying the working tree simultaneously with concurrent git commands like git merge seems like a losing proposition to me; it'd be a huge can of worms and have a million holes. I don't think that's worth it.

Yes.

Looks like part of a leftover debugging aid; sorry about that. Will clean up.

That's not what's happening here. It is not failing due to conflict, octopus is reporting that it cannot even leave things in a conflicted state for the user, and is actually incapable of handling this particular type of merge. Part of the output seen when attempting this merge includes:
fatal: merge program failed
Should not be doing an octopus.

See previous discussion at
https://lore.kernel.org/git/[email protected]/. To make this clearer, perhaps I should use "test_expect_code 2" instead of test_must_fail, and also grep the output/error for the above messages.

No, we are not changing what octopus can handle here. The code above is not triggered when octopus returns that it ran into conflicts for the user to resolve. It is only triggered when octopus says it cannot handle the merge in question. See your commit 98efc8f3d8 ("octopus:
allow manual resolve on the last round.", 2006-01-13), and note that the the "exit 2" code path is the one that we are hitting. I'll add some comments to the testcase and rewrite the commit message to try to make this clearer.

NOTE: Rebased on master, yet again, because (1) Junio merged his commit to master separately, and (2) Ævar's intentional duplication of my second patch[1] later conflicted with other changes I had to make in the same area. (Which isn't a big deal, but for future reference, it would be nicer to avoid conflicts by omitting the fixup I had already submitted[2] instead of intentionally duplicating it).

This started as a simple series to fix restore_state() in builtin/merge.c, fixing an issue reported by ZheNing Hu[3]. It's grown so much it's hard to call it simple. Anyway...

Changes since v2:

* Removed the first two patches, as noted above in the comment about rebasing.
* Inserted new patches 3, 4, and 5 to fix some related bugs. Folks are more likely to object to patch 5 than the others; people should probably take a look at that one if they have limited time.
* Dramatically reworded commit messages given the misunderstandings of what was being addressed and done. Hopefully it is much clearer what the last three patches are doing and what they are not doing, and why.
* Added several new testcases

[1]
https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/ [3]
https://lore.kernel.org/git/CAOLTT8R7QmpvaFPTRs3xTpxr7eiuxF-ZWtvUUSC0-JOo9Y+SqA@mail.gmail.com/

Elijah Newren (7):
merge-ort-wrappers: make printed message match the one from recursive
merge-resolve: abort if index does not match HEAD
merge: do not abort early if one strategy fails to handle the merge
merge: fix save_state() to work when there are stat-dirty files
merge: make restore_state() restore staged state too
merge: ensure we can actually restore pre-merge state
merge: do not exit restore_state() prematurely

 builtin/merge.c                          | 59 ++++++++++++++++++------
 git-merge-resolve.sh                     | 10 ++++
 merge-ort-wrappers.c                     |  7 ++-
 t/t6402-merge-rename.sh                  |  2 +-
 t/t6424-merge-unrelated-index-changes.sh | 58 +++++++++++++++++++++++
 t/t6439-merge-co-error-msgs.sh           |  1 +
 t/t7607-merge-state.sh                   | 32 +++++++++++++
 7 files changed, 154 insertions(+), 15 deletions(-)
 create mode 100755 t/t7607-merge-state.sh

base-commit: e72d93e88cb20b06e88e6e7d81bd1dc4effe453f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1231%2Fnewren%2Ffix-merge-restore-state-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1231/newren/fix-merge-restore-state-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1231

Range-diff vs v2:

 1:  6147e72c309 < -:  ----------- t6424: make sure a failed merge preserves local changes
 2:  230d84f09c8 < -:  ----------- merge: remove unused variable
 -:  ----------- > 1:  e39b2e15ece merge-ort-wrappers: make printed message match the one from recursive
 -:  ----------- > 2:  2810dec7608 merge-resolve: abort if index does not match HEAD
 -:  ----------- > 3:  b41853e3f99 merge: do not abort early if one strategy fails to handle the merge
 3:  89e5e633241 ! 4:  64700338a28 merge: fix save_state() to work when there are racy-dirty files

@@ Metadata
Author: Elijah Newren <[email protected]>

## Commit message ##
- merge: fix save_state() to work when there are racy-dirty files
+ merge: fix save_state() to work when there are stat-dirty files

- When there are racy-dirty files, but no files are modified,
+ When there are stat-dirty files, but no files are modified,
git stash create exits with unsuccessful status. This causes merge
- to fail. Refresh the index first to avoid this problem.
+ to fail. Copy some code from sequencer.c's create_autostash to refresh
+ the index first to avoid this problem.

@@ builtin/merge.c: static int save_state(struct object_id *stash)
strvec_pushl(&cp.args, "stash", "create", NULL);
cp.out = -1;
cp.git_cmd = 1;
+
+ ## t/t6424-merge-unrelated-index-changes.sh ##
+@@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'subtree' '
+ test_path_is_missing .git/MERGE_HEAD
+ '
+
++test_expect_success 'avoid failure due to stat-dirty files' '
++ git reset --hard &&
++ git checkout B^0 &&
++
++ # Make "a" be stat-dirty
++ test-tool chmtime =+1 a &&
++
++ # stat-dirty file should not prevent stash creation in builtin/merge.c
++ git merge -s resolve -s recursive D^0
++'
++
+ test_expect_success 'resolve && recursive && ort' '
+ git reset --hard &&
+ git checkout B^0 &&
4: 4a8b7c9e06d ! 5: 91c495c770e merge: make restore_state() restore staged state too
@@ Metadata
## Commit message ##
merge: make restore_state() restore staged state too

- merge can be invoked with uncommitted changes, including staged changes.
- merge is responsible for restoring this state if some of the merge
- strategies make changes. However, it was not restoring staged changes
- due to the lack of the "--index" option to "git stash apply". Add the
- option to fix this shortcoming.
+ There are multiple issues at play here:
+
+ 1) If git merge is invoked with staged changes, it should abort
+ without doing any merging, and the user's working tree and index
+ should be the same as before merge was invoked.
+ 2) Merge strategies are responsible for enforcing the index == HEAD
+ requirement. (See 9822175d2b ("Ensure index matches head before
+ invoking merge machinery, round N", 2019-08-17) for some history
+ around this.)
+ 3) Merge strategies can bail saying they are not an appropriate
+ handler for the merge in question (possibly allowing other
+ strategies to be used instead).
+ 4) Merge strategies can make changes to the index and working tree,
+ and have no expectation to clean up after themselves, *even* if
+ they bail out and say they are not an appropriate handler for
+ the merge in question. (The octopus merge strategy does this,
+ for example.)
+ 5) Because of (3) and (4), builtin/merge.c stashes state before
+ trying merge strategies and restores it afterward.
+
+ Unfortunately, if users had staged changes before calling git merge,
+ builtin/merge.c could do the following:
+
+    * stash the changes, in order to clean up after the strategies
+    * try all the merge strategies in turn, each of which report they
+ cannot function due to the index not matching HEAD
+    * restore the changes via "git stash apply"
+
+ But that last step would have the net effect of unstaging the user's
+ changes. Fix this by adding the "--index" option to "git stash apply".
+ While at it, also squelch the stash apply output; we already report
+ "Rewinding the tree to pristine..." and don't need a detailed `git
+ status` report afterwards.

@@ builtin/merge.c: static void reset_hard(const struct object_id *oid, int verbose
const struct object_id *stash)
{
- const char *args[] = { "stash", "apply", NULL, NULL };
-+ const char *args[] = { "stash", "apply", "--index", NULL, NULL };
++ const char *args[] = { "stash", "apply", "--index", "--quiet",
++ NULL, NULL };

if (is_null_oid(stash))
return;
@@ builtin/merge.c: static void reset_hard(const struct object_id *oid, int verbose
reset_hard(head, 1);

- args[2] = oid_to_hex(stash);
-+ args[3] = oid_to_hex(stash);
++ args[4] = oid_to_hex(stash);

/*
* It is OK to ignore error here, for example when there was
+
+ ## t/t6424-merge-unrelated-index-changes.sh ##
+@@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'resolve && recursive && ort' ' +
+ test_seq 0 10 >a &&
+ git add a &&
++ git rev-parse :a >expect &&
+
+ sane_unset GIT_TEST_MERGE_ALGORITHM &&
+ test_must_fail git merge -s resolve -s recursive -s ort C^0 >output 2>&1 &&
+@@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'resolve && recursive && ort' '
+ grep "Trying merge strategy resolve..." output &&
+ grep "Trying merge strategy recursive..." output &&
+ grep "Trying merge strategy ort..." output &&
+- grep "No merge strategy handled the merge." output
++ grep "No merge strategy handled the merge." output &&
++
++ # Changes to "a" should remain staged
++ git rev-parse :a >actual &&
++ test_cmp expect actual
+ '
+
+ test_done
5: a03075167c1 ! 6: 887967c1f3f merge: ensure we can actually restore pre-merge state
@@ Metadata
## Commit message ##
merge: ensure we can actually restore pre-merge state

- Merge strategies can fail -- not just have conflicts, but give up and
- say that they are unable to handle the current type of merge. However,
- they can also make changes to the index and working tree before giving
- up; merge-octopus does this, for example. Currently, we do not expect
- the individual strategies to clean up after themselves, but instead
- expect builtin/merge.c to do so. For it to be able to, it needs to save
- the state before trying the merge strategy so it can have something to
- restore to. Therefore, remove the shortcut bypassing the save_state()
- call.
+ Merge strategies can:
+ * succeed with a clean merge
+ * succeed with a conflicted merge
+ * fail to handle the given type of merge
+
+ If one is thinking in terms of automatic mergeability, they would use
+ the word "fail" instead of "succeed" for the second bullet, but I am
+ focusing here on ability of the merge strategy to handle the given
+ inputs, not on whether the given inputs are mergeable. The third
+ category is about the merge strategy failing to know how to handle the
+ given data; examples include:
+
+ * Passing more than 2 branches to 'recursive' or 'ort'
+ * Passing 2 or fewer branches to 'octopus'
+ * Trying to do more complicated merges with 'resolve' (I believe
+ directory/file conflicts will cause it to bail.)
+ * Octopus running into a merge conflict for any branch OTHER than
+ the final one (see the "exit 2" codepath of commit 98efc8f3d8
+ ("octopus: allow manual resolve on the last round.", 2006-01-13))
+
+ That final one is particularly interesting, because it shows that the
+ merge strategy can muck with the index and working tree, and THEN bail
+ and say "sorry, this strategy cannot handle this type of merge; use
+ something else".
+
+ Further, we do not currently expect the individual strategies to clean
+ up after themselves, but instead expect builtin/merge.c to do so. For
+ it to be able to, it needs to save the state before trying the merge
+ strategy so it can have something to restore to. Therefore, remove the
+ shortcut bypassing the save_state() call.
+
+ There is another bug on the restore_state() side of things, so no
+ testcase will be added until the next commit when we have addressed that
+ issue as well.

@@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)
* sync with the head commit. The strategies are responsible
* to ensure this.
+ *
-+ * Stash away the local changes so that we can try more than one.
++ * Stash away the local changes so that we can try more than one
++ * and/or recover from merge strategies bailing while leaving the
++ * index and working tree polluted.
*/
- if (use_strategies_nr == 1 ||
- /*
6: 0783b48c121 ! 7: 81c40492a62 merge: do not exit restore_state() prematurely
@@ Commit message
appropriate function to do the work which would update the in-memory index automatically. For now, just do the simple fix.)

+ Also, add a testcase checking this, one for which the octopus strategy
+ fails on the first commit it attempts to merge, and thus which it
+ cannot handle at all and must completely bail on (as per the "exit 2"
+ code path of commit 98efc8f3d8 ("octopus: allow manual resolve on the
+ last round.", 2006-01-13)).
+

## builtin/merge.c ##
@@ builtin/merge.c: static void restore_state(const struct object_id *head,
- {
- const char *args[] = { "stash", "apply", "--index", NULL, NULL };
+ const char *args[] = { "stash", "apply", "--index", "--quiet",
+ NULL, NULL };

- if (is_null_oid(stash))
- return;
@@ builtin/merge.c: static void restore_state(const struct object_id *head,
+ if (is_null_oid(stash))
+ goto refresh_cache;
+
- args[3] = oid_to_hex(stash);
+ args[4] = oid_to_hex(stash);

/*
@@ builtin/merge.c: static void restore_state(const struct object_id *head,
@@ t/t7607-merge-state.sh (new)
+
+test_expect_success 'set up custom strategy' '
+ test_commit --no-tag "Initial" base base &&
-+git show-ref &&
+
+ for b in branch1 branch2 branch3
+ do
+ git checkout -b $b main &&
-+ test_commit --no-tag "Change on $b" base $b
++ test_commit --no-tag "Change on $b" base $b || return 1
+ done &&
+
+ git checkout branch1 &&
-+ test_must_fail git merge branch2 branch3 &&
++ # This is a merge that octopus cannot handle. Note, that it does not
++ # just hit conflicts, it completely fails and says that it cannot
++ # handle this type of merge.
++ test_expect_code 2 git merge branch2 branch3 >output 2>&1 &&
++ grep "fatal: merge program failed" output &&
++ grep "Should not be doing an octopus" output &&
++
++ # Make sure we did not leave stray changes around when no appropriate
++ # merge strategy was found
+ git diff --exit-code --name-status &&
+ test_path_is_missing .git/MERGE_HEAD
+'

gitgitgadget

From: Elijah Newren <[email protected]>

When the index does not match HEAD, the merge strategies are responsible to detect that condition and abort. The merge-ort-wrappers had code to implement this and meant to copy the error message from merge-recursive but deviated in two ways, both due to the message in merge-recursive being processed by another function that made additional changes:
* It added an implicit "error: " prefix
* It added an implicit trailing newline

Add these things, but do so in a couple extra steps to avoid having translators need to translate another not-quite-identical string.

---
 merge-ort-wrappers.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index ad041061695..d2c416bb5c0 100644
@@ -10,8 +10,13 @@ static int unclean(struct merge_options *opt, struct tree *head)
 	struct strbuf sb = STRBUF_INIT;
 
 	if (head && repo_index_has_changes(opt->repo, head, &sb)) {
-		fprintf(stderr, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
+		struct strbuf err = STRBUF_INIT;
+		strbuf_addstr(&err, "error: ");
+		strbuf_addf(&err, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
 		    sb.buf);
+		strbuf_addch(&err, '\n');
+		fputs(err.buf, stderr);
+		strbuf_release(&err);
 		strbuf_release(&sb);
 		return -1;
 	}

gitgitgadget

From: Elijah Newren <[email protected]>

As noted in commit 9822175d2b ("Ensure index matches head before invoking merge machinery, round N", 2019-08-17), we have had a very long history of problems with failing to enforce the requirement that index matches HEAD when starting a merge. One of the commits referenced in the long tale of issues arising from lax enforcement of this requirement was commit 55f39cf755 ("merge: fix misleading pre-merge check documentation", 2018-06-30), which tried to document the requirement and noted there were some exceptions. As mentioned in that commit message, the resolve strategy was the one strategy that did not have an explicit index matching HEAD check, and the reason it didn't was that I wasn't able to discover any cases where the implementation would fail to catch the problem and abort, and didn't want to introduce unnecessary performance overhead of adding another check.

Well, today I discovered a testcase where the implementation does not catch the problem and so an explicit check is needed. Add a testcase that previously would have failed, and update git-merge-resolve.sh to have an explicit check. Note that the code is copied from 3ec62ad9ff ("merge-octopus: abort if index does not match HEAD", 2016-04-09), so that we reuse the same message and avoid making translators need to translate some new message.

---
 builtin/merge.c                          | 20 ++++++++++++++++++
 git-merge-resolve.sh                     | 10 +++++++++
 t/t6424-merge-unrelated-index-changes.sh | 26 ++++++++++++++++++++++++
 3 files changed, 56 insertions(+)

index 23170f2d2a6..13884b8e836 100644
@@ -1599,6 +1599,26 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 */
 		refresh_cache(REFRESH_QUIET);
 		if (allow_trivial && fast_forward != FF_ONLY) {
+			/*
+			 * Must first ensure that index matches HEAD before
+			 * attempting a trivial merge.
+			 */
+			struct tree *head_tree = get_commit_tree(head_commit);
+			struct strbuf sb = STRBUF_INIT;
+
+			if (repo_index_has_changes(the_repository, head_tree,
+						   &sb)) {
+				struct strbuf err = STRBUF_INIT;
+				strbuf_addstr(&err, "error: ");
+				strbuf_addf(&err, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
+					    sb.buf);
+				strbuf_addch(&err, '\n');
+				fputs(err.buf, stderr);
+				strbuf_release(&err);
+				strbuf_release(&sb);
+				return -1;
+			}
+
 			/* See if it is really trivial. */
 			git_committer_info(IDENT_STRICT);
 			printf(_("Trying really trivial in-index merge...\n"));

index 343fe7bccd0..77e93121bf8 100755
@@ -5,6 +5,16 @@
 #
 # Resolve two trees, using enhanced multi-base read-tree.
 
+. git-sh-setup
+
+# Abort if index does not match HEAD
+if ! git diff-index --quiet --cached HEAD --
+then
+    gettextln "Error: Your local changes to the following files would be overwritten by merge"
+    git diff-index --cached --name-only HEAD -- | sed -e 's/^/    /'
+    exit 2
+fi
+
 # The first parameters up to -- are merge bases; the rest are heads.
 bases= head= remotes= sep_seen=
 for arg

index b6e424a427b..f35d3182b86 100755
@@ -114,6 +114,32 @@ test_expect_success 'resolve, non-trivial' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
+test_expect_success 'resolve, trivial, related file removed' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	git rm a &&
+	test_path_is_missing a &&
+
+	test_must_fail git merge -s resolve C^0 &&
+
+	test_path_is_missing a &&
+	test_path_is_missing .git/MERGE_HEAD
+'
+
+test_expect_success 'resolve, non-trivial, related file removed' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	git rm a &&
+	test_path_is_missing a &&
+
+	test_must_fail git merge -s resolve D^0 &&
+
+	test_path_is_missing a &&
+	test_path_is_missing .git/MERGE_HEAD
+'
+
 test_expect_success 'recursive' '
 	git reset --hard &&
 	git checkout B^0 &&

gitgitgadget

From: Elijah Newren <[email protected]>

builtin/merge is setup to allow multiple strategies to be specified, and it will find the "best" result and use it. This is defeated if some of the merge strategies abort early when they cannot handle the merge. Fix the logic that calls recursive and ort to not do such an early abort, but instead return "2" or "unhandled" so that the next strategy can try to handle the merge.

Coming up with a testcase for this is somewhat difficult, since recursive and ort both handle nearly any two-headed merge (there is a separate code path that checks for non-two-headed merges and already returns "2" for them). So use a somewhat synthetic testcase of having the index not match HEAD before the merge starts, since all merge strategies will abort for that.

---
 builtin/merge.c                          |  6 ++++--
 t/t6402-merge-rename.sh                  |  2 +-
 t/t6424-merge-unrelated-index-changes.sh | 16 ++++++++++++++++
 t/t6439-merge-co-error-msgs.sh           |  1 +
 4 files changed, 22 insertions(+), 3 deletions(-)

index 13884b8e836..dec7375bf2a 100644
@@ -754,8 +754,10 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		else
 			clean = merge_recursive(&o, head, remoteheads->item,
 						reversed, &result);
-		if (clean < 0)
-			exit(128);
+		if (clean < 0) {
+			rollback_lock_file(&lock);
+			return 2;
+		}
 		if (write_locked_index(&the_index, &lock,
 				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 			die(_("unable to write %s"), get_index_file());

index 3a32b1a45cf..772238e582c 100755
@@ -210,7 +210,7 @@ test_expect_success 'updated working tree file should prevent the merge' '
 	echo >>M one line addition &&
 	cat M >M.saved &&
 	git update-index M &&
-	test_expect_code 128 git pull --no-rebase . yellow &&
+	test_expect_code 2 git pull --no-rebase . yellow &&
 	test_cmp M M.saved &&
 	rm -f M.saved
 '

index f35d3182b86..8b749e19083 100755
@@ -268,4 +268,20 @@ test_expect_success 'subtree' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
+test_expect_success 'resolve && recursive && ort' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	test_seq 0 10 >a &&
+	git add a &&
+
+	sane_unset GIT_TEST_MERGE_ALGORITHM &&
+	test_must_fail git merge -s resolve -s recursive -s ort C^0 >output 2>&1 &&
+
+	grep "Trying merge strategy resolve..." output &&
+	grep "Trying merge strategy recursive..." output &&
+	grep "Trying merge strategy ort..." output &&
+	grep "No merge strategy handled the merge." output
+'
+
 test_done

index 5bfb027099a..52cf0c87690 100755
@@ -47,6 +47,7 @@ test_expect_success 'untracked files overwritten by merge (fast and non-fast for
 		export GIT_MERGE_VERBOSITY &&
 		test_must_fail git merge branch 2>out2
 	) &&
+	echo "Merge with strategy ${GIT_TEST_MERGE_ALGORITHM:-ort} failed." >>expect &&
 	test_cmp out2 expect &&
 	git reset --hard HEAD^
 '

gitgitgadget

From: Elijah Newren <[email protected]>

When there are stat-dirty files, but no files are modified, git stash create exits with unsuccessful status. This causes merge to fail. Copy some code from sequencer.c's create_autostash to refresh the index first to avoid this problem.

---
 builtin/merge.c                          |  8 ++++++++
 t/t6424-merge-unrelated-index-changes.sh | 11 +++++++++++
 2 files changed, 19 insertions(+)

index dec7375bf2a..4170c30317e 100644
@@ -313,8 +313,16 @@ static int save_state(struct object_id *stash)
 	int len;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buffer = STRBUF_INIT;
+	struct lock_file lock_file = LOCK_INIT;
+	int fd;
 	int rc = -1;
 
+	fd = repo_hold_locked_index(the_repository, &lock_file, 0);
+	refresh_cache(REFRESH_QUIET);
+	if (0 <= fd)
+		repo_update_index_if_able(the_repository, &lock_file);
+	rollback_lock_file(&lock_file);
+
 	strvec_pushl(&cp.args, "stash", "create", NULL);
 	cp.out = -1;
 	cp.git_cmd = 1;

index 8b749e19083..3019d030e07 100755
@@ -268,6 +268,17 @@ test_expect_success 'subtree' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
+test_expect_success 'avoid failure due to stat-dirty files' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	# Make "a" be stat-dirty
+	test-tool chmtime =+1 a &&
+
+	# stat-dirty file should not prevent stash creation in builtin/merge.c
+	git merge -s resolve -s recursive D^0
+'
+
 test_expect_success 'resolve && recursive && ort' '
 	git reset --hard &&
 	git checkout B^0 &&

gitgitgadget

From: Elijah Newren <[email protected]>

Merge strategies can:
* succeed with a clean merge
* succeed with a conflicted merge
* fail to handle the given type of merge

If one is thinking in terms of automatic mergeability, they would use the word "fail" instead of "succeed" for the second bullet, but I am focusing here on ability of the merge strategy to handle the given inputs, not on whether the given inputs are mergeable. The third category is about the merge strategy failing to know how to handle the given data; examples include:

* Passing more than 2 branches to 'recursive' or 'ort'
* Passing 2 or fewer branches to 'octopus'
* Trying to do more complicated merges with 'resolve' (I believe directory/file conflicts will cause it to bail.)
* Octopus running into a merge conflict for any branch OTHER than the final one (see the "exit 2" codepath of commit 98efc8f3d8 ("octopus: allow manual resolve on the last round.", 2006-01-13))

That final one is particularly interesting, because it shows that the merge strategy can muck with the index and working tree, and THEN bail and say "sorry, this strategy cannot handle this type of merge; use something else".

Further, we do not currently expect the individual strategies to clean up after themselves, but instead expect builtin/merge.c to do so. For it to be able to, it needs to save the state before trying the merge strategy so it can have something to restore to. Therefore, remove the shortcut bypassing the save_state() call.

There is another bug on the restore_state() side of things, so no testcase will be added until the next commit when we have addressed that issue as well.

---
 builtin/merge.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index f807bf335bd..11bb4bab0a1 100644
@@ -1686,12 +1686,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * tree in the index -- this means that the index must be in
 	 * sync with the head commit.  The strategies are responsible
 	 * to ensure this.
+	 *
+	 * Stash away the local changes so that we can try more than one
+	 * and/or recover from merge strategies bailing while leaving the
+	 * index and working tree polluted.
 	 */
-	if (use_strategies_nr == 1 ||
-	    /*
-	     * Stash away the local changes so that we can try more than one.
-	     */
-	    save_state(&stash))
+	if (save_state(&stash))
 		oidclr(&stash);
 
 	for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {

gitgitgadget

From: Elijah Newren <[email protected]>

There are multiple issues at play here:

1) If git merge is invoked with staged changes, it should abort without doing any merging, and the user's working tree and index should be the same as before merge was invoked.
2) Merge strategies are responsible for enforcing the index == HEAD requirement. (See 9822175d2b ("Ensure index matches head before invoking merge machinery, round N", 2019-08-17) for some history around this.)
3) Merge strategies can bail saying they are not an appropriate handler for the merge in question (possibly allowing other strategies to be used instead).
4) Merge strategies can make changes to the index and working tree, and have no expectation to clean up after themselves, *even* if they bail out and say they are not an appropriate handler for the merge in question. (The octopus merge strategy does this, for example.)
5) Because of (3) and (4), builtin/merge.c stashes state before trying merge strategies and restores it afterward.

Unfortunately, if users had staged changes before calling git merge, builtin/merge.c could do the following:

   * stash the changes, in order to clean up after the strategies
   * try all the merge strategies in turn, each of which report they cannot function due to the index not matching HEAD
   * restore the changes via "git stash apply"

But that last step would have the net effect of unstaging the user's changes. Fix this by adding the "--index" option to "git stash apply". While at it, also squelch the stash apply output; we already report "Rewinding the tree to pristine..." and don't need a detailed git status report afterwards.

---
 builtin/merge.c                          | 5 +++--
 t/t6424-merge-unrelated-index-changes.sh | 7 ++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

index 4170c30317e..f807bf335bd 100644
@@ -383,14 +383,15 @@ static void reset_hard(const struct object_id *oid, int verbose)
 static void restore_state(const struct object_id *head,
 			  const struct object_id *stash)
 {
-	const char *args[] = { "stash", "apply", NULL, NULL };
+	const char *args[] = { "stash", "apply", "--index", "--quiet",
+			       NULL, NULL };
 
 	if (is_null_oid(stash))
 		return;
 
 	reset_hard(head, 1);
 
-	args[2] = oid_to_hex(stash);
+	args[4] = oid_to_hex(stash);
 
 	/*
 	 * It is OK to ignore error here, for example when there was

index 3019d030e07..c96649448fa 100755
@@ -285,6 +285,7 @@ test_expect_success 'resolve && recursive && ort' '
 
 	test_seq 0 10 >a &&
 	git add a &&
+	git rev-parse :a >expect &&
 
 	sane_unset GIT_TEST_MERGE_ALGORITHM &&
 	test_must_fail git merge -s resolve -s recursive -s ort C^0 >output 2>&1 &&

@@ -292,7 +293,11 @@ test_expect_success 'resolve && recursive && ort' '
 	grep "Trying merge strategy resolve..." output &&
 	grep "Trying merge strategy recursive..." output &&
 	grep "Trying merge strategy ort..." output &&
-	grep "No merge strategy handled the merge." output
+	grep "No merge strategy handled the merge." output &&
+
+	# Changes to "a" should remain staged
+	git rev-parse :a >actual &&
+	test_cmp expect actual
 '
 
 test_done

gitgitgadget

From: Elijah Newren <[email protected]>

Previously, if the user:

* Had no local changes before starting the merge
* A merge strategy makes changes to the working tree/index but returns with exit status 2

Then we'd call restore_state() to clean up the changes and either let the next merge strategy run (if there is one), or exit telling the user that no merge strategy could handle the merge. Unfortunately, restore_state() did not clean up the changes as expected; that function was a no-op if the stash was a null, and the stash would be null if there were no local changes before starting the merge. So, instead of "Rewinding the tree to pristine..." as the code claimed, restore_state() would leave garbage around in the index and working tree (possibly including conflicts) for either the next merge strategy or for the user after aborting the merge. And in the case of aborting the merge, the user would be unable to run "git merge --abort" to get rid of the unintended leftover conflicts, because the merge control files were not written as it was presumed that we had restored to a clean state already.

Fix the main problem by making sure that restore_state() only skips the stash application if the stash is null rather than skipping the whole function.

However, there is a secondary problem -- since merge.c forks subprocesses to do the cleanup, the in-memory index is left out-of-sync. While there was a refresh_cache(REFRESH_QUIET) call that attempted to correct that, that function would not handle cases where the previous merge strategy added conflicted entries. We need to drop the index and re-read it to handle such cases.

(Alternatively, we could stop forking subprocesses and instead call some appropriate function to do the work which would update the in-memory index automatically. For now, just do the simple fix.)

Also, add a testcase checking this, one for which the octopus strategy fails on the first commit it attempts to merge, and thus which it cannot handle at all and must completely bail on (as per the "exit 2" code path of commit 98efc8f3d8 ("octopus: allow manual resolve on the last round.", 2006-01-13)).

---
 builtin/merge.c        | 10 ++++++----
 t/t7607-merge-state.sh | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 4 deletions(-)
 create mode 100755 t/t7607-merge-state.sh

index 11bb4bab0a1..7fb4414ebb7 100644
@@ -386,11 +386,11 @@ static void restore_state(const struct object_id *head,
 	const char *args[] = { "stash", "apply", "--index", "--quiet",
 			       NULL, NULL };
 
-	if (is_null_oid(stash))
-		return;
-
 	reset_hard(head, 1);
 
+	if (is_null_oid(stash))
+		goto refresh_cache;
+
 	args[4] = oid_to_hex(stash);
 
 	/*

@@ -399,7 +399,9 @@ static void restore_state(const struct object_id *head,
 	 */
 	run_command_v_opt(args, RUN_GIT_CMD);
 
-	refresh_cache(REFRESH_QUIET);
+refresh_cache:
+	if (discard_cache() < 0 || read_cache() < 0)
+		die(_("could not read index"));
 }
 
 /* This is called when no merge was necessary. */

new file mode 100755
index 00000000000..fc33d57357b
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description="Test that merge state is as expected after failed merge"
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+. ./test-lib.sh
+
+test_expect_success 'set up custom strategy' '
+	test_commit --no-tag "Initial" base base &&
+
+	for b in branch1 branch2 branch3
+	do
+		git checkout -b $b main &&
+		test_commit --no-tag "Change on $b" base $b || return 1
+	done &&
+
+	git checkout branch1 &&
+	# This is a merge that octopus cannot handle.  Note, that it does not
+	# just hit conflicts, it completely fails and says that it cannot
+	# handle this type of merge.
+	test_expect_code 2 git merge branch2 branch3 >output 2>&1 &&
+	grep "fatal: merge program failed" output &&
+	grep "Should not be doing an octopus" output &&
+
+	# Make sure we did not leave stray changes around when no appropriate
+	# merge strategy was found
+	git diff --exit-code --name-status &&
+	test_path_is_missing .git/MERGE_HEAD
+'
+
+test_done

gitgitgadget

I personally consider this is a bug in octopus (and I can do so without offending anybody, as octopus was what I did), but because the point of having pluggable merge strategies is to allow end users and third parties write their own. So save-and-restore dance would be a prudent approach to the issue than forbidding this "buggy" behaviour.

A tangent that does not make much difference in the end, but I am finding these lines curious and somewhat annoying. Why do we have &nbsp; sandwitching the plain whitespace only on lines that begin with an asterisk?

Ah, that would be nicer; thanks for the pointer. I would still need to prefix it with an

    strbuf_addch(&sb, '\n');

but two lines certainly beats six.

Eek! That's nasty. I was switching back and forth between responding to emails and coding up fixes, and copy-pasted part of my email into the commit message. So, apparently gmail hates me, and I just didn't notice.

I'll clean these up; I'm rerolling for the "error()" cleanup you mentioned anyway.

Your "strbuf" version uses the same format string as my error() thing and then manually add one LF at the end, before sending it to fputs(), which, unlike puts() does not add any extra LF at the end.

error() gives a terminating newline at the end.

Do you still need to add one more?

Ah, sorry, my mistake. I somehow thought error() just added the "error: " prefix. So, indeed, this is just a change from fprintf(stderr, ...) to error(...). No second newline needed.

Thanks!

From: Elijah Newren <[email protected]>

When the index does not match HEAD, the merge strategies are responsible to detect that condition and abort. The merge-ort-wrappers had code to implement this and meant to copy the error message from merge-recursive but deviated in two ways, both due to the message in merge-recursive being processed by another function that made additional changes:
* It added an implicit "error: " prefix
* It added an implicit trailing newline
We can get these things by making use of the error() function.

---
 merge-ort-wrappers.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index ad041061695..748924a69ba 100644
@@ -10,8 +10,8 @@ static int unclean(struct merge_options *opt, struct tree *head)
 	struct strbuf sb = STRBUF_INIT;
 
 	if (head && repo_index_has_changes(opt->repo, head, &sb)) {
-		fprintf(stderr, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
-		    sb.buf);
+		error(_("Your local changes to the following files would be overwritten by merge:\n  %s"),
+		      sb.buf);
 		strbuf_release(&sb);
 		return -1;
 	}

gitgitgadget

This started as a simple series to fix restore_state() in builtin/merge.c, fixing an issue reported by ZheNing Hu[3]. It's grown so much it's hard to call it simple. Anyway...

Changes since v3:

* Removed some accidental &nbsp; characters from a commit message
* Made use of the error() function to simplify the first patch

[1]
https://lore.kernel.org/git/CAOLTT8R7QmpvaFPTRs3xTpxr7eiuxF-ZWtvUUSC0-JOo9Y+SqA@mail.gmail.com/

Elijah Newren (7):
merge-ort-wrappers: make printed message match the one from recursive
merge-resolve: abort if index does not match HEAD
merge: do not abort early if one strategy fails to handle the merge
merge: fix save_state() to work when there are stat-dirty files
merge: make restore_state() restore staged state too
merge: ensure we can actually restore pre-merge state
merge: do not exit restore_state() prematurely

 builtin/merge.c                          | 59 ++++++++++++++++++------
 git-merge-resolve.sh                     | 10 ++++
 merge-ort-wrappers.c                     |  4 +-
 t/t6402-merge-rename.sh                  |  2 +-
 t/t6424-merge-unrelated-index-changes.sh | 58 +++++++++++++++++++++++
 t/t6439-merge-co-error-msgs.sh           |  1 +
 t/t7607-merge-state.sh                   | 32 +++++++++++++
 7 files changed, 150 insertions(+), 16 deletions(-)
 create mode 100755 t/t7607-merge-state.sh

base-commit: e72d93e88cb20b06e88e6e7d81bd1dc4effe453f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1231%2Fnewren%2Ffix-merge-restore-state-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1231/newren/fix-merge-restore-state-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1231

Range-diff vs v3:

1: e39b2e15ece ! 1: bd36d16c8d9 merge-ort-wrappers: make printed message match the one from recursive
@@ Commit message
being processed by another function that made additional changes:
* It added an implicit "error: " prefix
* It added an implicit trailing newline
-
- Add these things, but do so in a couple extra steps to avoid having
- translators need to translate another not-quite-identical string.
+ We can get these things by making use of the error() function.

@@ merge-ort-wrappers.c: static int unclean(struct merge_options *opt, struct tree

if (head && repo_index_has_changes(opt->repo, head, &sb)) {
- fprintf(stderr, _("Your local changes to the following files would be overwritten by merge:\n %s"),
-+ struct strbuf err = STRBUF_INIT;
-+ strbuf_addstr(&err, "error: ");
-+ strbuf_addf(&err, _("Your local changes to the following files would be overwritten by merge:\n %s"),
- sb.buf);
-+ strbuf_addch(&err, '\n');
-+ fputs(err.buf, stderr);
-+ strbuf_release(&err);
+- sb.buf);
++ error(_("Your local changes to the following files would be overwritten by merge:\n %s"),
++ sb.buf);
strbuf_release(&sb);
return -1;
}

 2:  2810dec7608 = 2:  b79f44e54b9 merge-resolve: abort if index does not match HEAD
 3:  b41853e3f99 = 3:  02930448ea1 merge: do not abort early if one strategy fails to handle the merge
 4:  64700338a28 = 4:  daf8d224160 merge: fix save_state() to work when there are stat-dirty files
 5:  91c495c770e ! 5:  f401bd5ad0d merge: make restore_state() restore staged state too

@@ Commit message
Unfortunately, if users had staged changes before calling git merge, builtin/merge.c could do the following:

-    * stash the changes, in order to clean up after the strategies
-    * try all the merge strategies in turn, each of which report they
+ * stash the changes, in order to clean up after the strategies
+ * try all the merge strategies in turn, each of which report they
cannot function due to the index not matching HEAD
-    * restore the changes via "git stash apply"
+ * restore the changes via "git stash apply"

But that last step would have the net effect of unstaging the user's changes. Fix this by adding the "--index" option to "git stash apply".

 6:  887967c1f3f = 6:  ad5354c219c merge: ensure we can actually restore pre-merge state
 7:  81c40492a62 = 7:  6212d572604 merge: do not exit restore_state() prematurely

gitgitgadget

From: Elijah Newren <[email protected]>

As noted in commit 9822175d2b ("Ensure index matches head before invoking merge machinery, round N", 2019-08-17), we have had a very long history of problems with failing to enforce the requirement that index matches HEAD when starting a merge. One of the commits referenced in the long tale of issues arising from lax enforcement of this requirement was commit 55f39cf755 ("merge: fix misleading pre-merge check documentation", 2018-06-30), which tried to document the requirement and noted there were some exceptions. As mentioned in that commit message, the resolve strategy was the one strategy that did not have an explicit index matching HEAD check, and the reason it didn't was that I wasn't able to discover any cases where the implementation would fail to catch the problem and abort, and didn't want to introduce unnecessary performance overhead of adding another check.

Well, today I discovered a testcase where the implementation does not catch the problem and so an explicit check is needed. Add a testcase that previously would have failed, and update git-merge-resolve.sh to have an explicit check. Note that the code is copied from 3ec62ad9ff ("merge-octopus: abort if index does not match HEAD", 2016-04-09), so that we reuse the same message and avoid making translators need to translate some new message.

---
 builtin/merge.c                          | 20 ++++++++++++++++++
 git-merge-resolve.sh                     | 10 +++++++++
 t/t6424-merge-unrelated-index-changes.sh | 26 ++++++++++++++++++++++++
 3 files changed, 56 insertions(+)

index 23170f2d2a6..13884b8e836 100644
@@ -1599,6 +1599,26 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 */
 		refresh_cache(REFRESH_QUIET);
 		if (allow_trivial && fast_forward != FF_ONLY) {
+			/*
+			 * Must first ensure that index matches HEAD before
+			 * attempting a trivial merge.
+			 */
+			struct tree *head_tree = get_commit_tree(head_commit);
+			struct strbuf sb = STRBUF_INIT;
+
+			if (repo_index_has_changes(the_repository, head_tree,
+						   &sb)) {
+				struct strbuf err = STRBUF_INIT;
+				strbuf_addstr(&err, "error: ");
+				strbuf_addf(&err, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
+					    sb.buf);
+				strbuf_addch(&err, '\n');
+				fputs(err.buf, stderr);
+				strbuf_release(&err);
+				strbuf_release(&sb);
+				return -1;
+			}
+
 			/* See if it is really trivial. */
 			git_committer_info(IDENT_STRICT);
 			printf(_("Trying really trivial in-index merge...\n"));

index 343fe7bccd0..77e93121bf8 100755
@@ -5,6 +5,16 @@
 #
 # Resolve two trees, using enhanced multi-base read-tree.
 
+. git-sh-setup
+
+# Abort if index does not match HEAD
+if ! git diff-index --quiet --cached HEAD --
+then
+    gettextln "Error: Your local changes to the following files would be overwritten by merge"
+    git diff-index --cached --name-only HEAD -- | sed -e 's/^/    /'
+    exit 2
+fi
+
 # The first parameters up to -- are merge bases; the rest are heads.
 bases= head= remotes= sep_seen=
 for arg

index b6e424a427b..f35d3182b86 100755
@@ -114,6 +114,32 @@ test_expect_success 'resolve, non-trivial' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
+test_expect_success 'resolve, trivial, related file removed' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	git rm a &&
+	test_path_is_missing a &&
+
+	test_must_fail git merge -s resolve C^0 &&
+
+	test_path_is_missing a &&
+	test_path_is_missing .git/MERGE_HEAD
+'
+
+test_expect_success 'resolve, non-trivial, related file removed' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	git rm a &&
+	test_path_is_missing a &&
+
+	test_must_fail git merge -s resolve D^0 &&
+
+	test_path_is_missing a &&
+	test_path_is_missing .git/MERGE_HEAD
+'
+
 test_expect_success 'recursive' '
 	git reset --hard &&
 	git checkout B^0 &&

gitgitgadget

From: Elijah Newren <[email protected]>

builtin/merge is setup to allow multiple strategies to be specified, and it will find the "best" result and use it. This is defeated if some of the merge strategies abort early when they cannot handle the merge. Fix the logic that calls recursive and ort to not do such an early abort, but instead return "2" or "unhandled" so that the next strategy can try to handle the merge.

Coming up with a testcase for this is somewhat difficult, since recursive and ort both handle nearly any two-headed merge (there is a separate code path that checks for non-two-headed merges and already returns "2" for them). So use a somewhat synthetic testcase of having the index not match HEAD before the merge starts, since all merge strategies will abort for that.

---
 builtin/merge.c                          |  6 ++++--
 t/t6402-merge-rename.sh                  |  2 +-
 t/t6424-merge-unrelated-index-changes.sh | 16 ++++++++++++++++
 t/t6439-merge-co-error-msgs.sh           |  1 +
 4 files changed, 22 insertions(+), 3 deletions(-)

index 13884b8e836..dec7375bf2a 100644
@@ -754,8 +754,10 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		else
 			clean = merge_recursive(&o, head, remoteheads->item,
 						reversed, &result);
-		if (clean < 0)
-			exit(128);
+		if (clean < 0) {
+			rollback_lock_file(&lock);
+			return 2;
+		}
 		if (write_locked_index(&the_index, &lock,
 				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 			die(_("unable to write %s"), get_index_file());

index 3a32b1a45cf..772238e582c 100755
@@ -210,7 +210,7 @@ test_expect_success 'updated working tree file should prevent the merge' '
 	echo >>M one line addition &&
 	cat M >M.saved &&
 	git update-index M &&
-	test_expect_code 128 git pull --no-rebase . yellow &&
+	test_expect_code 2 git pull --no-rebase . yellow &&
 	test_cmp M M.saved &&
 	rm -f M.saved
 '

index f35d3182b86..8b749e19083 100755
@@ -268,4 +268,20 @@ test_expect_success 'subtree' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
+test_expect_success 'resolve && recursive && ort' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	test_seq 0 10 >a &&
+	git add a &&
+
+	sane_unset GIT_TEST_MERGE_ALGORITHM &&
+	test_must_fail git merge -s resolve -s recursive -s ort C^0 >output 2>&1 &&
+
+	grep "Trying merge strategy resolve..." output &&
+	grep "Trying merge strategy recursive..." output &&
+	grep "Trying merge strategy ort..." output &&
+	grep "No merge strategy handled the merge." output
+'
+
 test_done

index 5bfb027099a..52cf0c87690 100755
@@ -47,6 +47,7 @@ test_expect_success 'untracked files overwritten by merge (fast and non-fast for
 		export GIT_MERGE_VERBOSITY &&
 		test_must_fail git merge branch 2>out2
 	) &&
+	echo "Merge with strategy ${GIT_TEST_MERGE_ALGORITHM:-ort} failed." >>expect &&
 	test_cmp out2 expect &&
 	git reset --hard HEAD^
 '

gitgitgadget

From: Elijah Newren <[email protected]>

When there are stat-dirty files, but no files are modified, git stash create exits with unsuccessful status. This causes merge to fail. Copy some code from sequencer.c's create_autostash to refresh the index first to avoid this problem.

---
 builtin/merge.c                          |  8 ++++++++
 t/t6424-merge-unrelated-index-changes.sh | 11 +++++++++++
 2 files changed, 19 insertions(+)

index dec7375bf2a..4170c30317e 100644
@@ -313,8 +313,16 @@ static int save_state(struct object_id *stash)
 	int len;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buffer = STRBUF_INIT;
+	struct lock_file lock_file = LOCK_INIT;
+	int fd;
 	int rc = -1;
 
+	fd = repo_hold_locked_index(the_repository, &lock_file, 0);
+	refresh_cache(REFRESH_QUIET);
+	if (0 <= fd)
+		repo_update_index_if_able(the_repository, &lock_file);
+	rollback_lock_file(&lock_file);
+
 	strvec_pushl(&cp.args, "stash", "create", NULL);
 	cp.out = -1;
 	cp.git_cmd = 1;

index 8b749e19083..3019d030e07 100755
@@ -268,6 +268,17 @@ test_expect_success 'subtree' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
+test_expect_success 'avoid failure due to stat-dirty files' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	# Make "a" be stat-dirty
+	test-tool chmtime =+1 a &&
+
+	# stat-dirty file should not prevent stash creation in builtin/merge.c
+	git merge -s resolve -s recursive D^0
+'
+
 test_expect_success 'resolve && recursive && ort' '
 	git reset --hard &&
 	git checkout B^0 &&

gitgitgadget

From: Elijah Newren <[email protected]>

There are multiple issues at play here:

1) If git merge is invoked with staged changes, it should abort without doing any merging, and the user's working tree and index should be the same as before merge was invoked.
2) Merge strategies are responsible for enforcing the index == HEAD requirement. (See 9822175d2b ("Ensure index matches head before invoking merge machinery, round N", 2019-08-17) for some history around this.)
3) Merge strategies can bail saying they are not an appropriate handler for the merge in question (possibly allowing other strategies to be used instead).
4) Merge strategies can make changes to the index and working tree, and have no expectation to clean up after themselves, *even* if they bail out and say they are not an appropriate handler for the merge in question. (The octopus merge strategy does this, for example.)
5) Because of (3) and (4), builtin/merge.c stashes state before trying merge strategies and restores it afterward.

Unfortunately, if users had staged changes before calling git merge, builtin/merge.c could do the following:

* stash the changes, in order to clean up after the strategies
* try all the merge strategies in turn, each of which report they cannot function due to the index not matching HEAD
* restore the changes via "git stash apply"

But that last step would have the net effect of unstaging the user's changes. Fix this by adding the "--index" option to "git stash apply". While at it, also squelch the stash apply output; we already report "Rewinding the tree to pristine..." and don't need a detailed git status report afterwards.

---
 builtin/merge.c                          | 5 +++--
 t/t6424-merge-unrelated-index-changes.sh | 7 ++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

index 4170c30317e..f807bf335bd 100644
@@ -383,14 +383,15 @@ static void reset_hard(const struct object_id *oid, int verbose)
 static void restore_state(const struct object_id *head,
 			  const struct object_id *stash)
 {
-	const char *args[] = { "stash", "apply", NULL, NULL };
+	const char *args[] = { "stash", "apply", "--index", "--quiet",
+			       NULL, NULL };
 
 	if (is_null_oid(stash))
 		return;
 
 	reset_hard(head, 1);
 
-	args[2] = oid_to_hex(stash);
+	args[4] = oid_to_hex(stash);
 
 	/*
 	 * It is OK to ignore error here, for example when there was

index 3019d030e07..c96649448fa 100755
@@ -285,6 +285,7 @@ test_expect_success 'resolve && recursive && ort' '
 
 	test_seq 0 10 >a &&
 	git add a &&
+	git rev-parse :a >expect &&
 
 	sane_unset GIT_TEST_MERGE_ALGORITHM &&
 	test_must_fail git merge -s resolve -s recursive -s ort C^0 >output 2>&1 &&

@@ -292,7 +293,11 @@ test_expect_success 'resolve && recursive && ort' '
 	grep "Trying merge strategy resolve..." output &&
 	grep "Trying merge strategy recursive..." output &&
 	grep "Trying merge strategy ort..." output &&
-	grep "No merge strategy handled the merge." output
+	grep "No merge strategy handled the merge." output &&
+
+	# Changes to "a" should remain staged
+	git rev-parse :a >actual &&
+	test_cmp expect actual
 '
 
 test_done

gitgitgadget

From: Elijah Newren <[email protected]>

Merge strategies can:
* succeed with a clean merge
* succeed with a conflicted merge
* fail to handle the given type of merge

If one is thinking in terms of automatic mergeability, they would use the word "fail" instead of "succeed" for the second bullet, but I am focusing here on ability of the merge strategy to handle the given inputs, not on whether the given inputs are mergeable. The third category is about the merge strategy failing to know how to handle the given data; examples include:

* Passing more than 2 branches to 'recursive' or 'ort'
* Passing 2 or fewer branches to 'octopus'
* Trying to do more complicated merges with 'resolve' (I believe directory/file conflicts will cause it to bail.)
* Octopus running into a merge conflict for any branch OTHER than the final one (see the "exit 2" codepath of commit 98efc8f3d8 ("octopus: allow manual resolve on the last round.", 2006-01-13))

That final one is particularly interesting, because it shows that the merge strategy can muck with the index and working tree, and THEN bail and say "sorry, this strategy cannot handle this type of merge; use something else".

Further, we do not currently expect the individual strategies to clean up after themselves, but instead expect builtin/merge.c to do so. For it to be able to, it needs to save the state before trying the merge strategy so it can have something to restore to. Therefore, remove the shortcut bypassing the save_state() call.

There is another bug on the restore_state() side of things, so no testcase will be added until the next commit when we have addressed that issue as well.

---
 builtin/merge.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index f807bf335bd..11bb4bab0a1 100644
@@ -1686,12 +1686,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * tree in the index -- this means that the index must be in
 	 * sync with the head commit.  The strategies are responsible
 	 * to ensure this.
+	 *
+	 * Stash away the local changes so that we can try more than one
+	 * and/or recover from merge strategies bailing while leaving the
+	 * index and working tree polluted.
 	 */
-	if (use_strategies_nr == 1 ||
-	    /*
-	     * Stash away the local changes so that we can try more than one.
-	     */
-	    save_state(&stash))
+	if (save_state(&stash))
 		oidclr(&stash);
 
 	for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {

gitgitgadget

From: Elijah Newren <[email protected]>

Previously, if the user:

* Had no local changes before starting the merge
* A merge strategy makes changes to the working tree/index but returns with exit status 2

Then we'd call restore_state() to clean up the changes and either let the next merge strategy run (if there is one), or exit telling the user that no merge strategy could handle the merge. Unfortunately, restore_state() did not clean up the changes as expected; that function was a no-op if the stash was a null, and the stash would be null if there were no local changes before starting the merge. So, instead of "Rewinding the tree to pristine..." as the code claimed, restore_state() would leave garbage around in the index and working tree (possibly including conflicts) for either the next merge strategy or for the user after aborting the merge. And in the case of aborting the merge, the user would be unable to run "git merge --abort" to get rid of the unintended leftover conflicts, because the merge control files were not written as it was presumed that we had restored to a clean state already.

Fix the main problem by making sure that restore_state() only skips the stash application if the stash is null rather than skipping the whole function.

However, there is a secondary problem -- since merge.c forks subprocesses to do the cleanup, the in-memory index is left out-of-sync. While there was a refresh_cache(REFRESH_QUIET) call that attempted to correct that, that function would not handle cases where the previous merge strategy added conflicted entries. We need to drop the index and re-read it to handle such cases.

(Alternatively, we could stop forking subprocesses and instead call some appropriate function to do the work which would update the in-memory index automatically. For now, just do the simple fix.)

Also, add a testcase checking this, one for which the octopus strategy fails on the first commit it attempts to merge, and thus which it cannot handle at all and must completely bail on (as per the "exit 2" code path of commit 98efc8f3d8 ("octopus: allow manual resolve on the last round.", 2006-01-13)).

---
 builtin/merge.c        | 10 ++++++----
 t/t7607-merge-state.sh | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 4 deletions(-)
 create mode 100755 t/t7607-merge-state.sh

index 11bb4bab0a1..7fb4414ebb7 100644
@@ -386,11 +386,11 @@ static void restore_state(const struct object_id *head,
 	const char *args[] = { "stash", "apply", "--index", "--quiet",
 			       NULL, NULL };
 
-	if (is_null_oid(stash))
-		return;
-
 	reset_hard(head, 1);
 
+	if (is_null_oid(stash))
+		goto refresh_cache;
+
 	args[4] = oid_to_hex(stash);
 
 	/*

@@ -399,7 +399,9 @@ static void restore_state(const struct object_id *head,
 	 */
 	run_command_v_opt(args, RUN_GIT_CMD);
 
-	refresh_cache(REFRESH_QUIET);
+refresh_cache:
+	if (discard_cache() < 0 || read_cache() < 0)
+		die(_("could not read index"));
 }
 
 /* This is called when no merge was necessary. */

new file mode 100755
index 00000000000..fc33d57357b
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description="Test that merge state is as expected after failed merge"
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+. ./test-lib.sh
+
+test_expect_success 'set up custom strategy' '
+	test_commit --no-tag "Initial" base base &&
+
+	for b in branch1 branch2 branch3
+	do
+		git checkout -b $b main &&
+		test_commit --no-tag "Change on $b" base $b || return 1
+	done &&
+
+	git checkout branch1 &&
+	# This is a merge that octopus cannot handle.  Note, that it does not
+	# just hit conflicts, it completely fails and says that it cannot
+	# handle this type of merge.
+	test_expect_code 2 git merge branch2 branch3 >output 2>&1 &&
+	grep "fatal: merge program failed" output &&
+	grep "Should not be doing an octopus" output &&
+
+	# Make sure we did not leave stray changes around when no appropriate
+	# merge strategy was found
+	git diff --exit-code --name-status &&
+	test_path_is_missing .git/MERGE_HEAD
+'
+
+test_done

gitgitgadget

UGH! I fixed the other one of these in my reroll yesterday[1]. I _knew_ I had copied that code somewhere else, but for some reason I thought it was in a different series and went searching for it. Don't know why I couldn't remember that it was in the same series, and I'm not sure how I missed it when I went looking. I mean, I know I was tired yesterday, but that's still kinda bad.

Anyway, thanks for catching; I'll fix this one too.

[1] https://lore.kernel.org/git/[email protected]/

Nope it's not intentional. I had tested one fix (for these git-merge-resolve.sh changes) and verified they were good (and necessary), then found another bug (the one fixed by the builtin/merge.c changes) and added a test for it, and just decided to amend it into the same commit. Turns out the builtin/merge.c changes mask the fix for git-merge-resolve.sh here and makes that code go unexercised. I'll split these two bugfixes into separate patches, and tweak one of the two testcases to make sure it continues to exercise the new codepath added to git-merge-resolve.sh.

This seems like a curious objection. "We are trying to get rid of shell scripts, so don't even fix bugs in any of the existing ones." ?

Ah, I can kind of see where you're coming from now, but that seems to me to be bending over backwards in attempting to fix a component written in shell without actually modifying the shell. builtin/merge.c is some glue code that can call multiple different strategies, but isn't the place for the implementation of the strategies themselves, and I'd hate to see us put half the implementation in one place and half in another. In addition, besides the separation of concerns issue::

* We document that users can add their own merge strategies (a
shell or executable named git-merge-$USERNAME and "git merge -s $USERNAME" will call them)
* git-merge-resolve and git-merge-octopus serve as examples
* Our examples should demonstrate correct behavior and perform
documented, required steps. This particular check is important:

/*
* At this point, we need a real merge. No matter what strategy
* we use, it would operate on the index, possibly affecting the
* working tree, and when resolved cleanly, have the desired
* tree in the index -- this means that the index must be in
* sync with the head commit. The strategies are responsible
* to ensure this.
*/

So, even if someone were to reimplement git-merge-resolve.sh in C, and start the deprecation process with some merge.useBuiltinResolve config setting (similar to rebase.useBuiltin), I'd still want this shell fix added to git-merge-resolve.sh in the meantime, both as an important bugfix, and so that people looking for merge strategy examples who find this script hopefully find a new enough version with this important check included.

In general, if merge strategies do not perform this check, we have observed that they often will either (a) discard users' staged changes (best case) or (b) smash staged changes into the created commit and thus create some kind of evil merge (making it look like they created a merge normally, and then amended the merge with additional changes).

We're lucky that the way resolve was implemented, other git calls would usually incidentally catch such issues for us without an explicit check. We were also lucky that the observed behavior was '(a)' rather than '(b)' for resolve. But the issue should still be fixed.

Yes, that's fair, but as I mentioned in the commit message, it was copied from git-merge-octopus.sh. So, as you say below, "so did the older version".

Um, I don't understand this objection. I think you are referring to the pipe to sed, but if so...who cares? The exit code would be lost anyway because we aren't running under errexit, and the next line of code ignores any and all previous exit codes when it runs "exit 2". And if you're not referring to the pipe to sed but the fact that it unconditionally returns an exit code of 2 on the next line, then yes that is the expected return code. Whatever the diff-index segfault returns would be the wrong exit status and could fool the builtin/merge.c into doing the wrong thing. It expects merge strategies to return one of three exit codes: 0, 1, or 2:

/*
* The backend exits with 1 when conflicts are
* left to be resolved, with 2 when it does not
* handle the given merge at all.
*/

So, ignoring the return code from diff-index is correct behavior here.

Were you thinking this was a test script or something?

??

Copying a few lines from git-merge-octopus.sh to get the same fix it has is "bending over backwards"? That's what I call "doing the easiest thing possible" (and which _also_ has the benefit of being battle tested code), and then you describe a bunch of gymnastics as an alternative? I see your suggestion as running afoul of the objection you are raising, and the code I'm adding as being a solution to that particular objection. So this particular flag you are raising is confusing to me.

Yes, this is the same change suggested by Junio for patch 1 which I should have also applied here. Thanks for catching it!

Oops, 'resolve' should really be at the end of the list rather than at the beginning. And the test description should be better.

This testcase belongs in this patch. The use of "resolve" here was totally incidental to the testcase in question; I could have used "octopus" or "ours" or created a new strategy and used it.

(Actually, using 'ours' here runs into the problem we fix in the final patch. So maybe just like 'resolve', using 'ours' might be confusing to readers of the series as they think issues from other patches are involved.)

I'll switch this test to using 'octopus' instead of 'resolve' just so it doesn't get confused in this way.

This started as a simple series to fix restore_state() in builtin/merge.c, fixing an issue reported by ZheNing Hu[3]. It now fixes several bugs and has grown so much it's hard to call it simple. Anyway...

Changes since v4:

* Made use of the error() function in another place to simplify code (should have caught this in v3)
* Split the fixes for 'resolve' and the trivial merge into separate patches, and make sure one doesn't mask the other but both codepaths are exercises in the testsuite
* better test descriptions
* use strvec to simplify some code

[1]
https://lore.kernel.org/git/CAOLTT8R7QmpvaFPTRs3xTpxr7eiuxF-ZWtvUUSC0-JOo9Y+SqA@mail.gmail.com/

Elijah Newren (8):
merge-ort-wrappers: make printed message match the one from recursive
merge-resolve: abort if index does not match HEAD
merge: abort if index does not match HEAD for trivial merges
merge: do not abort early if one strategy fails to handle the merge
merge: fix save_state() to work when there are stat-dirty files
merge: make restore_state() restore staged state too
merge: ensure we can actually restore pre-merge state
merge: do not exit restore_state() prematurely

 builtin/merge.c                          | 57 ++++++++++++++++-----
 git-merge-resolve.sh                     | 10 ++++
 merge-ort-wrappers.c                     |  4 +-
 t/t6402-merge-rename.sh                  |  2 +-
 t/t6424-merge-unrelated-index-changes.sh | 65 ++++++++++++++++++++++++
 t/t6439-merge-co-error-msgs.sh           |  1 +
 t/t7607-merge-state.sh                   | 32 ++++++++++++
 7 files changed, 154 insertions(+), 17 deletions(-)
 create mode 100755 t/t7607-merge-state.sh

base-commit: e72d93e88cb20b06e88e6e7d81bd1dc4effe453f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1231%2Fnewren%2Ffix-merge-restore-state-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1231/newren/fix-merge-restore-state-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1231

Range-diff vs v4:

 1:  bd36d16c8d9 = 1:  bd36d16c8d9 merge-ort-wrappers: make printed message match the one from recursive
 2:  b79f44e54b9 ! 2:  b656756fd37 merge-resolve: abort if index does not match HEAD
     @@ Commit message
      
          Signed-off-by: Elijah Newren <[email protected]>
      
     - ## builtin/merge.c ##
     -@@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)
     - 		 */
     - 		refresh_cache(REFRESH_QUIET);
     - 		if (allow_trivial && fast_forward != FF_ONLY) {
     -+			/*
     -+			 * Must first ensure that index matches HEAD before
     -+			 * attempting a trivial merge.
     -+			 */
     -+			struct tree *head_tree = get_commit_tree(head_commit);
     -+			struct strbuf sb = STRBUF_INIT;
     -+
     -+			if (repo_index_has_changes(the_repository, head_tree,
     -+						   &sb)) {
     -+				struct strbuf err = STRBUF_INIT;
     -+				strbuf_addstr(&err, "error: ");
     -+				strbuf_addf(&err, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
     -+					    sb.buf);
     -+				strbuf_addch(&err, '\n');
     -+				fputs(err.buf, stderr);
     -+				strbuf_release(&err);
     -+				strbuf_release(&sb);
     -+				return -1;
     -+			}
     -+
     - 			/* See if it is really trivial. */
     - 			git_committer_info(IDENT_STRICT);
     - 			printf(_("Trying really trivial in-index merge...\n"));
     -
       ## git-merge-resolve.sh ##
      @@
       #
     @@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'resolve, non-triv
       	test_path_is_missing .git/MERGE_HEAD
       '
       
     -+test_expect_success 'resolve, trivial, related file removed' '
     -+	git reset --hard &&
     -+	git checkout B^0 &&
     -+
     -+	git rm a &&
     -+	test_path_is_missing a &&
     -+
     -+	test_must_fail git merge -s resolve C^0 &&
     -+
     -+	test_path_is_missing a &&
     -+	test_path_is_missing .git/MERGE_HEAD
     -+'
     -+
      +test_expect_success 'resolve, non-trivial, related file removed' '
      +	git reset --hard &&
      +	git checkout B^0 &&
 -:  ----------- > 3:  3adfd921995 merge: abort if index does not match HEAD for trivial merges
 3:  02930448ea1 ! 4:  c5755271cf1 merge: do not abort early if one strategy fails to handle the merge
     @@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'subtree' '
       	test_path_is_missing .git/MERGE_HEAD
       '
       
     -+test_expect_success 'resolve && recursive && ort' '
     ++test_expect_success 'with multiple strategies, recursive or ort failure do not early abort' '
      +	git reset --hard &&
      +	git checkout B^0 &&
      +
     @@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'subtree' '
      +	git add a &&
      +
      +	sane_unset GIT_TEST_MERGE_ALGORITHM &&
     -+	test_must_fail git merge -s resolve -s recursive -s ort C^0 >output 2>&1 &&
     ++	test_must_fail git merge -s recursive -s ort -s octopus C^0 >output 2>&1 &&
      +
     -+	grep "Trying merge strategy resolve..." output &&
      +	grep "Trying merge strategy recursive..." output &&
      +	grep "Trying merge strategy ort..." output &&
     ++	grep "Trying merge strategy octopus..." output &&
      +	grep "No merge strategy handled the merge." output
      +'
      +
 4:  daf8d224160 ! 5:  e7c6de9e0c1 merge: fix save_state() to work when there are stat-dirty files
     @@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'subtree' '
      +	git merge -s resolve -s recursive D^0
      +'
      +
     - test_expect_success 'resolve && recursive && ort' '
     + test_expect_success 'with multiple strategies, recursive or ort failure do not early abort' '
       	git reset --hard &&
       	git checkout B^0 &&
 5:  f401bd5ad0d ! 6:  d39d6472455 merge: make restore_state() restore staged state too
     @@ Commit message
          changes.  Fix this by adding the "--index" option to "git stash apply".
          While at it, also squelch the stash apply output; we already report
          "Rewinding the tree to pristine..." and don't need a detailed `git
     -    status` report afterwards.
     +    status` report afterwards.  Also while at it, switch to using strvec
     +    so folks don't have to count the arguments to ensure we avoided an
     +    off-by-one error, and so it's easier to add additional arguments to
     +    the command.
      
          Signed-off-by: Elijah Newren <[email protected]>
      
     @@ builtin/merge.c: static void reset_hard(const struct object_id *oid, int verbose
       			  const struct object_id *stash)
       {
      -	const char *args[] = { "stash", "apply", NULL, NULL };
     -+	const char *args[] = { "stash", "apply", "--index", "--quiet",
     -+			       NULL, NULL };
     ++	struct strvec args = STRVEC_INIT;
       
       	if (is_null_oid(stash))
       		return;
     @@ builtin/merge.c: static void reset_hard(const struct object_id *oid, int verbose
       	reset_hard(head, 1);
       
      -	args[2] = oid_to_hex(stash);
     -+	args[4] = oid_to_hex(stash);
     ++	strvec_pushl(&args, "stash", "apply", "--index", "--quiet", NULL);
     ++	strvec_push(&args, oid_to_hex(stash));
       
       	/*
       	 * It is OK to ignore error here, for example when there was
     + 	 * nothing to restore.
     + 	 */
     +-	run_command_v_opt(args, RUN_GIT_CMD);
     ++	run_command_v_opt(args.v, RUN_GIT_CMD);
     ++	strvec_clear(&args);
     + 
     + 	refresh_cache(REFRESH_QUIET);
     + }
      
       ## t/t6424-merge-unrelated-index-changes.sh ##
     -@@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'resolve && recursive && ort' '
     +@@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'with multiple strategies, recursive or ort failure do not e
       
       	test_seq 0 10 >a &&
       	git add a &&
      +	git rev-parse :a >expect &&
       
       	sane_unset GIT_TEST_MERGE_ALGORITHM &&
     - 	test_must_fail git merge -s resolve -s recursive -s ort C^0 >output 2>&1 &&
     -@@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'resolve && recursive && ort' '
     - 	grep "Trying merge strategy resolve..." output &&
     + 	test_must_fail git merge -s recursive -s ort -s octopus C^0 >output 2>&1 &&
     +@@ t/t6424-merge-unrelated-index-changes.sh: test_expect_success 'with multiple strategies, recursive or ort failure do not e
       	grep "Trying merge strategy recursive..." output &&
       	grep "Trying merge strategy ort..." output &&
     + 	grep "Trying merge strategy octopus..." output &&
      -	grep "No merge strategy handled the merge." output
      +	grep "No merge strategy handled the merge." output &&
      +
 6:  ad5354c219c = 7:  7f5c6884d68 merge: ensure we can actually restore pre-merge state
 7:  6212d572604 ! 8:  954dec526a2 merge: do not exit restore_state() prematurely

@@ Commit message

## builtin/merge.c ##
@@ builtin/merge.c: static void restore_state(const struct object_id *head,
- const char *args[] = { "stash", "apply", "--index", "--quiet",
- NULL, NULL };
+ {
+ struct strvec args = STRVEC_INIT;

- if (is_null_oid(stash))
- return;
@@ builtin/merge.c: static void restore_state(const struct object_id *head,
+ if (is_null_oid(stash))
+ goto refresh_cache;
+
- args[4] = oid_to_hex(stash);
+ strvec_pushl(&args, "stash", "apply", "--index", "--quiet", NULL);
+ strvec_push(&args, oid_to_hex(stash));

- /*
@@ builtin/merge.c: static void restore_state(const struct object_id *head,
- */
- run_command_v_opt(args, RUN_GIT_CMD);
+ run_command_v_opt(args.v, RUN_GIT_CMD);
+ strvec_clear(&args);

- refresh_cache(REFRESH_QUIET);
+refresh_cache:
@@ t/t7607-merge-state.sh (new)
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+. ./test-lib.sh
+
-+test_expect_success 'set up custom strategy' '
++test_expect_success 'Ensure we restore original state if no merge strategy handles it' '
+ test_commit --no-tag "Initial" base base &&
+
+ for b in branch1 branch2 branch3

gitgitgadget

From: Elijah Newren <[email protected]>

When the index does not match HEAD, the merge strategies are responsible to detect that condition and abort. The merge-ort-wrappers had code to implement this and meant to copy the error message from merge-recursive but deviated in two ways, both due to the message in merge-recursive being processed by another function that made additional changes:
* It added an implicit "error: " prefix
* It added an implicit trailing newline
We can get these things by making use of the error() function.

---
 merge-ort-wrappers.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index ad041061695..748924a69ba 100644
@@ -10,8 +10,8 @@ static int unclean(struct merge_options *opt, struct tree *head)
 	struct strbuf sb = STRBUF_INIT;
 
 	if (head && repo_index_has_changes(opt->repo, head, &sb)) {
-		fprintf(stderr, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
-		    sb.buf);
+		error(_("Your local changes to the following files would be overwritten by merge:\n  %s"),
+		      sb.buf);
 		strbuf_release(&sb);
 		return -1;
 	}

gitgitgadget

From: Elijah Newren <[email protected]>

As noted in commit 9822175d2b ("Ensure index matches head before invoking merge machinery, round N", 2019-08-17), we have had a very long history of problems with failing to enforce the requirement that index matches HEAD when starting a merge. One of the commits referenced in the long tale of issues arising from lax enforcement of this requirement was commit 55f39cf755 ("merge: fix misleading pre-merge check documentation", 2018-06-30), which tried to document the requirement and noted there were some exceptions. As mentioned in that commit message, the resolve strategy was the one strategy that did not have an explicit index matching HEAD check, and the reason it didn't was that I wasn't able to discover any cases where the implementation would fail to catch the problem and abort, and didn't want to introduce unnecessary performance overhead of adding another check.

Well, today I discovered a testcase where the implementation does not catch the problem and so an explicit check is needed. Add a testcase that previously would have failed, and update git-merge-resolve.sh to have an explicit check. Note that the code is copied from 3ec62ad9ff ("merge-octopus: abort if index does not match HEAD", 2016-04-09), so that we reuse the same message and avoid making translators need to translate some new message.

---
 git-merge-resolve.sh                     | 10 ++++++++++
 t/t6424-merge-unrelated-index-changes.sh | 13 +++++++++++++
 2 files changed, 23 insertions(+)

index 343fe7bccd0..77e93121bf8 100755
@@ -5,6 +5,16 @@
 #
 # Resolve two trees, using enhanced multi-base read-tree.
 
+. git-sh-setup
+
+# Abort if index does not match HEAD
+if ! git diff-index --quiet --cached HEAD --
+then
+    gettextln "Error: Your local changes to the following files would be overwritten by merge"
+    git diff-index --cached --name-only HEAD -- | sed -e 's/^/    /'
+    exit 2
+fi
+
 # The first parameters up to -- are merge bases; the rest are heads.
 bases= head= remotes= sep_seen=
 for arg

index b6e424a427b..eabe6bda832 100755
@@ -114,6 +114,19 @@ test_expect_success 'resolve, non-trivial' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
+test_expect_success 'resolve, non-trivial, related file removed' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	git rm a &&
+	test_path_is_missing a &&
+
+	test_must_fail git merge -s resolve D^0 &&
+
+	test_path_is_missing a &&
+	test_path_is_missing .git/MERGE_HEAD
+'
+
 test_expect_success 'recursive' '
 	git reset --hard &&
 	git checkout B^0 &&

gitgitgadget

From: Elijah Newren <[email protected]>

As noted in the last commit and the links therein (especially commit 9822175d2b ("Ensure index matches head before invoking merge machinery, round N", 2019-08-17), we have had a very long history of problems with failing to enforce the requirement that index matches HEAD when starting a merge.

The "trivial merge" logic in builtin/merge.c is yet another such case we previously missed. Add a check for it to ensure it aborts if the index does not match HEAD, and add a testcase where this fix is needed.

Note that the fix here would also incidentally be an alternative fix for the testcase added in the last patch, but the fix in the last patch is still needed when multiple merge strategies are in use, so tweak the testcase from the previous commit so that it continues to exercise the codepath added in the last commit.

---
 builtin/merge.c                          | 15 +++++++++++++++
 t/t6424-merge-unrelated-index-changes.sh | 22 +++++++++++++++++++++-
 2 files changed, 36 insertions(+), 1 deletion(-)

index 23170f2d2a6..b43876f68e4 100644
@@ -1599,6 +1599,21 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 */
 		refresh_cache(REFRESH_QUIET);
 		if (allow_trivial && fast_forward != FF_ONLY) {
+			/*
+			 * Must first ensure that index matches HEAD before
+			 * attempting a trivial merge.
+			 */
+			struct tree *head_tree = get_commit_tree(head_commit);
+			struct strbuf sb = STRBUF_INIT;
+
+			if (repo_index_has_changes(the_repository, head_tree,
+						   &sb)) {
+				error(_("Your local changes to the following files would be overwritten by merge:\n  %s"),
+				      sb.buf);
+				strbuf_release(&sb);
+				return 2;
+			}
+
 			/* See if it is really trivial. */
 			git_committer_info(IDENT_STRICT);
 			printf(_("Trying really trivial in-index merge...\n"));

index eabe6bda832..187c761ad84 100755
@@ -114,6 +114,19 @@ test_expect_success 'resolve, non-trivial' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
+test_expect_success 'resolve, trivial, related file removed' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	git rm a &&
+	test_path_is_missing a &&
+
+	test_must_fail git merge -s resolve C^0 &&
+
+	test_path_is_missing a &&
+	test_path_is_missing .git/MERGE_HEAD
+'
+
 test_expect_success 'resolve, non-trivial, related file removed' '
 	git reset --hard &&
 	git checkout B^0 &&

@@ -121,7 +134,14 @@ test_expect_success 'resolve, non-trivial, related file removed' '
 	git rm a &&
 	test_path_is_missing a &&
 
-	test_must_fail git merge -s resolve D^0 &&
+	# We also ask for recursive in order to turn off the "allow_trivial"
+	# setting in builtin/merge.c, and ensure that resolve really does
+	# correctly fail the merge (I guess this also tests that recursive
+	# correctly fails the merge, but the main thing we are attempting
+	# to test here is resolve and are just using the side effect of
+	# adding recursive to ensure that resolve is actually tested rather
+	# than the trivial merge codepath)
+	test_must_fail git merge -s resolve -s recursive D^0 &&
 
 	test_path_is_missing a &&
 	test_path_is_missing .git/MERGE_HEAD

gitgitgadget

From: Elijah Newren <[email protected]>

builtin/merge is setup to allow multiple strategies to be specified, and it will find the "best" result and use it. This is defeated if some of the merge strategies abort early when they cannot handle the merge. Fix the logic that calls recursive and ort to not do such an early abort, but instead return "2" or "unhandled" so that the next strategy can try to handle the merge.

Coming up with a testcase for this is somewhat difficult, since recursive and ort both handle nearly any two-headed merge (there is a separate code path that checks for non-two-headed merges and already returns "2" for them). So use a somewhat synthetic testcase of having the index not match HEAD before the merge starts, since all merge strategies will abort for that.

---
 builtin/merge.c                          |  6 ++++--
 t/t6402-merge-rename.sh                  |  2 +-
 t/t6424-merge-unrelated-index-changes.sh | 16 ++++++++++++++++
 t/t6439-merge-co-error-msgs.sh           |  1 +
 4 files changed, 22 insertions(+), 3 deletions(-)

index b43876f68e4..c120ad619c4 100644
@@ -754,8 +754,10 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		else
 			clean = merge_recursive(&o, head, remoteheads->item,
 						reversed, &result);
-		if (clean < 0)
-			exit(128);
+		if (clean < 0) {
+			rollback_lock_file(&lock);
+			return 2;
+		}
 		if (write_locked_index(&the_index, &lock,
 				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 			die(_("unable to write %s"), get_index_file());

index 3a32b1a45cf..772238e582c 100755
@@ -210,7 +210,7 @@ test_expect_success 'updated working tree file should prevent the merge' '
 	echo >>M one line addition &&
 	cat M >M.saved &&
 	git update-index M &&
-	test_expect_code 128 git pull --no-rebase . yellow &&
+	test_expect_code 2 git pull --no-rebase . yellow &&
 	test_cmp M M.saved &&
 	rm -f M.saved
 '

index 187c761ad84..615061c7af4 100755
@@ -275,4 +275,20 @@ test_expect_success 'subtree' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
+test_expect_success 'with multiple strategies, recursive or ort failure do not early abort' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	test_seq 0 10 >a &&
+	git add a &&
+
+	sane_unset GIT_TEST_MERGE_ALGORITHM &&
+	test_must_fail git merge -s recursive -s ort -s octopus C^0 >output 2>&1 &&
+
+	grep "Trying merge strategy recursive..." output &&
+	grep "Trying merge strategy ort..." output &&
+	grep "Trying merge strategy octopus..." output &&
+	grep "No merge strategy handled the merge." output
+'
+
 test_done

index 5bfb027099a..52cf0c87690 100755
@@ -47,6 +47,7 @@ test_expect_success 'untracked files overwritten by merge (fast and non-fast for
 		export GIT_MERGE_VERBOSITY &&
 		test_must_fail git merge branch 2>out2
 	) &&
+	echo "Merge with strategy ${GIT_TEST_MERGE_ALGORITHM:-ort} failed." >>expect &&
 	test_cmp out2 expect &&
 	git reset --hard HEAD^
 '

gitgitgadget

From: Elijah Newren <[email protected]>

When there are stat-dirty files, but no files are modified, git stash create exits with unsuccessful status. This causes merge to fail. Copy some code from sequencer.c's create_autostash to refresh the index first to avoid this problem.

---
 builtin/merge.c                          |  8 ++++++++
 t/t6424-merge-unrelated-index-changes.sh | 11 +++++++++++
 2 files changed, 19 insertions(+)

index c120ad619c4..780b4b9100a 100644
@@ -313,8 +313,16 @@ static int save_state(struct object_id *stash)
 	int len;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buffer = STRBUF_INIT;
+	struct lock_file lock_file = LOCK_INIT;
+	int fd;
 	int rc = -1;
 
+	fd = repo_hold_locked_index(the_repository, &lock_file, 0);
+	refresh_cache(REFRESH_QUIET);
+	if (0 <= fd)
+		repo_update_index_if_able(the_repository, &lock_file);
+	rollback_lock_file(&lock_file);
+
 	strvec_pushl(&cp.args, "stash", "create", NULL);
 	cp.out = -1;
 	cp.git_cmd = 1;

index 615061c7af4..2c83210f9fd 100755
@@ -275,6 +275,17 @@ test_expect_success 'subtree' '
 	test_path_is_missing .git/MERGE_HEAD
 '
 
+test_expect_success 'avoid failure due to stat-dirty files' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	# Make "a" be stat-dirty
+	test-tool chmtime =+1 a &&
+
+	# stat-dirty file should not prevent stash creation in builtin/merge.c
+	git merge -s resolve -s recursive D^0
+'
+
 test_expect_success 'with multiple strategies, recursive or ort failure do not early abort' '
 	git reset --hard &&
 	git checkout B^0 &&