| Message ID | 20181012212551.7689-3-newren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More merge cleanups | expand |
Elijah Newren <newren@gmail.com> writes: > When using merge.conflictstyle=diff3 to have the "base version" be shown > in conflicts, there is the possibility that the base version itself has > conflicts in it. This comes about when there are more than one merge > base, and the merging of those merge bases produces a conflict. > Since this process applies recursively, it is possible to have conflict > markers nested at an arbitrary depth; to be able to differentiate the > conflict markers from different nestings, we make them all of different > lengths. I know it is possible that the common ancestor part that is enclosed by the outermost makers can have arbitrary conflicts, and they can be even recursive conflicts. But I fail to see why it is a problem. Perhaps that is because I am not particularly good at resolving merge conflicts, but as long as the common part of the outermost merge is identifyable, would that really matter? What I would do while looking at common ancestor part with conflicts (not even a recursive one) is just to ignore it, so... Not that I strongly oppose to incrementing the marker length at every level. I do not think it would hurt, but I just do not see how it would help.
On Sun, Oct 14, 2018 at 10:12 PM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > When using merge.conflictstyle=diff3 to have the "base version" be shown > > in conflicts, there is the possibility that the base version itself has > > conflicts in it. This comes about when there are more than one merge > > base, and the merging of those merge bases produces a conflict. > > Since this process applies recursively, it is possible to have conflict > > markers nested at an arbitrary depth; to be able to differentiate the > > conflict markers from different nestings, we make them all of different > > lengths. > > I know it is possible that the common ancestor part that is enclosed > by the outermost makers can have arbitrary conflicts, and they can > be even recursive conflicts. But I fail to see why it is a problem. > Perhaps that is because I am not particularly good at resolving > merge conflicts, but as long as the common part of the outermost > merge is identifyable, would that really matter? What I would do > while looking at common ancestor part with conflicts (not even a > recursive one) is just to ignore it, so... > > Not that I strongly oppose to incrementing the marker length at > every level. I do not think it would hurt, but I just do not see > how it would help. Fair enough. The real motivation for these changes was the modification to rename/rename(2to1) conflicts (and rename/add conflicts) to behave like add/add conflicts -- that change means we can have nested conflict markers even without invoking the recursive part of the recursive machinery. So, I needed a way to increase the length of the conflict markers besides just checking opts->virtual_ancestor. Just using a fixed extra indent seemed problematic, because if I also had to worry about even one virtual_ancestor, then I was already dealing with the possibility of triply nested conflict markers and only one of them from a virtual merge base. See t6036 in https://public-inbox.org/git/20181014020537.17991-3-newren@gmail.com/. However, that series was long enough, so to try to simplify review I split as much as I could out of it. That resulted, among other things, in me submitting this marker nesting change as part of this series using a more limited rationale. Would you like me to edit the commit message to include this more difficult case?
Elijah Newren <newren@gmail.com> writes: > Would you like me to edit the commit message to include this more > difficult case? Neither. If the "marker length" change is required in a separate series that will build on top of the current 4-patch series, I think dropping this step from the current series and make it a part of the series that deals with rename/rename would make more sense.
On Mon, Oct 15, 2018 at 7:17 PM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > Would you like me to edit the commit message to include this more > > difficult case? > > Neither. If the "marker length" change is required in a separate > series that will build on top of the current 4-patch series, I think > dropping this step from the current series and make it a part of the > series that deals with rename/rename would make more sense. Okay, I'll resubmit this series without this patch or associated testcase, and include those in the later file collision series.
diff --git a/ll-merge.c b/ll-merge.c index 0e2800f7bb..aabc1b5c2e 100644 --- a/ll-merge.c +++ b/ll-merge.c @@ -384,7 +384,9 @@ int ll_merge(mmbuffer_t *result_buf, if (opts->virtual_ancestor) { if (driver->recursive) driver = find_ll_merge_driver(driver->recursive); - marker_size += 2; + } + if (opts->extra_marker_size) { + marker_size += opts->extra_marker_size; } return driver->fn(driver, result_buf, path, ancestor, ancestor_label, ours, our_label, theirs, their_label, diff --git a/ll-merge.h b/ll-merge.h index b72b19921e..5b4e158502 100644 --- a/ll-merge.h +++ b/ll-merge.h @@ -11,6 +11,7 @@ struct ll_merge_options { unsigned virtual_ancestor : 1; unsigned variant : 2; /* favor ours, favor theirs, or union merge */ unsigned renormalize : 1; + unsigned extra_marker_size; long xdl_opts; }; diff --git a/merge-recursive.c b/merge-recursive.c index 5206d6cfb6..2452788d28 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1039,7 +1039,8 @@ static int merge_3way(struct merge_options *o, const struct diff_filespec *a, const struct diff_filespec *b, const char *branch1, - const char *branch2) + const char *branch2, + const int extra_marker_size) { mmfile_t orig, src1, src2; struct ll_merge_options ll_opts = {0}; @@ -1047,6 +1048,7 @@ static int merge_3way(struct merge_options *o, int merge_status; ll_opts.renormalize = o->renormalize; + ll_opts.extra_marker_size = extra_marker_size; ll_opts.xdl_opts = o->xdl_opts; if (o->call_depth) { @@ -1281,6 +1283,7 @@ static int merge_mode_and_contents(struct merge_options *o, const char *filename, const char *branch1, const char *branch2, + const int extra_marker_size, struct merge_file_info *result) { result->merge = 0; @@ -1321,7 +1324,8 @@ static int merge_mode_and_contents(struct merge_options *o, int ret = 0, merge_status; merge_status = merge_3way(o, &result_buf, one, a, b, - branch1, branch2); + branch1, branch2, + extra_marker_size); if ((merge_status < 0) || !result_buf.ptr) ret = err(o, _("Failed to execute internal merge")); @@ -1610,7 +1614,8 @@ static int handle_rename_rename_1to2(struct merge_options *o, struct diff_filespec other; struct diff_filespec *add; if (merge_mode_and_contents(o, one, a, b, one->path, - ci->branch1, ci->branch2, &mfi)) + ci->branch1, ci->branch2, + o->call_depth * 2, &mfi)) return -1; /* @@ -1677,9 +1682,11 @@ static int handle_rename_rename_2to1(struct merge_options *o, path_side_1_desc = xstrfmt("%s (was %s)", path, a->path); path_side_2_desc = xstrfmt("%s (was %s)", path, b->path); if (merge_mode_and_contents(o, a, c1, &ci->ren1_other, path_side_1_desc, - o->branch1, o->branch2, &mfi_c1) || + o->branch1, o->branch2, + o->call_depth * 2, &mfi_c1) || merge_mode_and_contents(o, b, &ci->ren2_other, c2, path_side_2_desc, - o->branch1, o->branch2, &mfi_c2)) + o->branch1, o->branch2, + o->call_depth * 2, &mfi_c2)) return -1; free(path_side_1_desc); free(path_side_2_desc); @@ -2725,7 +2732,7 @@ static int process_renames(struct merge_options *o, if (merge_mode_and_contents(o, &one, &a, &b, ren1_dst, branch1, branch2, - &mfi)) { + o->call_depth * 2, &mfi)) { clean_merge = -1; goto cleanup_and_return; } @@ -3022,7 +3029,8 @@ static int handle_content_merge(struct merge_options *o, df_conflict_remains = 1; } if (merge_mode_and_contents(o, &one, &a, &b, path, - o->branch1, o->branch2, &mfi)) + o->branch1, o->branch2, + o->call_depth * 2, &mfi)) return -1; /* diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh index 1d1135082c..21954db624 100755 --- a/t/t6036-recursive-corner-cases.sh +++ b/t/t6036-recursive-corner-cases.sh @@ -1487,7 +1487,7 @@ test_expect_success "setup virtual merge base with nested conflicts" ' ) ' -test_expect_failure "check virtual merge base with nested conflicts" ' +test_expect_success "check virtual merge base with nested conflicts" ' ( cd virtual_merge_base_has_nested_conflicts &&
When using merge.conflictstyle=diff3 to have the "base version" be shown in conflicts, there is the possibility that the base version itself has conflicts in it. This comes about when there are more than one merge base, and the merging of those merge bases produces a conflict. Since this process applies recursively, it is possible to have conflict markers nested at an arbitrary depth; to be able to differentiate the conflict markers from different nestings, we make them all of different lengths. Signed-off-by: Elijah Newren <newren@gmail.com> --- ll-merge.c | 4 +++- ll-merge.h | 1 + merge-recursive.c | 22 +++++++++++++++------- t/t6036-recursive-corner-cases.sh | 2 +- 4 files changed, 20 insertions(+), 9 deletions(-)