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

From: Junio C Hamano <[email-masked]>
Date: Wed, 01 Feb 2017 12:56:03 -0800

Junio C Hamano <[email-masked]> writes:

> Matt McCutchen <[email-masked]> 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.

Thanks.

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