Subject: Re: [PATCH] merge-recursive: make "CONFLICT (rename/delete)" message show both paths

From: Junio C Hamano <[email-masked]>
Date: Mon, 30 Jan 2017 15:21:37 -0800

Matt McCutchen <[email-masked]> writes:

> The current message printed by "git merge-recursive" for a rename/delete
> conflict is like this:
>
> CONFLICT (rename/delete): new-path deleted in HEAD and renamed in
> other-branch. Version other-branch of new-path left in tree.
>
> To be more helpful, the message should show both paths of the rename and
> state that the deletion occurred at the old path, not the new path.  So
> change the message to the following format:
>
> CONFLICT (rename/delete): old-path deleted in HEAD and renamed to
> new-path in other-branch. Version other-branch of new-path left in tree.

Sounds like a sensible goal.

> Please check that my refactoring is indeed correct!  All the existing tests pass
> for me, but the existing test coverage of these conflict messages looks poor.

This unfortunately is doing a bit too many things at once from that
point of view.  I need to reserve a solid quiet 20-minutes without
distraction to check it, which I am hoping to do tonight.

Thanks.

>
>  merge-recursive.c              | 117 ++++++++++++++++++++++-------------------
>  t/t6045-merge-rename-delete.sh |  23 ++++++++
>  2 files changed, 86 insertions(+), 54 deletions(-)
>  create mode 100755 t/t6045-merge-rename-delete.sh
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index d327209..e8fce10 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1061,16 +1061,20 @@ static int merge_file_one(struct merge_options *o,
>  }
>  
>  static int handle_change_delete(struct merge_options *o,
> -				 const char *path,
> +				 const char *path, const char *old_path,
>  				 const struct object_id *o_oid, int o_mode,
> -				 const struct object_id *a_oid, int a_mode,
> -				 const struct object_id *b_oid, int b_mode,
> +				 const struct object_id *changed_oid,
> +				 int changed_mode,
> +				 const char *change_branch,
> +				 const char *delete_branch,
>  				 const char *change, const char *change_past)
>  {
> -	char *renamed = NULL;
> +	char *alt_path = NULL;
> +	const char *update_path = path;
>  	int ret = 0;
> +
>  	if (dir_in_way(path, !o->call_depth, 0)) {
> -		renamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2);
> +		update_path = alt_path = unique_path(o, path, change_branch);
>  	}
>  
>  	if (o->call_depth) {
> @@ -1081,43 +1085,43 @@ static int handle_change_delete(struct merge_options *o,
>  		 */
>  		ret = remove_file_from_cache(path);
>  		if (!ret)
> -			ret = update_file(o, 0, o_oid, o_mode,
> -					  renamed ? renamed : path);
> -	} else if (!a_oid) {
> -		if (!renamed) {
> -			output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> -			       "and %s in %s. Version %s of %s left in tree."),
> -			       change, path, o->branch1, change_past,
> -			       o->branch2, o->branch2, path);
> -			ret = update_file(o, 0, b_oid, b_mode, path);
> -		} else {
> -			output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> -			       "and %s in %s. Version %s of %s left in tree at %s."),
> -			       change, path, o->branch1, change_past,
> -			       o->branch2, o->branch2, path, renamed);
> -			ret = update_file(o, 0, b_oid, b_mode, renamed);
> -		}
> +			ret = update_file(o, 0, o_oid, o_mode, update_path);
>  	} else {
> -		if (!renamed) {
> -			output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> -			       "and %s in %s. Version %s of %s left in tree."),
> -			       change, path, o->branch2, change_past,
> -			       o->branch1, o->branch1, path);
> +		if (!alt_path) {
> +			if (!old_path) {
> +				output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> +				       "and %s in %s. Version %s of %s left in tree."),
> +				       change, path, delete_branch, change_past,
> +				       change_branch, change_branch, path);
> +			} else {
> +				output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> +				       "and %s to %s in %s. Version %s of %s left in tree."),
> +				       change, old_path, delete_branch, change_past, path,
> +				       change_branch, change_branch, path);
> +			}
>  		} else {
> -			output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> -			       "and %s in %s. Version %s of %s left in tree at %s."),
> -			       change, path, o->branch2, change_past,
> -			       o->branch1, o->branch1, path, renamed);
> -			ret = update_file(o, 0, a_oid, a_mode, renamed);
> +			if (!old_path) {
> +				output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> +				       "and %s in %s. Version %s of %s left in tree at %s."),
> +				       change, path, delete_branch, change_past,
> +				       change_branch, change_branch, path, alt_path);
> +			} else {
> +				output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
> +				       "and %s to %s in %s. Version %s of %s left in tree at %s."),
> +				       change, old_path, delete_branch, change_past, path,
> +				       change_branch, change_branch, path, alt_path);
> +			}
>  		}
>  		/*
> -		 * No need to call update_file() on path when !renamed, since
> -		 * that would needlessly touch path.  We could call
> -		 * update_file_flags() with update_cache=0 and update_wd=0,
> -		 * but that's a no-op.
> +		 * No need to call update_file() on path when change_branch ==
> +		 * o->branch1 && !alt_path, since that would needlessly touch
> +		 * path.  We could call update_file_flags() with update_cache=0
> +		 * and update_wd=0, but that's a no-op.
>  		 */
> +		if (change_branch != o->branch1 || alt_path)
> +			ret = update_file(o, 0, changed_oid, changed_mode, update_path);
>  	}
> -	free(renamed);
> +	free(alt_path);
>  
>  	return ret;
>  }
> @@ -1125,28 +1129,17 @@ static int handle_change_delete(struct merge_options *o,
>  static int conflict_rename_delete(struct merge_options *o,
>  				   struct diff_filepair *pair,
>  				   const char *rename_branch,
> -				   const char *other_branch)
> +				   const char *delete_branch)
>  {
>  	const struct diff_filespec *orig = pair->one;
>  	const struct diff_filespec *dest = pair->two;
> -	const struct object_id *a_oid = NULL;
> -	const struct object_id *b_oid = NULL;
> -	int a_mode = 0;
> -	int b_mode = 0;
> -
> -	if (rename_branch == o->branch1) {
> -		a_oid = &dest->oid;
> -		a_mode = dest->mode;
> -	} else {
> -		b_oid = &dest->oid;
> -		b_mode = dest->mode;
> -	}
>  
>  	if (handle_change_delete(o,
>  				 o->call_depth ? orig->path : dest->path,
> +				 o->call_depth ? NULL : orig->path,
>  				 &orig->oid, orig->mode,
> -				 a_oid, a_mode,
> -				 b_oid, b_mode,
> +				 &dest->oid, dest->mode,
> +				 rename_branch, delete_branch,
>  				 _("rename"), _("renamed")))
>  		return -1;
>  
> @@ -1665,11 +1658,27 @@ static int handle_modify_delete(struct merge_options *o,
>  				 struct object_id *a_oid, int a_mode,
>  				 struct object_id *b_oid, int b_mode)
>  {
> +	const char *modify_branch, *delete_branch;
> +	struct object_id *changed_oid;
> +	int changed_mode;
> +
> +	if (a_oid) {
> +		modify_branch = o->branch1;
> +		delete_branch = o->branch2;
> +		changed_oid = a_oid;
> +		changed_mode = a_mode;
> +	} else {
> +		modify_branch = o->branch2;
> +		delete_branch = o->branch1;
> +		changed_oid = b_oid;
> +		changed_mode = b_mode;
> +	}
> +
>  	return handle_change_delete(o,
> -				    path,
> +				    path, NULL,
>  				    o_oid, o_mode,
> -				    a_oid, a_mode,
> -				    b_oid, b_mode,
> +				    changed_oid, changed_mode,
> +				    modify_branch, delete_branch,
>  				    _("modify"), _("modified"));
>  }
>  
> diff --git a/t/t6045-merge-rename-delete.sh b/t/t6045-merge-rename-delete.sh
> new file mode 100755
> index 0000000..8f33244
> --- /dev/null
> +++ b/t/t6045-merge-rename-delete.sh
> @@ -0,0 +1,23 @@
> +#!/bin/sh
> +
> +test_description='Merge-recursive rename/delete conflict message'
> +. ./test-lib.sh
> +
> +test_expect_success 'rename/delete' '
> +echo foo >A &&
> +git add A &&
> +git commit -m "initial" &&
> +
> +git checkout -b rename &&
> +git mv A B &&
> +git commit -m "rename" &&
> +
> +git checkout master &&
> +git rm A &&
> +git commit -m "delete" &&
> +
> +test_must_fail git merge --strategy=recursive rename >output &&
> +test_i18ngrep "CONFLICT (rename/delete): A deleted in HEAD and renamed to B in rename. Version rename of B left in tree." output
> +'
> +
> +test_done