patchmerge-recursive: make "CONFLICT (rename/delete)" message show both paths

> 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.

>
>  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

> Matt McCutchen <matt@mattmccutchen.net> writes:
> ...
>> 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.

>> -	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

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Matt McCutchen <matt@mattmccutchen.net> writes:
> > ...
> > > 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.
> 
> Let me make sure if I understood your changes correctly:
> 
>  * conflict_rename_delete() knew which one is renamed and which one
>    is deleted (even though the deleted one was called "other"), but
>    because in the original code handle_change_delete() wants to
>    always see tree A first and then tree B in its parameter list,
>    the original code swapped a/b before calling it.  In the original
>    code, handle_change_delete() needed to figure out which one is
>    the deleted one by looking at a_oid or b_oid.
> 
>  * In the updated code, the knowledge of which branch survives and
>    which branch is deleted is passed from the caller to
>    handle_change_delete(), which no longer needs to figure out by
>    looking at a_oid/b_oid.  The updated API only passes the oid for
>    surviving branch (as deleted one would have been 0{40} anyway).
> 
>  * In the updated code, handle_change_delete() is told the names of
>    both branches (the one that survives and the other that was
>    deleted).  It no longer has to switch between o->branch[12]
>    depending on the NULLness of a_oid/b_oid; it knows both names and
>    which one is which.
> 
>  * handle_modify_delete() also calls handle_change_delete().  Unlike
>    conflict_rename_delete(), it is not told by its caller which
>    branch keeps the path and which branch deletes the path, and
>    instead relies on handle_change_delete() to figure it out.
>    Because of the above change to the API, now it needs to sort it
>    out before calling handle_change_delete().
> 
> It all makes sense to me.  
> 
> The single call to update_file() that appears near the end of
> handle_change_delete() in the updated code corresponds to calls to
> the same function in 3 among 4 codepaths in the function in the
> original code.  It is a bit tricky to reason about, though.
> 
> In the original code, update_file() was omitted when we didn't have
> to come up with a unique alternate filename and the one that is left
> is a_oid (i.e. our side).  The way to tell if we did not come up
> with a unique alternate filename used to be to see the "renamed"
> variable but now it is the NULL-ness of "alt_path".

> And the way to
> tell if the side that is left is ours, we check to see o->branch1
> is the change_branch, not delete_branch.
> 
> So the condition to guard the call to update_file() also looks
> correct to me.

> On Wed, 2017-02-01 at 12:56 -0800, Junio C Hamano wrote:
>
>> Let me make sure if I understood your changes correctly:
>>  ...
>> So the condition to guard the call to update_file() also looks
>> correct to me.
>
> All of the above matches my understanding.  Would it have saved you
> time if I had included some of this explanation in the patch "cover
> letter"?