{"thread":{"id":"44994","subject":"[PATCH] merge-recursive: make \"CONFLICT (rename/delete)\" message show both paths","startedAt":"2017-01-28T20:54:18Z","lastAt":"2017-02-01T23:29:14Z","messageCount":5,"participants":["Matt McCutchen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"310492","messageId":"1485636764.2482.2.camel@mattmccutchen.net","threadId":"44994","inReplyTo":null,"subject":"[PATCH] merge-recursive: make \"CONFLICT (rename/delete)\" message show both paths","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2017-01-28T20:37:01Z","receivedAt":"2017-01-28T20:54:18Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"The current message printed by \"git merge-recursive\" for a rename/delete\nconflict is like this:\n\nCONFLICT (rename/delete): new-path deleted in HEAD and renamed in\nother-branch. Version other-branch of new-path left in tree.\n\nTo be more helpful, the message should show both paths of the rename and\nstate that the deletion occurred at the old path, not the new path.  So\nchange the message to the following format:\n\nCONFLICT (rename/delete): old-path deleted in HEAD and renamed to\nnew-path in other-branch. Version other-branch of new-path left in tree.\n\nSince this doubles the number of cases in handle_change_delete (modify vs.\nrename), refactor the code to halve the number of cases again by merging the\ncases where o->branch1 has the change and o->branch2 has the delete with the\ncases that are the other way around.\n\nAlso add a simple test of the new conflict message.\n\nSigned-off-by: Matt McCutchen <matt@mattmccutchen.net>\n---\n\nThis came up at:\n\nhttps://github.com/cristibalan/braid/issues/41#issuecomment-275826716\n\nPlease check that my refactoring is indeed correct!  All the existing tests pass\nfor me, but the existing test coverage of these conflict messages looks poor.\n\n merge-recursive.c              | 117 ++++++++++++++++++++++-------------------\n t/t6045-merge-rename-delete.sh |  23 ++++++++\n 2 files changed, 86 insertions(+), 54 deletions(-)\n create mode 100755 t/t6045-merge-rename-delete.sh\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex d327209..e8fce10 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1061,16 +1061,20 @@ static int merge_file_one(struct merge_options *o,\n }\n \n static int handle_change_delete(struct merge_options *o,\n-\t\t\t\t const char *path,\n+\t\t\t\t const char *path, const char *old_path,\n \t\t\t\t const struct object_id *o_oid, int o_mode,\n-\t\t\t\t const struct object_id *a_oid, int a_mode,\n-\t\t\t\t const struct object_id *b_oid, int b_mode,\n+\t\t\t\t const struct object_id *changed_oid,\n+\t\t\t\t int changed_mode,\n+\t\t\t\t const char *change_branch,\n+\t\t\t\t const char *delete_branch,\n \t\t\t\t const char *change, const char *change_past)\n {\n-\tchar *renamed = NULL;\n+\tchar *alt_path = NULL;\n+\tconst char *update_path = path;\n \tint ret = 0;\n+\n \tif (dir_in_way(path, !o->call_depth, 0)) {\n-\t\trenamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2);\n+\t\tupdate_path = alt_path = unique_path(o, path, change_branch);\n \t}\n \n \tif (o->call_depth) {\n@@ -1081,43 +1085,43 @@ static int handle_change_delete(struct merge_options *o,\n \t\t */\n \t\tret = remove_file_from_cache(path);\n \t\tif (!ret)\n-\t\t\tret = update_file(o, 0, o_oid, o_mode,\n-\t\t\t\t\t  renamed ? renamed : path);\n-\t} else if (!a_oid) {\n-\t\tif (!renamed) {\n-\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n-\t\t\t       \"and %s in %s. Version %s of %s left in tree.\"),\n-\t\t\t       change, path, o->branch1, change_past,\n-\t\t\t       o->branch2, o->branch2, path);\n-\t\t\tret = update_file(o, 0, b_oid, b_mode, path);\n-\t\t} else {\n-\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n-\t\t\t       \"and %s in %s. Version %s of %s left in tree at %s.\"),\n-\t\t\t       change, path, o->branch1, change_past,\n-\t\t\t       o->branch2, o->branch2, path, renamed);\n-\t\t\tret = update_file(o, 0, b_oid, b_mode, renamed);\n-\t\t}\n+\t\t\tret = update_file(o, 0, o_oid, o_mode, update_path);\n \t} else {\n-\t\tif (!renamed) {\n-\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n-\t\t\t       \"and %s in %s. Version %s of %s left in tree.\"),\n-\t\t\t       change, path, o->branch2, change_past,\n-\t\t\t       o->branch1, o->branch1, path);\n+\t\tif (!alt_path) {\n+\t\t\tif (!old_path) {\n+\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n+\t\t\t\t       \"and %s in %s. Version %s of %s left in tree.\"),\n+\t\t\t\t       change, path, delete_branch, change_past,\n+\t\t\t\t       change_branch, change_branch, path);\n+\t\t\t} else {\n+\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n+\t\t\t\t       \"and %s to %s in %s. Version %s of %s left in tree.\"),\n+\t\t\t\t       change, old_path, delete_branch, change_past, path,\n+\t\t\t\t       change_branch, change_branch, path);\n+\t\t\t}\n \t\t} else {\n-\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n-\t\t\t       \"and %s in %s. Version %s of %s left in tree at %s.\"),\n-\t\t\t       change, path, o->branch2, change_past,\n-\t\t\t       o->branch1, o->branch1, path, renamed);\n-\t\t\tret = update_file(o, 0, a_oid, a_mode, renamed);\n+\t\t\tif (!old_path) {\n+\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n+\t\t\t\t       \"and %s in %s. Version %s of %s left in tree at %s.\"),\n+\t\t\t\t       change, path, delete_branch, change_past,\n+\t\t\t\t       change_branch, change_branch, path, alt_path);\n+\t\t\t} else {\n+\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n+\t\t\t\t       \"and %s to %s in %s. Version %s of %s left in tree at %s.\"),\n+\t\t\t\t       change, old_path, delete_branch, change_past, path,\n+\t\t\t\t       change_branch, change_branch, path, alt_path);\n+\t\t\t}\n \t\t}\n \t\t/*\n-\t\t * No need to call update_file() on path when !renamed, since\n-\t\t * that would needlessly touch path.  We could call\n-\t\t * update_file_flags() with update_cache=0 and update_wd=0,\n-\t\t * but that's a no-op.\n+\t\t * No need to call update_file() on path when change_branch ==\n+\t\t * o->branch1 && !alt_path, since that would needlessly touch\n+\t\t * path.  We could call update_file_flags() with update_cache=0\n+\t\t * and update_wd=0, but that's a no-op.\n \t\t */\n+\t\tif (change_branch != o->branch1 || alt_path)\n+\t\t\tret = update_file(o, 0, changed_oid, changed_mode, update_path);\n \t}\n-\tfree(renamed);\n+\tfree(alt_path);\n \n \treturn ret;\n }\n@@ -1125,28 +1129,17 @@ static int handle_change_delete(struct merge_options *o,\n static int conflict_rename_delete(struct merge_options *o,\n \t\t\t\t   struct diff_filepair *pair,\n \t\t\t\t   const char *rename_branch,\n-\t\t\t\t   const char *other_branch)\n+\t\t\t\t   const char *delete_branch)\n {\n \tconst struct diff_filespec *orig = pair->one;\n \tconst struct diff_filespec *dest = pair->two;\n-\tconst struct object_id *a_oid = NULL;\n-\tconst struct object_id *b_oid = NULL;\n-\tint a_mode = 0;\n-\tint b_mode = 0;\n-\n-\tif (rename_branch == o->branch1) {\n-\t\ta_oid = &dest->oid;\n-\t\ta_mode = dest->mode;\n-\t} else {\n-\t\tb_oid = &dest->oid;\n-\t\tb_mode = dest->mode;\n-\t}\n \n \tif (handle_change_delete(o,\n \t\t\t\t o->call_depth ? orig->path : dest->path,\n+\t\t\t\t o->call_depth ? NULL : orig->path,\n \t\t\t\t &orig->oid, orig->mode,\n-\t\t\t\t a_oid, a_mode,\n-\t\t\t\t b_oid, b_mode,\n+\t\t\t\t &dest->oid, dest->mode,\n+\t\t\t\t rename_branch, delete_branch,\n \t\t\t\t _(\"rename\"), _(\"renamed\")))\n \t\treturn -1;\n \n@@ -1665,11 +1658,27 @@ static int handle_modify_delete(struct merge_options *o,\n \t\t\t\t struct object_id *a_oid, int a_mode,\n \t\t\t\t struct object_id *b_oid, int b_mode)\n {\n+\tconst char *modify_branch, *delete_branch;\n+\tstruct object_id *changed_oid;\n+\tint changed_mode;\n+\n+\tif (a_oid) {\n+\t\tmodify_branch = o->branch1;\n+\t\tdelete_branch = o->branch2;\n+\t\tchanged_oid = a_oid;\n+\t\tchanged_mode = a_mode;\n+\t} else {\n+\t\tmodify_branch = o->branch2;\n+\t\tdelete_branch = o->branch1;\n+\t\tchanged_oid = b_oid;\n+\t\tchanged_mode = b_mode;\n+\t}\n+\n \treturn handle_change_delete(o,\n-\t\t\t\t    path,\n+\t\t\t\t    path, NULL,\n \t\t\t\t    o_oid, o_mode,\n-\t\t\t\t    a_oid, a_mode,\n-\t\t\t\t    b_oid, b_mode,\n+\t\t\t\t    changed_oid, changed_mode,\n+\t\t\t\t    modify_branch, delete_branch,\n \t\t\t\t    _(\"modify\"), _(\"modified\"));\n }\n \ndiff --git a/t/t6045-merge-rename-delete.sh b/t/t6045-merge-rename-delete.sh\nnew file mode 100755\nindex 0000000..8f33244\n--- /dev/null\n+++ b/t/t6045-merge-rename-delete.sh\n@@ -0,0 +1,23 @@\n+#!/bin/sh\n+\n+test_description='Merge-recursive rename/delete conflict message'\n+. ./test-lib.sh\n+\n+test_expect_success 'rename/delete' '\n+echo foo >A &&\n+git add A &&\n+git commit -m \"initial\" &&\n+\n+git checkout -b rename &&\n+git mv A B &&\n+git commit -m \"rename\" &&\n+\n+git checkout master &&\n+git rm A &&\n+git commit -m \"delete\" &&\n+\n+test_must_fail git merge --strategy=recursive rename >output &&\n+test_i18ngrep \"CONFLICT (rename/delete): A deleted in HEAD and renamed to B in rename. Version rename of B left in tree.\" output\n+'\n+\n+test_done\n-- \n2.9.3\n\n\n"},{"id":"310575","messageId":"xmqqvaswrv5q.fsf@gitster.mtv.corp.google.com","threadId":"44994","inReplyTo":"1485636764.2482.2.camel@mattmccutchen.net","subject":"Re: [PATCH] merge-recursive: make \"CONFLICT (rename/delete)\" message show both paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-30T23:21:37Z","receivedAt":"2017-01-30T23:21:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matt McCutchen <matt@mattmccutchen.net> writes:\n\n> The current message printed by \"git merge-recursive\" for a rename/delete\n> conflict is like this:\n>\n> CONFLICT (rename/delete): new-path deleted in HEAD and renamed in\n> other-branch. Version other-branch of new-path left in tree.\n>\n> To be more helpful, the message should show both paths of the rename and\n> state that the deletion occurred at the old path, not the new path.  So\n> change the message to the following format:\n>\n> CONFLICT (rename/delete): old-path deleted in HEAD and renamed to\n> new-path in other-branch. Version other-branch of new-path left in tree.\n\nSounds like a sensible goal.\n\n> Please check that my refactoring is indeed correct!  All the existing tests pass\n> for me, but the existing test coverage of these conflict messages looks poor.\n\nThis unfortunately is doing a bit too many things at once from that\npoint of view.  I need to reserve a solid quiet 20-minutes without\ndistraction to check it, which I am hoping to do tonight.\n\nThanks.\n\n>\n>  merge-recursive.c              | 117 ++++++++++++++++++++++-------------------\n>  t/t6045-merge-rename-delete.sh |  23 ++++++++\n>  2 files changed, 86 insertions(+), 54 deletions(-)\n>  create mode 100755 t/t6045-merge-rename-delete.sh\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index d327209..e8fce10 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1061,16 +1061,20 @@ static int merge_file_one(struct merge_options *o,\n>  }\n>  \n>  static int handle_change_delete(struct merge_options *o,\n> -\t\t\t\t const char *path,\n> +\t\t\t\t const char *path, const char *old_path,\n>  \t\t\t\t const struct object_id *o_oid, int o_mode,\n> -\t\t\t\t const struct object_id *a_oid, int a_mode,\n> -\t\t\t\t const struct object_id *b_oid, int b_mode,\n> +\t\t\t\t const struct object_id *changed_oid,\n> +\t\t\t\t int changed_mode,\n> +\t\t\t\t const char *change_branch,\n> +\t\t\t\t const char *delete_branch,\n>  \t\t\t\t const char *change, const char *change_past)\n>  {\n> -\tchar *renamed = NULL;\n> +\tchar *alt_path = NULL;\n> +\tconst char *update_path = path;\n>  \tint ret = 0;\n> +\n>  \tif (dir_in_way(path, !o->call_depth, 0)) {\n> -\t\trenamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2);\n> +\t\tupdate_path = alt_path = unique_path(o, path, change_branch);\n>  \t}\n>  \n>  \tif (o->call_depth) {\n> @@ -1081,43 +1085,43 @@ static int handle_change_delete(struct merge_options *o,\n>  \t\t */\n>  \t\tret = remove_file_from_cache(path);\n>  \t\tif (!ret)\n> -\t\t\tret = update_file(o, 0, o_oid, o_mode,\n> -\t\t\t\t\t  renamed ? renamed : path);\n> -\t} else if (!a_oid) {\n> -\t\tif (!renamed) {\n> -\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n> -\t\t\t       \"and %s in %s. Version %s of %s left in tree.\"),\n> -\t\t\t       change, path, o->branch1, change_past,\n> -\t\t\t       o->branch2, o->branch2, path);\n> -\t\t\tret = update_file(o, 0, b_oid, b_mode, path);\n> -\t\t} else {\n> -\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n> -\t\t\t       \"and %s in %s. Version %s of %s left in tree at %s.\"),\n> -\t\t\t       change, path, o->branch1, change_past,\n> -\t\t\t       o->branch2, o->branch2, path, renamed);\n> -\t\t\tret = update_file(o, 0, b_oid, b_mode, renamed);\n> -\t\t}\n> +\t\t\tret = update_file(o, 0, o_oid, o_mode, update_path);\n>  \t} else {\n> -\t\tif (!renamed) {\n> -\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n> -\t\t\t       \"and %s in %s. Version %s of %s left in tree.\"),\n> -\t\t\t       change, path, o->branch2, change_past,\n> -\t\t\t       o->branch1, o->branch1, path);\n> +\t\tif (!alt_path) {\n> +\t\t\tif (!old_path) {\n> +\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n> +\t\t\t\t       \"and %s in %s. Version %s of %s left in tree.\"),\n> +\t\t\t\t       change, path, delete_branch, change_past,\n> +\t\t\t\t       change_branch, change_branch, path);\n> +\t\t\t} else {\n> +\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n> +\t\t\t\t       \"and %s to %s in %s. Version %s of %s left in tree.\"),\n> +\t\t\t\t       change, old_path, delete_branch, change_past, path,\n> +\t\t\t\t       change_branch, change_branch, path);\n> +\t\t\t}\n>  \t\t} else {\n> -\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n> -\t\t\t       \"and %s in %s. Version %s of %s left in tree at %s.\"),\n> -\t\t\t       change, path, o->branch2, change_past,\n> -\t\t\t       o->branch1, o->branch1, path, renamed);\n> -\t\t\tret = update_file(o, 0, a_oid, a_mode, renamed);\n> +\t\t\tif (!old_path) {\n> +\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n> +\t\t\t\t       \"and %s in %s. Version %s of %s left in tree at %s.\"),\n> +\t\t\t\t       change, path, delete_branch, change_past,\n> +\t\t\t\t       change_branch, change_branch, path, alt_path);\n> +\t\t\t} else {\n> +\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n> +\t\t\t\t       \"and %s to %s in %s. Version %s of %s left in tree at %s.\"),\n> +\t\t\t\t       change, old_path, delete_branch, change_past, path,\n> +\t\t\t\t       change_branch, change_branch, path, alt_path);\n> +\t\t\t}\n>  \t\t}\n>  \t\t/*\n> -\t\t * No need to call update_file() on path when !renamed, since\n> -\t\t * that would needlessly touch path.  We could call\n> -\t\t * update_file_flags() with update_cache=0 and update_wd=0,\n> -\t\t * but that's a no-op.\n> +\t\t * No need to call update_file() on path when change_branch ==\n> +\t\t * o->branch1 && !alt_path, since that would needlessly touch\n> +\t\t * path.  We could call update_file_flags() with update_cache=0\n> +\t\t * and update_wd=0, but that's a no-op.\n>  \t\t */\n> +\t\tif (change_branch != o->branch1 || alt_path)\n> +\t\t\tret = update_file(o, 0, changed_oid, changed_mode, update_path);\n>  \t}\n> -\tfree(renamed);\n> +\tfree(alt_path);\n>  \n>  \treturn ret;\n>  }\n> @@ -1125,28 +1129,17 @@ static int handle_change_delete(struct merge_options *o,\n>  static int conflict_rename_delete(struct merge_options *o,\n>  \t\t\t\t   struct diff_filepair *pair,\n>  \t\t\t\t   const char *rename_branch,\n> -\t\t\t\t   const char *other_branch)\n> +\t\t\t\t   const char *delete_branch)\n>  {\n>  \tconst struct diff_filespec *orig = pair->one;\n>  \tconst struct diff_filespec *dest = pair->two;\n> -\tconst struct object_id *a_oid = NULL;\n> -\tconst struct object_id *b_oid = NULL;\n> -\tint a_mode = 0;\n> -\tint b_mode = 0;\n> -\n> -\tif (rename_branch == o->branch1) {\n> -\t\ta_oid = &dest->oid;\n> -\t\ta_mode = dest->mode;\n> -\t} else {\n> -\t\tb_oid = &dest->oid;\n> -\t\tb_mode = dest->mode;\n> -\t}\n>  \n>  \tif (handle_change_delete(o,\n>  \t\t\t\t o->call_depth ? orig->path : dest->path,\n> +\t\t\t\t o->call_depth ? NULL : orig->path,\n>  \t\t\t\t &orig->oid, orig->mode,\n> -\t\t\t\t a_oid, a_mode,\n> -\t\t\t\t b_oid, b_mode,\n> +\t\t\t\t &dest->oid, dest->mode,\n> +\t\t\t\t rename_branch, delete_branch,\n>  \t\t\t\t _(\"rename\"), _(\"renamed\")))\n>  \t\treturn -1;\n>  \n> @@ -1665,11 +1658,27 @@ static int handle_modify_delete(struct merge_options *o,\n>  \t\t\t\t struct object_id *a_oid, int a_mode,\n>  \t\t\t\t struct object_id *b_oid, int b_mode)\n>  {\n> +\tconst char *modify_branch, *delete_branch;\n> +\tstruct object_id *changed_oid;\n> +\tint changed_mode;\n> +\n> +\tif (a_oid) {\n> +\t\tmodify_branch = o->branch1;\n> +\t\tdelete_branch = o->branch2;\n> +\t\tchanged_oid = a_oid;\n> +\t\tchanged_mode = a_mode;\n> +\t} else {\n> +\t\tmodify_branch = o->branch2;\n> +\t\tdelete_branch = o->branch1;\n> +\t\tchanged_oid = b_oid;\n> +\t\tchanged_mode = b_mode;\n> +\t}\n> +\n>  \treturn handle_change_delete(o,\n> -\t\t\t\t    path,\n> +\t\t\t\t    path, NULL,\n>  \t\t\t\t    o_oid, o_mode,\n> -\t\t\t\t    a_oid, a_mode,\n> -\t\t\t\t    b_oid, b_mode,\n> +\t\t\t\t    changed_oid, changed_mode,\n> +\t\t\t\t    modify_branch, delete_branch,\n>  \t\t\t\t    _(\"modify\"), _(\"modified\"));\n>  }\n>  \n> diff --git a/t/t6045-merge-rename-delete.sh b/t/t6045-merge-rename-delete.sh\n> new file mode 100755\n> index 0000000..8f33244\n> --- /dev/null\n> +++ b/t/t6045-merge-rename-delete.sh\n> @@ -0,0 +1,23 @@\n> +#!/bin/sh\n> +\n> +test_description='Merge-recursive rename/delete conflict message'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'rename/delete' '\n> +echo foo >A &&\n> +git add A &&\n> +git commit -m \"initial\" &&\n> +\n> +git checkout -b rename &&\n> +git mv A B &&\n> +git commit -m \"rename\" &&\n> +\n> +git checkout master &&\n> +git rm A &&\n> +git commit -m \"delete\" &&\n> +\n> +test_must_fail git merge --strategy=recursive rename >output &&\n> +test_i18ngrep \"CONFLICT (rename/delete): A deleted in HEAD and renamed to B in rename. Version rename of B left in tree.\" output\n> +'\n> +\n> +test_done\n"},{"id":"310661","messageId":"xmqqh94dockc.fsf@gitster.mtv.corp.google.com","threadId":"44994","inReplyTo":"xmqqvaswrv5q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] merge-recursive: make \"CONFLICT (rename/delete)\" message show both paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-01T20:56:03Z","receivedAt":"2017-02-01T20:56:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Matt McCutchen <matt@mattmccutchen.net> writes:\n> ...\n>> Please check that my refactoring is indeed correct!  All the existing tests pass\n>> for me, but the existing test coverage of these conflict messages looks poor.\n>\n> This unfortunately is doing a bit too many things at once from that\n> point of view.  I need to reserve a solid quiet 20-minutes without\n> distraction to check it, which I am hoping to do tonight.\n\nLet me make sure if I understood your changes correctly:\n\n * conflict_rename_delete() knew which one is renamed and which one\n   is deleted (even though the deleted one was called \"other\"), but\n   because in the original code handle_change_delete() wants to\n   always see tree A first and then tree B in its parameter list,\n   the original code swapped a/b before calling it.  In the original\n   code, handle_change_delete() needed to figure out which one is\n   the deleted one by looking at a_oid or b_oid.\n\n * In the updated code, the knowledge of which branch survives and\n   which branch is deleted is passed from the caller to\n   handle_change_delete(), which no longer needs to figure out by\n   looking at a_oid/b_oid.  The updated API only passes the oid for\n   surviving branch (as deleted one would have been 0{40} anyway).\n\n * In the updated code, handle_change_delete() is told the names of\n   both branches (the one that survives and the other that was\n   deleted).  It no longer has to switch between o->branch[12]\n   depending on the NULLness of a_oid/b_oid; it knows both names and\n   which one is which.\n\n * handle_modify_delete() also calls handle_change_delete().  Unlike\n   conflict_rename_delete(), it is not told by its caller which\n   branch keeps the path and which branch deletes the path, and\n   instead relies on handle_change_delete() to figure it out.\n   Because of the above change to the API, now it needs to sort it\n   out before calling handle_change_delete().\n\nIt all makes sense to me.  \n\nThe single call to update_file() that appears near the end of\nhandle_change_delete() in the updated code corresponds to calls to\nthe same function in 3 among 4 codepaths in the function in the\noriginal code.  It is a bit tricky to reason about, though.\n\nIn the original code, update_file() was omitted when we didn't have\nto come up with a unique alternate filename and the one that is left\nis a_oid (i.e. our side).  The way to tell if we did not come up\nwith a unique alternate filename used to be to see the \"renamed\"\nvariable but now it is the NULL-ness of \"alt_path\".  And the way to\ntell if the side that is left is ours, we check to see o->branch1\nis the change_branch, not delete_branch.\n\nSo the condition to guard the call to update_file() also looks\ncorrect to me.\n\nThanks.\n\n>> -\tchar *renamed = NULL;\n>> +\tchar *alt_path = NULL;\n>> +\tconst char *update_path = path;\n>>  \tint ret = 0;\n>> +\n>>  \tif (dir_in_way(path, !o->call_depth, 0)) {\n>> -\t\trenamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2);\n>> +\t\tupdate_path = alt_path = unique_path(o, path, change_branch);\n>>  \t}\n>>  \n>>  \tif (o->call_depth) {\n>> @@ -1081,43 +1085,43 @@ static int handle_change_delete(struct merge_options *o,\n>>  \t\t */\n>>  \t\tret = remove_file_from_cache(path);\n>>  \t\tif (!ret)\n>> -\t\t\tret = update_file(o, 0, o_oid, o_mode,\n>> -\t\t\t\t\t  renamed ? renamed : path);\n>> -\t} else if (!a_oid) {\n>> -\t\tif (!renamed) {\n>> -\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n>> -\t\t\t       \"and %s in %s. Version %s of %s left in tree.\"),\n>> -\t\t\t       change, path, o->branch1, change_past,\n>> -\t\t\t       o->branch2, o->branch2, path);\n>> -\t\t\tret = update_file(o, 0, b_oid, b_mode, path);\n>> -\t\t} else {\n>> -\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n>> -\t\t\t       \"and %s in %s. Version %s of %s left in tree at %s.\"),\n>> -\t\t\t       change, path, o->branch1, change_past,\n>> -\t\t\t       o->branch2, o->branch2, path, renamed);\n>> -\t\t\tret = update_file(o, 0, b_oid, b_mode, renamed);\n>> -\t\t}\n>> +\t\t\tret = update_file(o, 0, o_oid, o_mode, update_path);\n>>  \t} else {\n>> -\t\tif (!renamed) {\n>> -\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n>> -\t\t\t       \"and %s in %s. Version %s of %s left in tree.\"),\n>> -\t\t\t       change, path, o->branch2, change_past,\n>> -\t\t\t       o->branch1, o->branch1, path);\n>> +\t\tif (!alt_path) {\n>> +\t\t\tif (!old_path) {\n>> +\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n>> +\t\t\t\t       \"and %s in %s. Version %s of %s left in tree.\"),\n>> +\t\t\t\t       change, path, delete_branch, change_past,\n>> +\t\t\t\t       change_branch, change_branch, path);\n>> +\t\t\t} else {\n>> +\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n>> +\t\t\t\t       \"and %s to %s in %s. Version %s of %s left in tree.\"),\n>> +\t\t\t\t       change, old_path, delete_branch, change_past, path,\n>> +\t\t\t\t       change_branch, change_branch, path);\n>> +\t\t\t}\n>>  \t\t} else {\n>> -\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n>> -\t\t\t       \"and %s in %s. Version %s of %s left in tree at %s.\"),\n>> -\t\t\t       change, path, o->branch2, change_past,\n>> -\t\t\t       o->branch1, o->branch1, path, renamed);\n>> -\t\t\tret = update_file(o, 0, a_oid, a_mode, renamed);\n>> +\t\t\tif (!old_path) {\n>> +\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n>> +\t\t\t\t       \"and %s in %s. Version %s of %s left in tree at %s.\"),\n>> +\t\t\t\t       change, path, delete_branch, change_past,\n>> +\t\t\t\t       change_branch, change_branch, path, alt_path);\n>> +\t\t\t} else {\n>> +\t\t\t\toutput(o, 1, _(\"CONFLICT (%s/delete): %s deleted in %s \"\n>> +\t\t\t\t       \"and %s to %s in %s. Version %s of %s left in tree at %s.\"),\n>> +\t\t\t\t       change, old_path, delete_branch, change_past, path,\n>> +\t\t\t\t       change_branch, change_branch, path, alt_path);\n>> +\t\t\t}\n>>  \t\t}\n>>  \t\t/*\n>> -\t\t * No need to call update_file() on path when !renamed, since\n>> -\t\t * that would needlessly touch path.  We could call\n>> -\t\t * update_file_flags() with update_cache=0 and update_wd=0,\n>> -\t\t * but that's a no-op.\n>> +\t\t * No need to call update_file() on path when change_branch ==\n>> +\t\t * o->branch1 && !alt_path, since that would needlessly touch\n>> +\t\t * path.  We could call update_file_flags() with update_cache=0\n>> +\t\t * and update_wd=0, but that's a no-op.\n>>  \t\t */\n>> +\t\tif (change_branch != o->branch1 || alt_path)\n>> +\t\t\tret = update_file(o, 0, changed_oid, changed_mode, update_path);\n>>  \t}\n>> -\tfree(renamed);\n>> +\tfree(alt_path);\n>>  \n>>  \treturn ret;\n>>  }\n>> @@ -1125,28 +1129,17 @@ static int handle_change_delete(struct merge_options *o,\n>>  static int conflict_rename_delete(struct merge_options *o,\n>>  \t\t\t\t   struct diff_filepair *pair,\n>>  \t\t\t\t   const char *rename_branch,\n>> -\t\t\t\t   const char *other_branch)\n>> +\t\t\t\t   const char *delete_branch)\n>>  {\n>>  \tconst struct diff_filespec *orig = pair->one;\n>>  \tconst struct diff_filespec *dest = pair->two;\n>> -\tconst struct object_id *a_oid = NULL;\n>> -\tconst struct object_id *b_oid = NULL;\n>> -\tint a_mode = 0;\n>> -\tint b_mode = 0;\n>> -\n>> -\tif (rename_branch == o->branch1) {\n>> -\t\ta_oid = &dest->oid;\n>> -\t\ta_mode = dest->mode;\n>> -\t} else {\n>> -\t\tb_oid = &dest->oid;\n>> -\t\tb_mode = dest->mode;\n>> -\t}\n>>  \n>>  \tif (handle_change_delete(o,\n>>  \t\t\t\t o->call_depth ? orig->path : dest->path,\n>> +\t\t\t\t o->call_depth ? NULL : orig->path,\n>>  \t\t\t\t &orig->oid, orig->mode,\n>> -\t\t\t\t a_oid, a_mode,\n>> -\t\t\t\t b_oid, b_mode,\n>> +\t\t\t\t &dest->oid, dest->mode,\n>> +\t\t\t\t rename_branch, delete_branch,\n>>  \t\t\t\t _(\"rename\"), _(\"renamed\")))\n>>  \t\treturn -1;\n>>  \n>> @@ -1665,11 +1658,27 @@ static int handle_modify_delete(struct merge_options *o,\n>>  \t\t\t\t struct object_id *a_oid, int a_mode,\n>>  \t\t\t\t struct object_id *b_oid, int b_mode)\n>>  {\n>> +\tconst char *modify_branch, *delete_branch;\n>> +\tstruct object_id *changed_oid;\n>> +\tint changed_mode;\n>> +\n>> +\tif (a_oid) {\n>> +\t\tmodify_branch = o->branch1;\n>> +\t\tdelete_branch = o->branch2;\n>> +\t\tchanged_oid = a_oid;\n>> +\t\tchanged_mode = a_mode;\n>> +\t} else {\n>> +\t\tmodify_branch = o->branch2;\n>> +\t\tdelete_branch = o->branch1;\n>> +\t\tchanged_oid = b_oid;\n>> +\t\tchanged_mode = b_mode;\n>> +\t}\n>> +\n>>  \treturn handle_change_delete(o,\n>> -\t\t\t\t    path,\n>> +\t\t\t\t    path, NULL,\n>>  \t\t\t\t    o_oid, o_mode,\n>> -\t\t\t\t    a_oid, a_mode,\n>> -\t\t\t\t    b_oid, b_mode,\n>> +\t\t\t\t    changed_oid, changed_mode,\n>> +\t\t\t\t    modify_branch, delete_branch,\n>>  \t\t\t\t    _(\"modify\"), _(\"modified\"));\n>>  }\n>>  \n>> diff --git a/t/t6045-merge-rename-delete.sh b/t/t6045-merge-rename-delete.sh\n>> new file mode 100755\n>> index 0000000..8f33244\n>> --- /dev/null\n>> +++ b/t/t6045-merge-rename-delete.sh\n>> @@ -0,0 +1,23 @@\n>> +#!/bin/sh\n>> +\n>> +test_description='Merge-recursive rename/delete conflict message'\n>> +. ./test-lib.sh\n>> +\n>> +test_expect_success 'rename/delete' '\n>> +echo foo >A &&\n>> +git add A &&\n>> +git commit -m \"initial\" &&\n>> +\n>> +git checkout -b rename &&\n>> +git mv A B &&\n>> +git commit -m \"rename\" &&\n>> +\n>> +git checkout master &&\n>> +git rm A &&\n>> +git commit -m \"delete\" &&\n>> +\n>> +test_must_fail git merge --strategy=recursive rename >output &&\n>> +test_i18ngrep \"CONFLICT (rename/delete): A deleted in HEAD and renamed to B in rename. Version rename of B left in tree.\" output\n>> +'\n>> +\n>> +test_done\n"},{"id":"310684","messageId":"1485991441.28767.2.camel@mattmccutchen.net","threadId":"44994","inReplyTo":"xmqqh94dockc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] merge-recursive: make \"CONFLICT (rename/delete)\" message show both paths","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2017-02-01T23:24:01Z","receivedAt":"2017-02-01T23:24:13Z","isPatch":true,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Wed, 2017-02-01 at 12:56 -0800, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Matt McCutchen <matt@mattmccutchen.net> writes:\n> > ...\n> > > Please check that my refactoring is indeed correct!  All the\n> > > existing tests pass\n> > > for me, but the existing test coverage of these conflict messages\n> > > looks poor.\n> > \n> > This unfortunately is doing a bit too many things at once from that\n> > point of view.  I need to reserve a solid quiet 20-minutes without\n> > distraction to check it, which I am hoping to do tonight.\n> \n> Let me make sure if I understood your changes correctly:\n> \n>  * conflict_rename_delete() knew which one is renamed and which one\n>    is deleted (even though the deleted one was called \"other\"), but\n>    because in the original code handle_change_delete() wants to\n>    always see tree A first and then tree B in its parameter list,\n>    the original code swapped a/b before calling it.  In the original\n>    code, handle_change_delete() needed to figure out which one is\n>    the deleted one by looking at a_oid or b_oid.\n> \n>  * In the updated code, the knowledge of which branch survives and\n>    which branch is deleted is passed from the caller to\n>    handle_change_delete(), which no longer needs to figure out by\n>    looking at a_oid/b_oid.  The updated API only passes the oid for\n>    surviving branch (as deleted one would have been 0{40} anyway).\n> \n>  * In the updated code, handle_change_delete() is told the names of\n>    both branches (the one that survives and the other that was\n>    deleted).  It no longer has to switch between o->branch[12]\n>    depending on the NULLness of a_oid/b_oid; it knows both names and\n>    which one is which.\n> \n>  * handle_modify_delete() also calls handle_change_delete().  Unlike\n>    conflict_rename_delete(), it is not told by its caller which\n>    branch keeps the path and which branch deletes the path, and\n>    instead relies on handle_change_delete() to figure it out.\n>    Because of the above change to the API, now it needs to sort it\n>    out before calling handle_change_delete().\n> \n> It all makes sense to me.  \n> \n> The single call to update_file() that appears near the end of\n> handle_change_delete() in the updated code corresponds to calls to\n> the same function in 3 among 4 codepaths in the function in the\n> original code.  It is a bit tricky to reason about, though.\n> \n> In the original code, update_file() was omitted when we didn't have\n> to come up with a unique alternate filename and the one that is left\n> is a_oid (i.e. our side).  The way to tell if we did not come up\n> with a unique alternate filename used to be to see the \"renamed\"\n> variable but now it is the NULL-ness of \"alt_path\".\n\n\"alt_path\" is the same variable that used to be \"renamed\".  I just\nrenamed it to be less confusing.\n\n> And the way to\n> tell if the side that is left is ours, we check to see o->branch1\n> is the change_branch, not delete_branch.\n> \n> So the condition to guard the call to update_file() also looks\n> correct to me.\n\nAll of the above matches my understanding.  Would it have saved you\ntime if I had included some of this explanation in the patch \"cover\nletter\"?\n\nMatt\n\n"},{"id":"310686","messageId":"xmqqa8a5mqxa.fsf@gitster.mtv.corp.google.com","threadId":"44994","inReplyTo":"1485991441.28767.2.camel@mattmccutchen.net","subject":"Re: [PATCH] merge-recursive: make \"CONFLICT (rename/delete)\" message show both paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-01T23:28:49Z","receivedAt":"2017-02-01T23:29:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matt McCutchen <matt@mattmccutchen.net> writes:\n\n> On Wed, 2017-02-01 at 12:56 -0800, Junio C Hamano wrote:\n>\n>> Let me make sure if I understood your changes correctly:\n>>  ...\n>> So the condition to guard the call to update_file() also looks\n>> correct to me.\n>\n> All of the above matches my understanding.  Would it have saved you\n> time if I had included some of this explanation in the patch \"cover\n> letter\"?\n\nThe fact that I arrived at the same understanding by reading the\nchange without peeking at such a cheat-sheet gives me more peace of\nmind ;-)\n\nThanks.\n"}]}