merge-recursive: handle rename-to-self case

Directory rename detection can cause transitive renames, e.g. if the two different sides of history each do one half of: A/file -> B/file B/ -> C/ then directory rename detection transitively renames to give us A/file -> C/file However, when C/ == A/, note that this gives us A/file -> A/file. merge-recursive assumed that any rename D -> E would have D != E. While that is almost always true, the above is a special case where it is not. So we cannot do things like delete the rename source, we cannot assume that a file existing at path E implies a rename/add conflict and we have to be careful about what stages end up in the output. This change feels a bit hackish. It took me surprisingly many hours to find, and given merge-recursive's design causing it to attempt to enumerate all combinations of edge and corner cases with special code for each combination, I'm worried there are other similar fixes needed elsewhere if we can just come up with the right special testcase. Perhaps an audit would rule it out, but I have not the energy. merge-recursive deserves to die, and since it is on its way out anyway, fixing this particular bug narrowly will have to be good enough. Reported-by: Anders Kaseorg <andersk@mit.edu> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Jun 30, 2021 at 17:30 UTC 3585d0ea232b1a9c5498ab5785b11f61e93967c8
2 files changed +16 -9
merge-recursive.c
+13 -6
@@ -2804,12 +2804,19 @@ static int process_renames(struct merge_options *opt,
2804 int renamed_stage = a_renames == renames1 ? 2 : 3;
2805 int other_stage = a_renames == renames1 ? 3 : 2;
2806
2807 + /*
2808 + * Directory renames have a funny corner case...
2809 + */
2810 + int renamed_to_self = !strcmp(ren1_src, ren1_dst);
2811 +
2812 /* BUG: We should only remove ren1_src in the base
2813 * stage and in other_stage (think of rename +
2814 * add-source case).
2815 */
2811 - remove_file(opt, 1, ren1_src,
2812 - renamed_stage == 2 || !was_tracked(opt, ren1_src));
2816 + if (!renamed_to_self)
2817 + remove_file(opt, 1, ren1_src,
2818 + renamed_stage == 2 ||
2819 + !was_tracked(opt, ren1_src));
2820
2821 oidcpy(&src_other.oid,
2822 &ren1->src_entry->stages[other_stage].oid);
@@ -2823,6 +2830,9 @@ static int process_renames(struct merge_options *opt,
2830 ren1->dir_rename_original_type == 'A') {
2831 setup_rename_conflict_info(RENAME_VIA_DIR,
2832 opt, ren1, NULL);
2833 + } else if (renamed_to_self) {
2834 + setup_rename_conflict_info(RENAME_NORMAL,
2835 + opt, ren1, NULL);
2836 } else if (oideq(&src_other.oid, null_oid())) {
2837 setup_rename_conflict_info(RENAME_DELETE,
2838 opt, ren1, NULL);
@@ -3180,7 +3190,6 @@ static int handle_rename_normal(struct merge_options *opt,
3190 struct rename *ren = ci->ren1;
3191 struct merge_file_info mfi;
3192 int clean;
3183 - int side = (ren->branch == opt->branch1 ? 2 : 3);
3193
3194 /* Merge the content and write it out */
3195 clean = handle_content_merge(&mfi, opt, path, was_dirty(opt, path),
@@ -3190,9 +3199,7 @@ static int handle_rename_normal(struct merge_options *opt,
3199 opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT &&
3200 ren->dir_rename_original_dest) {
3201 if (update_stages(opt, path,
3193 - NULL,
3194 - side == 2 ? &mfi.blob : NULL,
3195 - side == 2 ? NULL : &mfi.blob))
3202 + &mfi.blob, &mfi.blob, &mfi.blob))
3203 return -1;
3204 clean = 0; /* not clean, but conflicted */
3205 }
t/t6423-merge-rename-directories.sh
+3 -3
@@ -5000,7 +5000,7 @@ test_setup_12i () {
5000 )
5001 }
5002
5003 -test_expect_merge_algorithm failure success '12i: Directory rename causes rename-to-self' '
5003 +test_expect_success '12i: Directory rename causes rename-to-self' '
5004 test_setup_12i &&
5005 (
5006 cd 12i &&
@@ -5058,7 +5058,7 @@ test_setup_12j () {
5058 )
5059 }
5060
5061 -test_expect_merge_algorithm failure success '12j: Directory rename to root causes rename-to-self' '
5061 +test_expect_success '12j: Directory rename to root causes rename-to-self' '
5062 test_setup_12j &&
5063 (
5064 cd 12j &&
@@ -5116,7 +5116,7 @@ test_setup_12k () {
5116 )
5117 }
5118
5119 -test_expect_merge_algorithm failure success '12k: Directory rename with sibling causes rename-to-self' '
5119 +test_expect_success '12k: Directory rename with sibling causes rename-to-self' '
5120 test_setup_12k &&
5121 (
5122 cd 12k &&