Junio C Hamano <gits...@pobox.com> writes:
> Matt McCutchen <m...@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
