git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 3/3] merge-recursive: Avoid incorporating uncommitted changes in a merge

From
Elijah Newren <newren@gmail.com>
Date
Dec 21, 2017, 19:19 UTC
Message-ID
<20171221191907.4251-3-newren@gmail.com>
In-Reply-To
<20171221191907.4251-1-newren@gmail.com>
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 <newren@gmail.com>
---
 merge-recursive.c                        | 7 +++++++
 t/t6044-merge-unrelated-index-changes.sh | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 2ecf495cc2..780f81a8bd 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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;
diff --git a/t/t6044-merge-unrelated-index-changes.sh b/t/t6044-merge-unrelated-index-changes.sh
index 5e472be92b..23b86fb977 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -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 &&
 
-- 
2.15.1.436.g63a861020b
Previous: Elijah NewrenNext: Junio C Hamano
Message 4 of 15 in “git merge commits staged files (when two trees are identical)”
  1. Andreas KreyDec 20, 2017
  2. Elijah NewrenDec 21, 2017
  3. 1/3 t6044: recursive can silently incorporate dirty changes in a mergeElijah Newren, Dec 21, 2017
  4. 3/3 merge-recursive: Avoid incorporating uncommitted changes in a mergeElijah Newren, Dec 21, 2017
  5. Junio C HamanoDec 22, 2017
  6. Junio C HamanoJan 8, 2018
  7. merge-recursive: do not look at the index during recursive mergeJunio C Hamano, Jan 9, 2018
  8. Junio C HamanoJan 9, 2018
  9. Eric SunshineJan 9, 2018
  10. Elijah NewrenJan 9, 2018
  11. Junio C HamanoJan 9, 2018
  12. 2/3 move index_has_changes() from builtin/am.c to merge.c for reuseElijah Newren, Dec 21, 2017
  13. Elijah NewrenDec 21, 2017
  14. Junio C HamanoDec 22, 2017
  15. Elijah NewrenDec 23, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.