"Elijah Newren via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Elijah Newren <newren@xxxxxxxxx>
>
> 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.
Heh, of course this is recent because the "guess that directory B
has been moved to C in its entirety" heuristic is quite new.
Nicely fixed and described. Will queue. Thanks.
>
> 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@xxxxxxx>
> Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
> ---
> merge-recursive.c | 19 +++++++++++++------
> t/t6423-merge-rename-directories.sh | 4 ++--
> 2 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index d146bb116f7..c895145a8f5 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2804,12 +2804,19 @@ static int process_renames(struct merge_options *opt,
> int renamed_stage = a_renames == renames1 ? 2 : 3;
> int other_stage = a_renames == renames1 ? 3 : 2;
>
> + /*
> + * Directory renames have a funny corner case...
> + */
> + int renamed_to_self = !strcmp(ren1_src, ren1_dst);
> +
> /* BUG: We should only remove ren1_src in the base
> * stage and in other_stage (think of rename +
> * add-source case).
> */
> - remove_file(opt, 1, ren1_src,
> - renamed_stage == 2 || !was_tracked(opt, ren1_src));
> + if (!renamed_to_self)
> + remove_file(opt, 1, ren1_src,
> + renamed_stage == 2 ||
> + !was_tracked(opt, ren1_src));
>
> oidcpy(&src_other.oid,
> &ren1->src_entry->stages[other_stage].oid);
> @@ -2823,6 +2830,9 @@ static int process_renames(struct merge_options *opt,
> ren1->dir_rename_original_type == 'A') {
> setup_rename_conflict_info(RENAME_VIA_DIR,
> opt, ren1, NULL);
> + } else if (renamed_to_self) {
> + setup_rename_conflict_info(RENAME_NORMAL,
> + opt, ren1, NULL);
> } else if (oideq(&src_other.oid, null_oid())) {
> setup_rename_conflict_info(RENAME_DELETE,
> opt, ren1, NULL);
> @@ -3180,7 +3190,6 @@ static int handle_rename_normal(struct merge_options *opt,
> struct rename *ren = ci->ren1;
> struct merge_file_info mfi;
> int clean;
> - int side = (ren->branch == opt->branch1 ? 2 : 3);
>
> /* Merge the content and write it out */
> clean = handle_content_merge(&mfi, opt, path, was_dirty(opt, path),
> @@ -3190,9 +3199,7 @@ static int handle_rename_normal(struct merge_options *opt,
> opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT &&
> ren->dir_rename_original_dest) {
> if (update_stages(opt, path,
> - NULL,
> - side == 2 ? &mfi.blob : NULL,
> - side == 2 ? NULL : &mfi.blob))
> + &mfi.blob, &mfi.blob, &mfi.blob))
> return -1;
> clean = 0; /* not clean, but conflicted */
> }
> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
> index 7480daab46a..d67b5c28944 100755
> --- a/t/t6423-merge-rename-directories.sh
> +++ b/t/t6423-merge-rename-directories.sh
> @@ -5058,7 +5058,7 @@ test_setup_12i () {
> )
> }
>
> -test_expect_merge_algorithm failure success '12i: Directory rename causes rename-to-self' '
> +test_expect_success '12i: Directory rename causes rename-to-self' '
> test_setup_12i &&
> (
> cd 12i &&
> @@ -5116,7 +5116,7 @@ test_setup_12j () {
> )
> }
>
> -test_expect_merge_algorithm failure success '12j: Directory rename to root causes rename-to-self' '
> +test_expect_success '12j: Directory rename to root causes rename-to-self' '
> test_setup_12j &&
> (
> cd 12j &&
