Merge branch 'en/ort-rename-another-fix' into jch

Yet another corner case fix around renames in the "ort" merge
strategy.

* en/ort-rename-another-fix:
  merge-ort: fix failing merges in special corner case
  merge-ort: remove debugging crud
  t6429: update comment to mention correct tool

2 files changed, 114 insertions, 10 deletions

diff --git a/merge-ort.c b/merge-ort.c
index 29858074f9..a1f3333e44 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2913,6 +2913,32 @@ static int process_renames(struct merge_options *opt,
 			continue;
 
 		/*
+		 * Rename caching from a previous commit might give us an
+		 * irrelevant rename for the current commit.
+		 *
+		 * Imagine:
+		 *     foo/A -> bar/A
+		 * was a cached rename for the upstream side from the
+		 * previous commit (without the directories being renamed),
+		 * but the next commit being replayed
+		 *     * does NOT add or delete files
+		 *     * does NOT have directory renames
+		 *     * does NOT modify any files under bar/
+		 *     * does NOT modify foo/A
+		 *     * DOES modify other files under foo/ (otherwise the
+		 *       !oldinfo check above would have already exited for
+		 *       us)
+		 * In such a case, our trivial directory resolution will
+		 * have already merged bar/, and our attempt to process
+		 * the cached
+		 *     foo/A -> bar/A
+		 * would be counterproductive, and lack the necessary
+		 * information anyway.  Skip such renames.
+		 */
+		if (!newinfo)
+			continue;
+
+		/*
 		 * diff_filepairs have copies of pathnames, thus we have to
 		 * use standard 'strcmp()' (negated) instead of '=='.
 		 */
@@ -3438,7 +3464,7 @@ static int collect_renames(struct merge_options *opt,
 			continue;
 		}
 		if (opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_NONE &&
-		    p->status == 'R' && 1) {
+		    p->status == 'R') {
 			possibly_cache_new_pair(renames, p, side_index, NULL);
 			goto skip_directory_renames;
 		}
@@ -5118,7 +5144,8 @@ static void merge_check_renames_reusable(struct merge_options *opt,
 	 * optimization" comment near that case).
 	 *
 	 * This could be revisited in the future; see the commit message
-	 * where this comment was added for some possible pointers.
+	 * where this comment was added for some possible pointers, or the
+	 * later commit where this comment was added.
 	 */
 	if (opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_NONE) {
 		renames->cached_pairs_valid_side = 0; /* neither side valid */
diff --git a/t/t6429-merge-sequence-rename-caching.sh b/t/t6429-merge-sequence-rename-caching.sh
index 0f39ed0d08..15dd2d94b7 100755
--- a/t/t6429-merge-sequence-rename-caching.sh
+++ b/t/t6429-merge-sequence-rename-caching.sh
@@ -11,14 +11,13 @@ test_description="remember regular & dir renames in sequence of merges"
 #         sure that we are triggering rename caching rather than rename
 #         bypassing.
 #
-# NOTE 2: this testfile uses 'test-tool fast-rebase' instead of either
-#         cherry-pick or rebase.  sequencer.c is only superficially
-#         integrated with merge-ort; it calls merge_switch_to_result()
-#         after EACH merge, which updates the index and working copy AND
-#         throws away the cached results (because merge_switch_to_result()
-#         is only supposed to be called at the end of the sequence).
-#         Integrating them more deeply is a big task, so for now the tests
-#         use 'test-tool fast-rebase'.
+# NOTE 2: this testfile uses replay instead of either cherry-pick or rebase.
+#         sequencer.c is only superficially integrated with merge-ort; it
+#         calls merge_switch_to_result() after EACH merge, which updates the
+#         index and working copy AND throws away the cached results (because
+#         merge_switch_to_result() is only supposed to be called at the end
+#         of the sequence).  Integrating them more deeply is a big task, so
+#         for now the tests use 'git replay'.
 #
 
 
@@ -769,4 +768,82 @@ test_expect_success 'avoid assuming we detected renames' '
 	)
 '
 
+#
+# In the following testcase:
+#   Base:     olddir/{valuesX_1, valuesY_1, valuesZ_1}
+#             other/content
+#   Upstream: rename olddir/valuesX_1 -> newdir/valuesX_2
+#   Topic_1:  modify olddir/valuesX_1 -> olddir/valuesX_3
+#   Topic_2:  modify olddir/valuesY,
+#             modify other/content
+#   Expected Pick1: olddir/{valuesY, valuesZ}, newdir/valuesX, other/content
+#   Expected Pick2: olddir/{valuesY, valuesZ}, newdir/valuesX, other/content
+#
+# This testcase presents no problems for git traditionally, but the fact that
+#    olddir/valuesX -> newdir/valuesX
+# gets cached after the first pick presents a problem for the second commit to
+# be replayed, because it appears to be an irrelevant rename, so the trivial
+# directory resolution will resolve newdir/ without recursing into it, giving
+# us no way to apply the cached rename to anything.
+#
+test_expect_success 'rename a file, use it on first pick, but irrelevant on second' '
+	git init rename_a_file_use_it_once_irrelevant_on_second &&
+	(
+		cd rename_a_file_use_it_once_irrelevant_on_second &&
+
+		mkdir olddir/ other/ &&
+		test_seq 3 8 >olddir/valuesX &&
+		test_seq 3 8 >olddir/valuesY &&
+		test_seq 3 8 >olddir/valuesZ &&
+		printf "%s\n" A B C D E F G >other/content &&
+		git add olddir other &&
+		git commit -m orig &&
+
+		git branch upstream &&
+		git branch topic &&
+
+		git switch upstream &&
+		test_seq 1 8 >olddir/valuesX &&
+		git add olddir &&
+		mkdir newdir &&
+		git mv olddir/valuesX newdir &&
+		git commit -m "Renamed (and modified) olddir/valuesX into newdir/" &&
+
+		git switch topic &&
+
+		test_seq 3 10 >olddir/valuesX &&
+		git add olddir &&
+		git commit -m A &&
+
+		test_seq 1 8 >olddir/valuesY &&
+		printf "%s\n" A B C D E F G H I >other/content &&
+		git add olddir/valuesY other &&
+		git commit -m B &&
+
+		#
+		# Actual testing; mostly we want to verify that we do not hit
+		#     git: merge-ort.c:3032: process_renames: Assertion `newinfo && !newinfo->merged.clean` failed.
+		#
+
+		git switch upstream &&
+		git config merge.directoryRenames true &&
+
+		git replay --onto HEAD upstream~1..topic >out &&
+
+		#
+		# ...but we may as well check that the replay gave us a reasonable result
+		#
+
+		git update-ref --stdin <out &&
+		git checkout topic &&
+
+		git ls-files >tracked &&
+		test_line_count = 4 tracked &&
+		test_path_is_file newdir/valuesX &&
+		test_path_is_file olddir/valuesY &&
+		test_path_is_file olddir/valuesZ &&
+		test_path_is_file other/content
+	)
+'
+
 test_done