Merge branch 'en/dirty-merge-fixes'

The recursive merge strategy did not properly ensure there was no
change between HEAD and the index before performing its operation,
which has been corrected.

* en/dirty-merge-fixes:
  merge: fix misleading pre-merge check documentation
  merge-recursive: enforce rule that index matches head before merging
  t6044: add more testcases with staged changes before a merge is invoked
  merge-recursive: fix assumption that head tree being merged is HEAD
  merge-recursive: make sure when we say we abort that we actually abort
  t6044: add a testcase for index matching head, when head doesn't match HEAD
  t6044: verify that merges expected to abort actually abort
  index_has_changes(): avoid assuming operating on the_index
  read-cache.c: move index_has_changes() from merge.c

Author: gitster

Documentation/git-merge.txt

@@ -130,9 +130,9 @@ merge' may need to update.
 
 To avoid recording unrelated changes in the merge commit,
 'git pull' and 'git merge' will also abort if there are any changes
-registered in the index relative to the `HEAD` commit.  (One
-exception is when the changed index entries are in the state that
-would result from the merge already.)
+registered in the index relative to the `HEAD` commit.  (Special
+narrow exceptions to this rule may exist depending on which merge
+strategy is in use, but generally, the index must match HEAD.)
 
 If all named commits are already ancestors of `HEAD`, 'git merge'
 will exit early with the message "Already up to date."

builtin/am.c

@@ -1766,7 +1766,7 @@ static void am_run(struct am_state *state, int resume)
 
 	refresh_and_write_cache();
 
-	if (index_has_changes(&sb)) {
+	if (index_has_changes(&the_index, NULL, &sb)) {
 		write_state_bool(state, "dirtyindex", 1);
 		die(_("Dirty index: cannot apply patches (dirty: %s)"), sb.buf);
 	}
@@ -1823,7 +1823,8 @@ static void am_run(struct am_state *state, int resume)
 			 * Applying the patch to an earlier tree and merging
 			 * the result may have produced the same tree as ours.
 			 */
-			if (!apply_status && !index_has_changes(NULL)) {
+			if (!apply_status &&
+			    !index_has_changes(&the_index, NULL, NULL)) {
 				say(state, stdout, _("No changes -- Patch already applied."));
 				goto next;
 			}
@@ -1877,7 +1878,7 @@ static void am_resolve(struct am_state *state)
 
 	say(state, stdout, _("Applying: %.*s"), linelen(state->msg), state->msg);
 
-	if (!index_has_changes(NULL)) {
+	if (!index_has_changes(&the_index, NULL, NULL)) {
 		printf_ln(_("No changes - did you forget to use 'git add'?\n"
 			"If there is nothing left to stage, chances are that something else\n"
 			"already introduced the same changes; you might want to skip this patch."));

cache.h

@@ -220,6 +220,7 @@ struct cache_entry {
 /* Forward structure decls */
 struct pathspec;
 struct child_process;
+struct tree;
 
 /*
  * Copy the sha1 and stat state of a cache entry from one to
@@ -696,12 +697,15 @@ extern void move_index_extensions(struct index_state *dst, struct index_state *s
 extern int unmerged_index(const struct index_state *);
 
 /**
- * Returns 1 if the index differs from HEAD, 0 otherwise. When on an unborn
- * branch, returns 1 if there are entries in the index, 0 otherwise. If an
- * strbuf is provided, the space-separated list of files that differ will be
- * appended to it.
- */
-extern int index_has_changes(struct strbuf *sb);
+ * Returns 1 if istate differs from tree, 0 otherwise.  If tree is NULL,
+ * compares istate to HEAD.  If tree is NULL and on an unborn branch,
+ * returns 1 if there are entries in istate, 0 otherwise.  If an strbuf is
+ * provided, the space-separated list of files that differ will be appended
+ * to it.
+ */
+extern int index_has_changes(const struct index_state *istate,
+			     struct tree *tree,
+			     struct strbuf *sb);
 
 extern int verify_path(const char *path, unsigned mode);
 extern int strcmp_offset(const char *s1, const char *s2, size_t *first_change);

merge-recursive.c

@@ -3281,20 +3281,20 @@ int merge_trees(struct merge_options *o,
 		struct tree **result)
 {
 	int code, clean;
+	struct strbuf sb = STRBUF_INIT;
+
+	if (!o->call_depth && index_has_changes(&the_index, head, &sb)) {
+		err(o, _("Your local changes to the following files would be overwritten by merge:\n  %s"),
+		    sb.buf);
+		return -1;
+	}
 
 	if (o->subtree_shift) {
 		merge = shift_tree_object(head, merge, o->subtree_shift);
 		common = shift_tree_object(head, common, o->subtree_shift);
 	}
 
 	if (oid_eq(&common->object.oid, &merge->object.oid)) {
-		struct strbuf sb = STRBUF_INIT;
-
-		if (!o->call_depth && 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;

merge.c

@@ -14,37 +14,6 @@ static const char *merge_argument(struct commit *commit)
 	return oid_to_hex(commit ? &commit->object.oid : the_hash_algo->empty_tree);
 }
 
-int index_has_changes(struct strbuf *sb)
-{
-	struct object_id head;
-	int i;
-
-	if (!get_oid_tree("HEAD", &head)) {
-		struct diff_options opt;
-
-		diff_setup(&opt);
-		opt.flags.exit_with_status = 1;
-		if (!sb)
-			opt.flags.quick = 1;
-		do_diff_cache(&head, &opt);
-		diffcore_std(&opt);
-		for (i = 0; sb && i < diff_queued_diff.nr; i++) {
-			if (i)
-				strbuf_addch(sb, ' ');
-			strbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);
-		}
-		diff_flush(&opt);
-		return opt.flags.has_changes != 0;
-	} else {
-		for (i = 0; sb && i < active_nr; i++) {
-			if (i)
-				strbuf_addch(sb, ' ');
-			strbuf_addstr(sb, active_cache[i]->name);
-		}
-		return !!active_nr;
-	}
-}
-
 int try_merge_command(const char *strategy, size_t xopts_nr,
 		      const char **xopts, struct commit_list *common,
 		      const char *head_arg, struct commit_list *remotes)

read-cache.c

@@ -6,6 +6,8 @@
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
 #include "config.h"
+#include "diff.h"
+#include "diffcore.h"
 #include "tempfile.h"
 #include "lockfile.h"
 #include "cache-tree.h"
@@ -2120,6 +2122,44 @@ int unmerged_index(const struct index_state *istate)
 	return 0;
 }
 
+int index_has_changes(const struct index_state *istate,
+		      struct tree *tree,
+		      struct strbuf *sb)
+{
+	struct object_id cmp;
+	int i;
+
+	if (istate != &the_index) {
+		BUG("index_has_changes cannot yet accept istate != &the_index; do_diff_cache needs updating first.");
+	}
+	if (tree)
+		cmp = tree->object.oid;
+	if (tree || !get_oid_tree("HEAD", &cmp)) {
+		struct diff_options opt;
+
+		diff_setup(&opt);
+		opt.flags.exit_with_status = 1;
+		if (!sb)
+			opt.flags.quick = 1;
+		do_diff_cache(&cmp, &opt);
+		diffcore_std(&opt);
+		for (i = 0; sb && i < diff_queued_diff.nr; i++) {
+			if (i)
+				strbuf_addch(sb, ' ');
+			strbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);
+		}
+		diff_flush(&opt);
+		return opt.flags.has_changes != 0;
+	} else {
+		for (i = 0; sb && i < istate->cache_nr; i++) {
+			if (i)
+				strbuf_addch(sb, ' ');
+			strbuf_addstr(sb, istate->cache[i]->name);
+		}
+		return !!istate->cache_nr;
+	}
+}
+
 #define WRITE_BUFFER_SIZE 8192
 static unsigned char write_buffer[WRITE_BUFFER_SIZE];
 static unsigned long write_buffer_len;

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

@@ -82,7 +82,8 @@ test_expect_success 'ff update, important file modified' '
 	touch subdir/e &&
 	git add subdir/e &&
 
-	test_must_fail git merge E^0
+	test_must_fail git merge E^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'resolve, trivial' '
@@ -91,7 +92,8 @@ test_expect_success 'resolve, trivial' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s resolve C^0
+	test_must_fail git merge -s resolve C^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'resolve, non-trivial' '
@@ -100,7 +102,8 @@ test_expect_success 'resolve, non-trivial' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s resolve D^0
+	test_must_fail git merge -s resolve D^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'recursive' '
@@ -109,7 +112,8 @@ test_expect_success 'recursive' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s recursive C^0
+	test_must_fail git merge -s recursive C^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'recursive, when merge branch matches merge base' '
@@ -118,7 +122,45 @@ test_expect_success 'recursive, when merge branch matches merge base' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s recursive F^0
+	test_must_fail git merge -s recursive F^0 &&
+	test_path_is_missing .git/MERGE_HEAD
+'
+
+test_expect_success 'merge-recursive, when index==head but head!=HEAD' '
+	git reset --hard &&
+	git checkout C^0 &&
+
+	# Make index match B
+	git diff C B -- | git apply --cached &&
+	# Merge B & F, with B as "head"
+	git merge-recursive A -- B F > out &&
+	test_i18ngrep "Already up to date" out
+'
+
+test_expect_success 'recursive, when file has staged changes not matching HEAD nor what a merge would give' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	mkdir subdir &&
+	test_seq 1 10 >subdir/a &&
+	git add subdir/a &&
+
+	# We have staged changes; merge should error out
+	test_must_fail git merge -s recursive E^0 2>err &&
+	test_i18ngrep "changes to the following files would be overwritten" err
+'
+
+test_expect_success 'recursive, when file has staged changes matching what a merge would give' '
+	git reset --hard &&
+	git checkout B^0 &&
+
+	mkdir subdir &&
+	test_seq 1 11 >subdir/a &&
+	git add subdir/a &&
+
+	# We have staged changes; merge should error out
+	test_must_fail git merge -s recursive E^0 2>err &&
+	test_i18ngrep "changes to the following files would be overwritten" err
 '
 
 test_expect_success 'octopus, unrelated file touched' '
@@ -127,7 +169,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_must_fail git merge C^0 D^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'octopus, related file removed' '
@@ -136,7 +179,8 @@ test_expect_success 'octopus, related file removed' '
 
 	git rm b &&
 
-	test_must_fail git merge C^0 D^0
+	test_must_fail git merge C^0 D^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'octopus, related file modified' '
@@ -145,7 +189,8 @@ test_expect_success 'octopus, related file modified' '
 
 	echo 12 >>a && git add a &&
 
-	test_must_fail git merge C^0 D^0
+	test_must_fail git merge C^0 D^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'ours' '
@@ -154,7 +199,8 @@ test_expect_success 'ours' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s ours C^0
+	test_must_fail git merge -s ours C^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_expect_success 'subtree' '
@@ -163,7 +209,8 @@ test_expect_success 'subtree' '
 
 	touch random_file && git add random_file &&
 
-	test_must_fail git merge -s subtree E^0
+	test_must_fail git merge -s subtree E^0 &&
+	test_path_is_missing .git/MERGE_HEAD
 '
 
 test_done

t/t7504-commit-msg-hook.sh

@@ -157,6 +157,7 @@ test_expect_success 'merge bypasses failing hook with --no-verify' '
 	test_when_finished "git branch -D newbranch" &&
 	test_when_finished "git checkout -f master" &&
 	git checkout --orphan newbranch &&
+	git rm -f file &&
 	: >file2 &&
 	git add file2 &&
 	git commit --no-verify file2 -m in-side-branch &&
@@ -168,7 +169,7 @@ test_expect_success 'merge bypasses failing hook with --no-verify' '
 chmod -x "$HOOK"
 test_expect_success POSIXPERM 'with non-executable hook' '
 
-	echo "content" >> file &&
+	echo "content" >file &&
 	git add file &&
 	git commit -m "content"
 
@@ -249,6 +250,7 @@ test_expect_success 'hook called in git-merge picks up commit message' '
 	test_when_finished "git branch -D newbranch" &&
 	test_when_finished "git checkout -f master" &&
 	git checkout --orphan newbranch &&
+	git rm -f file &&
 	: >file2 &&
 	git add file2 &&
 	git commit --no-verify file2 -m in-side-branch &&