Skip to content

Commit 63b48f3

Browse files
gitstergitster
committed
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 parents fb260a5 + b3106a8 commit 63b48f3

File tree

2 files changed

+114
-10
lines changed

2 files changed

+114
-10
lines changed

‎merge-ort.c‎

Lines changed: 29 additions & 2 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 '=='.
@@ -3438,7 +3464,7 @@ static int collect_renames(struct merge_options *opt,
34383464
continue;
34393465
}
34403466
if (opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_NONE &&
3441-
p->status == 'R' && 1) {
3467+
p->status == 'R') {
34423468
possibly_cache_new_pair(renames, p, side_index, NULL);
34433469
goto skip_directory_renames;
34443470
}
@@ -5118,7 +5144,8 @@ static void merge_check_renames_reusable(struct merge_options *opt,
51185144
* optimization" comment near that case).
51195145
*
51205146
* This could be revisited in the future; see the commit message
5121-
* where this comment was added for some possible pointers.
5147+
* where this comment was added for some possible pointers, or the
5148+
* later commit where this comment was added.
51225149
*/
51235150
if (opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_NONE) {
51245151
renames->cached_pairs_valid_side = 0; /* neither side valid */

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

Lines changed: 85 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,13 @@ test_description="remember regular & dir renames in sequence of merges"
1111
# sure that we are triggering rename caching rather than rename
1212
# bypassing.
1313
#
14-
# NOTE 2: this testfile uses 'test-tool fast-rebase' instead of either
15-
# cherry-pick or rebase. sequencer.c is only superficially
16-
# integrated with merge-ort; it calls merge_switch_to_result()
17-
# after EACH merge, which updates the index and working copy AND
18-
# throws away the cached results (because merge_switch_to_result()
19-
# is only supposed to be called at the end of the sequence).
20-
# Integrating them more deeply is a big task, so for now the tests
21-
# use 'test-tool fast-rebase'.
14+
# NOTE 2: this testfile uses replay instead of either cherry-pick or rebase.
15+
# sequencer.c is only superficially integrated with merge-ort; it
16+
# calls merge_switch_to_result() after EACH merge, which updates the
17+
# index and working copy AND throws away the cached results (because
18+
# merge_switch_to_result() is only supposed to be called at the end
19+
# of the sequence). Integrating them more deeply is a big task, so
20+
# for now the tests use 'git replay'.
2221
#
2322

2423

@@ -769,4 +768,82 @@ test_expect_success 'avoid assuming we detected renames' '
769768
)
770769
'
771770

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+
772849
test_done

0 commit comments

Comments
 (0)