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

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

[Matt McCutchen]

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.

Since this doubles the number of cases in handle_change_delete (modify vs.
rename), refactor the code to halve the number of cases again by merging the
cases where o->branch1 has the change and o->branch2 has the delete with the
cases that are the other way around.

Also add a simple test of the new conflict message.

Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
---

This came up at:

https://github.com/cristibalan/braid/issues/41#issuecomment-275826716

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.

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

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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.

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

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

[Matt McCutchen]

On Wed, 2017-02-01 at 12:56 -0800, Junio C Hamano wrote:
> 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".

"alt_path" is the same variable that used to be "renamed".  I just
renamed it to be less confusing.

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

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

Matt

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

[Junio C Hamano]

Matt McCutchen <matt@mattmccutchen.net> writes:

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

The fact that I arrived at the same understanding by reading the
change without peeking at such a cheat-sheet gives me more peace of
mind ;-)

Thanks.