merge-recursive: fix check for skipability of working tree updates

The can-working-tree-updates-be-skipped check has had a long and blemished
history.  The update can be skipped iff:
  a) The merge is clean
  b) The merge matches what was in HEAD (content, mode, pathname)
  c) The target path is usable (i.e. not involved in D/F conflict)

Traditionally, we split b into parts:
  b1) The merged result matches the content and mode found in HEAD
  b2) The merged target path existed in HEAD

Steps a & b1 are easy to check; we have always gotten those right.  While
it is easy to overlook step c, this was fixed seven years ago with commit
4ab9a157d069 ("merge_content(): Check whether D/F conflicts are still
present", 2010-09-20).  merge-recursive didn't have a readily available
way to directly check step b2, so various approximations were used:

  * In commit b2c8c0a76274 ("merge-recursive: When we detect we can skip
    an update, actually skip it", 2011-02-28), it was noted that although
    the code claimed it was skipping the update, it did not actually skip
    the update.  The code was made to skip it, but used lstat(path, ...)
    as an approximation to path-was-tracked-in-index-before-merge.

  * In commit 5b448b853030 ("merge-recursive: When we detect we can skip
    an update, actually skip it", 2011-08-11), the problem with using
    lstat was noted.  It was changed to the approximation
       path2 && strcmp(path, path2)
    which is also wrong.  !path2 || strcmp(path, path2) would have been
    better, but would have fallen short with directory renames.

  * In c5b761fb2711 ("merge-recursive: ensure we write updates for
    directory-renamed file", 2018-02-14), the problem with the previous
    approximation was noted and changed to
       was_tracked(path)
    That looks close to what we were trying to answer, but was_tracked()
    as implemented at the time should have been named is_tracked(); it
    returned something different than what we were looking for.

  * To make matters more complex, fixing was_tracked() isn't sufficient
    because the splitting of b into b1 and b2 is wrong.  Consider the
    following merge with a rename/add conflict:
       side A: modify foo, add unrelated bar
       side B: rename foo->bar (but don't modify the mode or contents)
    In this case, the three-way merge of original foo, A's foo, and B's
    bar will result in a desired pathname of bar with the same
    mode/contents that A had for foo.  Thus, A had the right mode and
    contents for the file, and it had the right pathname present (namely,
    bar), but the bar that was present was unrelated to the contents, so
    the working tree update was not skippable.

Fix this by introducing a new function:
   was_tracked_and_matches(o, path, &mfi.oid, mfi.mode)
and use it to directly check for condition b.

Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

4 files changed, 39 insertions, 23 deletions

diff --git a/merge-recursive.c b/merge-recursive.c
index f9021dd..13b4762 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -780,6 +780,25 @@ static int dir_in_way(const char *path, int check_working_copy, int empty_ok)
 }
 
 /*
+ * Returns whether path was tracked in the index before the merge started,
+ * and its oid and mode match the specified values
+ */
+static int was_tracked_and_matches(struct merge_options *o, const char *path,
+				   const struct object_id *oid, unsigned mode)
+{
+	int pos = index_name_pos(&o->orig_index, path, strlen(path));
+	struct cache_entry *ce;
+
+	if (0 > pos)
+		/* we were not tracking this path before the merge */
+		return 0;
+
+	/* See if the file we were tracking before matches */
+	ce = o->orig_index.cache[pos];
+	return (oid_eq(&ce->oid, oid) && ce->ce_mode == mode);
+}
+
+/*
  * Returns whether path was tracked in the index before the merge started
  */
 static int was_tracked(struct merge_options *o, const char *path)
@@ -2821,23 +2840,20 @@ static int merge_content(struct merge_options *o,
 				       o->branch2, path2, &mfi))
 		return -1;
 
-	if (mfi.clean && !df_conflict_remains &&
-	    oid_eq(&mfi.oid, a_oid) && mfi.mode == a_mode) {
-		int path_renamed_outside_HEAD;
+	/*
+	 * We can skip updating the working tree file iff:
+	 *   a) The merge is clean
+	 *   b) The merge matches what was in HEAD (content, mode, pathname)
+	 *   c) The target path is usable (i.e. not involved in D/F conflict)
+	 */
+	if (mfi.clean &&
+	    was_tracked_and_matches(o, path, &mfi.oid, mfi.mode) &&
+	    !df_conflict_remains) {
 		output(o, 3, _("Skipped %s (merged same as existing)"), path);
-		/*
-		 * The content merge resulted in the same file contents we
-		 * already had.  We can return early if those file contents
-		 * are recorded at the correct path (which may not be true
-		 * if the merge involves a rename).
-		 */
-		path_renamed_outside_HEAD = !path2 || !strcmp(path, path2);
-		if (!path_renamed_outside_HEAD) {
-			if (add_cacheinfo(o, mfi.mode, &mfi.oid, path,
-					  0, (!o->call_depth && !is_dirty), 0))
-				return -1;
-			return mfi.clean;
-		}
+		if (add_cacheinfo(o, mfi.mode, &mfi.oid, path,
+				  0, (!o->call_depth && !is_dirty), 0))
+			return -1;
+		return mfi.clean;
 	}
 
 	if (!mfi.clean) {
diff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh
index a1fad69..6df2650 100755
--- a/t/t6022-merge-rename.sh
+++ b/t/t6022-merge-rename.sh
@@ -247,7 +247,7 @@ test_expect_success 'merge of identical changes in a renamed file' '
 	git reset --hard HEAD^ &&
 	git checkout change &&
 	GIT_MERGE_VERBOSITY=3 git merge change+rename >out &&
-	test_i18ngrep "^Skipped B" out
+	test_i18ngrep ! "^Skipped B" out
 '
 
 test_expect_success 'setup for rename + d/f conflicts' '
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
index 45f6206..2e28f29 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -3884,7 +3884,7 @@ test_expect_success '12b-setup: Moving one directory hierarchy into another' '
 	)
 '
 
-test_expect_failure '12b-check: Moving one directory hierarchy into another' '
+test_expect_success '12b-check: Moving one directory hierarchy into another' '
 	(
 		cd 12b &&
 
diff --git a/t/t6046-merge-skip-unneeded-updates.sh b/t/t6046-merge-skip-unneeded-updates.sh
index a0f482e..fcefffc 100755
--- a/t/t6046-merge-skip-unneeded-updates.sh
+++ b/t/t6046-merge-skip-unneeded-updates.sh
@@ -64,7 +64,7 @@ test_expect_success '1a-setup: Modify(A)/Modify(B), change on B subset of A' '
 	)
 '
 
-test_expect_failure '1a-check-L: Modify(A)/Modify(B), change on B subset of A' '
+test_expect_success '1a-check-L: Modify(A)/Modify(B), change on B subset of A' '
 	test_when_finished "git -C 1a reset --hard" &&
 	test_when_finished "git -C 1a clean -fd" &&
 	(
@@ -160,7 +160,7 @@ test_expect_success '2a-setup: Modify(A)/rename(B)' '
 	)
 '
 
-test_expect_failure '2a-check-L: Modify/rename, merge into modify side' '
+test_expect_success '2a-check-L: Modify/rename, merge into modify side' '
 	test_when_finished "git -C 2a reset --hard" &&
 	test_when_finished "git -C 2a clean -fd" &&
 	(
@@ -360,7 +360,7 @@ test_expect_success '2c-setup: Modify b & add c VS rename b->c' '
 	)
 '
 
-test_expect_failure '2c-check: Modify b & add c VS rename b->c' '
+test_expect_success '2c-check: Modify b & add c VS rename b->c' '
 	(
 		cd 2c &&
 
@@ -456,7 +456,7 @@ test_expect_success '3a-setup: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
 	)
 '
 
-test_expect_failure '3a-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
+test_expect_success '3a-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
 	test_when_finished "git -C 3a reset --hard" &&
 	test_when_finished "git -C 3a clean -fd" &&
 	(
@@ -579,7 +579,7 @@ test_expect_success '3b-check-L: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
 	)
 '
 
-test_expect_failure '3b-check-R: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
+test_expect_success '3b-check-R: bq_1->foo/bq_2 on A, foo/->bar/ on B' '
 	test_when_finished "git -C 3b reset --hard" &&
 	test_when_finished "git -C 3b clean -fd" &&
 	(