Skip to content

Commit 9fc91d9

Browse files
committed
merge-ort: fix failing merges in special corner case
At GitHub, we had a repository that was triggering git: merge-ort.c:3032: process_renames: Assertion `newinfo && !newinfo->merged.clean` failed. during git replay. This sounds similar to the somewhat recent f6ecb60 (merge-ort: fix directory rename on top of source of other rename/delete, 2025-08-06), but the cause is different. Unlike that case, there are no rename-to-self situations arising in this case, and new to this case it can only be triggered during a replay operation on the 2nd or later commit being replayed, never on the first merge in the sequence. To trigger, the repository needs: * an upstream which: * renames a file to a different directory, e.g. old/file -> new/file * leaves other files remaining in the original directory (so that e.g. "old/" still exists upstream even though file has been removed from it and placed elsewhere) * a topic branch being rebased where: * a commit in the sequence: * modifies old/file * a subsequent commit in the sequence being replayed: * does NOT touch *anything* under new/ * does NOT touch old/file * DOES modify other paths under old/ * does NOT have any relevant renames that we need to detect _anywhere_ elsewhere in the tree (meaning this interacts interestingly with both directory renames and cached renames) In such a case, the assertion will trigger. The fix turns out to be surprisingly simple. I have a very vague recollection that I actually considered whether to add such an if-check years ago when I added the very similar one for oldinfo in 1b6b902 (merge-ort: process_renames() now needs more defensiveness, 2021-01-19), but I think I couldn't figure out a possible way to trigger it and was worried at the time that if I didn't know how to trigger it then I wasn't so sure that simply skipping it was correct. Waiting did give me a chance to put more thorough tests and checks into place for the rename-to-self cases a few months back, which I might not have found as easily otherwise. Anyway, put the check in place now and add a test that demonstrates the fix. Signed-off-by: Elijah Newren <newren@gmail.com>
1 parent 69e5428 commit 9fc91d9

File tree

2 files changed

+104
-0
lines changed

2 files changed

+104
-0
lines changed

merge-ort.c

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2912,6 +2912,32 @@ static int process_renames(struct merge_options *opt,
29122912
if (!oldinfo || oldinfo->merged.clean)
29132913
continue;
29142914

2915+
/*
2916+
* Rename caching from a previous commit might give us an
2917+
* irrelevant rename for the current commit.
2918+
*
2919+
* Imagine:
2920+
* foo/A -> bar/A
2921+
* was a cached rename for the upstream side from the
2922+
* previous commit (without the directories being renamed),
2923+
* but the next commit being replayed
2924+
* * does NOT add or delete files
2925+
* * does NOT have directory renames
2926+
* * does NOT modify any files under bar/
2927+
* * does NOT modify foo/A
2928+
* * DOES modify other files under foo/ (otherwise the
2929+
* !oldinfo check above would have already exited for
2930+
* us)
2931+
* In such a case, our trivial directory resolution will
2932+
* have already merged bar/, and our attempt to process
2933+
* the cached
2934+
* foo/A -> bar/A
2935+
* would be counterproductive, and lack the necessary
2936+
* information anyway. Skip such renames.
2937+
*/
2938+
if (!newinfo)
2939+
continue;
2940+
29152941
/*
29162942
* diff_filepairs have copies of pathnames, thus we have to
29172943
* use standard 'strcmp()' (negated) instead of '=='.

t/t6429-merge-sequence-rename-caching.sh

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -768,4 +768,82 @@ test_expect_success 'avoid assuming we detected renames' '
768768
)
769769
'
770770

771+
#
772+
# In the following testcase:
773+
# Base: olddir/{valuesX_1, valuesY_1, valuesZ_1}
774+
# other/content
775+
# Upstream: rename olddir/valuesX_1 -> newdir/valuesX_2
776+
# Topic_1: modify olddir/valuesX_1 -> olddir/valuesX_3
777+
# Topic_2: modify olddir/valuesY,
778+
# modify other/content
779+
# Expected Pick1: olddir/{valuesY, valuesZ}, newdir/valuesX, other/content
780+
# Expected Pick2: olddir/{valuesY, valuesZ}, newdir/valuesX, other/content
781+
#
782+
# This testcase presents no problems for git traditionally, but the fact that
783+
# olddir/valuesX -> newdir/valuesX
784+
# gets cached after the first pick presents a problem for the second commit to
785+
# be replayed, because it appears to be an irrelevant rename, so the trivial
786+
# directory resolution will resolve newdir/ without recursing into it, giving
787+
# us no way to apply the cached rename to anything.
788+
#
789+
test_expect_success 'rename a file, use it on first pick, but irrelevant on second' '
790+
git init rename_a_file_use_it_once_irrelevant_on_second &&
791+
(
792+
cd rename_a_file_use_it_once_irrelevant_on_second &&
793+
794+
mkdir olddir/ other/ &&
795+
test_seq 3 8 >olddir/valuesX &&
796+
test_seq 3 8 >olddir/valuesY &&
797+
test_seq 3 8 >olddir/valuesZ &&
798+
printf "%s\n" A B C D E F G >other/content &&
799+
git add olddir other &&
800+
git commit -m orig &&
801+
802+
git branch upstream &&
803+
git branch topic &&
804+
805+
git switch upstream &&
806+
test_seq 1 8 >olddir/valuesX &&
807+
git add olddir &&
808+
mkdir newdir &&
809+
git mv olddir/valuesX newdir &&
810+
git commit -m "Renamed (and modified) olddir/valuesX into newdir/" &&
811+
812+
git switch topic &&
813+
814+
test_seq 3 10 >olddir/valuesX &&
815+
git add olddir &&
816+
git commit -m A &&
817+
818+
test_seq 1 8 >olddir/valuesY &&
819+
printf "%s\n" A B C D E F G H I >other/content &&
820+
git add olddir/valuesY other &&
821+
git commit -m B &&
822+
823+
#
824+
# Actual testing; mostly we want to verify that we do not hit
825+
# git: merge-ort.c:3032: process_renames: Assertion `newinfo && !newinfo->merged.clean` failed.
826+
#
827+
828+
git switch upstream &&
829+
git config merge.directoryRenames true &&
830+
831+
git replay --onto HEAD upstream~1..topic >out &&
832+
833+
#
834+
# ...but we may as well check that the replay gave us a reasonable result
835+
#
836+
837+
git update-ref --stdin <out &&
838+
git checkout topic &&
839+
840+
git ls-files >tracked &&
841+
test_line_count = 4 tracked &&
842+
test_path_is_file newdir/valuesX &&
843+
test_path_is_file olddir/valuesY &&
844+
test_path_is_file olddir/valuesZ &&
845+
test_path_is_file other/content
846+
)
847+
'
848+
771849
test_done

0 commit comments

Comments
 (0)