git merge commits staged files (when two trees are identical)

Hi everybody,

we just stumbled over a situation in which a merge commits staged changes into the merge commit. This happens when the merged-in branch does have commits ('main') but has the same tree ('--allow-empty') as the merge base:

git init
echo eins >a
git add a
git commit -m initial
git branch sub
git commit -m main --allow-empty
git checkout sub
: two
echo zwei >>a
git add a
git commit -m underway
: three
echo drei >>a
git add a # important
git status
git diff --cached
git merge master -m 'merge'
git status
git log --cc -1

If the change isn't staged (comment out the '# important' line) the change survives as unstaged.

That is a bug?

index_has_changes() is a function we want to reuse outside of just am, making it also available for merge-recursive and merge-ort.

Signed-off-by Elijah Newren <[email protected]>

---

Quoted 5 lines · Elijah Newren

index_has_changes() is a function we want to reuse outside of just am, making it also available for merge-recursive and merge-ort.

Signed-off-by Elijah Newren <[email protected]>

---

Note: These patches built on master, and merge cleanly with next and pu. However, this patch has a minor conflict with maint. If you'd prefer a version that applies on top of maint, let me know and I'll resubmit.

Quoted 11 lines · Elijah Newren

Quoted 5 lines · Elijah Newren

index_has_changes() is a function we want to reuse outside of just am, making it also available for merge-recursive and merge-ort.

Signed-off-by Elijah Newren <[email protected]>

---

Note: These patches built on master, and merge cleanly with next and pu. However, this patch has a minor conflict with maint. If you'd prefer a version that applies on top of maint, let me know and I'll resubmit.

I think I managed to create two topics, one that is with these three patches (2/3 backported) on top of maint and the other one merges the former on top of master. Please see if you found a mismerge when I push the results out.

builtin/merge.c contains this important requirement for merge strategies:
/*
* 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.
*/

merge-recursive does not do this check directly, instead it relies on unpack_trees() to do it. However, merge_trees() has a special check for the merge branch exactly matching the merge base; when it detects that situation, it returns early without calling unpack_trees(), because it knows that the HEAD commit already has the correct result. Unfortunately, it didn't check that the index matched HEAD, so after it returned, the outer logic ended up creating a merge commit that included something other than HEAD.

Signed-off-by Elijah Newren <[email protected]>

---
 merge-recursive.c                        | 7 +++++++
 t/t6044-merge-unrelated-index-changes.sh | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

merge-recursive.c +7 -0

index 2ecf495cc2..780f81a8bd 100644
@@ -1952,6 +1952,13 @@ int merge_trees(struct merge_options *o,
 	}
 
 	if (oid_eq(&common->object.oid, &merge->object.oid)) {
+		struct strbuf sb = STRBUF_INIT;
+
+		if (index_has_changes(&sb)) {
+			err(o, _("Dirty index: cannot merge (dirty: %s)"),
+			    sb.buf);
+			return 0;
+		}
 		output(o, 0, _("Already up to date!"));
 		*result = head;
 		return 1;

t/t6044-merge-unrelated-index-changes.sh +1 -1

index 5e472be92b..23b86fb977 100755
@@ -112,7 +112,7 @@ test_expect_success 'recursive' '
 	test_must_fail git merge -s recursive C^0
 '
 
-test_expect_failure 'recursive, when merge branch matches merge base' '
+test_expect_success 'recursive, when merge branch matches merge base' '
 	git reset --hard &&
 	git checkout B^0 &&

merge-recursive.c +0 -0

index 2ecf495cc2..780f81a8bd 100644
@@ -1952,6 +1952,13 @@ int merge_trees(struct merge_options *o,
 	}
 
 	if (oid_eq(&common->object.oid, &merge->object.oid)) {

t/t3030-merge-recursive.sh +13 -0

+
+	test_tick &&
+	git merge -s ours $O1 &&
+	B=$(git rev-parse HEAD) &&
+
+	git reset --hard $O1 &&
+	test_tick &&
+	git merge -s ours $O2 &&
+	A=$(git rev-parse HEAD) &&
+
+	git merge $B
+'
+
 test_done

When merging another branch into ours, if their tree is the same as the common ancestor's, we can declare that our tree represents the result of three-way merge. In such a case, the recursive merge backend incorrectly used to create a commit out of our index, even when the index has changes.

A recent fix attempted to prevent this by adding a comparison between "our" tree and the index, but forgot that this check must be restricted only to the outermost merge. Inner merges performed by the recursive backend across merge bases are by definition made from scratch without having any local changes added to the index. The call to index_has_changes() during an inner merge is working on the index that has no relation to the merge being performed, preventing legitimate merges from getting carried out.

Fix it by limiting the check to the outermost merge.

Signed-off-by Junio C Hamano <[email protected]>

---

t/t3030-merge-recursive.sh +18 -0

+       O1=$(git rev-parse HEAD) &&
+
+       git reset --hard $O0 &&
+       test_tick &&
+       git commit --allow-empty -m "O2" &&
+       O2=$(git rev-parse HEAD) &&
+
+       test_tick &&
+       git merge -s ours $O1 &&
+       B=$(git rev-parse HEAD) &&
+
+       git reset --hard $O1 &&
+       test_tick &&
+       git merge -s ours $O2 &&
+       A=$(git rev-parse HEAD) &&
+
+       git merge $B
+'

Quoted 3 lines

I haven't come up with an addition to the test suite, but I suspect this change is conceptually wrong. What if a call to this function is made during a recursive, inner merge?

 merge-recursive.c          |  2 +-
 t/t3030-merge-recursive.sh | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 1 deletion(-)

merge-recursive.c +0 -0

index 780f81a8bd..0fc580d8ca 100644

Hi,

Quoted 3 lines · Junio C Hamano

Quoted 3 lines

I haven't come up with an addition to the test suite, but I suspect this change is conceptually wrong. What if a call to this function is made during a recursive, inner merge?

Eek, good catch.

Quoted 9 lines

 merge-recursive.c          |  2 +-
 t/t3030-merge-recursive.sh | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 1 deletion(-)

merge-recursive.c +0 -0

index 780f81a8bd..0fc580d8ca 100644

merge-recursive.c int merge_trees(struct merge_options *o, · line 1960

Quoted 8 lines

merge-recursive.c +1 -1

@@ -1954,7 +1954,7 @@ int merge_trees(struct merge_options *o,
        if (oid_eq(&common->object.oid, &merge->object.oid)) {
                struct strbuf sb = STRBUF_INIT;
 
-               if (index_has_changes(&sb)) {
+               if (!o->call_depth && index_has_changes(&sb)) {
                        err(o, _("Dirty index: cannot merge (dirty: %s)"),
                            sb.buf);
                        return 0;

Yep, looks good to me; sorry for overlooking this.

Elijah