diff --git a/merge-ort.c b/merge-ort.c index 29858074f9d8bf..a1f3333e44a1ef 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -2912,6 +2912,32 @@ static int process_renames(struct merge_options *opt, if (!oldinfo || oldinfo->merged.clean) 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 0f39ed0d08a342..15dd2d94b75f0a 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 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