{"thread":{"id":"49554","subject":"[PATCH 0/4] More merge cleanups","startedAt":"2018-10-12T21:25:59Z","lastAt":"2018-10-18T06:09:56Z","messageCount":15,"participants":["Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"360351","messageId":"20181012212551.7689-1-newren@gmail.com","threadId":"49554","inReplyTo":null,"subject":"[PATCH 0/4] More merge cleanups","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-10-12T21:25:47Z","receivedAt":"2018-10-12T21:25:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series builds on en/merge-cleanup. Technically, this could be\nbroken into three separate topics with only one of them depending on\nen/merge-cleanup, but I have a subsequent series that depends on both\nen/merge-cleanup and the fixes here, so I'm submitting these four\npatches as a set.\n\nElijah Newren (4):\n t6036: add testcase where virtual merge base contains nested conflicts\n merge-recursive: increase marker length with depth of recursion\n\n Improving diff3 conflict markers in the face of arbitrarily deeply\n nested conflicts\n\n merge-recursive: improve auto-merging messages with path collisions\n\n Simple attempt at wording improvement\n\n merge-recursive: Avoid showing conflicts with merge branch before HEAD\n\n Conflict markers are expected to be shown in a certain order\n\n ll-merge.c | 4 +-\n ll-merge.h | 1 +\n merge-recursive.c | 59 +++++++++--\n t/t6036-recursive-corner-cases.sh | 159 +++++++++++++++++++++++++++++-\n 4 files changed, 208 insertions(+), 15 deletions(-)\n\n-- \n2.19.0.235.g7c386e1068\n\n"},{"id":"360352","messageId":"20181012212551.7689-4-newren@gmail.com","threadId":"49554","inReplyTo":"20181012212551.7689-1-newren@gmail.com","subject":"[PATCH 3/4] merge-recursive: improve auto-merging messages with path collisions","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-10-12T21:25:50Z","receivedAt":"2018-10-12T21:26:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Each individual file involved in a rename could have also been modified\non both sides of history, meaning it may need to have content merges.\nIf two such files are renamed into the same location, then on top of the\ntwo natural auto-merging messages we also have to two-way merge the\nresult, giving us messages that look like\n\n Auto-merging somefile.c (was somecase.c)\n Auto-merging somefile.c (was somefolder.c)\n Auto-merging somefile.c\n\nHowever, despite the fact that I was the one who put the \"(was %s)\"\nportions into the messages (and just a few months ago), I was still\ninitially confused when running into a rename/rename(2to1) case and\nwondered if somefile.c had been merged three times. Update this to\ninstead be:\n\n Auto-merging version of somefile.c from somecase.c\n Auto-merging version of somefile.c from someportfolio.c\n Auto-merging somefile.c\n\nThis is an admittedly long set of messages for a single path, but you\nonly get all three messages when dealing with the rare case of a\nrename/rename(2to1) conflict where both sides of both original files\nwere also modified, in conflicting ways.\n\nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 2452788d28..33cd9ee81f 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1679,8 +1679,8 @@ static int handle_rename_rename_2to1(struct merge_options *o,\n \tremove_file(o, 1, a->path, o->call_depth || would_lose_untracked(a->path));\n \tremove_file(o, 1, b->path, o->call_depth || would_lose_untracked(b->path));\n \n-\tpath_side_1_desc = xstrfmt(\"%s (was %s)\", path, a->path);\n-\tpath_side_2_desc = xstrfmt(\"%s (was %s)\", path, b->path);\n+\tpath_side_1_desc = xstrfmt(\"version of %s from %s\", path, a->path);\n+\tpath_side_2_desc = xstrfmt(\"version of %s from %s\", path, b->path);\n \tif (merge_mode_and_contents(o, a, c1, &ci->ren1_other, path_side_1_desc,\n \t\t\t\t o->branch1, o->branch2,\n \t\t\t\t o->call_depth * 2, &mfi_c1) ||\n-- \n2.19.0.235.g7c386e1068\n\n"},{"id":"360353","messageId":"20181012212551.7689-5-newren@gmail.com","threadId":"49554","inReplyTo":"20181012212551.7689-1-newren@gmail.com","subject":"[PATCH 4/4] merge-recursive: Avoid showing conflicts with merge branch before HEAD","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-10-12T21:25:51Z","receivedAt":"2018-10-12T21:26:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"We want to load unmerged entries from HEAD into the index at stage 2 and\nfrom MERGE_HEAD into stage 3. Similarly, folks expect merge conflicts\nto look like\n\n<<<<<<<< HEAD\ncontent from our side\n========\ncontent from their side\n>>>>>>>> MERGE_HEAD\n\nnot\n\n<<<<<<<< MERGE_HEAD\ncontent from their side\n========\ncontent from our side\n>>>>>>>> HEAD\n\nThe correct order usually comes naturally and for free, but with renames\nwe often have data in the form {rename_branch, other_branch}, and\nworking relative to the rename first (e.g. for rename/add) is more\nconvenient elsewhere in the code. Address the slight impedance\nmismatch by having some functions re-call themselves with flipped\narguments when the branch order is reversed.\n\nNote that setup_rename_conflict_info() has one asymmetry in it, in\nsetting dst_entry1->processed=0 but not doing similarly for\ndst_entry2->processed. When dealing with rename/rename and similar\nconflicts, we do not want the processing to happen twice, so the\ndesire to only set one of the entries to unprocessed is intentional.\nSo, while this change modifies which branch's entry will be marked as\nunprocessed, that dovetails nicely with putting HEAD first so that we\nget the index stage entries and conflict markers in the right order.\n\nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 33 ++++++++++++++++++++++++++++++-\n t/t6036-recursive-corner-cases.sh | 8 ++++----\n 2 files changed, 36 insertions(+), 5 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 33cd9ee81f..f795c92a69 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -228,7 +228,26 @@ static inline void setup_rename_conflict_info(enum rename_type rename_type,\n \t\t\t\t\t struct stage_data *src_entry1,\n \t\t\t\t\t struct stage_data *src_entry2)\n {\n-\tstruct rename_conflict_info *ci = xcalloc(1, sizeof(struct rename_conflict_info));\n+\tstruct rename_conflict_info *ci;\n+\n+\t/*\n+\t * When we have two renames involved, it's easiest to get the\n+\t * correct things into stage 2 and 3, and to make sure that the\n+\t * content merge puts HEAD before the other branch if we just\n+\t * ensure that branch1 == o->branch1. So, simply flip arguments\n+\t * around if we don't have that.\n+\t */\n+\tif (dst_entry2 && branch1 != o->branch1) {\n+\t\tsetup_rename_conflict_info(rename_type,\n+\t\t\t\t\t pair2, pair1,\n+\t\t\t\t\t branch2, branch1,\n+\t\t\t\t\t dst_entry2, dst_entry1,\n+\t\t\t\t\t o,\n+\t\t\t\t\t src_entry2, src_entry1);\n+\t\treturn;\n+\t}\n+\n+\tci = xcalloc(1, sizeof(struct rename_conflict_info));\n \tci->rename_type = rename_type;\n \tci->pair1 = pair1;\n \tci->branch1 = branch1;\n@@ -1286,6 +1305,18 @@ static int merge_mode_and_contents(struct merge_options *o,\n \t\t\t\t const int extra_marker_size,\n \t\t\t\t struct merge_file_info *result)\n {\n+\tif (o->branch1 != branch1) {\n+\t\t/*\n+\t\t * It's weird getting a reverse merge with HEAD on the bottom\n+\t\t * side of the conflict markers and the other branch on the\n+\t\t * top. Fix that.\n+\t\t */\n+\t\treturn merge_mode_and_contents(o, one, b, a,\n+\t\t\t\t\t filename,\n+\t\t\t\t\t branch2, branch1,\n+\t\t\t\t\t extra_marker_size, result);\n+\t}\n+\n \tresult->merge = 0;\n \tresult->clean = 1;\n \ndiff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh\nindex 21954db624..276b4e8792 100755\n--- a/t/t6036-recursive-corner-cases.sh\n+++ b/t/t6036-recursive-corner-cases.sh\n@@ -230,13 +230,13 @@ test_expect_success 'git detects differently handled merges conflict' '\n \t\t\t:2:new_a :3:new_a &&\n \t\ttest_cmp expect actual &&\n \n-\t\tgit cat-file -p B:new_a >ours &&\n-\t\tgit cat-file -p C:new_a >theirs &&\n+\t\tgit cat-file -p C:new_a >ours &&\n+\t\tgit cat-file -p B:new_a >theirs &&\n \t\t>empty &&\n \t\ttest_must_fail git merge-file \\\n-\t\t\t-L \"Temporary merge branch 2\" \\\n-\t\t\t-L \"\" \\\n \t\t\t-L \"Temporary merge branch 1\" \\\n+\t\t\t-L \"\" \\\n+\t\t\t-L \"Temporary merge branch 2\" \\\n \t\t\tours empty theirs &&\n \t\tsed -e \"s/^\\([<=>]\\)/\\1\\1\\1/\" ours >expect &&\n \t\tgit cat-file -p :1:new_a >actual &&\n-- \n2.19.0.235.g7c386e1068\n\n"},{"id":"360354","messageId":"20181012212551.7689-2-newren@gmail.com","threadId":"49554","inReplyTo":"20181012212551.7689-1-newren@gmail.com","subject":"[PATCH 1/4] t6036: add testcase where virtual merge base contains nested conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-10-12T21:25:48Z","receivedAt":"2018-10-12T21:26:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"When merges involve content conflicts, merge.conflictstyle=diff3 can be\nused to also show the content for the file from the base version. If\nthere was more than one merge base, then the \"base version\" is\ndetermined by merging the merge bases to create a virtual merge base.\nThis means that the \"base version\" of the file can itself already have\nconflict markers. In the past, this could have created a situation\nwhere users have difficulty distinguishing between the inner and outer\nconflict markers. However, commit d694a17986a2 (\"ll-merge: use a longer\nconflict marker for internal merge\", 2016-04-14) addressed this issue by\nmaking conflict markers two characters longer for a virtual merge base.\n\nUnfortunately, this solution was incomplete. The recursive merge\nalgorithm is, as you'd expect from it's name, recursive in nature. In\nparticular, this means that the virtual merge base itself could have had\na virtual merge base of its own that contained conflict markers. In\nother words, we can have three (or more) levels of conflict markers when\nusing merge.conflictstyle=diff3. Thus, what we need is for conflict\nmarker length to increase with the depth of recursion. Add a testcase\ndemonstrating the problem and testing for marker length increasing with\nincreasing recursion depth.\n\nSigned-off-by: Elijah Newren \n---\n t/t6036-recursive-corner-cases.sh | 151 ++++++++++++++++++++++++++++++\n 1 file changed, 151 insertions(+)\n\ndiff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh\nindex 59e52c5a09..1d1135082c 100755\n--- a/t/t6036-recursive-corner-cases.sh\n+++ b/t/t6036-recursive-corner-cases.sh\n@@ -1402,4 +1402,155 @@ test_expect_failure 'check conflicting modes for regular file' '\n \t)\n '\n \n+# Setup:\n+# L1---L2---L3\n+# / \\ / \\ / \\\n+# master X1 X2 ?\n+# \\ / \\ / \\ /\n+# R1---R2---R3\n+#\n+# Where:\n+# master has one file named 'content'\n+# branches L1 and R1 both modify each of the two files in conflicting ways\n+#\n+# L (n>1) is a merge of R into L\n+# R (n>1) is a merge of L into R\n+# L and R resolve the conflicts differently.\n+#\n+# X is an auto-generated merge-base used when merging L and R.\n+# By construction, X1 has conflict markers due to conflicting versions.\n+# X2, due to using merge.conflictstyle=3, has nested conflict markers.\n+#\n+# So, merging R3 into L3 using merge.conflictstyle=3 should show the\n+# nested conflict markers from X2 in the base version -- that means we\n+# have three levels of conflict markers. Can we distinguish all three?\n+\n+test_expect_success \"setup virtual merge base with nested conflicts\" '\n+\ttest_create_repo virtual_merge_base_has_nested_conflicts &&\n+\t(\n+\t\tcd virtual_merge_base_has_nested_conflicts &&\n+\n+\t\t# Create some related files now\n+\t\tfor i in $(test_seq 1 10)\n+\t\tdo\n+\t\t\techo Random base content line $i\n+\t\tdone >content &&\n+\n+\t\t# Setup original commit\n+\t\tgit add content &&\n+\t\ttest_tick && git commit -m initial &&\n+\n+\t\tgit branch L &&\n+\t\tgit branch R &&\n+\n+\t\t# Create L1\n+\t\tgit checkout L &&\n+\t\techo left >>content &&\n+\t\tgit add content &&\n+\t\ttest_tick && git commit -m \"version L1 of content\" &&\n+\t\tgit tag L1 &&\n+\n+\t\t# Create R1\n+\t\tgit checkout R &&\n+\t\techo right >>content &&\n+\t\tgit add content &&\n+\t\ttest_tick && git commit -m \"verson R1 of content\" &&\n+\t\tgit tag R1 &&\n+\n+\t\t# Create L2\n+\t\tgit checkout L &&\n+\t\ttest_must_fail git -c merge.conflictstyle=diff3 merge R1 &&\n+\t\tgit checkout L1 content &&\n+\t\ttest_tick && git commit -m \"version L2 of content\" &&\n+\t\tgit tag L2 &&\n+\n+\t\t# Create R2\n+\t\tgit checkout R &&\n+\t\ttest_must_fail git -c merge.conflictstyle=diff3 merge L1 &&\n+\t\tgit checkout R1 content &&\n+\t\ttest_tick && git commit -m \"version R2 of content\" &&\n+\t\tgit tag R2 &&\n+\n+\t\t# Create L3\n+\t\tgit checkout L &&\n+\t\ttest_must_fail git -c merge.conflictstyle=diff3 merge R2 &&\n+\t\tgit checkout L1 content &&\n+\t\ttest_tick && git commit -m \"version L3 of content\" &&\n+\t\tgit tag L3 &&\n+\n+\t\t# Create R3\n+\t\tgit checkout R &&\n+\t\ttest_must_fail git -c merge.conflictstyle=diff3 merge L2 &&\n+\t\tgit checkout R1 content &&\n+\t\ttest_tick && git commit -m \"version R3 of content\" &&\n+\t\tgit tag R3\n+\t)\n+'\n+\n+test_expect_failure \"check virtual merge base with nested conflicts\" '\n+\t(\n+\t\tcd virtual_merge_base_has_nested_conflicts &&\n+\n+\t\tgit checkout L3^0 &&\n+\n+\t\t# Merge must fail; there is a conflict\n+\t\ttest_must_fail git -c merge.conflictstyle=diff3 merge -s recursive R3^0 &&\n+\n+\t\t# Make sure the index has the right number of entries\n+\t\tgit ls-files -s >out &&\n+\t\ttest_line_count = 3 out &&\n+\t\tgit ls-files -u >out &&\n+\t\ttest_line_count = 3 out &&\n+\t\t# Ensure we have the correct number of untracked files\n+\t\tgit ls-files -o >out &&\n+\t\ttest_line_count = 1 out &&\n+\n+\t\t# Compare :[23]:content to expected values\n+\t\tgit rev-parse L1:content R1:content >expect &&\n+\t\tgit rev-parse :2:content :3:content >actual &&\n+\t\ttest_cmp expect actual &&\n+\n+\t\t# Imitate X1 merge base, except without long enough conflict\n+\t\t# markers because a subsequent sed will modify them. Put\n+\t\t# result into vmb.\n+\t\tgit cat-file -p master:content >base &&\n+\t\tgit cat-file -p L:content >left &&\n+\t\tgit cat-file -p R:content >right &&\n+\t\tcp left merged-once &&\n+\t\ttest_must_fail git merge-file --diff3 \\\n+\t\t\t-L \"Temporary merge branch 1\" \\\n+\t\t\t-L \"merged common ancestors\" \\\n+\t\t\t-L \"Temporary merge branch 2\" \\\n+\t\t\tmerged-once \\\n+\t\t\tbase \\\n+\t\t\tright &&\n+\t\tsed -e \"s/^\\([<|=>]\\)/\\1\\1\\1/\" merged-once >vmb &&\n+\n+\t\t# Imitate X2 merge base, overwriting vmb. Note that we\n+\t\t# extend both sets of conflict markers to make them longer\n+\t\t# with the sed command.\n+\t\tcp left merged-twice &&\n+\t\ttest_must_fail git merge-file --diff3 \\\n+\t\t\t-L \"Temporary merge branch 1\" \\\n+\t\t\t-L \"merged common ancestors\" \\\n+\t\t\t-L \"Temporary merge branch 2\" \\\n+\t\t\tmerged-twice \\\n+\t\t\tvmb \\\n+\t\t\tright &&\n+\t\tsed -e \"s/^\\([<|=>]\\)/\\1\\1\\1/\" merged-twice >vmb &&\n+\n+\t\t# Compare :1:content to expected value\n+\t\tgit cat-file -p :1:content >actual &&\n+\t\ttest_cmp vmb actual &&\n+\n+\t\t# Determine expected content in final outer merge, compare to\n+\t\t# what the merge generated.\n+\t\tcp -f left expect &&\n+\t\ttest_must_fail git merge-file --diff3 \\\n+\t\t\t-L \"HEAD\" -L \"merged common ancestors\" -L \"R3^0\" \\\n+\t\t\texpect vmb right &&\n+\t\ttest_cmp expect content\n+\t)\n+'\n+\n test_done\n-- \n2.19.0.235.g7c386e1068\n\n"},{"id":"360355","messageId":"20181012212551.7689-3-newren@gmail.com","threadId":"49554","inReplyTo":"20181012212551.7689-1-newren@gmail.com","subject":"[PATCH 2/4] merge-recursive: increase marker length with depth of recursion","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-10-12T21:25:49Z","receivedAt":"2018-10-12T21:26:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"When using merge.conflictstyle=diff3 to have the \"base version\" be shown\nin conflicts, there is the possibility that the base version itself has\nconflicts in it. This comes about when there are more than one merge\nbase, and the merging of those merge bases produces a conflict.\nSince this process applies recursively, it is possible to have conflict\nmarkers nested at an arbitrary depth; to be able to differentiate the\nconflict markers from different nestings, we make them all of different\nlengths.\n\nSigned-off-by: Elijah Newren \n---\n ll-merge.c | 4 +++-\n ll-merge.h | 1 +\n merge-recursive.c | 22 +++++++++++++++-------\n t/t6036-recursive-corner-cases.sh | 2 +-\n 4 files changed, 20 insertions(+), 9 deletions(-)\n\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 0e2800f7bb..aabc1b5c2e 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -384,7 +384,9 @@ int ll_merge(mmbuffer_t *result_buf,\n \tif (opts->virtual_ancestor) {\n \t\tif (driver->recursive)\n \t\t\tdriver = find_ll_merge_driver(driver->recursive);\n-\t\tmarker_size += 2;\n+\t}\n+\tif (opts->extra_marker_size) {\n+\t\tmarker_size += opts->extra_marker_size;\n \t}\n \treturn driver->fn(driver, result_buf, path, ancestor, ancestor_label,\n \t\t\t ours, our_label, theirs, their_label,\ndiff --git a/ll-merge.h b/ll-merge.h\nindex b72b19921e..5b4e158502 100644\n--- a/ll-merge.h\n+++ b/ll-merge.h\n@@ -11,6 +11,7 @@ struct ll_merge_options {\n \tunsigned virtual_ancestor : 1;\n \tunsigned variant : 2;\t/* favor ours, favor theirs, or union merge */\n \tunsigned renormalize : 1;\n+\tunsigned extra_marker_size;\n \tlong xdl_opts;\n };\n \ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 5206d6cfb6..2452788d28 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1039,7 +1039,8 @@ static int merge_3way(struct merge_options *o,\n \t\t const struct diff_filespec *a,\n \t\t const struct diff_filespec *b,\n \t\t const char *branch1,\n-\t\t const char *branch2)\n+\t\t const char *branch2,\n+\t\t const int extra_marker_size)\n {\n \tmmfile_t orig, src1, src2;\n \tstruct ll_merge_options ll_opts = {0};\n@@ -1047,6 +1048,7 @@ static int merge_3way(struct merge_options *o,\n \tint merge_status;\n \n \tll_opts.renormalize = o->renormalize;\n+\tll_opts.extra_marker_size = extra_marker_size;\n \tll_opts.xdl_opts = o->xdl_opts;\n \n \tif (o->call_depth) {\n@@ -1281,6 +1283,7 @@ static int merge_mode_and_contents(struct merge_options *o,\n \t\t\t\t const char *filename,\n \t\t\t\t const char *branch1,\n \t\t\t\t const char *branch2,\n+\t\t\t\t const int extra_marker_size,\n \t\t\t\t struct merge_file_info *result)\n {\n \tresult->merge = 0;\n@@ -1321,7 +1324,8 @@ static int merge_mode_and_contents(struct merge_options *o,\n \t\t\tint ret = 0, merge_status;\n \n \t\t\tmerge_status = merge_3way(o, &result_buf, one, a, b,\n-\t\t\t\t\t\t branch1, branch2);\n+\t\t\t\t\t\t branch1, branch2,\n+\t\t\t\t\t\t extra_marker_size);\n \n \t\t\tif ((merge_status < 0) || !result_buf.ptr)\n \t\t\t\tret = err(o, _(\"Failed to execute internal merge\"));\n@@ -1610,7 +1614,8 @@ static int handle_rename_rename_1to2(struct merge_options *o,\n \t\tstruct diff_filespec other;\n \t\tstruct diff_filespec *add;\n \t\tif (merge_mode_and_contents(o, one, a, b, one->path,\n-\t\t\t\t\t ci->branch1, ci->branch2, &mfi))\n+\t\t\t\t\t ci->branch1, ci->branch2,\n+\t\t\t\t\t o->call_depth * 2, &mfi))\n \t\t\treturn -1;\n \n \t\t/*\n@@ -1677,9 +1682,11 @@ static int handle_rename_rename_2to1(struct merge_options *o,\n \tpath_side_1_desc = xstrfmt(\"%s (was %s)\", path, a->path);\n \tpath_side_2_desc = xstrfmt(\"%s (was %s)\", path, b->path);\n \tif (merge_mode_and_contents(o, a, c1, &ci->ren1_other, path_side_1_desc,\n-\t\t\t\t o->branch1, o->branch2, &mfi_c1) ||\n+\t\t\t\t o->branch1, o->branch2,\n+\t\t\t\t o->call_depth * 2, &mfi_c1) ||\n \t merge_mode_and_contents(o, b, &ci->ren2_other, c2, path_side_2_desc,\n-\t\t\t\t o->branch1, o->branch2, &mfi_c2))\n+\t\t\t\t o->branch1, o->branch2,\n+\t\t\t\t o->call_depth * 2, &mfi_c2))\n \t\treturn -1;\n \tfree(path_side_1_desc);\n \tfree(path_side_2_desc);\n@@ -2725,7 +2732,7 @@ static int process_renames(struct merge_options *o,\n \n \t\t\t\t\tif (merge_mode_and_contents(o, &one, &a, &b, ren1_dst,\n \t\t\t\t\t\t\t\t branch1, branch2,\n-\t\t\t\t\t\t\t\t &mfi)) {\n+\t\t\t\t\t\t\t\t o->call_depth * 2, &mfi)) {\n \t\t\t\t\t\tclean_merge = -1;\n \t\t\t\t\t\tgoto cleanup_and_return;\n \t\t\t\t\t}\n@@ -3022,7 +3029,8 @@ static int handle_content_merge(struct merge_options *o,\n \t\t\tdf_conflict_remains = 1;\n \t}\n \tif (merge_mode_and_contents(o, &one, &a, &b, path,\n-\t\t\t\t o->branch1, o->branch2, &mfi))\n+\t\t\t\t o->branch1, o->branch2,\n+\t\t\t\t o->call_depth * 2, &mfi))\n \t\treturn -1;\n \n \t/*\ndiff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh\nindex 1d1135082c..21954db624 100755\n--- a/t/t6036-recursive-corner-cases.sh\n+++ b/t/t6036-recursive-corner-cases.sh\n@@ -1487,7 +1487,7 @@ test_expect_success \"setup virtual merge base with nested conflicts\" '\n \t)\n '\n \n-test_expect_failure \"check virtual merge base with nested conflicts\" '\n+test_expect_success \"check virtual merge base with nested conflicts\" '\n \t(\n \t\tcd virtual_merge_base_has_nested_conflicts &&\n \n-- \n2.19.0.235.g7c386e1068\n\n"},{"id":"360482","messageId":"xmqq8t2z6bem.fsf@gitster-ct.c.googlers.com","threadId":"49554","inReplyTo":"20181012212551.7689-3-newren@gmail.com","subject":"Re: [PATCH 2/4] merge-recursive: increase marker length with depth of recursion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-15T05:12:33Z","receivedAt":"2018-10-15T05:12:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> When using merge.conflictstyle=diff3 to have the \"base version\" be shown\n> in conflicts, there is the possibility that the base version itself has\n> conflicts in it. This comes about when there are more than one merge\n> base, and the merging of those merge bases produces a conflict.\n> Since this process applies recursively, it is possible to have conflict\n> markers nested at an arbitrary depth; to be able to differentiate the\n> conflict markers from different nestings, we make them all of different\n> lengths.\n\nI know it is possible that the common ancestor part that is enclosed\nby the outermost makers can have arbitrary conflicts, and they can\nbe even recursive conflicts. But I fail to see why it is a problem.\nPerhaps that is because I am not particularly good at resolving\nmerge conflicts, but as long as the common part of the outermost\nmerge is identifyable, would that really matter? What I would do\nwhile looking at common ancestor part with conflicts (not even a\nrecursive one) is just to ignore it, so...\n\nNot that I strongly oppose to incrementing the marker length at\nevery level. I do not think it would hurt, but I just do not see\nhow it would help.\n"},{"id":"360483","messageId":"xmqq4ldn6b44.fsf@gitster-ct.c.googlers.com","threadId":"49554","inReplyTo":"20181012212551.7689-4-newren@gmail.com","subject":"Re: [PATCH 3/4] merge-recursive: improve auto-merging messages with path collisions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-15T05:18:51Z","receivedAt":"2018-10-15T05:18:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Each individual file involved in a rename could have also been modified\n> on both sides of history, meaning it may need to have content merges.\n> If two such files are renamed into the same location, then on top of the\n> two natural auto-merging messages we also have to two-way merge the\n> result, giving us messages that look like\n>\n> Auto-merging somefile.c (was somecase.c)\n> Auto-merging somefile.c (was somefolder.c)\n> Auto-merging somefile.c\n>\n> However, despite the fact that I was the one who put the \"(was %s)\"\n> portions into the messages (and just a few months ago), I was still\n> initially confused when running into a rename/rename(2to1) case and\n> wondered if somefile.c had been merged three times. Update this to\n> instead be:\n>\n> Auto-merging version of somefile.c from somecase.c\n> Auto-merging version of somefile.c from someportfolio.c\n> Auto-merging somefile.c\n>\n> This is an admittedly long set of messages for a single path, but you\n> only get all three messages when dealing with the rare case of a\n> rename/rename(2to1) conflict where both sides of both original files\n> were also modified, in conflicting ways.\n\nYeah, that does look mouthful, but definitely is more\nunderstandable.\n"},{"id":"360484","messageId":"xmqqzhvf4wcc.fsf@gitster-ct.c.googlers.com","threadId":"49554","inReplyTo":"20181012212551.7689-5-newren@gmail.com","subject":"Re: [PATCH 4/4] merge-recursive: Avoid showing conflicts with merge branch before HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-15T05:23:15Z","receivedAt":"2018-10-15T05:23:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> The correct order usually comes naturally and for free, but with renames\n> we often have data in the form {rename_branch, other_branch}, and\n> working relative to the rename first (e.g. for rename/add) is more\n> convenient elsewhere in the code. Address the slight impedance\n> mismatch by having some functions re-call themselves with flipped\n> arguments when the branch order is reversed.\n\nI've never noticed or felt disturbed myself, but thanks for this\nlevel of attention to the detail.\n\n> @@ -228,7 +228,26 @@ static inline void setup_rename_conflict_info(enum rename_type rename_type,\n> \t\t\t\t\t struct stage_data *src_entry1,\n> \t\t\t\t\t struct stage_data *src_entry2)\n> {\n> -\tstruct rename_conflict_info *ci = xcalloc(1, sizeof(struct rename_conflict_info));\n> +\tstruct rename_conflict_info *ci;\n> +\n> +\t/*\n> +\t * When we have two renames involved, it's easiest to get the\n> +\t * correct things into stage 2 and 3, and to make sure that the\n> +\t * content merge puts HEAD before the other branch if we just\n> +\t * ensure that branch1 == o->branch1. So, simply flip arguments\n> +\t * around if we don't have that.\n> +\t */\n> +\tif (dst_entry2 && branch1 != o->branch1) {\n> +\t\tsetup_rename_conflict_info(rename_type,\n> +\t\t\t\t\t pair2, pair1,\n> +\t\t\t\t\t branch2, branch1,\n> +\t\t\t\t\t dst_entry2, dst_entry1,\n> +\t\t\t\t\t o,\n> +\t\t\t\t\t src_entry2, src_entry1);\n> +\t\treturn;\n> +\t}\n\n;-)\n"},{"id":"360531","messageId":"CABPp-BFJTFN1prCwkidnQiKNP=5wdAV0AdXEkTjCT3HTHyB=Mg@mail.gmail.com","threadId":"49554","inReplyTo":"xmqq8t2z6bem.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/4] merge-recursive: increase marker length with depth of recursion","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-10-15T15:02:05Z","receivedAt":"2018-10-15T15:02:20Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Oct 14, 2018 at 10:12 PM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > When using merge.conflictstyle=diff3 to have the \"base version\" be shown\n> > in conflicts, there is the possibility that the base version itself has\n> > conflicts in it. This comes about when there are more than one merge\n> > base, and the merging of those merge bases produces a conflict.\n> > Since this process applies recursively, it is possible to have conflict\n> > markers nested at an arbitrary depth; to be able to differentiate the\n> > conflict markers from different nestings, we make them all of different\n> > lengths.\n>\n> I know it is possible that the common ancestor part that is enclosed\n> by the outermost makers can have arbitrary conflicts, and they can\n> be even recursive conflicts. But I fail to see why it is a problem.\n> Perhaps that is because I am not particularly good at resolving\n> merge conflicts, but as long as the common part of the outermost\n> merge is identifyable, would that really matter? What I would do\n> while looking at common ancestor part with conflicts (not even a\n> recursive one) is just to ignore it, so...\n>\n> Not that I strongly oppose to incrementing the marker length at\n> every level. I do not think it would hurt, but I just do not see\n> how it would help.\n\nFair enough. The real motivation for these changes was the\nmodification to rename/rename(2to1) conflicts (and rename/add\nconflicts) to behave like add/add conflicts -- that change means we\ncan have nested conflict markers even without invoking the recursive\npart of the recursive machinery. So, I needed a way to increase the\nlength of the conflict markers besides just checking\nopts->virtual_ancestor. Just using a fixed extra indent seemed\nproblematic, because if I also had to worry about even one\nvirtual_ancestor, then I was already dealing with the possibility of\ntriply nested conflict markers and only one of them from a virtual\nmerge base. See t6036 in\nhttps://public-inbox.org/git/20181014020537.17991-3-newren@gmail.com/.\n\nHowever, that series was long enough, so to try to simplify review I\nsplit as much as I could out of it. That resulted, among other\nthings, in me submitting this marker nesting change as part of this\nseries using a more limited rationale.\n\nWould you like me to edit the commit message to include this more\ndifficult case?\n"},{"id":"360568","messageId":"xmqqr2gq3aau.fsf@gitster-ct.c.googlers.com","threadId":"49554","inReplyTo":"CABPp-BFJTFN1prCwkidnQiKNP=5wdAV0AdXEkTjCT3HTHyB=Mg@mail.gmail.com","subject":"Re: [PATCH 2/4] merge-recursive: increase marker length with depth of recursion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-16T02:16:57Z","receivedAt":"2018-10-16T02:17:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Would you like me to edit the commit message to include this more\n> difficult case?\n\nNeither. If the \"marker length\" change is required in a separate\nseries that will build on top of the current 4-patch series, I think\ndropping this step from the current series and make it a part of the\nseries that deals with rename/rename would make more sense.\n"},{"id":"360645","messageId":"CABPp-BGQv35K+wt2pY6ddVCiU=GL7jhqqEmAB0eWRVsaMTk8tA@mail.gmail.com","threadId":"49554","inReplyTo":"xmqqr2gq3aau.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/4] merge-recursive: increase marker length with depth of recursion","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-10-16T18:00:00Z","receivedAt":"2018-10-16T18:00:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Oct 15, 2018 at 7:17 PM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > Would you like me to edit the commit message to include this more\n> > difficult case?\n>\n> Neither. If the \"marker length\" change is required in a separate\n> series that will build on top of the current 4-patch series, I think\n> dropping this step from the current series and make it a part of the\n> series that deals with rename/rename would make more sense.\n\nOkay, I'll resubmit this series without this patch or associated\ntestcase, and include those in the later file collision series.\n"},{"id":"360661","messageId":"20181016201948.16025-2-newren@gmail.com","threadId":"49554","inReplyTo":"20181016201948.16025-1-newren@gmail.com","subject":"[PATCH v2 1/2] merge-recursive: improve auto-merging messages with path collisions","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-10-16T20:19:47Z","receivedAt":"2018-10-16T20:20:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Each individual file involved in a rename could have also been modified\non both sides of history, meaning it may need to have content merges.\nIf two such files are renamed into the same location, then on top of the\ntwo natural auto-merging messages we also have to two-way merge the\nresult, giving us messages that look like\n\n Auto-merging somefile.c (was somecase.c)\n Auto-merging somefile.c (was somefolder.c)\n Auto-merging somefile.c\n\nHowever, despite the fact that I was the one who put the \"(was %s)\"\nportions into the messages (and just a few months ago), I was still\ninitially confused when running into a rename/rename(2to1) case and\nwondered if somefile.c had been merged three times. Update this to\ninstead be:\n\n Auto-merging version of somefile.c from somecase.c\n Auto-merging version of somefile.c from someportfolio.c\n Auto-merging somefile.c\n\nThis is an admittedly long set of messages for a single path, but you\nonly get all three messages when dealing with the rare case of a\nrename/rename(2to1) conflict where both sides of both original files\nwere also modified, in conflicting ways.\n\nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 5206d6cfb6..8a47e54e2f 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1674,8 +1674,8 @@ static int handle_rename_rename_2to1(struct merge_options *o,\n \tremove_file(o, 1, a->path, o->call_depth || would_lose_untracked(a->path));\n \tremove_file(o, 1, b->path, o->call_depth || would_lose_untracked(b->path));\n \n-\tpath_side_1_desc = xstrfmt(\"%s (was %s)\", path, a->path);\n-\tpath_side_2_desc = xstrfmt(\"%s (was %s)\", path, b->path);\n+\tpath_side_1_desc = xstrfmt(\"version of %s from %s\", path, a->path);\n+\tpath_side_2_desc = xstrfmt(\"version of %s from %s\", path, b->path);\n \tif (merge_mode_and_contents(o, a, c1, &ci->ren1_other, path_side_1_desc,\n \t\t\t\t o->branch1, o->branch2, &mfi_c1) ||\n \t merge_mode_and_contents(o, b, &ci->ren2_other, c2, path_side_2_desc,\n-- \n2.19.1.280.g0c175526bf\n\n"},{"id":"360662","messageId":"20181016201948.16025-1-newren@gmail.com","threadId":"49554","inReplyTo":"20181012212551.7689-1-newren@gmail.com","subject":"[PATCH v2 0/2] More merge cleanups","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-10-16T20:19:46Z","receivedAt":"2018-10-16T20:20:06Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series adds a few more cleanups on top of en/merge-cleanup.\nChanges since v1:\n - Removed two patches that will instead be included in a follow-on\n series, as suggested by Junio.\n - Incorporated commit message cleanups (capitalization and indents)\n made by Junio to the previous round.\n\nElijah Newren (2):\n merge-recursive: improve auto-merging messages with path collisions\n merge-recursive: avoid showing conflicts with merge branch before HEAD\n\n merge-recursive.c | 37 ++++++++++++++++++++++++++++---\n t/t6036-recursive-corner-cases.sh | 8 +++----\n 2 files changed, 38 insertions(+), 7 deletions(-)\n\n-- \n2.19.1.280.g0c175526bf\n\n"},{"id":"360663","messageId":"20181016201948.16025-3-newren@gmail.com","threadId":"49554","inReplyTo":"20181016201948.16025-1-newren@gmail.com","subject":"[PATCH v2 2/2] merge-recursive: avoid showing conflicts with merge branch before HEAD","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-10-16T20:19:48Z","receivedAt":"2018-10-16T20:20:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"We want to load unmerged entries from HEAD into the index at stage 2 and\nfrom MERGE_HEAD into stage 3. Similarly, folks expect merge conflicts\nto look like\n\n <<<<<<<< HEAD\n content from our side\n ========\n content from their side\n >>>>>>>> MERGE_HEAD\n\nnot\n\n <<<<<<<< MERGE_HEAD\n content from their side\n ========\n content from our side\n >>>>>>>> HEAD\n\nThe correct order usually comes naturally and for free, but with renames\nwe often have data in the form {rename_branch, other_branch}, and\nworking relative to the rename first (e.g. for rename/add) is more\nconvenient elsewhere in the code. Address the slight impedance\nmismatch by having some functions re-call themselves with flipped\narguments when the branch order is reversed.\n\nNote that setup_rename_conflict_info() has one asymmetry in it, in\nsetting dst_entry1->processed=0 but not doing similarly for\ndst_entry2->processed. When dealing with rename/rename and similar\nconflicts, we do not want the processing to happen twice, so the\ndesire to only set one of the entries to unprocessed is intentional.\nSo, while this change modifies which branch's entry will be marked as\nunprocessed, that dovetails nicely with putting HEAD first so that we\nget the index stage entries and conflict markers in the right order.\n\nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 33 ++++++++++++++++++++++++++++++-\n t/t6036-recursive-corner-cases.sh | 8 ++++----\n 2 files changed, 36 insertions(+), 5 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 8a47e54e2f..16980db7f9 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -228,7 +228,26 @@ static inline void setup_rename_conflict_info(enum rename_type rename_type,\n \t\t\t\t\t struct stage_data *src_entry1,\n \t\t\t\t\t struct stage_data *src_entry2)\n {\n-\tstruct rename_conflict_info *ci = xcalloc(1, sizeof(struct rename_conflict_info));\n+\tstruct rename_conflict_info *ci;\n+\n+\t/*\n+\t * When we have two renames involved, it's easiest to get the\n+\t * correct things into stage 2 and 3, and to make sure that the\n+\t * content merge puts HEAD before the other branch if we just\n+\t * ensure that branch1 == o->branch1. So, simply flip arguments\n+\t * around if we don't have that.\n+\t */\n+\tif (dst_entry2 && branch1 != o->branch1) {\n+\t\tsetup_rename_conflict_info(rename_type,\n+\t\t\t\t\t pair2, pair1,\n+\t\t\t\t\t branch2, branch1,\n+\t\t\t\t\t dst_entry2, dst_entry1,\n+\t\t\t\t\t o,\n+\t\t\t\t\t src_entry2, src_entry1);\n+\t\treturn;\n+\t}\n+\n+\tci = xcalloc(1, sizeof(struct rename_conflict_info));\n \tci->rename_type = rename_type;\n \tci->pair1 = pair1;\n \tci->branch1 = branch1;\n@@ -1283,6 +1302,18 @@ static int merge_mode_and_contents(struct merge_options *o,\n \t\t\t\t const char *branch2,\n \t\t\t\t struct merge_file_info *result)\n {\n+\tif (o->branch1 != branch1) {\n+\t\t/*\n+\t\t * It's weird getting a reverse merge with HEAD on the bottom\n+\t\t * side of the conflict markers and the other branch on the\n+\t\t * top. Fix that.\n+\t\t */\n+\t\treturn merge_mode_and_contents(o, one, b, a,\n+\t\t\t\t\t filename,\n+\t\t\t\t\t branch2, branch1,\n+\t\t\t\t\t extra_marker_size, result);\n+\t}\n+\n \tresult->merge = 0;\n \tresult->clean = 1;\n \ndiff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh\nindex 59e52c5a09..e1cef58f2a 100755\n--- a/t/t6036-recursive-corner-cases.sh\n+++ b/t/t6036-recursive-corner-cases.sh\n@@ -230,13 +230,13 @@ test_expect_success 'git detects differently handled merges conflict' '\n \t\t\t:2:new_a :3:new_a &&\n \t\ttest_cmp expect actual &&\n \n-\t\tgit cat-file -p B:new_a >ours &&\n-\t\tgit cat-file -p C:new_a >theirs &&\n+\t\tgit cat-file -p C:new_a >ours &&\n+\t\tgit cat-file -p B:new_a >theirs &&\n \t\t>empty &&\n \t\ttest_must_fail git merge-file \\\n-\t\t\t-L \"Temporary merge branch 2\" \\\n-\t\t\t-L \"\" \\\n \t\t\t-L \"Temporary merge branch 1\" \\\n+\t\t\t-L \"\" \\\n+\t\t\t-L \"Temporary merge branch 2\" \\\n \t\t\tours empty theirs &&\n \t\tsed -e \"s/^\\([<=>]\\)/\\1\\1\\1/\" ours >expect &&\n \t\tgit cat-file -p :1:new_a >actual &&\n-- \n2.19.1.280.g0c175526bf\n\n"},{"id":"360816","messageId":"xmqqva5zu6oi.fsf@gitster-ct.c.googlers.com","threadId":"49554","inReplyTo":"20181016201948.16025-3-newren@gmail.com","subject":"Re: [PATCH v2 2/2] merge-recursive: avoid showing conflicts with merge branch before HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-18T06:09:49Z","receivedAt":"2018-10-18T06:09:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> @@ -1283,6 +1302,18 @@ static int merge_mode_and_contents(struct merge_options *o,\n> \t\t\t\t const char *branch2,\n> \t\t\t\t struct merge_file_info *result)\n> {\n> +\tif (o->branch1 != branch1) {\n> +\t\t/*\n> +\t\t * It's weird getting a reverse merge with HEAD on the bottom\n> +\t\t * side of the conflict markers and the other branch on the\n> +\t\t * top. Fix that.\n> +\t\t */\n> +\t\treturn merge_mode_and_contents(o, one, b, a,\n> +\t\t\t\t\t filename,\n> +\t\t\t\t\t branch2, branch1,\n> +\t\t\t\t\t extra_marker_size, result);\n> +\t}\n\nWill queue with the following squashed in.\n\nThanks.\n\n merge-recursive.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 16980db7f9..73b5710386 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1310,8 +1310,7 @@ static int merge_mode_and_contents(struct merge_options *o,\n \t\t */\n \t\treturn merge_mode_and_contents(o, one, b, a,\n \t\t\t\t\t filename,\n-\t\t\t\t\t branch2, branch1,\n-\t\t\t\t\t extra_marker_size, result);\n+\t\t\t\t\t branch2, branch1, result);\n \t}\n \n \tresult->merge = 0;\n"}]}