Matt McCutchen <m...@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
