Re: [PATCH 3/3] merge-recursive: handle rename-to-self case

[Junio C Hamano <gitster@pobox.com>]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> 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@mit.edu>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  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 &&