{"thread":{"id":"55890","subject":"[PATCH 1/7] test: add merge style config test","startedAt":"2021-06-09T19:28:59Z","lastAt":"2021-06-17T18:24:53Z","messageCount":69,"participants":["Felipe Contreras","Eric Sunshine","Johannes Sixt","Đoàn Trần Công Danh","Phillip Wood","Jeff King","Junio C Hamano","Sergey Organov","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"426919","messageId":"20210609192842.696646-2-felipe.contreras@gmail.com","threadId":"55890","inReplyTo":"20210609192842.696646-1-felipe.contreras@gmail.com","subject":"[PATCH 1/7] test: add merge style config test","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-09T19:28:36Z","receivedAt":"2021-06-09T19:28:59Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"We want to test different combinations of merge.conflictstyle, and a new\nfile is the best place to do that.\n\nSigned-off-by: Felipe Contreras \n---\n t/t6440-config-conflict-markers.sh | 44 ++++++++++++++++++++++++++++++\n 1 file changed, 44 insertions(+)\n create mode 100755 t/t6440-config-conflict-markers.sh\n\ndiff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\nnew file mode 100755\nindex 0000000000..6952552c58\n--- /dev/null\n+++ b/t/t6440-config-conflict-markers.sh\n@@ -0,0 +1,44 @@\n+#!/bin/sh\n+\n+test_description='merge style conflict markers configurations'\n+\n+. ./test-lib.sh\n+\n+fill () {\n+\tfor i\n+\tdo\n+\t\techo \"$i\"\n+\tdone\n+}\n+\n+test_expect_success 'merge' '\n+\ttest_create_repo merge &&\n+\t(\n+\t\tcd merge &&\n+\n+\t\tfill 1 2 3 >content &&\n+\t\tgit add content &&\n+\t\tgit commit -m base &&\n+\n+\t\tgit checkout -b r &&\n+\t\techo six >>content &&\n+\t\tgit commit -a -m right &&\n+\n+\t\tgit checkout master &&\n+\t\techo 7 >>content &&\n+\t\tgit commit -a -m left &&\n+\n+\t\ttest_must_fail git merge r &&\n+\t\t! grep -E \"\\|+\" content &&\n+\n+\t\tgit reset --hard &&\n+\t\ttest_must_fail git -c merge.conflictstyle=diff3 merge r &&\n+\t\tgrep -E \"\\|+\" content &&\n+\n+\t\tgit reset --hard &&\n+\t\ttest_must_fail git -c merge.conflictstyle=merge merge r &&\n+\t\t! grep -E \"\\|+\" content\n+\t)\n+'\n+\n+test_done\n-- \n2.32.0.2.g41be0a4e50\n\n"},{"id":"426920","messageId":"20210609192842.696646-4-felipe.contreras@gmail.com","threadId":"55890","inReplyTo":"20210609192842.696646-1-felipe.contreras@gmail.com","subject":"[PATCH 3/7] notes: fix merge.conflictstyle handling","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-09T19:28:38Z","receivedAt":"2021-06-09T19:29:02Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Currently it's completely ignored.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/notes.c | 3 ++-\n t/t6440-config-conflict-markers.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 74bba39ca8..a333cc68ec 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -23,6 +23,7 @@\n #include \"notes-merge.h\"\n #include \"notes-utils.h\"\n #include \"worktree.h\"\n+#include \"xdiff-interface.h\"\n \n static const char * const git_notes_usage[] = {\n \tN_(\"git notes [--ref ] [list []]\"),\n@@ -1000,7 +1001,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tgit_config(git_default_config, NULL);\n+\tgit_config(git_xmerge_config, NULL);\n \targc = parse_options(argc, argv, prefix, options, git_notes_usage,\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n \ndiff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\nindex 978f4e3e70..44f79ac91b 100755\n--- a/t/t6440-config-conflict-markers.sh\n+++ b/t/t6440-config-conflict-markers.sh\n@@ -62,4 +62,31 @@ test_expect_success 'merge-tree' '\n \t)\n '\n \n+test_expect_success 'notes' '\n+\ttest_create_repo notes &&\n+\t(\n+\t\ttest_commit initial &&\n+\n+\t\tgit -c core.notesRef=refs/notes/b notes add -m b initial &&\n+\n+\t\tgit update-ref refs/notes/r refs/notes/b &&\n+\t\tgit -c core.notesRef=refs/notes/r notes add -f -m r initial &&\n+\n+\t\tgit update-ref refs/notes/l refs/notes/b &&\n+\t\tgit config core.notesRef refs/notes/l &&\n+\t\tgit notes add -f -m l initial &&\n+\n+\t\ttest_must_fail git notes merge r &&\n+\t\t! grep -E \"\\|+\" .git/NOTES_MERGE_WORKTREE/* &&\n+\n+\t\tgit notes merge --abort &&\n+\t\ttest_must_fail git -c merge.conflictstyle=diff3 notes merge r &&\n+\t\tgrep -E \"\\|+\" .git/NOTES_MERGE_WORKTREE/* &&\n+\n+\t\tgit notes merge --abort &&\n+\t\ttest_must_fail git -c merge.conflictstyle=merge notes merge r &&\n+\t\t! grep -E \"\\|+\" .git/NOTES_MERGE_WORKTREE/*\n+\t)\n+'\n+\n test_done\n-- \n2.32.0.2.g41be0a4e50\n\n"},{"id":"426921","messageId":"20210609192842.696646-8-felipe.contreras@gmail.com","threadId":"55890","inReplyTo":"20210609192842.696646-1-felipe.contreras@gmail.com","subject":"[PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-09T19:28:42Z","receivedAt":"2021-06-09T19:29:08Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Virtually everyone is using it, and it's one of the first things we\nteach newcomers in order to resolve conflicts efficiently.\n\nLet's make it the default.\n\nThis generates a ton of changes in the tests. Although we probably will\nwant to update them to use th new default, override the configuration so\nwe use the old one for now.\n\nSigned-off-by: Felipe Contreras \n---\n Documentation/config/merge.txt | 12 +++++-----\n Documentation/git-merge-file.txt | 2 ++\n Documentation/git-merge.txt | 28 +++++++-----------------\n Documentation/git-rerere.txt | 2 +-\n Documentation/gitattributes.txt | 6 ++---\n Documentation/technical/rerere.txt | 3 +--\n Documentation/user-manual.txt | 6 ++++-\n t/t2023-checkout-m.sh | 2 ++\n t/t3310-notes-merge-manual-resolve.sh | 2 ++\n t/t3311-notes-merge-fanout.sh | 2 ++\n t/t3404-rebase-interactive.sh | 2 ++\n t/t3507-cherry-pick-conflict.sh | 2 ++\n t/t4017-diff-retval.sh | 2 ++\n t/t4048-diff-combined-binary.sh | 2 ++\n t/t4200-rerere.sh | 2 ++\n t/t4300-merge-tree.sh | 2 ++\n t/t6402-merge-rename.sh | 2 ++\n t/t6403-merge-file.sh | 2 ++\n t/t6404-recursive-merge.sh | 2 ++\n t/t6416-recursive-corner-cases.sh | 2 ++\n t/t6417-merge-ours-theirs.sh | 2 ++\n t/t6418-merge-text-auto.sh | 2 ++\n t/t6422-merge-rename-corner-cases.sh | 2 ++\n t/t6423-merge-rename-directories.sh | 1 +\n t/t6428-merge-conflicts-sparse.sh | 1 +\n t/t6432-merge-recursive-space-options.sh | 2 ++\n t/t6440-config-conflict-markers.sh | 8 +++----\n t/t7201-co.sh | 2 ++\n t/t7506-status-submodule.sh | 1 +\n xdiff-interface.c | 2 +-\n 30 files changed, 70 insertions(+), 38 deletions(-)\n\ndiff --git a/Documentation/config/merge.txt b/Documentation/config/merge.txt\nindex cb2ed58907..2dba937dd0 100644\n--- a/Documentation/config/merge.txt\n+++ b/Documentation/config/merge.txt\n@@ -1,10 +1,10 @@\n merge.conflictStyle::\n-\tSpecify the style in which conflicted hunks are written out to\n-\tworking tree files upon merge. The default is \"merge\", which\n-\tshows a `<<<<<<<` conflict marker, changes made by one side,\n-\ta `=======` marker, changes made by the other side, and then\n-\ta `>>>>>>>` marker. An alternate style, \"diff3\", adds a `|||||||`\n-\tmarker and the original text before the `=======` marker.\n+\tSpecify the style in which conflicted hunks are written out to working\n+\ttree files upon merge. The default is \"diff3\", which shows a `<<<<<<<`\n+\tconflict marker, changes made by one side, a `|||||||` marker, the\n+\toriginal text, a `=======` marker, changes made by the other side, and\n+\tthen a `>>>>>>>` marker. A simpler mode \"merge\" omits the `|||||||`\n+\tmarker and the original text.\n \n merge.defaultToUpstream::\n \tIf merge is called without any commit argument, merge the upstream\ndiff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt\nindex f856032613..7d8e74c872 100644\n--- a/Documentation/git-merge-file.txt\n+++ b/Documentation/git-merge-file.txt\n@@ -30,6 +30,8 @@ normally outputs a warning and brackets the conflict with lines containing\n \n \t<<<<<<< A\n \tlines in file A\n+\t|||||||\n+\tlines in merge base\n \t=======\n \tlines in file B\n \t>>>>>>> B\ndiff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\nindex 3819fadac1..14dadf2e16 100644\n--- a/Documentation/git-merge.txt\n+++ b/Documentation/git-merge.txt\n@@ -233,7 +233,7 @@ final result verbatim. When both sides made changes to the same area,\n however, Git cannot randomly pick one side over the other, and asks you to\n resolve it by leaving what both sides did to that area.\n \n-By default, Git uses the same style as the one used by the \"merge\" program\n+By default, Git uses a similar style to the one used by the \"merge\" program\n from the RCS suite to present such a conflicted hunk, like this:\n \n ------------\n@@ -242,6 +242,8 @@ ancestor, or cleanly resolved because only one side changed.\n <<<<<<< yours:sample.txt\n Conflict resolution is hard;\n let's go shopping.\n+|||||||\n+Originally there's no conflict.\n =======\n Git makes conflict resolution easy.\n >>>>>>> theirs:sample.txt\n@@ -249,17 +251,12 @@ And here is another line that is cleanly resolved or unmodified.\n ------------\n \n The area where a pair of conflicting changes happened is marked with markers\n-`<<<<<<<`, `=======`, and `>>>>>>>`. The part before the `=======`\n-is typically your side, and the part afterwards is typically their side.\n-\n-The default format does not show what the original said in the conflicting\n-area. You cannot tell how many lines are deleted and replaced with\n-Barbie's remark on your side. The only thing you can tell is that your\n-side wants to say it is hard and you'd prefer to go shopping, while the\n-other side wants to claim it is easy.\n+`<<<<<<<`, `=======`, and `>>>>>>>`. The part before the `|||||||`\n+is typically your side, and the part after `=======` is typically their side.\n+In-between is the original code.\n \n-An alternative style can be used by setting the \"merge.conflictStyle\"\n-configuration variable to \"diff3\". In \"diff3\" style, the above conflict\n+An more basic style can be used by setting the \"merge.conflictStyle\"\n+configuration variable to \"merge\". In \"merge\" style, the above conflict\n may look like this:\n \n ------------\n@@ -268,21 +265,12 @@ ancestor, or cleanly resolved because only one side changed.\n <<<<<<< yours:sample.txt\n Conflict resolution is hard;\n let's go shopping.\n-|||||||\n-Conflict resolution is hard.\n =======\n Git makes conflict resolution easy.\n >>>>>>> theirs:sample.txt\n And here is another line that is cleanly resolved or unmodified.\n ------------\n \n-In addition to the `<<<<<<<`, `=======`, and `>>>>>>>` markers, it uses\n-another `|||||||` marker that is followed by the original text. You can\n-tell that the original just stated a fact, and your side simply gave in to\n-that statement and gave up, while the other side tried to have a more\n-positive attitude. You can sometimes come up with a better resolution by\n-viewing the original.\n-\n \n HOW TO RESOLVE CONFLICTS\n ------------------------\ndiff --git a/Documentation/git-rerere.txt b/Documentation/git-rerere.txt\nindex 4cfc883378..89b0820995 100644\n--- a/Documentation/git-rerere.txt\n+++ b/Documentation/git-rerere.txt\n@@ -159,7 +159,7 @@ resolve.\n \n Running the 'git rerere' command immediately after a conflicted\n automerge records the conflicted working tree files, with the\n-usual conflict markers `<<<<<<<`, `=======`, and `>>>>>>>` in\n+usual conflict markers `<<<<<<<`, `|||||||`, `=======`, and `>>>>>>>` in\n them. Later, after you are done resolving the conflicts,\n running 'git rerere' again will record the resolved state of these\n files. Suppose you did this when you created the test merge of\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 83fd4e19a4..b767215ac2 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -1042,10 +1042,10 @@ text::\n \n \tUsual 3-way file level merge for text files. Conflicted\n \tregions are marked with conflict markers `<<<<<<<`,\n-\t`=======` and `>>>>>>>`. The version from your branch\n-\tappears before the `=======` marker, and the version\n+\t`|||||||`, `=======` and `>>>>>>>`. The version from your branch\n+\tappears before the `|||||||` marker, and the version\n \tfrom the merged branch appears after the `=======`\n-\tmarker.\n+\tmarker. In-between is the original.\n \n binary::\n \ndiff --git a/Documentation/technical/rerere.txt b/Documentation/technical/rerere.txt\nindex af5f9fc24f..38b44f4430 100644\n--- a/Documentation/technical/rerere.txt\n+++ b/Documentation/technical/rerere.txt\n@@ -42,8 +42,7 @@ get a conflict like the following:\n >>>>>>> AC\n \n Doing the analogous with AC2 (forking a branch ABAC2 off of branch AB\n-and then merging branch AC2 into it), using the diff3 conflict style,\n-we get a conflict like the following:\n+and then merging branch AC2 into it), we get a conflict like the following:\n \n <<<<<<< HEAD\n B\ndiff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt\nindex f9e54b8674..3ddde87482 100644\n--- a/Documentation/user-manual.txt\n+++ b/Documentation/user-manual.txt\n@@ -1243,6 +1243,8 @@ files with conflicts will have conflict markers added, like this:\n -------------------------------------------------\n <<<<<<< HEAD:file.txt\n Hello world\n+|||||||\n+Original\n =======\n Goodbye\n >>>>>>> 77976da35a11db4580b80ae27e8d65caf5208086:file.txt\n@@ -1276,9 +1278,11 @@ diff --cc file.txt\n index 802992c,2b60207..0000000\n --- a/file.txt\n +++ b/file.txt\n-@@@ -1,1 -1,1 +1,5 @@@\n+@@@ -1,1 -1,1 +1,7 @@@\n ++<<<<<<< HEAD:file.txt\n +Hello world\n+++|||||||\n+++Original\n ++=======\n + Goodbye\n ++>>>>>>> 77976da35a11db4580b80ae27e8d65caf5208086:file.txt\ndiff --git a/t/t2023-checkout-m.sh b/t/t2023-checkout-m.sh\nindex 7b327b7544..219c82532a 100755\n--- a/t/t2023-checkout-m.sh\n+++ b/t/t2023-checkout-m.sh\n@@ -9,6 +9,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n \n+git config merge.conflictstyle merge # TODO: use the default\n+\n test_expect_success setup '\n \ttest_tick &&\n \ttest_commit both.txt both.txt initial &&\ndiff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\nindex d3d72e25fe..cbd5d8302e 100755\n--- a/t/t3310-notes-merge-manual-resolve.sh\n+++ b/t/t3310-notes-merge-manual-resolve.sh\n@@ -7,6 +7,8 @@ test_description='Test notes merging with manual conflict resolution'\n \n . ./test-lib.sh\n \n+git config --global merge.conflictstyle merge # TODO: use the default\n+\n # Set up a notes merge scenario with different kinds of conflicts\n test_expect_success 'setup commits' '\n \ttest_commit 1st &&\ndiff --git a/t/t3311-notes-merge-fanout.sh b/t/t3311-notes-merge-fanout.sh\nindex 5b675417e9..4aeaa05c15 100755\n--- a/t/t3311-notes-merge-fanout.sh\n+++ b/t/t3311-notes-merge-fanout.sh\n@@ -7,6 +7,8 @@ test_description='Test notes merging at various fanout levels'\n \n . ./test-lib.sh\n \n+git config merge.conflictstyle merge # TODO: use the default\n+\n verify_notes () {\n \tnotes_ref=\"$1\"\n \tcommit=\"$2\"\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 66bcbbf952..769079a71c 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -32,6 +32,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . \"$TEST_DIRECTORY\"/lib-rebase.sh\n \n+git config merge.conflictstyle merge # TODO: use the default\n+\n test_expect_success 'setup' '\n \tgit switch -C primary &&\n \ttest_commit A file1 &&\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 014001b8f3..647a40f314 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -281,6 +281,7 @@ test_expect_success \\\n \n test_expect_success 'failed cherry-pick describes conflict in work tree' '\n \tpristine_detach initial &&\n+\tgit config merge.conflictstyle merge && # TODO: use the default\n \tcat <<-EOF >expected &&\n \t<<<<<<< HEAD\n \ta\n@@ -316,6 +317,7 @@ test_expect_success 'diff3 -m style' '\n \n test_expect_success 'revert also handles conflicts sanely' '\n \tgit config --unset merge.conflictstyle &&\n+\tgit config merge.conflictstyle merge && # TODO: use the default\n \tpristine_detach initial &&\n \tcat <<-EOF >expected &&\n \t<<<<<<< HEAD\ndiff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh\nindex ed461f481e..04b77af2a4 100755\n--- a/t/t4017-diff-retval.sh\n+++ b/t/t4017-diff-retval.sh\n@@ -7,6 +7,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n \n+git config merge.conflictstyle merge # TODO: use the default\n+\n test_expect_success 'setup' '\n \techo \"1 \" >a &&\n \tgit add . &&\ndiff --git a/t/t4048-diff-combined-binary.sh b/t/t4048-diff-combined-binary.sh\nindex 0260cf64f5..49a56731dd 100755\n--- a/t/t4048-diff-combined-binary.sh\n+++ b/t/t4048-diff-combined-binary.sh\n@@ -6,6 +6,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n \n+git config merge.conflictstyle merge # TODO: use the default\n+\n test_expect_success 'setup binary merge conflict' '\n \techo oneQ1 | q_to_nul >binary &&\n \tgit add binary &&\ndiff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\nindex 9f8c76dffb..e9ae3d6fde 100755\n--- a/t/t4200-rerere.sh\n+++ b/t/t4200-rerere.sh\n@@ -27,6 +27,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n \n+git config merge.conflictstyle merge # TODO: use the default\n+\n test_expect_success 'setup' '\n \tcat >a1 <<-\\EOF &&\n \tSome title\ndiff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\nindex e59601e5fe..f21ccaf0a6 100755\n--- a/t/t4300-merge-tree.sh\n+++ b/t/t4300-merge-tree.sh\n@@ -6,6 +6,8 @@\n test_description='git merge-tree'\n . ./test-lib.sh\n \n+git config merge.conflictstyle merge # TODO: use the default\n+\n test_expect_success setup '\n \ttest_commit \"initial\" \"initial-file\" \"initial\"\n '\ndiff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh\nindex 425dad97d5..4f01bbc451 100755\n--- a/t/t6402-merge-rename.sh\n+++ b/t/t6402-merge-rename.sh\n@@ -270,6 +270,7 @@ test_expect_success 'setup for rename + d/f conflicts' '\n \tgit checkout --orphan dir-in-way &&\n \tgit rm -rf . &&\n \tgit clean -fdqx &&\n+\tgit config merge.conflictstyle merge && # TODO: use the default\n \n \tmkdir sub &&\n \tmkdir dir &&\n@@ -871,6 +872,7 @@ test_expect_success 'setup for use of extended merge markers' '\n \tgit clean -fdqx &&\n \trm -rf .git &&\n \tgit init &&\n+\tgit config merge.conflictstyle merge && # TODO: use the default\n \n \tprintf \"1\\n2\\n3\\n4\\n5\\n6\\n7\\n8\\n\" >original_file &&\n \tgit add original_file &&\ndiff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh\nindex 2f421d967a..1428dfb5c6 100755\n--- a/t/t6403-merge-file.sh\n+++ b/t/t6403-merge-file.sh\n@@ -3,6 +3,8 @@\n test_description='RCS merge replacement: merge-file'\n . ./test-lib.sh\n \n+git config merge.conflictstyle merge # TODO: use the default\n+\n test_expect_success 'setup' '\n \tcat >orig.txt <<-\\EOF &&\n \tDominus regit me,\ndiff --git a/t/t6404-recursive-merge.sh b/t/t6404-recursive-merge.sh\nindex eaf48e941e..a3354b8f9a 100755\n--- a/t/t6404-recursive-merge.sh\n+++ b/t/t6404-recursive-merge.sh\n@@ -6,6 +6,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n \n+git config merge.conflictstyle merge # TODO: use the default\n+\n # This scenario is based on a real-world repository of Shawn Pearce.\n \n # 1 - A - D - F\ndiff --git a/t/t6416-recursive-corner-cases.sh b/t/t6416-recursive-corner-cases.sh\nindex 84f5082366..ac4e69a325 100755\n--- a/t/t6416-recursive-corner-cases.sh\n+++ b/t/t6416-recursive-corner-cases.sh\n@@ -8,6 +8,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-merge.sh\n \n+git config --global merge.conflictstyle merge # TODO: use the default\n+\n #\n # L1 L2\n # o---o\ndiff --git a/t/t6417-merge-ours-theirs.sh b/t/t6417-merge-ours-theirs.sh\nindex ac9aee9a66..b8208a383b 100755\n--- a/t/t6417-merge-ours-theirs.sh\n+++ b/t/t6417-merge-ours-theirs.sh\n@@ -6,6 +6,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n \n+git config merge.conflictstyle merge # TODO: use the default\n+\n test_expect_success setup '\n \tfor i in 1 2 3 4 5 6 7 8 9\n \tdo\ndiff --git a/t/t6418-merge-text-auto.sh b/t/t6418-merge-text-auto.sh\nindex 1e0296dd17..e18f67776c 100755\n--- a/t/t6418-merge-text-auto.sh\n+++ b/t/t6418-merge-text-auto.sh\n@@ -17,6 +17,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n \n+git config merge.conflictstyle merge # TODO: use the default\n+\n test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b\n \n compare_files () {\ndiff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh\nindex bf4ce3c63d..6bb4b6d968 100755\n--- a/t/t6422-merge-rename-corner-cases.sh\n+++ b/t/t6422-merge-rename-corner-cases.sh\n@@ -9,6 +9,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-merge.sh\n \n+git config --global merge.conflictstyle merge # TODO: use the default\n+\n test_setup_rename_delete_untracked () {\n \ttest_create_repo rename-delete-untracked &&\n \t(\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 7134769149..5f6cacd064 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -28,6 +28,7 @@ test_description=\"recursive merge with directory renames\"\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-merge.sh\n \n+git config --global merge.conflictstyle merge # TODO: use the default\n \n ###########################################################################\n # SECTION 1: Basic cases we should be able to handle\ndiff --git a/t/t6428-merge-conflicts-sparse.sh b/t/t6428-merge-conflicts-sparse.sh\nindex 7e8bf497f8..18975801db 100755\n--- a/t/t6428-merge-conflicts-sparse.sh\n+++ b/t/t6428-merge-conflicts-sparse.sh\n@@ -25,6 +25,7 @@ test_description=\"merge cases\"\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-merge.sh\n \n+git config --global merge.conflictstyle merge # TODO: use the default\n \n # Testcase basic, conflicting changes in 'numerals'\n \ndiff --git a/t/t6432-merge-recursive-space-options.sh b/t/t6432-merge-recursive-space-options.sh\nindex db4b77e63d..5cfe8a4fbd 100755\n--- a/t/t6432-merge-recursive-space-options.sh\n+++ b/t/t6432-merge-recursive-space-options.sh\n@@ -16,6 +16,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n \n+git config merge.conflictstyle merge # TODO: use the default\n+\n test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b\n if test_have_prereq GREP_STRIPS_CR\n then\ndiff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\nindex 485ad0eee0..ae0bab37ad 100755\n--- a/t/t6440-config-conflict-markers.sh\n+++ b/t/t6440-config-conflict-markers.sh\n@@ -29,7 +29,7 @@ test_expect_success 'merge' '\n \t\tgit commit -a -m left &&\n \n \t\ttest_must_fail git merge r &&\n-\t\t! grep -E \"\\|+\" content &&\n+\t\tgrep -E \"\\|+\" content &&\n \n \t\tgit reset --hard &&\n \t\ttest_must_fail git -c merge.conflictstyle=diff3 merge r &&\n@@ -52,7 +52,7 @@ test_expect_success 'merge-tree' '\n \t\ttest_commit l content l &&\n \n \t\tgit merge-tree initial r l >actual &&\n-\t\t! grep -E \"\\|+\" actual &&\n+\t\tgrep -E \"\\|+\" actual &&\n \n \t\tgit -c merge.conflictstyle=diff3 merge-tree initial r l >actual &&\n \t\tgrep -E \"\\|+\" actual &&\n@@ -77,7 +77,7 @@ test_expect_success 'notes' '\n \t\tgit notes add -f -m l initial &&\n \n \t\ttest_must_fail git notes merge r &&\n-\t\t! grep -E \"\\|+\" .git/NOTES_MERGE_WORKTREE/* &&\n+\t\tgrep -E \"\\|+\" .git/NOTES_MERGE_WORKTREE/* &&\n \n \t\tgit notes merge --abort &&\n \t\ttest_must_fail git -c merge.conflictstyle=diff3 notes merge r &&\n@@ -104,7 +104,7 @@ test_expect_success 'checkout' '\n \n \t\tfill b d >content &&\n \t\tgit checkout --merge master &&\n-\t\t! grep -E \"\\|+\" content &&\n+\t\tgrep -E \"\\|+\" content &&\n \n \t\tgit config merge.conflictstyle merge &&\n \ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 7f6e23a4bb..11444d5360 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -25,6 +25,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \n . ./test-lib.sh\n \n+git config merge.conflictstyle merge # TODO: use the default\n+\n test_tick\n \n fill () {\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 3fcb44767f..90449e80c3 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -253,6 +253,7 @@ test_expect_success 'status with merge conflict in .gitmodules' '\n \ttest_create_repo_with_commit sub2 &&\n \t(\n \t\tcd super &&\n+\t\tgit config merge.conflictstyle merge && # TODO: use the default\n \t\tprev=$(git rev-parse HEAD) &&\n \t\tgit checkout -b add_sub1 &&\n \t\tgit submodule add ../sub1 &&\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 19a030fbe2..1447771724 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -299,7 +299,7 @@ int xdiff_compare_lines(const char *l1, long s1,\n \treturn xdl_recmatch(l1, s1, l2, s2, flags);\n }\n \n-int git_xmerge_style = XDL_MERGE_STYLE_MERGE;\n+int git_xmerge_style = XDL_MERGE_STYLE_DIFF3;\n \n int git_xmerge_config(const char *var, const char *value, void *cb)\n {\n-- \n2.32.0.2.g41be0a4e50\n\n"},{"id":"426922","messageId":"20210609192842.696646-6-felipe.contreras@gmail.com","threadId":"55890","inReplyTo":"20210609192842.696646-1-felipe.contreras@gmail.com","subject":"[PATCH 5/7] xdiff: rename XDL_MERGE_STYLE_DIFF3","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-09T19:28:40Z","receivedAt":"2021-06-09T19:29:21Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"If we don't specify we are talking about a style, XDL_MERGE_MINIMAL\ncould be confused with a valid value instead of XDL_MERGE_DIFF3, which\nit isn't.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/merge-file.c | 2 +-\n xdiff-interface.c | 2 +-\n xdiff/xdiff.h | 2 +-\n xdiff/xmerge.c | 4 ++--\n 4 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 06a2f90c48..a4097a596f 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -33,7 +33,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \tint quiet = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL('p', \"stdout\", &to_stdout, N_(\"send results to standard output\")),\n-\t\tOPT_SET_INT(0, \"diff3\", &xmp.style, N_(\"use a diff3 based merge\"), XDL_MERGE_DIFF3),\n+\t\tOPT_SET_INT(0, \"diff3\", &xmp.style, N_(\"use a diff3 based merge\"), XDL_MERGE_STYLE_DIFF3),\n \t\tOPT_SET_INT(0, \"ours\", &xmp.favor, N_(\"for conflicts, use our version\"),\n \t\t\t XDL_MERGE_FAVOR_OURS),\n \t\tOPT_SET_INT(0, \"theirs\", &xmp.favor, N_(\"for conflicts, use their version\"),\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 609615db2c..64e2c4e301 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -307,7 +307,7 @@ int git_xmerge_config(const char *var, const char *value, void *cb)\n \t\tif (!value)\n \t\t\tdie(\"'%s' is not a boolean\", var);\n \t\tif (!strcmp(value, \"diff3\"))\n-\t\t\tgit_xmerge_style = XDL_MERGE_DIFF3;\n+\t\t\tgit_xmerge_style = XDL_MERGE_STYLE_DIFF3;\n \t\telse if (!strcmp(value, \"merge\"))\n \t\t\tgit_xmerge_style = 0;\n \t\t/*\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 7a04605146..45883f5eb3 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -64,7 +64,7 @@ extern \"C\" {\n #define XDL_MERGE_FAVOR_UNION 3\n \n /* merge output styles */\n-#define XDL_MERGE_DIFF3 1\n+#define XDL_MERGE_STYLE_DIFF3 1\n \n typedef struct s_mmfile {\n \tchar *ptr;\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex 1659edb453..f6916a4ba4 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -230,7 +230,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n \tsize += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, 1,\n \t\t\t dest ? dest + size : NULL);\n \n-\tif (style == XDL_MERGE_DIFF3) {\n+\tif (style == XDL_MERGE_STYLE_DIFF3) {\n \t\t/* Shared preimage */\n \t\tif (!dest) {\n \t\t\tsize += marker_size + 1 + needs_cr + marker3_size;\n@@ -482,7 +482,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n \tint style = xmp->style;\n \tint favor = xmp->favor;\n \n-\tif (style == XDL_MERGE_DIFF3) {\n+\tif (style == XDL_MERGE_STYLE_DIFF3) {\n \t\t/*\n \t\t * \"diff3 -m\" output does not make sense for anything\n \t\t * more aggressive than XDL_MERGE_EAGER.\n-- \n2.32.0.2.g41be0a4e50\n\n"},{"id":"426923","messageId":"20210609192842.696646-7-felipe.contreras@gmail.com","threadId":"55890","inReplyTo":"20210609192842.696646-1-felipe.contreras@gmail.com","subject":"[PATCH 6/7] xdiff: simplify style assignments","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-09T19:28:41Z","receivedAt":"2021-06-09T19:30:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"There is little value in checking that git_xmerge_style isn't 0 before\nchanging it's default value.\n\nMost of the time it isn't 0 anyway, so just assign the value directly.\n\nAlso, add the missing constant for the default value: XDL_MERGE_STYLE_MERGE.\n\nAdditionally this change has the benefit that it gets rid of a Yoda\ncondition.\n\nNo functional changes.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/merge-file.c | 3 +--\n ll-merge.c | 3 +--\n xdiff-interface.c | 4 ++--\n xdiff/xdiff.h | 1 +\n 4 files changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex a4097a596f..01951762ec 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -55,8 +55,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \tif (startup_info->have_repository) {\n \t\t/* Read the configuration file */\n \t\tgit_config(git_xmerge_config, NULL);\n-\t\tif (0 <= git_xmerge_style)\n-\t\t\txmp.style = git_xmerge_style;\n+\t\txmp.style = git_xmerge_style;\n \t}\n \n \targc = parse_options(argc, argv, prefix, options, merge_file_usage, 0);\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 9a8a2c365c..4ce8d3f9cc 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -124,8 +124,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n \txmp.level = XDL_MERGE_ZEALOUS;\n \txmp.favor = opts->variant;\n \txmp.xpp.flags = opts->xdl_opts;\n-\tif (git_xmerge_style >= 0)\n-\t\txmp.style = git_xmerge_style;\n+\txmp.style = git_xmerge_style;\n \tif (marker_size > 0)\n \t\txmp.marker_size = marker_size;\n \txmp.ancestor = orig_name;\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 64e2c4e301..19a030fbe2 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -299,7 +299,7 @@ int xdiff_compare_lines(const char *l1, long s1,\n \treturn xdl_recmatch(l1, s1, l2, s2, flags);\n }\n \n-int git_xmerge_style = -1;\n+int git_xmerge_style = XDL_MERGE_STYLE_MERGE;\n \n int git_xmerge_config(const char *var, const char *value, void *cb)\n {\n@@ -309,7 +309,7 @@ int git_xmerge_config(const char *var, const char *value, void *cb)\n \t\tif (!strcmp(value, \"diff3\"))\n \t\t\tgit_xmerge_style = XDL_MERGE_STYLE_DIFF3;\n \t\telse if (!strcmp(value, \"merge\"))\n-\t\t\tgit_xmerge_style = 0;\n+\t\t\tgit_xmerge_style = XDL_MERGE_STYLE_MERGE;\n \t\t/*\n \t\t * Please update _git_checkout() in\n \t\t * git-completion.bash when you add new merge config\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 45883f5eb3..d24cd9f6ae 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -64,6 +64,7 @@ extern \"C\" {\n #define XDL_MERGE_FAVOR_UNION 3\n \n /* merge output styles */\n+#define XDL_MERGE_STYLE_MERGE 0\n #define XDL_MERGE_STYLE_DIFF3 1\n \n typedef struct s_mmfile {\n-- \n2.32.0.2.g41be0a4e50\n\n"},{"id":"426924","messageId":"20210609192842.696646-1-felipe.contreras@gmail.com","threadId":"55890","inReplyTo":null,"subject":"[PATCH 0/7] Make diff3 the default conflict style","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-09T19:28:35Z","receivedAt":"2021-06-09T19:30:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This patch series turned out much more complicated that simply flipping\nthe switch and dealing with the consequences. Apparently some commands\nare completely ignoring the configuration (notes and merge-tree), and\nothers are handling it wrong (checkout).\n\nSo in preparation I created a new test to make sure these rowdy\ncommands handle the configuration correctly, and then step by step I fix\nthem.\n\nOnce all the commands are fixed I proceed to cleanup xdiff-interface in\npreparation for the switch.\n\nAnd finally once the switch is flipped the documnetation is updated, and\nfunch of test scripts receive a temporary configuration that returns\nthem to the old \"merge\" (diff2) behavior so they pass with minimum\nchanges.\n\nI have already written patches to update the tests so no configuration\nis needed and they parse the diff3 style directly, but the series is\nalready quite verbose as it is.\n\nOne salient thorn is that from my point of view merge_recursive_config()\nis implemtend wrongly and thus can't be called as other configuration\nfunctions, like git_diff_basic_config(). It seems there's a huge area of\nopportunity there to clean all that up, but that's for another series.\n\nFelipe Contreras (7):\n test: add merge style config test\n merge-tree: fix merge.conflictstyle handling\n notes: fix merge.conflictstyle handling\n checkout: fix merge.conflictstyle handling\n xdiff: rename XDL_MERGE_STYLE_DIFF3\n xdiff: simplify style assignments\n xdiff: make diff3 the default conflictStyle\n\n Documentation/config/merge.txt | 12 +--\n Documentation/git-merge-file.txt | 2 +\n Documentation/git-merge.txt | 28 ++----\n Documentation/git-rerere.txt | 2 +-\n Documentation/gitattributes.txt | 6 +-\n Documentation/technical/rerere.txt | 3 +-\n Documentation/user-manual.txt | 6 +-\n builtin/merge-file.c | 5 +-\n builtin/merge-recursive.c | 3 +\n builtin/merge-tree.c | 4 +\n builtin/merge.c | 4 +\n builtin/notes.c | 3 +-\n ll-merge.c | 3 +-\n merge-recursive.c | 2 +-\n sequencer.c | 5 +\n t/t2023-checkout-m.sh | 2 +\n t/t3310-notes-merge-manual-resolve.sh | 2 +\n t/t3311-notes-merge-fanout.sh | 2 +\n t/t3404-rebase-interactive.sh | 2 +\n t/t3507-cherry-pick-conflict.sh | 2 +\n t/t4017-diff-retval.sh | 2 +\n t/t4048-diff-combined-binary.sh | 2 +\n t/t4200-rerere.sh | 2 +\n t/t4300-merge-tree.sh | 2 +\n t/t6402-merge-rename.sh | 2 +\n t/t6403-merge-file.sh | 2 +\n t/t6404-recursive-merge.sh | 2 +\n t/t6416-recursive-corner-cases.sh | 2 +\n t/t6417-merge-ours-theirs.sh | 2 +\n t/t6418-merge-text-auto.sh | 2 +\n t/t6422-merge-rename-corner-cases.sh | 2 +\n t/t6423-merge-rename-directories.sh | 1 +\n t/t6428-merge-conflicts-sparse.sh | 1 +\n t/t6432-merge-recursive-space-options.sh | 2 +\n t/t6440-config-conflict-markers.sh | 123 +++++++++++++++++++++++\n t/t7201-co.sh | 2 +\n t/t7506-status-submodule.sh | 1 +\n xdiff-interface.c | 6 +-\n xdiff/xdiff.h | 3 +-\n xdiff/xmerge.c | 4 +-\n 40 files changed, 217 insertions(+), 46 deletions(-)\n create mode 100755 t/t6440-config-conflict-markers.sh\n\n-- \n2.32.0.2.g41be0a4e50\n\n"},{"id":"426925","messageId":"20210609192842.696646-3-felipe.contreras@gmail.com","threadId":"55890","inReplyTo":"20210609192842.696646-1-felipe.contreras@gmail.com","subject":"[PATCH 2/7] merge-tree: fix merge.conflictstyle handling","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-09T19:28:37Z","receivedAt":"2021-06-09T19:30:12Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Currently it's completely ignored.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/merge-tree.c | 4 ++++\n t/t6440-config-conflict-markers.sh | 21 +++++++++++++++++++++\n 2 files changed, 25 insertions(+)\n\ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex de8520778d..7d677bd75c 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -7,6 +7,8 @@\n #include \"blob.h\"\n #include \"exec-cmd.h\"\n #include \"merge-blobs.h\"\n+#include \"config.h\"\n+#include \"xdiff-interface.h\"\n \n static const char merge_tree_usage[] = \"git merge-tree \";\n \n@@ -378,6 +380,8 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)\n \tif (argc != 4)\n \t\tusage(merge_tree_usage);\n \n+\tgit_config(git_xmerge_config, NULL);\n+\n \tbuf1 = get_tree_descriptor(r, t+0, argv[1]);\n \tbuf2 = get_tree_descriptor(r, t+1, argv[2]);\n \tbuf3 = get_tree_descriptor(r, t+2, argv[3]);\ndiff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\nindex 6952552c58..978f4e3e70 100755\n--- a/t/t6440-config-conflict-markers.sh\n+++ b/t/t6440-config-conflict-markers.sh\n@@ -41,4 +41,25 @@ test_expect_success 'merge' '\n \t)\n '\n \n+test_expect_success 'merge-tree' '\n+\ttest_create_repo merge-tree &&\n+\t(\n+\t\tcd merge-tree &&\n+\n+\t\ttest_commit initial initial-file initial &&\n+\t\ttest_commit r content r &&\n+\t\tgit reset --hard initial &&\n+\t\ttest_commit l content l &&\n+\n+\t\tgit merge-tree initial r l >actual &&\n+\t\t! grep -E \"\\|+\" actual &&\n+\n+\t\tgit -c merge.conflictstyle=diff3 merge-tree initial r l >actual &&\n+\t\tgrep -E \"\\|+\" actual &&\n+\n+\t\tgit -c merge.conflictstyle=merge merge-tree initial r l >actual &&\n+\t\t! grep -E \"\\|+\" actual\n+\t)\n+'\n+\n test_done\n-- \n2.32.0.2.g41be0a4e50\n\n"},{"id":"426926","messageId":"20210609192842.696646-5-felipe.contreras@gmail.com","threadId":"55890","inReplyTo":"20210609192842.696646-1-felipe.contreras@gmail.com","subject":"[PATCH 4/7] checkout: fix merge.conflictstyle handling","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-09T19:28:39Z","receivedAt":"2021-06-09T19:30:16Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Currently both merge.conflictStyle and `git commit --merge\n--conflict=diff3` don't work together, since the former wrongly\noverrides the later.\n\nThe way merge configurations are handled is not correct.\nIt should be possible to do git_config(merge_recursive_config, ...) just\nlike we can with git_diff_basic_config and others.\n\nTherefore builtins like `git merge` can't call this function at the\nright time.\n\nWe shuffle the functions a little bit so at least merge_recursive_config\ndoesn't call git_xmerge_config directly and thus override previous\nconfigurations.\n\nSigned-off-by: Felipe Contreras \n---\n builtin/merge-recursive.c | 3 +++\n builtin/merge.c | 4 ++++\n merge-recursive.c | 2 +-\n sequencer.c | 5 +++++\n t/t6440-config-conflict-markers.sh | 31 ++++++++++++++++++++++++++++++\n 5 files changed, 44 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex a4bfd8fc51..80f9279b4c 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -4,6 +4,7 @@\n #include \"tag.h\"\n #include \"merge-recursive.h\"\n #include \"xdiff-interface.h\"\n+#include \"config.h\"\n \n static const char builtin_merge_recursive_usage[] =\n \t\"git %s ... -- ...\";\n@@ -30,6 +31,8 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)\n \tchar *better1, *better2;\n \tstruct commit *result;\n \n+\tgit_config(git_xmerge_config, NULL);\n+\n \tinit_merge_options(&o, the_repository);\n \tif (argv[0] && ends_with(argv[0], \"-subtree\"))\n \t\to.subtree_shift = \"\";\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex eddb8ae70d..7aa3dbb111 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -43,6 +43,7 @@\n #include \"commit-reach.h\"\n #include \"wt-status.h\"\n #include \"commit-graph.h\"\n+#include \"xdiff-interface.h\"\n \n #define DEFAULT_TWOHEAD (1<<0)\n #define DEFAULT_OCTOPUS (1<<1)\n@@ -659,6 +660,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n \tif (status)\n \t\treturn status;\n \tstatus = git_gpg_config(k, v, NULL);\n+\tif (status)\n+\t\treturn status;\n+\tstatus = git_xmerge_config(k, v, NULL);\n \tif (status)\n \t\treturn status;\n \treturn git_diff_ui_config(k, v, cb);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex d146bb116f..10e6e1e4d1 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3845,7 +3845,7 @@ static void merge_recursive_config(struct merge_options *opt)\n \t\t} /* avoid erroring on values from future versions of git */\n \t\tfree(value);\n \t}\n-\tgit_config(git_xmerge_config, NULL);\n+\tgit_config(git_default_config, NULL);\n }\n \n void init_merge_options(struct merge_options *opt,\ndiff --git a/sequencer.c b/sequencer.c\nindex 0bec01cf38..9e2bdca0f6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -34,6 +34,7 @@\n #include \"commit-reach.h\"\n #include \"rebase-interactive.h\"\n #include \"reset.h\"\n+#include \"xdiff-interface.h\"\n \n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n@@ -224,6 +225,10 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n \tif (status)\n \t\treturn status;\n \n+\tstatus = git_xmerge_config(k, v, NULL);\n+\tif (status)\n+\t\treturn status;\n+\n \treturn git_diff_basic_config(k, v, NULL);\n }\n \ndiff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\nindex 44f79ac91b..485ad0eee0 100755\n--- a/t/t6440-config-conflict-markers.sh\n+++ b/t/t6440-config-conflict-markers.sh\n@@ -89,4 +89,35 @@ test_expect_success 'notes' '\n \t)\n '\n \n+test_expect_success 'checkout' '\n+\ttest_create_repo checkout &&\n+\t(\n+\t\ttest_commit checkout &&\n+\n+\t\tfill a b c d e >content &&\n+\t\tgit add content &&\n+\t\tgit commit -m initial &&\n+\n+\t\tgit checkout -b simple master &&\n+\t\tfill a c e >content &&\n+\t\tgit commit -a -m simple &&\n+\n+\t\tfill b d >content &&\n+\t\tgit checkout --merge master &&\n+\t\t! grep -E \"\\|+\" content &&\n+\n+\t\tgit config merge.conflictstyle merge &&\n+\n+\t\tgit checkout -f simple &&\n+\t\tfill b d >content &&\n+\t\tgit checkout --merge --conflict=diff3 master &&\n+\t\tgrep -E \"\\|+\" content &&\n+\n+\t\tgit checkout -f simple &&\n+\t\tfill b d >content &&\n+\t\tgit checkout --merge --conflict=merge master &&\n+\t\t! grep -E \"\\|+\" content\n+\t)\n+'\n+\n test_done\n-- \n2.32.0.2.g41be0a4e50\n\n"},{"id":"426927","messageId":"CAPig+cSK1YkFwQASuV1n4fr5Ko8tH8QGxvVgQ=Vd0MeY9BSrzw@mail.gmail.com","threadId":"55890","inReplyTo":"20210609192842.696646-2-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/7] test: add merge style config test","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-06-09T19:42:54Z","receivedAt":"2021-06-09T19:43:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 9, 2021 at 3:29 PM Felipe Contreras\n wrote:\n> We want to test different combinations of merge.conflictstyle, and a new\n> file is the best place to do that.\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\n> @@ -0,0 +1,44 @@\n> +fill () {\n> + for i\n> + do\n> + echo \"$i\"\n> + done\n> +}\n\nThis seems to duplicate the behavior of test_write_lines()...\n\n> +test_expect_success 'merge' '\n> + test_create_repo merge &&\n> + (\n> + cd merge &&\n> + fill 1 2 3 >content &&\n\n... which could be used here instead:\n\n test_write_lines 1 2 3 >content &&\n"},{"id":"426932","messageId":"60c124a82436_aa2ee208d9@natae.notmuch","threadId":"55890","inReplyTo":"CAPig+cSK1YkFwQASuV1n4fr5Ko8tH8QGxvVgQ=Vd0MeY9BSrzw@mail.gmail.com","subject":"Re: [PATCH 1/7] test: add merge style config test","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-09T20:29:28Z","receivedAt":"2021-06-09T20:29:38Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Eric Sunshine wrote:\n> On Wed, Jun 9, 2021 at 3:29 PM Felipe Contreras\n> wrote:\n> > We want to test different combinations of merge.conflictstyle, and a new\n> > file is the best place to do that.\n> >\n> > Signed-off-by: Felipe Contreras \n> > ---\n> > diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\n> > @@ -0,0 +1,44 @@\n> > +fill () {\n> > + for i\n> > + do\n> > + echo \"$i\"\n> > + done\n> > +}\n> \n> This seems to duplicate the behavior of test_write_lines()...\n\nRight, I'll update the patch.\n\nThe above function is used in:\n\n t6440-config-conflict-markers.sh\n t7201-co.sh\n\nSo those two probably should be updated.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"426948","messageId":"60883e1b-787f-5ec2-a9af-f2f6757d3c43@kdbg.org","threadId":"55890","inReplyTo":"20210609192842.696646-8-felipe.contreras@gmail.com","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-06-10T06:41:21Z","receivedAt":"2021-06-10T06:41:26Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 09.06.21 um 21:28 schrieb Felipe Contreras:\n> Virtually everyone is using it, and it's one of the first things we\n> teach newcomers in order to resolve conflicts efficiently.\n> \n> Let's make it the default.\n\nI tested diff3 style the VERY FIRST TIME the other day and was greated\nwith the below. Needless to say that this change is a no-go from my POV.\n\nWithout diff3:\n\n<<<<<<< HEAD\n CClustering ComputeSSLClusters(double threshPercent, const CDataInfo* scale) const override;\n void ComputeDist(DistFunc distFunc, CDoubleArray& dist,\n double& minDist, double& maxDist) const;\n double EstimateNodeDist2() const override;\n std::vector EstimateNeighborMinDist() const override;\n=======\n CClustering ComputeSSLClusters(double threshPercent,\n const CDoubleArray& compWeights, const CDataInfo* scale) const override;\n static void ComputeDist(const CNetNodeHolder& vecs, CDoubleArray& dist,\n double& minDist, double& maxDist);\n>>>>>>> no-compweights-in-cnet \n\nWith diff3:\n\n<<<<<<< HEAD\n CClustering ComputeSSLClusters(double threshPercent, const CDataInfo* scale) const override;\n void ComputeDist(DistFunc distFunc, CDoubleArray& dist,\n double& minDist, double& maxDist) const;\n double EstimateNodeDist2() const override;\n std::vector EstimateNeighborMinDist() const override;\n||||||| merged common ancestors\n<<<<<<<<< Temporary merge branch 1\n CClustering ComputeSSLClusters(double threshPercent, const CDataInfo* scale) const override;\n void ComputeDist(DistFunc distFunc, CDoubleArray& dist,\n double& minDist, double& maxDist) const;\n virtual void ComputeKNearest(int K, const double*,\n Neighborhood& result) const;\n||||||||| d261d9944\n CClustering ComputeClusters(const double* dist, double threshold,\n const CDataInfo* scale) const override;\n virtual void ComputeDist(DistFunc distFunc, CDoubleArray& dist,\n double& minDist, double& maxDist);\n virtual void ComputeUMatrix();\n virtual void ComputeKNearest(int K, const double*,\n Neighborhood& result) const;\n=========\n CClustering ComputeClusters(const double* dist, double threshold,\n const CDataInfo* scale) const override;\n virtual void ComputeDist(DistFunc distFunc, CDoubleArray& dist,\n double& minDist, double& maxDist);\n virtual void ComputeUMatrix();\n=======\n CClustering ComputeSSLClusters(double threshPercent,\n const CDoubleArray& compWeights, const CDataInfo* scale) const override;\n static void ComputeDist(const CNetNodeHolder& vecs, CDoubleArray& dist,\n double& minDist, double& maxDist);\n>>>>>>> no-compweights-in-cnet \n\n-- Hannes\n"},{"id":"426951","messageId":"YMHE5xuxN90HJepT@danh.dev","threadId":"55890","inReplyTo":"60883e1b-787f-5ec2-a9af-f2f6757d3c43@kdbg.org","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2021-06-10T07:53:29Z","receivedAt":"2021-06-10T07:54:37Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2021-06-10 08:41:21+0200, Johannes Sixt wrote:\n> Am 09.06.21 um 21:28 schrieb Felipe Contreras:\n> > Virtually everyone is using it, and it's one of the first things we\n> > teach newcomers in order to resolve conflicts efficiently.\n> > \n> > Let's make it the default.\n> \n> I tested diff3 style the VERY FIRST TIME the other day and was greated\n> with the below. Needless to say that this change is a no-go from my POV.\n\nI agree, despite using 3-way merging (with external tools) to resolve conflicts.\n\nI prefer the current conflict style, aka no-diff3 conflict style.\n\n-- Danh\n> \n> Without diff3:\n> \n> <<<<<<< HEAD\n> CClustering ComputeSSLClusters(double threshPercent, const CDataInfo* scale) const override;\n> void ComputeDist(DistFunc distFunc, CDoubleArray& dist,\n> double& minDist, double& maxDist) const;\n> double EstimateNodeDist2() const override;\n> std::vector EstimateNeighborMinDist() const override;\n> =======\n> CClustering ComputeSSLClusters(double threshPercent,\n> const CDoubleArray& compWeights, const CDataInfo* scale) const override;\n> static void ComputeDist(const CNetNodeHolder& vecs, CDoubleArray& dist,\n> double& minDist, double& maxDist);\n> >>>>>>> no-compweights-in-cnet \n> \n> With diff3:\n> \n> <<<<<<< HEAD\n> CClustering ComputeSSLClusters(double threshPercent, const CDataInfo* scale) const override;\n> void ComputeDist(DistFunc distFunc, CDoubleArray& dist,\n> double& minDist, double& maxDist) const;\n> double EstimateNodeDist2() const override;\n> std::vector EstimateNeighborMinDist() const override;\n> ||||||| merged common ancestors\n> <<<<<<<<< Temporary merge branch 1\n> CClustering ComputeSSLClusters(double threshPercent, const CDataInfo* scale) const override;\n> void ComputeDist(DistFunc distFunc, CDoubleArray& dist,\n> double& minDist, double& maxDist) const;\n> virtual void ComputeKNearest(int K, const double*,\n> Neighborhood& result) const;\n> ||||||||| d261d9944\n> CClustering ComputeClusters(const double* dist, double threshold,\n> const CDataInfo* scale) const override;\n> virtual void ComputeDist(DistFunc distFunc, CDoubleArray& dist,\n> double& minDist, double& maxDist);\n> virtual void ComputeUMatrix();\n> virtual void ComputeKNearest(int K, const double*,\n> Neighborhood& result) const;\n> =========\n> CClustering ComputeClusters(const double* dist, double threshold,\n> const CDataInfo* scale) const override;\n> virtual void ComputeDist(DistFunc distFunc, CDoubleArray& dist,\n> double& minDist, double& maxDist);\n> virtual void ComputeUMatrix();\n> =======\n> CClustering ComputeSSLClusters(double threshPercent,\n> const CDoubleArray& compWeights, const CDataInfo* scale) const override;\n> static void ComputeDist(const CNetNodeHolder& vecs, CDoubleArray& dist,\n> double& minDist, double& maxDist);\n> >>>>>>> no-compweights-in-cnet \n> \n> -- Hannes\n\n-- \nDanh\n"},{"id":"426963","messageId":"b2436790-bbf0-2623-5e29-a6f6b4959974@gmail.com","threadId":"55890","inReplyTo":"20210609192842.696646-2-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/7] test: add merge style config test","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-06-10T09:18:22Z","receivedAt":"2021-06-10T09:18:29Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 09/06/2021 20:28, Felipe Contreras wrote:\n> We want to test different combinations of merge.conflictstyle, and a new\n> file is the best place to do that.\n\nI'm not sure what this particular tests adds over the existing ones in \nt6427-diff3-conflict-markers.sh. The commit message does not explain why \na new file is better than adding this test to that file. There are \nalready diff3 tests for checkout so it ends up being confusing when a \ncheckout test gets added to this file rather than with those tests later \nin the series because there is no longer a single location for diff3 \ncheckout tests.\n\n> Signed-off-by: Felipe Contreras \n> ---\n> t/t6440-config-conflict-markers.sh | 44 ++++++++++++++++++++++++++++++\n> 1 file changed, 44 insertions(+)\n> create mode 100755 t/t6440-config-conflict-markers.sh\n> \n> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\n> new file mode 100755\n> index 0000000000..6952552c58\n> --- /dev/null\n> +++ b/t/t6440-config-conflict-markers.sh\n> @@ -0,0 +1,44 @@\n> +#!/bin/sh\n> +\n> +test_description='merge style conflict markers configurations'\n> +\n> +. ./test-lib.sh\n> +\n> +fill () {\n> +\tfor i\n> +\tdo\n> +\t\techo \"$i\"\n> +\tdone\n> +}\n> +\n> +test_expect_success 'merge' '\n> +\ttest_create_repo merge &&\n> +\t(\n> +\t\tcd merge &&\n> +\n> +\t\tfill 1 2 3 >content &&\n> +\t\tgit add content &&\n> +\t\tgit commit -m base &&\n> +\n> +\t\tgit checkout -b r &&\n> +\t\techo six >>content &&\n> +\t\tgit commit -a -m right &&\n> +\n> +\t\tgit checkout master &&\n> +\t\techo 7 >>content &&\n> +\t\tgit commit -a -m left &&\n> +\n> +\t\ttest_must_fail git merge r &&\n> +\t\t! grep -E \"\\|+\" content &&\n\n! grep \"|\" would be simpler and just as effective. This is quite a weak \ntest, something like \"^|||||| \" would be a stronger test for conflict \nmarkers\n\nBest Wishes\n\nPhillip\n\n> +\t\tgit reset --hard &&\n> +\t\ttest_must_fail git -c merge.conflictstyle=diff3 merge r &&\n> +\t\tgrep -E \"\\|+\" content &&\n> +\n> +\t\tgit reset --hard &&\n> +\t\ttest_must_fail git -c merge.conflictstyle=merge merge r &&\n> +\t\t! grep -E \"\\|+\" content\n> +\t)\n> +'\n> +\n> +test_done\n> \n"},{"id":"426964","messageId":"3168eb15-25a6-a3eb-a498-8effa0c79855@gmail.com","threadId":"55890","inReplyTo":"20210609192842.696646-6-felipe.contreras@gmail.com","subject":"Re: [PATCH 5/7] xdiff: rename XDL_MERGE_STYLE_DIFF3","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-06-10T09:21:42Z","receivedAt":"2021-06-10T09:22:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 09/06/2021 20:28, Felipe Contreras wrote:\n\nThe subject would make more sense as 'xdiff: rename XDL_MERGE_DIFF3 to \nXDL_MERGE_STYLE_DIFF3' rather than using the new name of the constant alone.\n\n> If we don't specify we are talking about a style, XDL_MERGE_MINIMAL\n> could be confused with a valid value instead of XDL_MERGE_DIFF3, which\n> it isn't.\n\nI don't object to the rename but what is the source of the confusion \nwith XDL_MERGE_MINIMAL?\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Felipe Contreras \n> ---\n> builtin/merge-file.c | 2 +-\n> xdiff-interface.c | 2 +-\n> xdiff/xdiff.h | 2 +-\n> xdiff/xmerge.c | 4 ++--\n> 4 files changed, 5 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin/merge-file.c b/builtin/merge-file.c\n> index 06a2f90c48..a4097a596f 100644\n> --- a/builtin/merge-file.c\n> +++ b/builtin/merge-file.c\n> @@ -33,7 +33,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n> \tint quiet = 0;\n> \tstruct option options[] = {\n> \t\tOPT_BOOL('p', \"stdout\", &to_stdout, N_(\"send results to standard output\")),\n> -\t\tOPT_SET_INT(0, \"diff3\", &xmp.style, N_(\"use a diff3 based merge\"), XDL_MERGE_DIFF3),\n> +\t\tOPT_SET_INT(0, \"diff3\", &xmp.style, N_(\"use a diff3 based merge\"), XDL_MERGE_STYLE_DIFF3),\n> \t\tOPT_SET_INT(0, \"ours\", &xmp.favor, N_(\"for conflicts, use our version\"),\n> \t\t\t XDL_MERGE_FAVOR_OURS),\n> \t\tOPT_SET_INT(0, \"theirs\", &xmp.favor, N_(\"for conflicts, use their version\"),\n> diff --git a/xdiff-interface.c b/xdiff-interface.c\n> index 609615db2c..64e2c4e301 100644\n> --- a/xdiff-interface.c\n> +++ b/xdiff-interface.c\n> @@ -307,7 +307,7 @@ int git_xmerge_config(const char *var, const char *value, void *cb)\n> \t\tif (!value)\n> \t\t\tdie(\"'%s' is not a boolean\", var);\n> \t\tif (!strcmp(value, \"diff3\"))\n> -\t\t\tgit_xmerge_style = XDL_MERGE_DIFF3;\n> +\t\t\tgit_xmerge_style = XDL_MERGE_STYLE_DIFF3;\n> \t\telse if (!strcmp(value, \"merge\"))\n> \t\t\tgit_xmerge_style = 0;\n> \t\t/*\n> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\n> index 7a04605146..45883f5eb3 100644\n> --- a/xdiff/xdiff.h\n> +++ b/xdiff/xdiff.h\n> @@ -64,7 +64,7 @@ extern \"C\" {\n> #define XDL_MERGE_FAVOR_UNION 3\n> \n> /* merge output styles */\n> -#define XDL_MERGE_DIFF3 1\n> +#define XDL_MERGE_STYLE_DIFF3 1\n> \n> typedef struct s_mmfile {\n> \tchar *ptr;\n> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\n> index 1659edb453..f6916a4ba4 100644\n> --- a/xdiff/xmerge.c\n> +++ b/xdiff/xmerge.c\n> @@ -230,7 +230,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,\n> \tsize += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, 1,\n> \t\t\t dest ? dest + size : NULL);\n> \n> -\tif (style == XDL_MERGE_DIFF3) {\n> +\tif (style == XDL_MERGE_STYLE_DIFF3) {\n> \t\t/* Shared preimage */\n> \t\tif (!dest) {\n> \t\t\tsize += marker_size + 1 + needs_cr + marker3_size;\n> @@ -482,7 +482,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,\n> \tint style = xmp->style;\n> \tint favor = xmp->favor;\n> \n> -\tif (style == XDL_MERGE_DIFF3) {\n> +\tif (style == XDL_MERGE_STYLE_DIFF3) {\n> \t\t/*\n> \t\t * \"diff3 -m\" output does not make sense for anything\n> \t\t * more aggressive than XDL_MERGE_EAGER.\n> \n"},{"id":"426966","messageId":"cc8bbc34-1048-0e7b-c783-33f556843a3f@gmail.com","threadId":"55890","inReplyTo":"20210609192842.696646-7-felipe.contreras@gmail.com","subject":"Re: [PATCH 6/7] xdiff: simplify style assignments","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-06-10T09:26:28Z","receivedAt":"2021-06-10T09:26:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 09/06/2021 20:28, Felipe Contreras wrote:\n\nI don't find the commit message explains this change very well\n\n> There is little value in checking that git_xmerge_style isn't 0 before\n> changing it's default value.\n\nI think the check is actually that git_xmerge_style isn't -1. Why is \nthere little value in the check?\n\n> Most of the time it isn't 0 anyway, so just assign the value directly.\n\nWhy to the times when it is zero (or -1) not matter?\n\n> Also, add the missing constant for the default value: XDL_MERGE_STYLE_MERGE.\n> \n> Additionally this change has the benefit that it gets rid of a Yoda\n> condition.\n> \n> No functional changes.\n\nI think that is probably correct but it would be helpful if the commit \nmessage offered a bit more explanation.\n\nBest Wishes\n\nPhillip\n\n> \n> Signed-off-by: Felipe Contreras \n> ---\n> builtin/merge-file.c | 3 +--\n> ll-merge.c | 3 +--\n> xdiff-interface.c | 4 ++--\n> xdiff/xdiff.h | 1 +\n> 4 files changed, 5 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/merge-file.c b/builtin/merge-file.c\n> index a4097a596f..01951762ec 100644\n> --- a/builtin/merge-file.c\n> +++ b/builtin/merge-file.c\n> @@ -55,8 +55,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n> \tif (startup_info->have_repository) {\n> \t\t/* Read the configuration file */\n> \t\tgit_config(git_xmerge_config, NULL);\n> -\t\tif (0 <= git_xmerge_style)\n> -\t\t\txmp.style = git_xmerge_style;\n> +\t\txmp.style = git_xmerge_style;\n> \t}\n> \n> \targc = parse_options(argc, argv, prefix, options, merge_file_usage, 0);\n> diff --git a/ll-merge.c b/ll-merge.c\n> index 9a8a2c365c..4ce8d3f9cc 100644\n> --- a/ll-merge.c\n> +++ b/ll-merge.c\n> @@ -124,8 +124,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n> \txmp.level = XDL_MERGE_ZEALOUS;\n> \txmp.favor = opts->variant;\n> \txmp.xpp.flags = opts->xdl_opts;\n> -\tif (git_xmerge_style >= 0)\n> -\t\txmp.style = git_xmerge_style;\n> +\txmp.style = git_xmerge_style;\n> \tif (marker_size > 0)\n> \t\txmp.marker_size = marker_size;\n> \txmp.ancestor = orig_name;\n> diff --git a/xdiff-interface.c b/xdiff-interface.c\n> index 64e2c4e301..19a030fbe2 100644\n> --- a/xdiff-interface.c\n> +++ b/xdiff-interface.c\n> @@ -299,7 +299,7 @@ int xdiff_compare_lines(const char *l1, long s1,\n> \treturn xdl_recmatch(l1, s1, l2, s2, flags);\n> }\n> \n> -int git_xmerge_style = -1;\n> +int git_xmerge_style = XDL_MERGE_STYLE_MERGE;\n> \n> int git_xmerge_config(const char *var, const char *value, void *cb)\n> {\n> @@ -309,7 +309,7 @@ int git_xmerge_config(const char *var, const char *value, void *cb)\n> \t\tif (!strcmp(value, \"diff3\"))\n> \t\t\tgit_xmerge_style = XDL_MERGE_STYLE_DIFF3;\n> \t\telse if (!strcmp(value, \"merge\"))\n> -\t\t\tgit_xmerge_style = 0;\n> +\t\t\tgit_xmerge_style = XDL_MERGE_STYLE_MERGE;\n> \t\t/*\n> \t\t * Please update _git_checkout() in\n> \t\t * git-completion.bash when you add new merge config\n> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\n> index 45883f5eb3..d24cd9f6ae 100644\n> --- a/xdiff/xdiff.h\n> +++ b/xdiff/xdiff.h\n> @@ -64,6 +64,7 @@ extern \"C\" {\n> #define XDL_MERGE_FAVOR_UNION 3\n> \n> /* merge output styles */\n> +#define XDL_MERGE_STYLE_MERGE 0\n> #define XDL_MERGE_STYLE_DIFF3 1\n> \n> typedef struct s_mmfile {\n> \n"},{"id":"426967","messageId":"b4b9264d-1c17-1a62-f0ec-2791ab20adeb@gmail.com","threadId":"55890","inReplyTo":"20210609192842.696646-5-felipe.contreras@gmail.com","subject":"Re: [PATCH 4/7] checkout: fix merge.conflictstyle handling","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-06-10T09:32:24Z","receivedAt":"2021-06-10T09:32:50Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 09/06/2021 20:28, Felipe Contreras wrote:\n> Currently both merge.conflictStyle and `git commit --merge\n> --conflict=diff3` don't work together, since the former wrongly\n> overrides the later.\n> \n> The way merge configurations are handled is not correct.\n> It should be possible to do git_config(merge_recursive_config, ...) just\n> like we can with git_diff_basic_config and others.\n\nIt would be helpful to explain what the problem with \nmerge_recursive_config() actually is rather than just saying \"it should \nbe possible ...\"\n\n> Therefore builtins like `git merge` can't call this function at the\n> right time.\n >\n> We shuffle the functions a little bit so at least merge_recursive_config\n> doesn't call git_xmerge_config directly and thus override previous\n> configurations.\n\nRather than papering of the problem, how difficult would it be to add a \nfield to ll_merge_options and pass the conflict style with that rather \nthan fiddling with the order that we set a global variable.\n\nDoes this change affect 'am/apply -3'? - Do they still read the config \nsetting properly?\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Felipe Contreras \n> ---\n> builtin/merge-recursive.c | 3 +++\n> builtin/merge.c | 4 ++++\n> merge-recursive.c | 2 +-\n> sequencer.c | 5 +++++\n> t/t6440-config-conflict-markers.sh | 31 ++++++++++++++++++++++++++++++\n> 5 files changed, 44 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\n> index a4bfd8fc51..80f9279b4c 100644\n> --- a/builtin/merge-recursive.c\n> +++ b/builtin/merge-recursive.c\n> @@ -4,6 +4,7 @@\n> #include \"tag.h\"\n> #include \"merge-recursive.h\"\n> #include \"xdiff-interface.h\"\n> +#include \"config.h\"\n> \n> static const char builtin_merge_recursive_usage[] =\n> \t\"git %s ... -- ...\";\n> @@ -30,6 +31,8 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)\n> \tchar *better1, *better2;\n> \tstruct commit *result;\n> \n> +\tgit_config(git_xmerge_config, NULL);\n> +\n> \tinit_merge_options(&o, the_repository);\n> \tif (argv[0] && ends_with(argv[0], \"-subtree\"))\n> \t\to.subtree_shift = \"\";\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index eddb8ae70d..7aa3dbb111 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -43,6 +43,7 @@\n> #include \"commit-reach.h\"\n> #include \"wt-status.h\"\n> #include \"commit-graph.h\"\n> +#include \"xdiff-interface.h\"\n> \n> #define DEFAULT_TWOHEAD (1<<0)\n> #define DEFAULT_OCTOPUS (1<<1)\n> @@ -659,6 +660,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)\n> \tif (status)\n> \t\treturn status;\n> \tstatus = git_gpg_config(k, v, NULL);\n> +\tif (status)\n> +\t\treturn status;\n> +\tstatus = git_xmerge_config(k, v, NULL);\n> \tif (status)\n> \t\treturn status;\n> \treturn git_diff_ui_config(k, v, cb);\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index d146bb116f..10e6e1e4d1 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -3845,7 +3845,7 @@ static void merge_recursive_config(struct merge_options *opt)\n> \t\t} /* avoid erroring on values from future versions of git */\n> \t\tfree(value);\n> \t}\n> -\tgit_config(git_xmerge_config, NULL);\n> +\tgit_config(git_default_config, NULL);\n> }\n> \n> void init_merge_options(struct merge_options *opt,\n> diff --git a/sequencer.c b/sequencer.c\n> index 0bec01cf38..9e2bdca0f6 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -34,6 +34,7 @@\n> #include \"commit-reach.h\"\n> #include \"rebase-interactive.h\"\n> #include \"reset.h\"\n> +#include \"xdiff-interface.h\"\n> \n> #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n> \n> @@ -224,6 +225,10 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n> \tif (status)\n> \t\treturn status;\n> \n> +\tstatus = git_xmerge_config(k, v, NULL);\n> +\tif (status)\n> +\t\treturn status;\n> +\n> \treturn git_diff_basic_config(k, v, NULL);\n> }\n> \n> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\n> index 44f79ac91b..485ad0eee0 100755\n> --- a/t/t6440-config-conflict-markers.sh\n> +++ b/t/t6440-config-conflict-markers.sh\n> @@ -89,4 +89,35 @@ test_expect_success 'notes' '\n> \t)\n> '\n> \n> +test_expect_success 'checkout' '\n> +\ttest_create_repo checkout &&\n> +\t(\n> +\t\ttest_commit checkout &&\n> +\n> +\t\tfill a b c d e >content &&\n> +\t\tgit add content &&\n> +\t\tgit commit -m initial &&\n> +\n> +\t\tgit checkout -b simple master &&\n> +\t\tfill a c e >content &&\n> +\t\tgit commit -a -m simple &&\n> +\n> +\t\tfill b d >content &&\n> +\t\tgit checkout --merge master &&\n> +\t\t! grep -E \"\\|+\" content &&\n> +\n> +\t\tgit config merge.conflictstyle merge &&\n> +\n> +\t\tgit checkout -f simple &&\n> +\t\tfill b d >content &&\n> +\t\tgit checkout --merge --conflict=diff3 master &&\n> +\t\tgrep -E \"\\|+\" content &&\n> +\n> +\t\tgit checkout -f simple &&\n> +\t\tfill b d >content &&\n> +\t\tgit checkout --merge --conflict=merge master &&\n> +\t\t! grep -E \"\\|+\" content\n> +\t)\n> +'\n> +\n> test_done\n> \n"},{"id":"426968","messageId":"bddfe587-704c-a7b2-1182-246226aa09ba@gmail.com","threadId":"55890","inReplyTo":"20210609192842.696646-8-felipe.contreras@gmail.com","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-06-10T09:40:51Z","receivedAt":"2021-06-10T09:40:57Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 09/06/2021 20:28, Felipe Contreras wrote:\n> Virtually everyone is using it, and it's one of the first things we\n> teach newcomers in order to resolve conflicts efficiently.\n\nGiven there are millions of users I'm not sure how you established that \nvirtually everyone is using it. I think that while this change would be \nuseful to some users (though not many if virtually everyone already has \nit set) it has the potential to annoy a lot of users who are happy with \nthe existing default. I do not think that it is a positive change over all.\n\nHad the default been diff3 from early on in git's history then I would \nnot advocate changing it to the current default but I think the time has \npassed when it can be changed without inconveniencing existing users.\n\nThe patches up to this point have useful fixes in them which would \nimprove git, thanks for working on them.\n\nBest Wishes\n\nPhillip\n\n> Let's make it the default.\n> \n> This generates a ton of changes in the tests. Although we probably will\n> want to update them to use th new default, override the configuration so\n> we use the old one for now.\n> \n> Signed-off-by: Felipe Contreras \n> ---\n> Documentation/config/merge.txt | 12 +++++-----\n> Documentation/git-merge-file.txt | 2 ++\n> Documentation/git-merge.txt | 28 +++++++-----------------\n> Documentation/git-rerere.txt | 2 +-\n> Documentation/gitattributes.txt | 6 ++---\n> Documentation/technical/rerere.txt | 3 +--\n> Documentation/user-manual.txt | 6 ++++-\n> t/t2023-checkout-m.sh | 2 ++\n> t/t3310-notes-merge-manual-resolve.sh | 2 ++\n> t/t3311-notes-merge-fanout.sh | 2 ++\n> t/t3404-rebase-interactive.sh | 2 ++\n> t/t3507-cherry-pick-conflict.sh | 2 ++\n> t/t4017-diff-retval.sh | 2 ++\n> t/t4048-diff-combined-binary.sh | 2 ++\n> t/t4200-rerere.sh | 2 ++\n> t/t4300-merge-tree.sh | 2 ++\n> t/t6402-merge-rename.sh | 2 ++\n> t/t6403-merge-file.sh | 2 ++\n> t/t6404-recursive-merge.sh | 2 ++\n> t/t6416-recursive-corner-cases.sh | 2 ++\n> t/t6417-merge-ours-theirs.sh | 2 ++\n> t/t6418-merge-text-auto.sh | 2 ++\n> t/t6422-merge-rename-corner-cases.sh | 2 ++\n> t/t6423-merge-rename-directories.sh | 1 +\n> t/t6428-merge-conflicts-sparse.sh | 1 +\n> t/t6432-merge-recursive-space-options.sh | 2 ++\n> t/t6440-config-conflict-markers.sh | 8 +++----\n> t/t7201-co.sh | 2 ++\n> t/t7506-status-submodule.sh | 1 +\n> xdiff-interface.c | 2 +-\n> 30 files changed, 70 insertions(+), 38 deletions(-)\n> \n> diff --git a/Documentation/config/merge.txt b/Documentation/config/merge.txt\n> index cb2ed58907..2dba937dd0 100644\n> --- a/Documentation/config/merge.txt\n> +++ b/Documentation/config/merge.txt\n> @@ -1,10 +1,10 @@\n> merge.conflictStyle::\n> -\tSpecify the style in which conflicted hunks are written out to\n> -\tworking tree files upon merge. The default is \"merge\", which\n> -\tshows a `<<<<<<<` conflict marker, changes made by one side,\n> -\ta `=======` marker, changes made by the other side, and then\n> -\ta `>>>>>>>` marker. An alternate style, \"diff3\", adds a `|||||||`\n> -\tmarker and the original text before the `=======` marker.\n> +\tSpecify the style in which conflicted hunks are written out to working\n> +\ttree files upon merge. The default is \"diff3\", which shows a `<<<<<<<`\n> +\tconflict marker, changes made by one side, a `|||||||` marker, the\n> +\toriginal text, a `=======` marker, changes made by the other side, and\n> +\tthen a `>>>>>>>` marker. A simpler mode \"merge\" omits the `|||||||`\n> +\tmarker and the original text.\n> \n> merge.defaultToUpstream::\n> \tIf merge is called without any commit argument, merge the upstream\n> diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt\n> index f856032613..7d8e74c872 100644\n> --- a/Documentation/git-merge-file.txt\n> +++ b/Documentation/git-merge-file.txt\n> @@ -30,6 +30,8 @@ normally outputs a warning and brackets the conflict with lines containing\n> \n> \t<<<<<<< A\n> \tlines in file A\n> +\t|||||||\n> +\tlines in merge base\n> \t=======\n> \tlines in file B\n> \t>>>>>>> B\n> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt\n> index 3819fadac1..14dadf2e16 100644\n> --- a/Documentation/git-merge.txt\n> +++ b/Documentation/git-merge.txt\n> @@ -233,7 +233,7 @@ final result verbatim. When both sides made changes to the same area,\n> however, Git cannot randomly pick one side over the other, and asks you to\n> resolve it by leaving what both sides did to that area.\n> \n> -By default, Git uses the same style as the one used by the \"merge\" program\n> +By default, Git uses a similar style to the one used by the \"merge\" program\n> from the RCS suite to present such a conflicted hunk, like this:\n> \n> ------------\n> @@ -242,6 +242,8 @@ ancestor, or cleanly resolved because only one side changed.\n> <<<<<<< yours:sample.txt\n> Conflict resolution is hard;\n> let's go shopping.\n> +|||||||\n> +Originally there's no conflict.\n> =======\n> Git makes conflict resolution easy.\n> >>>>>>> theirs:sample.txt\n> @@ -249,17 +251,12 @@ And here is another line that is cleanly resolved or unmodified.\n> ------------\n> \n> The area where a pair of conflicting changes happened is marked with markers\n> -`<<<<<<<`, `=======`, and `>>>>>>>`. The part before the `=======`\n> -is typically your side, and the part afterwards is typically their side.\n> -\n> -The default format does not show what the original said in the conflicting\n> -area. You cannot tell how many lines are deleted and replaced with\n> -Barbie's remark on your side. The only thing you can tell is that your\n> -side wants to say it is hard and you'd prefer to go shopping, while the\n> -other side wants to claim it is easy.\n> +`<<<<<<<`, `=======`, and `>>>>>>>`. The part before the `|||||||`\n> +is typically your side, and the part after `=======` is typically their side.\n> +In-between is the original code.\n> \n> -An alternative style can be used by setting the \"merge.conflictStyle\"\n> -configuration variable to \"diff3\". In \"diff3\" style, the above conflict\n> +An more basic style can be used by setting the \"merge.conflictStyle\"\n> +configuration variable to \"merge\". In \"merge\" style, the above conflict\n> may look like this:\n> \n> ------------\n> @@ -268,21 +265,12 @@ ancestor, or cleanly resolved because only one side changed.\n> <<<<<<< yours:sample.txt\n> Conflict resolution is hard;\n> let's go shopping.\n> -|||||||\n> -Conflict resolution is hard.\n> =======\n> Git makes conflict resolution easy.\n> >>>>>>> theirs:sample.txt\n> And here is another line that is cleanly resolved or unmodified.\n> ------------\n> \n> -In addition to the `<<<<<<<`, `=======`, and `>>>>>>>` markers, it uses\n> -another `|||||||` marker that is followed by the original text. You can\n> -tell that the original just stated a fact, and your side simply gave in to\n> -that statement and gave up, while the other side tried to have a more\n> -positive attitude. You can sometimes come up with a better resolution by\n> -viewing the original.\n> -\n> \n> HOW TO RESOLVE CONFLICTS\n> ------------------------\n> diff --git a/Documentation/git-rerere.txt b/Documentation/git-rerere.txt\n> index 4cfc883378..89b0820995 100644\n> --- a/Documentation/git-rerere.txt\n> +++ b/Documentation/git-rerere.txt\n> @@ -159,7 +159,7 @@ resolve.\n> \n> Running the 'git rerere' command immediately after a conflicted\n> automerge records the conflicted working tree files, with the\n> -usual conflict markers `<<<<<<<`, `=======`, and `>>>>>>>` in\n> +usual conflict markers `<<<<<<<`, `|||||||`, `=======`, and `>>>>>>>` in\n> them. Later, after you are done resolving the conflicts,\n> running 'git rerere' again will record the resolved state of these\n> files. Suppose you did this when you created the test merge of\n> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\n> index 83fd4e19a4..b767215ac2 100644\n> --- a/Documentation/gitattributes.txt\n> +++ b/Documentation/gitattributes.txt\n> @@ -1042,10 +1042,10 @@ text::\n> \n> \tUsual 3-way file level merge for text files. Conflicted\n> \tregions are marked with conflict markers `<<<<<<<`,\n> -\t`=======` and `>>>>>>>`. The version from your branch\n> -\tappears before the `=======` marker, and the version\n> +\t`|||||||`, `=======` and `>>>>>>>`. The version from your branch\n> +\tappears before the `|||||||` marker, and the version\n> \tfrom the merged branch appears after the `=======`\n> -\tmarker.\n> +\tmarker. In-between is the original.\n> \n> binary::\n> \n> diff --git a/Documentation/technical/rerere.txt b/Documentation/technical/rerere.txt\n> index af5f9fc24f..38b44f4430 100644\n> --- a/Documentation/technical/rerere.txt\n> +++ b/Documentation/technical/rerere.txt\n> @@ -42,8 +42,7 @@ get a conflict like the following:\n> >>>>>>> AC\n> \n> Doing the analogous with AC2 (forking a branch ABAC2 off of branch AB\n> -and then merging branch AC2 into it), using the diff3 conflict style,\n> -we get a conflict like the following:\n> +and then merging branch AC2 into it), we get a conflict like the following:\n> \n> <<<<<<< HEAD\n> B\n> diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt\n> index f9e54b8674..3ddde87482 100644\n> --- a/Documentation/user-manual.txt\n> +++ b/Documentation/user-manual.txt\n> @@ -1243,6 +1243,8 @@ files with conflicts will have conflict markers added, like this:\n> -------------------------------------------------\n> <<<<<<< HEAD:file.txt\n> Hello world\n> +|||||||\n> +Original\n> =======\n> Goodbye\n> >>>>>>> 77976da35a11db4580b80ae27e8d65caf5208086:file.txt\n> @@ -1276,9 +1278,11 @@ diff --cc file.txt\n> index 802992c,2b60207..0000000\n> --- a/file.txt\n> +++ b/file.txt\n> -@@@ -1,1 -1,1 +1,5 @@@\n> +@@@ -1,1 -1,1 +1,7 @@@\n> ++<<<<<<< HEAD:file.txt\n> +Hello world\n> +++|||||||\n> +++Original\n> ++=======\n> + Goodbye\n> ++>>>>>>> 77976da35a11db4580b80ae27e8d65caf5208086:file.txt\n> diff --git a/t/t2023-checkout-m.sh b/t/t2023-checkout-m.sh\n> index 7b327b7544..219c82532a 100755\n> --- a/t/t2023-checkout-m.sh\n> +++ b/t/t2023-checkout-m.sh\n> @@ -9,6 +9,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> \n> . ./test-lib.sh\n> \n> +git config merge.conflictstyle merge # TODO: use the default\n> +\n> test_expect_success setup '\n> \ttest_tick &&\n> \ttest_commit both.txt both.txt initial &&\n> diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\n> index d3d72e25fe..cbd5d8302e 100755\n> --- a/t/t3310-notes-merge-manual-resolve.sh\n> +++ b/t/t3310-notes-merge-manual-resolve.sh\n> @@ -7,6 +7,8 @@ test_description='Test notes merging with manual conflict resolution'\n> \n> . ./test-lib.sh\n> \n> +git config --global merge.conflictstyle merge # TODO: use the default\n> +\n> # Set up a notes merge scenario with different kinds of conflicts\n> test_expect_success 'setup commits' '\n> \ttest_commit 1st &&\n> diff --git a/t/t3311-notes-merge-fanout.sh b/t/t3311-notes-merge-fanout.sh\n> index 5b675417e9..4aeaa05c15 100755\n> --- a/t/t3311-notes-merge-fanout.sh\n> +++ b/t/t3311-notes-merge-fanout.sh\n> @@ -7,6 +7,8 @@ test_description='Test notes merging at various fanout levels'\n> \n> . ./test-lib.sh\n> \n> +git config merge.conflictstyle merge # TODO: use the default\n> +\n> verify_notes () {\n> \tnotes_ref=\"$1\"\n> \tcommit=\"$2\"\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 66bcbbf952..769079a71c 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -32,6 +32,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> \n> . \"$TEST_DIRECTORY\"/lib-rebase.sh\n> \n> +git config merge.conflictstyle merge # TODO: use the default\n> +\n> test_expect_success 'setup' '\n> \tgit switch -C primary &&\n> \ttest_commit A file1 &&\n> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> index 014001b8f3..647a40f314 100755\n> --- a/t/t3507-cherry-pick-conflict.sh\n> +++ b/t/t3507-cherry-pick-conflict.sh\n> @@ -281,6 +281,7 @@ test_expect_success \\\n> \n> test_expect_success 'failed cherry-pick describes conflict in work tree' '\n> \tpristine_detach initial &&\n> +\tgit config merge.conflictstyle merge && # TODO: use the default\n> \tcat <<-EOF >expected &&\n> \t<<<<<<< HEAD\n> \ta\n> @@ -316,6 +317,7 @@ test_expect_success 'diff3 -m style' '\n> \n> test_expect_success 'revert also handles conflicts sanely' '\n> \tgit config --unset merge.conflictstyle &&\n> +\tgit config merge.conflictstyle merge && # TODO: use the default\n> \tpristine_detach initial &&\n> \tcat <<-EOF >expected &&\n> \t<<<<<<< HEAD\n> diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh\n> index ed461f481e..04b77af2a4 100755\n> --- a/t/t4017-diff-retval.sh\n> +++ b/t/t4017-diff-retval.sh\n> @@ -7,6 +7,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> \n> . ./test-lib.sh\n> \n> +git config merge.conflictstyle merge # TODO: use the default\n> +\n> test_expect_success 'setup' '\n> \techo \"1 \" >a &&\n> \tgit add . &&\n> diff --git a/t/t4048-diff-combined-binary.sh b/t/t4048-diff-combined-binary.sh\n> index 0260cf64f5..49a56731dd 100755\n> --- a/t/t4048-diff-combined-binary.sh\n> +++ b/t/t4048-diff-combined-binary.sh\n> @@ -6,6 +6,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> \n> . ./test-lib.sh\n> \n> +git config merge.conflictstyle merge # TODO: use the default\n> +\n> test_expect_success 'setup binary merge conflict' '\n> \techo oneQ1 | q_to_nul >binary &&\n> \tgit add binary &&\n> diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh\n> index 9f8c76dffb..e9ae3d6fde 100755\n> --- a/t/t4200-rerere.sh\n> +++ b/t/t4200-rerere.sh\n> @@ -27,6 +27,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> \n> . ./test-lib.sh\n> \n> +git config merge.conflictstyle merge # TODO: use the default\n> +\n> test_expect_success 'setup' '\n> \tcat >a1 <<-\\EOF &&\n> \tSome title\n> diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh\n> index e59601e5fe..f21ccaf0a6 100755\n> --- a/t/t4300-merge-tree.sh\n> +++ b/t/t4300-merge-tree.sh\n> @@ -6,6 +6,8 @@\n> test_description='git merge-tree'\n> . ./test-lib.sh\n> \n> +git config merge.conflictstyle merge # TODO: use the default\n> +\n> test_expect_success setup '\n> \ttest_commit \"initial\" \"initial-file\" \"initial\"\n> '\n> diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh\n> index 425dad97d5..4f01bbc451 100755\n> --- a/t/t6402-merge-rename.sh\n> +++ b/t/t6402-merge-rename.sh\n> @@ -270,6 +270,7 @@ test_expect_success 'setup for rename + d/f conflicts' '\n> \tgit checkout --orphan dir-in-way &&\n> \tgit rm -rf . &&\n> \tgit clean -fdqx &&\n> +\tgit config merge.conflictstyle merge && # TODO: use the default\n> \n> \tmkdir sub &&\n> \tmkdir dir &&\n> @@ -871,6 +872,7 @@ test_expect_success 'setup for use of extended merge markers' '\n> \tgit clean -fdqx &&\n> \trm -rf .git &&\n> \tgit init &&\n> +\tgit config merge.conflictstyle merge && # TODO: use the default\n> \n> \tprintf \"1\\n2\\n3\\n4\\n5\\n6\\n7\\n8\\n\" >original_file &&\n> \tgit add original_file &&\n> diff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh\n> index 2f421d967a..1428dfb5c6 100755\n> --- a/t/t6403-merge-file.sh\n> +++ b/t/t6403-merge-file.sh\n> @@ -3,6 +3,8 @@\n> test_description='RCS merge replacement: merge-file'\n> . ./test-lib.sh\n> \n> +git config merge.conflictstyle merge # TODO: use the default\n> +\n> test_expect_success 'setup' '\n> \tcat >orig.txt <<-\\EOF &&\n> \tDominus regit me,\n> diff --git a/t/t6404-recursive-merge.sh b/t/t6404-recursive-merge.sh\n> index eaf48e941e..a3354b8f9a 100755\n> --- a/t/t6404-recursive-merge.sh\n> +++ b/t/t6404-recursive-merge.sh\n> @@ -6,6 +6,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> \n> . ./test-lib.sh\n> \n> +git config merge.conflictstyle merge # TODO: use the default\n> +\n> # This scenario is based on a real-world repository of Shawn Pearce.\n> \n> # 1 - A - D - F\n> diff --git a/t/t6416-recursive-corner-cases.sh b/t/t6416-recursive-corner-cases.sh\n> index 84f5082366..ac4e69a325 100755\n> --- a/t/t6416-recursive-corner-cases.sh\n> +++ b/t/t6416-recursive-corner-cases.sh\n> @@ -8,6 +8,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> . ./test-lib.sh\n> . \"$TEST_DIRECTORY\"/lib-merge.sh\n> \n> +git config --global merge.conflictstyle merge # TODO: use the default\n> +\n> #\n> # L1 L2\n> # o---o\n> diff --git a/t/t6417-merge-ours-theirs.sh b/t/t6417-merge-ours-theirs.sh\n> index ac9aee9a66..b8208a383b 100755\n> --- a/t/t6417-merge-ours-theirs.sh\n> +++ b/t/t6417-merge-ours-theirs.sh\n> @@ -6,6 +6,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> \n> . ./test-lib.sh\n> \n> +git config merge.conflictstyle merge # TODO: use the default\n> +\n> test_expect_success setup '\n> \tfor i in 1 2 3 4 5 6 7 8 9\n> \tdo\n> diff --git a/t/t6418-merge-text-auto.sh b/t/t6418-merge-text-auto.sh\n> index 1e0296dd17..e18f67776c 100755\n> --- a/t/t6418-merge-text-auto.sh\n> +++ b/t/t6418-merge-text-auto.sh\n> @@ -17,6 +17,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> \n> . ./test-lib.sh\n> \n> +git config merge.conflictstyle merge # TODO: use the default\n> +\n> test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b\n> \n> compare_files () {\n> diff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh\n> index bf4ce3c63d..6bb4b6d968 100755\n> --- a/t/t6422-merge-rename-corner-cases.sh\n> +++ b/t/t6422-merge-rename-corner-cases.sh\n> @@ -9,6 +9,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> . ./test-lib.sh\n> . \"$TEST_DIRECTORY\"/lib-merge.sh\n> \n> +git config --global merge.conflictstyle merge # TODO: use the default\n> +\n> test_setup_rename_delete_untracked () {\n> \ttest_create_repo rename-delete-untracked &&\n> \t(\n> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\n> index 7134769149..5f6cacd064 100755\n> --- a/t/t6423-merge-rename-directories.sh\n> +++ b/t/t6423-merge-rename-directories.sh\n> @@ -28,6 +28,7 @@ test_description=\"recursive merge with directory renames\"\n> . ./test-lib.sh\n> . \"$TEST_DIRECTORY\"/lib-merge.sh\n> \n> +git config --global merge.conflictstyle merge # TODO: use the default\n> \n> ###########################################################################\n> # SECTION 1: Basic cases we should be able to handle\n> diff --git a/t/t6428-merge-conflicts-sparse.sh b/t/t6428-merge-conflicts-sparse.sh\n> index 7e8bf497f8..18975801db 100755\n> --- a/t/t6428-merge-conflicts-sparse.sh\n> +++ b/t/t6428-merge-conflicts-sparse.sh\n> @@ -25,6 +25,7 @@ test_description=\"merge cases\"\n> . ./test-lib.sh\n> . \"$TEST_DIRECTORY\"/lib-merge.sh\n> \n> +git config --global merge.conflictstyle merge # TODO: use the default\n> \n> # Testcase basic, conflicting changes in 'numerals'\n> \n> diff --git a/t/t6432-merge-recursive-space-options.sh b/t/t6432-merge-recursive-space-options.sh\n> index db4b77e63d..5cfe8a4fbd 100755\n> --- a/t/t6432-merge-recursive-space-options.sh\n> +++ b/t/t6432-merge-recursive-space-options.sh\n> @@ -16,6 +16,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> \n> . ./test-lib.sh\n> \n> +git config merge.conflictstyle merge # TODO: use the default\n> +\n> test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b\n> if test_have_prereq GREP_STRIPS_CR\n> then\n> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\n> index 485ad0eee0..ae0bab37ad 100755\n> --- a/t/t6440-config-conflict-markers.sh\n> +++ b/t/t6440-config-conflict-markers.sh\n> @@ -29,7 +29,7 @@ test_expect_success 'merge' '\n> \t\tgit commit -a -m left &&\n> \n> \t\ttest_must_fail git merge r &&\n> -\t\t! grep -E \"\\|+\" content &&\n> +\t\tgrep -E \"\\|+\" content &&\n> \n> \t\tgit reset --hard &&\n> \t\ttest_must_fail git -c merge.conflictstyle=diff3 merge r &&\n> @@ -52,7 +52,7 @@ test_expect_success 'merge-tree' '\n> \t\ttest_commit l content l &&\n> \n> \t\tgit merge-tree initial r l >actual &&\n> -\t\t! grep -E \"\\|+\" actual &&\n> +\t\tgrep -E \"\\|+\" actual &&\n> \n> \t\tgit -c merge.conflictstyle=diff3 merge-tree initial r l >actual &&\n> \t\tgrep -E \"\\|+\" actual &&\n> @@ -77,7 +77,7 @@ test_expect_success 'notes' '\n> \t\tgit notes add -f -m l initial &&\n> \n> \t\ttest_must_fail git notes merge r &&\n> -\t\t! grep -E \"\\|+\" .git/NOTES_MERGE_WORKTREE/* &&\n> +\t\tgrep -E \"\\|+\" .git/NOTES_MERGE_WORKTREE/* &&\n> \n> \t\tgit notes merge --abort &&\n> \t\ttest_must_fail git -c merge.conflictstyle=diff3 notes merge r &&\n> @@ -104,7 +104,7 @@ test_expect_success 'checkout' '\n> \n> \t\tfill b d >content &&\n> \t\tgit checkout --merge master &&\n> -\t\t! grep -E \"\\|+\" content &&\n> +\t\tgrep -E \"\\|+\" content &&\n> \n> \t\tgit config merge.conflictstyle merge &&\n> \n> diff --git a/t/t7201-co.sh b/t/t7201-co.sh\n> index 7f6e23a4bb..11444d5360 100755\n> --- a/t/t7201-co.sh\n> +++ b/t/t7201-co.sh\n> @@ -25,6 +25,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> \n> . ./test-lib.sh\n> \n> +git config merge.conflictstyle merge # TODO: use the default\n> +\n> test_tick\n> \n> fill () {\n> diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\n> index 3fcb44767f..90449e80c3 100755\n> --- a/t/t7506-status-submodule.sh\n> +++ b/t/t7506-status-submodule.sh\n> @@ -253,6 +253,7 @@ test_expect_success 'status with merge conflict in .gitmodules' '\n> \ttest_create_repo_with_commit sub2 &&\n> \t(\n> \t\tcd super &&\n> +\t\tgit config merge.conflictstyle merge && # TODO: use the default\n> \t\tprev=$(git rev-parse HEAD) &&\n> \t\tgit checkout -b add_sub1 &&\n> \t\tgit submodule add ../sub1 &&\n> diff --git a/xdiff-interface.c b/xdiff-interface.c\n> index 19a030fbe2..1447771724 100644\n> --- a/xdiff-interface.c\n> +++ b/xdiff-interface.c\n> @@ -299,7 +299,7 @@ int xdiff_compare_lines(const char *l1, long s1,\n> \treturn xdl_recmatch(l1, s1, l2, s2, flags);\n> }\n> \n> -int git_xmerge_style = XDL_MERGE_STYLE_MERGE;\n> +int git_xmerge_style = XDL_MERGE_STYLE_DIFF3;\n> \n> int git_xmerge_config(const char *var, const char *value, void *cb)\n> {\n> \n"},{"id":"427000","messageId":"60c21110a2e68_b25b12085a@natae.notmuch","threadId":"55890","inReplyTo":"60883e1b-787f-5ec2-a9af-f2f6757d3c43@kdbg.org","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-10T13:18:08Z","receivedAt":"2021-06-10T13:18:43Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Johannes Sixt wrote:\n> Am 09.06.21 um 21:28 schrieb Felipe Contreras:\n> > Virtually everyone is using it, and it's one of the first things we\n> > teach newcomers in order to resolve conflicts efficiently.\n> > \n> > Let's make it the default.\n> \n> I tested diff3 style the VERY FIRST TIME the other day and was greated\n> with the below. Needless to say that this change is a no-go from my POV.\n\n\"I found a bug\" is not a valid reason not to do approach X.\n\nThe bug gets fixed and the approach continues.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427001","messageId":"60c211423f46f_b25b1208de@natae.notmuch","threadId":"55890","inReplyTo":"YMHE5xuxN90HJepT@danh.dev","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-10T13:18:58Z","receivedAt":"2021-06-10T13:20:01Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Đoàn Trần Công Danh wrote:\n> On 2021-06-10 08:41:21+0200, Johannes Sixt wrote:\n> > Am 09.06.21 um 21:28 schrieb Felipe Contreras:\n> > > Virtually everyone is using it, and it's one of the first things we\n> > > teach newcomers in order to resolve conflicts efficiently.\n> > > \n> > > Let's make it the default.\n> > \n> > I tested diff3 style the VERY FIRST TIME the other day and was greated\n> > with the below. Needless to say that this change is a no-go from my POV.\n> \n> I agree, despite using 3-way merging (with external tools) to resolve conflicts.\n> \n> I prefer the current conflict style, aka no-diff3 conflict style.\n\nDefaults are not for you, they are for the majority of users.\n\n-- \nFelipe Contreras"},{"id":"427002","messageId":"60c2130178831_b25b1208f5@natae.notmuch","threadId":"55890","inReplyTo":"b2436790-bbf0-2623-5e29-a6f6b4959974@gmail.com","subject":"Re: [PATCH 1/7] test: add merge style config test","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-10T13:26:25Z","receivedAt":"2021-06-10T13:26:31Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Phillip Wood wrote:\n> On 09/06/2021 20:28, Felipe Contreras wrote:\n> > We want to test different combinations of merge.conflictstyle, and a new\n> > file is the best place to do that.\n> \n> I'm not sure what this particular tests adds over the existing ones in \n> t6427-diff3-conflict-markers.sh.\n\nThat file is for diff3 conflict markers. The tests in this file are not.\n\n> The commit message does not explain why a new file is better than\n> adding this test to that file.\n\nBecause there's no file that is testing for this.\n\n> There are already diff3 tests for checkout\n\nThis file is not doing diff3 tests.\n\n\nAs stated above, it's testing different *combinations* of\nmerge.conflictstyle, diff3 is only *one* of the possibilities, another\npossibility is:\n\n git -c merge.conflictstyle=diff3 checkout -m --conflict=merge\n\nThat is *not* a diff3 test.\n\n> > diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\n> > new file mode 100755\n\n> > +test_expect_success 'merge' '\n> > +\ttest_create_repo merge &&\n> > +\t(\n> > +\t\tcd merge &&\n> > +\n> > +\t\tfill 1 2 3 >content &&\n> > +\t\tgit add content &&\n> > +\t\tgit commit -m base &&\n> > +\n> > +\t\tgit checkout -b r &&\n> > +\t\techo six >>content &&\n> > +\t\tgit commit -a -m right &&\n> > +\n> > +\t\tgit checkout master &&\n> > +\t\techo 7 >>content &&\n> > +\t\tgit commit -a -m left &&\n> > +\n> > +\t\ttest_must_fail git merge r &&\n> > +\t\t! grep -E \"\\|+\" content &&\n> \n> ! grep \"|\" would be simpler and just as effective.\n\nBut that would fail if there's a \"command1 | command2\".\n\n> This is quite a weak \n> test, something like \"^|||||| \" would be a stronger test for conflict \n> markers\n\nBut that doesn't work in all the tests.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427004","messageId":"60c214ace1191_b25b12086e@natae.notmuch","threadId":"55890","inReplyTo":"3168eb15-25a6-a3eb-a498-8effa0c79855@gmail.com","subject":"Re: [PATCH 5/7] xdiff: rename XDL_MERGE_STYLE_DIFF3","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-10T13:33:32Z","receivedAt":"2021-06-10T13:33:52Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Phillip Wood wrote:\n> On 09/06/2021 20:28, Felipe Contreras wrote:\n> \n> The subject would make more sense as 'xdiff: rename XDL_MERGE_DIFF3 to \n> XDL_MERGE_STYLE_DIFF3' rather than using the new name of the constant alone.\n\nThat is 55 characters, more than the recommended commit title length.\n\n> > If we don't specify we are talking about a style, XDL_MERGE_MINIMAL\n> > could be confused with a valid value instead of XDL_MERGE_DIFF3, which\n> > it isn't.\n> \n> I don't object to the rename but what is the source of the confusion \n> with XDL_MERGE_MINIMAL?\n\nXDL_MERGE_MINIMAL and other XDL_MERGE_FOO constants go into xmparam_t.level,\nXDL_MERGE_DIFF3 does not.\n\nAs stated in the commit message, the name XDL_MERGE_DIFF3 doesn't\ndistinguish it as a style.\n\nChers.\n\n-- \nFelipe Contreras\n"},{"id":"427006","messageId":"YMIYUgo71aKJ1Nnx@coredump.intra.peff.net","threadId":"55890","inReplyTo":"60883e1b-787f-5ec2-a9af-f2f6757d3c43@kdbg.org","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-10T13:49:06Z","receivedAt":"2021-06-10T13:49:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 10, 2021 at 08:41:21AM +0200, Johannes Sixt wrote:\n\n> Am 09.06.21 um 21:28 schrieb Felipe Contreras:\n> > Virtually everyone is using it, and it's one of the first things we\n> > teach newcomers in order to resolve conflicts efficiently.\n> > \n> > Let's make it the default.\n> \n> I tested diff3 style the VERY FIRST TIME the other day and was greated\n> with the below. Needless to say that this change is a no-go from my POV.\n> [...]\n\nI didn't look too deeply at your example, but I suspect it may be\nrelated to the fact that diff3 does not try to minimize the conflicts as\nmuch (and then the recursive merge on top of that piles on extra layers\nof confusion).\n\nThere's a lot more discussion in this old thread:\n\n https://lore.kernel.org/git/20130306150548.GC15375@pengutronix.de/\n\n-Peff\n"},{"id":"427007","messageId":"60c218a4e47e9_b25b1208ef@natae.notmuch","threadId":"55890","inReplyTo":"cc8bbc34-1048-0e7b-c783-33f556843a3f@gmail.com","subject":"Re: [PATCH 6/7] xdiff: simplify style assignments","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-10T13:50:28Z","receivedAt":"2021-06-10T13:51:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Phillip Wood wrote:\n> On 09/06/2021 20:28, Felipe Contreras wrote:\n> \n> I don't find the commit message explains this change very well\n> \n> > There is little value in checking that git_xmerge_style isn't 0 before\n> > changing it's default value.\n> \n> I think the check is actually that git_xmerge_style isn't -1.\n\nActually I meant \"< 0\", but yeah, that's mainly to check the -1 case.\n\n> Why is there little value in the check?\n\nIt's explained in the very next sentence.\n\n> > Most of the time it isn't 0 anyway, so just assign the value directly.\n> \n> Why to the times when it is zero (or -1) not matter?\n\nWhen it's 0 it's a no-op, and now it can't be -1.\n\nBy default structures are zeroed in git, so the defaults of integers\nare 0, and in the case of xmparam_t.style that is no exception.\n\nThese are all the same:\n\n\tstatic xmparam_t xmp;\n\n\tstatic xmparam_t xmp;\n\txmp.style = 0;\n\n\tstatic xmparam_t xmp;\n\tif (1)\n\t\txmp.style = 0;\n\nBut of course as it's explained most of the time that's not what\nhappens, what happens is:\n\n if (1)\n xml.style = 1;\n\nPerhaps this is clearer:\n\n There is little value in checking that git_xmerge_style isn't < 0\n before changing its value to xmp.style. If it's 0 then assigning 0 to\n xmp.style is a no-op, and if it's 1 (as it usually is), we are going\n to assign the value anyway.\n\n The only exception is when git_xmerge_style is -1, but there is no\n value in having that as default, so we just don't, and set the default\n to 0.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427009","messageId":"60c21d9ce8848_b25b120884@natae.notmuch","threadId":"55890","inReplyTo":"b4b9264d-1c17-1a62-f0ec-2791ab20adeb@gmail.com","subject":"Re: [PATCH 4/7] checkout: fix merge.conflictstyle handling","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-10T14:11:40Z","receivedAt":"2021-06-10T14:12:57Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Phillip Wood wrote:\n> On 09/06/2021 20:28, Felipe Contreras wrote:\n> > Currently both merge.conflictStyle and `git commit --merge\n> > --conflict=diff3` don't work together, since the former wrongly\n> > overrides the later.\n> > \n> > The way merge configurations are handled is not correct.\n> > It should be possible to do git_config(merge_recursive_config, ...) just\n> > like we can with git_diff_basic_config and others.\n> \n> It would be helpful to explain what the problem with \n> merge_recursive_config() actually is rather than just saying \"it should \n> be possible ...\"\n\nThe problem is that you can't do this:\n\n git_config(merge_recursive_config, NULL);\n\nAs it was explained.\n\nThat is the problem. I don't know how that's not clear.\n\n> > Therefore builtins like `git merge` can't call this function at the\n> > right time.\n> >\n> > We shuffle the functions a little bit so at least merge_recursive_config\n> > doesn't call git_xmerge_config directly and thus override previous\n> > configurations.\n> \n> Rather than papering of the problem, how difficult would it be to add a \n> field to ll_merge_options and pass the conflict style with that rather \n> than fiddling with the order that we set a global variable.\n\nProbably not that difficult, but then we also need a parser that\nconverts from \"diff3\" to whatever values we decide in that new field. We\nwould need a new parse_config_conflict_style() function.\n\nAnd that function will be only used by `git checkout` and nothing else.\nSo I don't think there's much value in it.\n\nThat problem whoever, is orthogonal to this series.\n\n> Does this change affect 'am/apply -3'? - Do they still read the config \n> setting properly?\n\nGood question. I'll have to add more tests to make sure that works\nproperly.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427010","messageId":"60c21f5eb4c62_b25b1208ae@natae.notmuch","threadId":"55890","inReplyTo":"bddfe587-704c-a7b2-1182-246226aa09ba@gmail.com","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-10T14:19:10Z","receivedAt":"2021-06-10T14:20:27Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Phillip Wood wrote:\n> On 09/06/2021 20:28, Felipe Contreras wrote:\n> > Virtually everyone is using it, and it's one of the first things we\n> > teach newcomers in order to resolve conflicts efficiently.\n> \n> Given there are millions of users I'm not sure how you established that \n> virtually everyone is using it.\n\nBecause it's the stablished consensus that resolving conflicts with\nmerge.conflictstyle=merge is suboptimal.\n\nEven if it's not the majority using it (say 49%), the majority would\nbenefit from using it.\n\n> I think that while this change would be useful to some users (though\n> not many if virtually everyone already has it set) it has the\n> potential to annoy a lot of users who are happy with the existing\n> default.\n\nEvery change has the potential to annoy some users.\n\nThat's an argument against further development of git, not this\nparticular patch.\n\n> I do not think that it is a positive change over all.\n\nOK. Others disagree.\n\n> Had the default been diff3 from early on in git's history then I would \n> not advocate changing it to the current default but I think the time has \n> passed when it can be changed without inconveniencing existing users.\n\ngit has changed defaults in the past, and it will change defaults in the\nfuture.\n\nThere will always be people pushing back against progress, and that is\ngood. But progress happens regardless.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427018","messageId":"49da6d73-c49f-d75e-0eb2-b12bd529fcfa@gmail.com","threadId":"55890","inReplyTo":"60c21d9ce8848_b25b120884@natae.notmuch","subject":"Re: [PATCH 4/7] checkout: fix merge.conflictstyle handling","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-06-10T14:50:42Z","receivedAt":"2021-06-10T14:52:00Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/06/2021 15:11, Felipe Contreras wrote:\n> Phillip Wood wrote:\n>> On 09/06/2021 20:28, Felipe Contreras wrote:\n>>> Currently both merge.conflictStyle and `git commit --merge\n>>> --conflict=diff3` don't work together, since the former wrongly\n>>> overrides the later.\n>>>\n>>> The way merge configurations are handled is not correct.\n>>> It should be possible to do git_config(merge_recursive_config, ...) just\n>>> like we can with git_diff_basic_config and others.\n>>\n>> It would be helpful to explain what the problem with\n>> merge_recursive_config() actually is rather than just saying \"it should\n>> be possible ...\"\n> \n> The problem is that you can't do this:\n> \n> git_config(merge_recursive_config, NULL);\n> \n> As it was explained.\n\nYou do not explain why you cannot do that\n\n> That is the problem. I don't know how that's not clear.\n> \n>>> Therefore builtins like `git merge` can't call this function at the\n>>> right time.\n>> >\n>>> We shuffle the functions a little bit so at least merge_recursive_config\n>>> doesn't call git_xmerge_config directly and thus override previous\n>>> configurations.\n>>\n>> Rather than papering of the problem, how difficult would it be to add a\n>> field to ll_merge_options and pass the conflict style with that rather\n>> than fiddling with the order that we set a global variable.\n> \n> Probably not that difficult, but then we also need a parser that\n> converts from \"diff3\" to whatever values we decide in that new field. We\n> would need a new parse_config_conflict_style() function.\n> And that function will be only used by `git checkout` and nothing else.\n> So I don't think there's much value in it.\n\nIt would allow us to add a --conflict option to all the mergey commands \nin the future and would be much easier to reason about than the approach \nof juggling where we call git_xmerge_config(). This patch requires us to \naudit all the code paths that end up in merge_recursive_config() to make \nsure they now call git_xmerge_config() themselves. You don't seem to \nhave done that as you don't know if am/apply are affected or not.\n\n\nBest Wishes\n\nPhillip\n\n> That problem whoever, is orthogonal to this series.\n> \n>> Does this change affect 'am/apply -3'? - Do they still read the config\n>> setting properly?\n> \n> Good question. I'll have to add more tests to make sure that works\n> properly.\n> \n> Cheers.\n> \n"},{"id":"427019","messageId":"a819dd7d-e690-baa7-b6c8-2b1fabe51522@gmail.com","threadId":"55890","inReplyTo":"60c2130178831_b25b1208f5@natae.notmuch","subject":"Re: [PATCH 1/7] test: add merge style config test","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-06-10T14:54:41Z","receivedAt":"2021-06-10T14:55:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/06/2021 14:26, Felipe Contreras wrote:\n> Phillip Wood wrote:\n>> On 09/06/2021 20:28, Felipe Contreras wrote:\n>>> We want to test different combinations of merge.conflictstyle, and a new\n>>> file is the best place to do that.\n>>\n>> I'm not sure what this particular tests adds over the existing ones in\n>> t6427-diff3-conflict-markers.sh.\n> \n> That file is for diff3 conflict markers. The tests in this file are not.\n> \n>> The commit message does not explain why a new file is better than\n>> adding this test to that file.\n> \n> Because there's no file that is testing for this.\n> \n>> There are already diff3 tests for checkout\n> \n> This file is not doing diff3 tests.\n> \n> \n> As stated above, it's testing different *combinations* of\n> merge.conflictstyle, diff3 is only *one* of the possibilities, another\n> possibility is:\n> \n> git -c merge.conflictstyle=diff3 checkout -m --conflict=merge\n> \n> That is *not* a diff3 test.\n\nI think that is an artificial distinction, it is testing the behavior of \ncheckout when merge.conflictStyle=diff3 just like the other tests, it \njust happens to be checking that the config option can be combined with \na command line option.\n\nBest Wishes\n\nPhillip\n\n>>> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\n>>> new file mode 100755\n> \n>>> +test_expect_success 'merge' '\n>>> +\ttest_create_repo merge &&\n>>> +\t(\n>>> +\t\tcd merge &&\n>>> +\n>>> +\t\tfill 1 2 3 >content &&\n>>> +\t\tgit add content &&\n>>> +\t\tgit commit -m base &&\n>>> +\n>>> +\t\tgit checkout -b r &&\n>>> +\t\techo six >>content &&\n>>> +\t\tgit commit -a -m right &&\n>>> +\n>>> +\t\tgit checkout master &&\n>>> +\t\techo 7 >>content &&\n>>> +\t\tgit commit -a -m left &&\n>>> +\n>>> +\t\ttest_must_fail git merge r &&\n>>> +\t\t! grep -E \"\\|+\" content &&\n>>\n>> ! grep \"|\" would be simpler and just as effective.\n> \n> But that would fail if there's a \"command1 | command2\".\n> \n>> This is quite a weak\n>> test, something like \"^|||||| \" would be a stronger test for conflict\n>> markers\n> \n> But that doesn't work in all the tests.\n> \n> Cheers.\n> \n"},{"id":"427021","messageId":"bef8e373-20d8-3545-ba43-00108b2e77df@gmail.com","threadId":"55890","inReplyTo":"60c2130178831_b25b1208f5@natae.notmuch","subject":"Re: [PATCH 1/7] test: add merge style config test","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-06-10T14:58:31Z","receivedAt":"2021-06-10T14:59:47Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/06/2021 14:26, Felipe Contreras wrote:\n> Phillip Wood wrote:\n>> On 09/06/2021 20:28, Felipe Contreras wrote:\n>>> We want to test different combinations of merge.conflictstyle, and a new\n>>> file is the best place to do that.\n>>[...]\n>>> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\n>>> new file mode 100755\n> \n>>> +test_expect_success 'merge' '\n>>> +\ttest_create_repo merge &&\n>>> +\t(\n>>> +\t\tcd merge &&\n>>> +\n>>> +\t\tfill 1 2 3 >content &&\n>>> +\t\tgit add content &&\n>>> +\t\tgit commit -m base &&\n>>> +\n>>> +\t\tgit checkout -b r &&\n>>> +\t\techo six >>content &&\n>>> +\t\tgit commit -a -m right &&\n>>> +\n>>> +\t\tgit checkout master &&\n>>> +\t\techo 7 >>content &&\n>>> +\t\tgit commit -a -m left &&\n>>> +\n>>> +\t\ttest_must_fail git merge r &&\n>>> +\t\t! grep -E \"\\|+\" content &&\n>>\n>> ! grep \"|\" would be simpler and just as effective.\n> \n> But that would fail if there's a \"command1 | command2\".\n\nI don't understand. What are you expecting content to contain? Why \ndoesn't \"\\|+\" fail in that case?\n\n>> This is quite a weak\n>> test, something like \"^|||||| \" would be a stronger test for conflict\n>> markers\n> \n> But that doesn't work in all the tests.\n\nSo test for what you actually expect, you don't need to have the same \ncheck in all the tests if the expected output is different.\n\nBest Wishes\n\nPhillip\n\n> Cheers.\n> \n"},{"id":"427032","messageId":"60c2373b93140_b25b1208c2@natae.notmuch","threadId":"55890","inReplyTo":"YMIYUgo71aKJ1Nnx@coredump.intra.peff.net","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-10T16:00:59Z","receivedAt":"2021-06-10T16:01:13Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Jeff King wrote:\n> On Thu, Jun 10, 2021 at 08:41:21AM +0200, Johannes Sixt wrote:\n> \n> > Am 09.06.21 um 21:28 schrieb Felipe Contreras:\n> > > Virtually everyone is using it, and it's one of the first things we\n> > > teach newcomers in order to resolve conflicts efficiently.\n> > > \n> > > Let's make it the default.\n> > \n> > I tested diff3 style the VERY FIRST TIME the other day and was greated\n> > with the below. Needless to say that this change is a no-go from my POV.\n> > [...]\n> \n> I didn't look too deeply at your example, but I suspect it may be\n> related to the fact that diff3 does not try to minimize the conflicts as\n> much (and then the recursive merge on top of that piles on extra layers\n> of confusion).\n> \n> There's a lot more discussion in this old thread:\n> \n> https://lore.kernel.org/git/20130306150548.GC15375@pengutronix.de/\n\nGeezus. My patches always end up kicking the hornest nest don't they?\n\nMaybe it would make sense to revive the zdiff3 patch and attempt to make\nthat the default. That would take a lot of time though, so it wasn't as\neasy as just flipping a switch from \"merge\" to \"diff3\".\n\nBut there is value in attempting to make the default merge.conflictstyle\nwork for everyone (or last least as many people as possible). It's\nshinning a light on issues that are already present now.\n\nFor reference--and since gmane links don't work any more--here are the\nrelevant links for the past discussions:\n\nhttps://lore.kernel.org/git/20130306150548.GC15375@pengutronix.de/\nhttps://lore.kernel.org/git/alpine.LFD.1.10.0808311021120.12958@nehalem.linux-foundation.org/\nhttps://lore.kernel.org/git/1220056963-2352-5-git-send-email-gitster@pobox.com/\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427035","messageId":"YMI+R5LFTj7ezlZE@coredump.intra.peff.net","threadId":"55890","inReplyTo":"60c2373b93140_b25b1208c2@natae.notmuch","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-10T16:31:03Z","receivedAt":"2021-06-10T16:31:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 10, 2021 at 11:00:59AM -0500, Felipe Contreras wrote:\n\n> > I didn't look too deeply at your example, but I suspect it may be\n> > related to the fact that diff3 does not try to minimize the conflicts as\n> > much (and then the recursive merge on top of that piles on extra layers\n> > of confusion).\n> > \n> > There's a lot more discussion in this old thread:\n> > \n> > https://lore.kernel.org/git/20130306150548.GC15375@pengutronix.de/\n> \n> Geezus. My patches always end up kicking the hornest nest don't they?\n> \n> Maybe it would make sense to revive the zdiff3 patch and attempt to make\n> that the default. That would take a lot of time though, so it wasn't as\n> easy as just flipping a switch from \"merge\" to \"diff3\".\n\nI had that patch in my daily build for several years, and I would\noccasionally trigger it when seeing an ugly conflict. IIRC, it\nsegfaulted on me a few times, but I never tracked down the bug. Just a\ncaution in case anybody wants to resurrect it.\n\n-Peff\n"},{"id":"427036","messageId":"60c23e8b7f672_b25b1208d2@natae.notmuch","threadId":"55890","inReplyTo":"49da6d73-c49f-d75e-0eb2-b12bd529fcfa@gmail.com","subject":"Re: [PATCH 4/7] checkout: fix merge.conflictstyle handling","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-10T16:32:11Z","receivedAt":"2021-06-10T16:32:29Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Phillip Wood wrote:\n> On 10/06/2021 15:11, Felipe Contreras wrote:\n> > Phillip Wood wrote:\n> >> On 09/06/2021 20:28, Felipe Contreras wrote:\n> >>> Currently both merge.conflictStyle and `git commit --merge\n> >>> --conflict=diff3` don't work together, since the former wrongly\n> >>> overrides the later.\n> >>>\n> >>> The way merge configurations are handled is not correct.\n> >>> It should be possible to do git_config(merge_recursive_config, ...) just\n> >>> like we can with git_diff_basic_config and others.\n> >>\n> >> It would be helpful to explain what the problem with\n> >> merge_recursive_config() actually is rather than just saying \"it should\n> >> be possible ...\"\n> > \n> > The problem is that you can't do this:\n> > \n> > git_config(merge_recursive_config, NULL);\n> > \n> > As it was explained.\n> \n> You do not explain why you cannot do that\n\n % git grep merge_recursive_config\n static void merge_recursive_config(struct merge_options *opt)\n\nFor starters it's a static function.\n\nSecond, clearly the type of functions git_config() receives are not\n`void (*)(struct merge_options *)`.\n\nI mean, I do see value in explaning as much detail as needed in the\ncommit message, but it shouldn't be a lesson on git's codebase:\ngit_config() is a standard thing, and it's even mentioned in the user\nmanual.\n\nhttps://git-scm.com/docs/user-manual#birdview-on-the-source-code\n\n> > That is the problem. I don't know how that's not clear.\n> > \n> >>> Therefore builtins like `git merge` can't call this function at the\n> >>> right time.\n> >> >\n> >>> We shuffle the functions a little bit so at least merge_recursive_config\n> >>> doesn't call git_xmerge_config directly and thus override previous\n> >>> configurations.\n> >>\n> >> Rather than papering of the problem, how difficult would it be to add a\n> >> field to ll_merge_options and pass the conflict style with that rather\n> >> than fiddling with the order that we set a global variable.\n> > \n> > Probably not that difficult, but then we also need a parser that\n> > converts from \"diff3\" to whatever values we decide in that new field. We\n> > would need a new parse_config_conflict_style() function.\n> > And that function will be only used by `git checkout` and nothing else.\n> > So I don't think there's much value in it.\n> \n> It would allow us to add a --conflict option to all the mergey commands \n> in the future and would be much easier to reason about than the approach \n> of juggling where we call git_xmerge_config().\n\nFeel free to write a patch for that.\n\n> This patch requires us to audit all the code paths that end up in\n> merge_recursive_config() to make sure they now call\n> git_xmerge_config() themselves. You don't seem to have done that as\n> you don't know if am/apply are affected or not.\n\nThat is a separate issue, that I already mentioned...\n\n> > That problem whoever, is orthogonal to this series.\n> > \n> >> Does this change affect 'am/apply -3'? - Do they still read the config\n> >> setting properly?\n> > \n> > Good question. I'll have to add more tests to make sure that works\n> > properly.\n\nhere.\n\n-- \nFelipe Contreras\n"},{"id":"427037","messageId":"60c23efe4405d_b25b1208e3@natae.notmuch","threadId":"55890","inReplyTo":"a819dd7d-e690-baa7-b6c8-2b1fabe51522@gmail.com","subject":"Re: [PATCH 1/7] test: add merge style config test","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-10T16:34:06Z","receivedAt":"2021-06-10T16:35:25Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Phillip Wood wrote:\n> On 10/06/2021 14:26, Felipe Contreras wrote:\n> > Phillip Wood wrote:\n> >> There are already diff3 tests for checkout\n> > \n> > This file is not doing diff3 tests.\n> > \n> > \n> > As stated above, it's testing different *combinations* of\n> > merge.conflictstyle, diff3 is only *one* of the possibilities, another\n> > possibility is:\n> > \n> > git -c merge.conflictstyle=diff3 checkout -m --conflict=merge\n> > \n> > That is *not* a diff3 test.\n> \n> I think that is an artificial distinction, it is testing the behavior of \n> checkout when merge.conflictStyle=diff3\n\nThat is one of the things it's testing, it's not the only thing it's\ntesting, it's testing other things as well.\n\n-- \nFelipe Contreras\n"},{"id":"427039","messageId":"60c2420a2dd73_b25b1208f1@natae.notmuch","threadId":"55890","inReplyTo":"bef8e373-20d8-3545-ba43-00108b2e77df@gmail.com","subject":"Re: [PATCH 1/7] test: add merge style config test","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-10T16:47:06Z","receivedAt":"2021-06-10T16:48:22Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Phillip Wood wrote:\n> On 10/06/2021 14:26, Felipe Contreras wrote:\n> > Phillip Wood wrote:\n> >> On 09/06/2021 20:28, Felipe Contreras wrote:\n> >>> We want to test different combinations of merge.conflictstyle, and a new\n> >>> file is the best place to do that.\n> >>[...]\n> >>> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\n> >>> new file mode 100755\n> > \n> >>> +test_expect_success 'merge' '\n> >>> +\ttest_create_repo merge &&\n> >>> +\t(\n> >>> +\t\tcd merge &&\n> >>> +\n> >>> +\t\tfill 1 2 3 >content &&\n> >>> +\t\tgit add content &&\n> >>> +\t\tgit commit -m base &&\n> >>> +\n> >>> +\t\tgit checkout -b r &&\n> >>> +\t\techo six >>content &&\n> >>> +\t\tgit commit -a -m right &&\n> >>> +\n> >>> +\t\tgit checkout master &&\n> >>> +\t\techo 7 >>content &&\n> >>> +\t\tgit commit -a -m left &&\n> >>> +\n> >>> +\t\ttest_must_fail git merge r &&\n> >>> +\t\t! grep -E \"\\|+\" content &&\n> >>\n> >> ! grep \"|\" would be simpler and just as effective.\n> > \n> > But that would fail if there's a \"command1 | command2\".\n> \n> I don't understand. What are you expecting content to contain?\n\nNot a sequence of |.\n\n> Why doesn't \"\\|+\" fail in that case?\n\nIt would, perhaps \"\\|\\|+\" would be better, or maybe \"\\|{2,}\".\n\n> >> This is quite a weak\n> >> test, something like \"^|||||| \" would be a stronger test for conflict\n> >> markers\n> > \n> > But that doesn't work in all the tests.\n> \n> So test for what you actually expect, you don't need to have the same \n> check in all the tests if the expected output is different.\n\nI don't need to, but it makes the tests simpler, and as you pointed out\nin another comment: more tests are needed.\n\nPerhaps once we know exactly what we want to test, and how to fix the\ncurrent issues it would make sense to revisit these.\n\n-- \nFelipe Contreras\n"},{"id":"427074","messageId":"xmqqh7i5ci3t.fsf@gitster.g","threadId":"55890","inReplyTo":"YMIYUgo71aKJ1Nnx@coredump.intra.peff.net","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-11T01:20:22Z","receivedAt":"2021-06-11T01:20:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I didn't look too deeply at your example, but I suspect it may be\n> related to the fact that diff3 does not try to minimize the conflicts as\n> much (and then the recursive merge on top of that piles on extra layers\n> of confusion).\n>\n> There's a lot more discussion in this old thread:\n>\n> https://lore.kernel.org/git/20130306150548.GC15375@pengutronix.de/\n\nYes, it does not help that the given sample involves conflicts in\nthe inner merge, which is unfortunately almost unreadable. For less\nconfusing merges, diff3 style is often a lifesaver necessary for\navoiding mismerges by showing what the common ancestor looked like,\nso that the reader/merger/integrator can tell what each side wanted\nto do to the conflicted section.\n\nRejecting diff3 style output because of the way a conflicted part in\nthe inner merge appears as a common ancestor version may be throwing\nthe baby out with the bathwater. A different way to say it is that\nuntil we improve the way the conflicted inner merge is shown, diff3\nstyle is not something we can recommend to new users as a default, I\nwould think.\n\n"},{"id":"427077","messageId":"xmqq5yyl5buz.fsf@gitster.g","threadId":"55890","inReplyTo":"3168eb15-25a6-a3eb-a498-8effa0c79855@gmail.com","subject":"Re: [PATCH 5/7] xdiff: rename XDL_MERGE_STYLE_DIFF3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-11T03:17:08Z","receivedAt":"2021-06-11T03:17:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> The subject would make more sense as 'xdiff: rename XDL_MERGE_DIFF3 to\n> XDL_MERGE_STYLE_DIFF3' rather than using the new name of the constant\n> alone.\n\nTrue.\n\n>> If we don't specify we are talking about a style, XDL_MERGE_MINIMAL\n>> could be confused with a valid value instead of XDL_MERGE_DIFF3, which\n>> it isn't.\n>\n> I don't object to the rename but what is the source of the confusion\n> with XDL_MERGE_MINIMAL?\n\nI do not see any confusion, either, but the current XDL_MERGE_DIFF3\nbeing a boolean (i.e. if false, use the output style of the 'merge'\ncommand) and our lack of an enumeration constant for 'merge' means\nthat a future addition of the third output style would require us to\nadd XDL_MERGE_$STYLE for both the new style and the traditional\n'merge' style. And If we would end up with XDL_MERGE_DIFF3,\nXDL_MERGE_MERGE and XDL_MERGE_FOO for that third output style.\n\nThe 'merge' one simply looks strange in that context. And from that\npoint of view, this change might be a good way to futureproof the\ncodebase.\n\nThanks.\n"},{"id":"427084","messageId":"638a0500-459a-a25b-afca-904ec0e09866@kdbg.org","threadId":"55890","inReplyTo":"xmqqh7i5ci3t.fsf@gitster.g","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-06-11T06:23:30Z","receivedAt":"2021-06-11T06:23:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.06.21 um 03:20 schrieb Junio C Hamano:\n> Yes, it does not help that the given sample involves conflicts in\n> the inner merge, which is unfortunately almost unreadable. For less\n> confusing merges, diff3 style is often a lifesaver necessary for\n> avoiding mismerges by showing what the common ancestor looked like,\n> so that the reader/merger/integrator can tell what each side wanted\n> to do to the conflicted section.\n> \n> Rejecting diff3 style output because of the way a conflicted part in\n> the inner merge appears as a common ancestor version may be throwing\n> the baby out with the bathwater. A different way to say it is that\n> until we improve the way the conflicted inner merge is shown, diff3\n> style is not something we can recommend to new users as a default, I\n> would think.\n\nI understand that diff3 is very useful for an integrator like you who\ndoes a lot of merges of code that was not written by yourself.\n\nBut I would estimate that most conflicts (in absolute number among all\ndevelopers using Git) arise during rebase operations and cherry-picking,\ni.e., while one is working on their own code. In such sitations, the\nsimpler conflict markup is sufficient, because one knows the background\nand reason of the conflicts. And then the ability to compact conflicts\nis a life-saver. Therefore, I argue that simple conflict style should\nremain the default even if the presentation of inner conflicts under\ndiff3 style is improved.\n\n-- Hannes\n"},{"id":"427087","messageId":"xmqqy2bg3nqw.fsf@gitster.g","threadId":"55890","inReplyTo":"638a0500-459a-a25b-afca-904ec0e09866@kdbg.org","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-11T06:43:19Z","receivedAt":"2021-06-11T06:43:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> But I would estimate that most conflicts (in absolute number among all\n> developers using Git) arise during rebase operations and cherry-picking,\n> i.e., while one is working on their own code. In such sitations, the\n> simpler conflict markup is sufficient, because one knows the background\n> and reason of the conflicts.\n\n\"rebase -i\" to reorganize one's own series would be a prime example\nof \"conflicts you need to resolve in code that is purely your own\nand nobody else's\", and cherry-picking used while reorganizing one's\nown series falls into the same category. I agree that a simpler\nmarkup would be more appropriate in such cases.\n\nRebasing to catch up with updated upstream is a different story,\nthough. The same for cherry-picking an earlier change to an updated\nupstream.\n\n"},{"id":"427088","messageId":"07ef3a3b-4812-4fa1-c60c-b9085a268bc3@kdbg.org","threadId":"55890","inReplyTo":"xmqqy2bg3nqw.fsf@gitster.g","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-06-11T07:02:54Z","receivedAt":"2021-06-11T07:03:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.06.21 um 08:43 schrieb Junio C Hamano:\n> Johannes Sixt writes:\n> \n>> But I would estimate that most conflicts (in absolute number among all\n>> developers using Git) arise during rebase operations and cherry-picking,\n>> i.e., while one is working on their own code. In such sitations, the\n>> simpler conflict markup is sufficient, because one knows the background\n>> and reason of the conflicts.\n> \n> \"rebase -i\" to reorganize one's own series would be a prime example\n> of \"conflicts you need to resolve in code that is purely your own\n> and nobody else's\", and cherry-picking used while reorganizing one's\n> own series falls into the same category. I agree that a simpler\n> markup would be more appropriate in such cases.\n> \n> Rebasing to catch up with updated upstream is a different story,\n> though. The same for cherry-picking an earlier change to an updated\n> upstream.\n\nI've reflected on this a bit more as well. I've forgotten about the\n\"catch up with someone else\" case. That is certainly helped by diff3\nstyle. I retract my opposition and am now neutral to a potential change\nof default. (I still don't endorse it because it upsets my workflow.)\n\nThe case that inner conflicts are presented sub-optimally under diff3\nremains, though.\n\n-- Hannes\n"},{"id":"427089","messageId":"xmqqo8cc3maq.fsf@gitster.g","threadId":"55890","inReplyTo":"07ef3a3b-4812-4fa1-c60c-b9085a268bc3@kdbg.org","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-11T07:14:37Z","receivedAt":"2021-06-11T07:14:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> The case that inner conflicts are presented sub-optimally under diff3\n> remains, though.\n\nI agree that until that happens (necessary but not sufficient\ncondition), it is premature to recommend diff3 style to be the\ndefault.\n\nI notice that \"git merge --help\" tells what each part separated by\nconflict markers mean in both output styles, but does not make a\nspecific recommendation as to which one to use in what situation,\nand it might benefit a few additional sentences to help readers\nbased on what you said, i.e. the \"RCS merge\" style that hides the\noriginal is succinct and easier to work with when you are familiar\nwith what both sides did, while a more verbose \"diff3\" style helps\nwhen you are unfamiliar with what one side (or both sides) did.\n"},{"id":"427093","messageId":"bf1125b9-fbf1-f937-5197-e5128233952c@gmail.com","threadId":"55890","inReplyTo":"20210609192842.696646-5-felipe.contreras@gmail.com","subject":"Re: [PATCH 4/7] checkout: fix merge.conflictstyle handling","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-06-11T09:18:59Z","receivedAt":"2021-06-11T09:19:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 09/06/2021 20:28, Felipe Contreras wrote:\n>[...]\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index d146bb116f..10e6e1e4d1 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -3845,7 +3845,7 @@ static void merge_recursive_config(struct merge_options *opt)\n> \t\t} /* avoid erroring on values from future versions of git */\n> \t\tfree(value);\n> \t}\n> -\tgit_config(git_xmerge_config, NULL);\n> +\tgit_config(git_default_config, NULL);\n\nNow that all callers are required to call git_config(git_xmerge_config) \nbefore calling init_merge_options() this line can be deleted.\n\nBest Wishes\n\nPhillip\n\n> }\n >\n> void init_merge_options(struct merge_options *opt,\n> diff --git a/sequencer.c b/sequencer.c\n> index 0bec01cf38..9e2bdca0f6 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -34,6 +34,7 @@\n> #include \"commit-reach.h\"\n> #include \"rebase-interactive.h\"\n> #include \"reset.h\"\n> +#include \"xdiff-interface.h\"\n> \n> #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n> \n> @@ -224,6 +225,10 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)\n> \tif (status)\n> \t\treturn status;\n> \n> +\tstatus = git_xmerge_config(k, v, NULL);\n> +\tif (status)\n> +\t\treturn status;\n> +\n> \treturn git_diff_basic_config(k, v, NULL);\n> }\n> \n> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\n> index 44f79ac91b..485ad0eee0 100755\n> --- a/t/t6440-config-conflict-markers.sh\n> +++ b/t/t6440-config-conflict-markers.sh\n> @@ -89,4 +89,35 @@ test_expect_success 'notes' '\n> \t)\n> '\n> \n> +test_expect_success 'checkout' '\n> +\ttest_create_repo checkout &&\n> +\t(\n> +\t\ttest_commit checkout &&\n> +\n> +\t\tfill a b c d e >content &&\n> +\t\tgit add content &&\n> +\t\tgit commit -m initial &&\n> +\n> +\t\tgit checkout -b simple master &&\n> +\t\tfill a c e >content &&\n> +\t\tgit commit -a -m simple &&\n> +\n> +\t\tfill b d >content &&\n> +\t\tgit checkout --merge master &&\n> +\t\t! grep -E \"\\|+\" content &&\n> +\n> +\t\tgit config merge.conflictstyle merge &&\n> +\n> +\t\tgit checkout -f simple &&\n> +\t\tfill b d >content &&\n> +\t\tgit checkout --merge --conflict=diff3 master &&\n> +\t\tgrep -E \"\\|+\" content &&\n> +\n> +\t\tgit checkout -f simple &&\n> +\t\tfill b d >content &&\n> +\t\tgit checkout --merge --conflict=merge master &&\n> +\t\t! grep -E \"\\|+\" content\n> +\t)\n> +'\n> +\n> test_done\n> \n"},{"id":"427094","messageId":"5164b67c-d701-077f-7743-aea4ae13779d@gmail.com","threadId":"55890","inReplyTo":"60c23e8b7f672_b25b1208d2@natae.notmuch","subject":"Re: [PATCH 4/7] checkout: fix merge.conflictstyle handling","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-06-11T09:18:41Z","receivedAt":"2021-06-11T09:20:02Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/06/2021 17:32, Felipe Contreras wrote:\n> Phillip Wood wrote:\n>> On 10/06/2021 15:11, Felipe Contreras wrote:\n>>> Phillip Wood wrote:\n>>>> On 09/06/2021 20:28, Felipe Contreras wrote:\n>>>>> Currently both merge.conflictStyle and `git commit --merge\n>>>>> --conflict=diff3` don't work together, since the former wrongly\n>>>>> overrides the later.\n>>>>>\n>>>>> The way merge configurations are handled is not correct.\n>>>>> It should be possible to do git_config(merge_recursive_config, ...) just\n>>>>> like we can with git_diff_basic_config and others.\n>>>>\n>>>> It would be helpful to explain what the problem with\n>>>> merge_recursive_config() actually is rather than just saying \"it should\n>>>> be possible ...\"\n>>>\n>>> The problem is that you can't do this:\n>>>\n>>> git_config(merge_recursive_config, NULL);\n>>>\n>>> As it was explained.\n>>\n>> You do not explain why you cannot do that\n> \n> % git grep merge_recursive_config\n> static void merge_recursive_config(struct merge_options *opt)\n> \n> For starters it's a static function.\n> \n> Second, clearly the type of functions git_config() receives are not\n> `void (*)(struct merge_options *)`.\n> \n> I mean, I do see value in explaning as much detail as needed in the\n> commit message, but it shouldn't be a lesson on git's codebase:\n> git_config() is a standard thing, and it's even mentioned in the user\n> manual.\n\nI'm not asking for a lesson on git's config system, I'm asking you to \nadd a sentence to the commit message to explain what the problem is \nrather than just saying \"you should be able to do this but you can't\".\n\n> https://git-scm.com/docs/user-manual#birdview-on-the-source-code\n> \n>>> That is the problem. I don't know how that's not clear.\n>>>\n>>>>> Therefore builtins like `git merge` can't call this function at the\n>>>>> right time.\n>>>> >\n>>>>> We shuffle the functions a little bit so at least merge_recursive_config\n>>>>> doesn't call git_xmerge_config directly and thus override previous\n>>>>> configurations.\n>>>>\n>>>> Rather than papering of the problem, how difficult would it be to add a\n>>>> field to ll_merge_options and pass the conflict style with that rather\n>>>> than fiddling with the order that we set a global variable.\n>>>\n>>> Probably not that difficult, but then we also need a parser that\n>>> converts from \"diff3\" to whatever values we decide in that new field. We\n>>> would need a new parse_config_conflict_style() function.\n>>> And that function will be only used by `git checkout` and nothing else.\n>>> So I don't think there's much value in it.\n>>\n>> It would allow us to add a --conflict option to all the mergey commands\n>> in the future and would be much easier to reason about than the approach\n>> of juggling where we call git_xmerge_config().\n> \n> Feel free to write a patch for that.\n> \n>> This patch requires us to audit all the code paths that end up in\n>> merge_recursive_config() to make sure they now call\n>> git_xmerge_config() themselves. You don't seem to have done that as\n>> you don't know if am/apply are affected or not.\n> \n> That is a separate issue, that I already mentioned...\n\nI know you are going to add some tests but the point is that when making \na change like this you need to actively audit all the callers of \ninit_merge_options() and ensure that they are now calling \ngit_config(git_xmerge_config) and base you tests on what you find while \ndoing that. Have you run 'grep init_merge_options()' to see where it is \nbeing called?\n\nBest Wishes\n\nPhillip\n\n>>> That problem whoever, is orthogonal to this series.\n>>>\n>>>> Does this change affect 'am/apply -3'? - Do they still read the config\n>>>> setting properly?\n>>>\n>>> Good question. I'll have to add more tests to make sure that works\n>>> properly.\n> \n> here.\n> \n"},{"id":"427095","messageId":"0975b12c-82e1-344c-e09f-37f568c10514@gmail.com","threadId":"55890","inReplyTo":"60c2420a2dd73_b25b1208f1@natae.notmuch","subject":"Re: [PATCH 1/7] test: add merge style config test","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-06-11T09:19:11Z","receivedAt":"2021-06-11T09:20:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 10/06/2021 17:47, Felipe Contreras wrote:\n> Phillip Wood wrote:\n>> On 10/06/2021 14:26, Felipe Contreras wrote:\n>>> Phillip Wood wrote:\n>>>> On 09/06/2021 20:28, Felipe Contreras wrote:\n>>>>> We want to test different combinations of merge.conflictstyle, and a new\n>>>>> file is the best place to do that.\n>>>> [...]\n>>>>> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\n>>>>> new file mode 100755\n>>>\n>>>>> +test_expect_success 'merge' '\n>>>>> +\ttest_create_repo merge &&\n>>>>> +\t(\n>>>>> +\t\tcd merge &&\n>>>>> +\n>>>>> +\t\tfill 1 2 3 >content &&\n>>>>> +\t\tgit add content &&\n>>>>> +\t\tgit commit -m base &&\n>>>>> +\n>>>>> +\t\tgit checkout -b r &&\n>>>>> +\t\techo six >>content &&\n>>>>> +\t\tgit commit -a -m right &&\n>>>>> +\n>>>>> +\t\tgit checkout master &&\n>>>>> +\t\techo 7 >>content &&\n>>>>> +\t\tgit commit -a -m left &&\n>>>>> +\n>>>>> +\t\ttest_must_fail git merge r &&\n>>>>> +\t\t! grep -E \"\\|+\" content &&\n>>>>\n>>>> ! grep \"|\" would be simpler and just as effective.\n>>>\n>>> But that would fail if there's a \"command1 | command2\".\n>>\n>> I don't understand. What are you expecting content to contain?\n> \n> Not a sequence of |.\n> \n>> Why doesn't \"\\|+\" fail in that case?\n> \n> It would, perhaps \"\\|\\|+\" would be better, or maybe \"\\|{2,}\".\n\nThe point of my original comment was that you do not need an ERE - 'grep \n\"||\"' matches the same set of lines as 'grep -E \"\\|\\|+\"'. As it is \ntesting for conflict markers anchoring the pattern to the beginning of a \nline would probably be a good idea.\n\nBest Wishes\n\nPhillip\n\n>>>> This is quite a weak\n>>>> test, something like \"^|||||| \" would be a stronger test for conflict\n>>>> markers\n>>>\n>>> But that doesn't work in all the tests.\n>>\n>> So test for what you actually expect, you don't need to have the same\n>> check in all the tests if the expected output is different.\n> \n> I don't need to, but it makes the tests simpler, and as you pointed out\n> in another comment: more tests are needed.\n> \n> Perhaps once we know exactly what we want to test, and how to fix the\n> current issues it would make sense to revisit these.\n> \n"},{"id":"427104","messageId":"875yykipq2.fsf@osv.gnss.ru","threadId":"55890","inReplyTo":"xmqqo8cc3maq.fsf@gitster.g","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-06-11T11:51:33Z","receivedAt":"2021-06-11T11:52:39Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Sixt writes:\n>\n>> The case that inner conflicts are presented sub-optimally under diff3\n>> remains, though.\n>\n> I agree that until that happens (necessary but not sufficient\n> condition), it is premature to recommend diff3 style to be the\n> default.\n\nYep. A work-around could be to fix diff3 to rather produce RCS merge\nstyle in such situations?\n\n>\n> I notice that \"git merge --help\" tells what each part separated by\n> conflict markers mean in both output styles, but does not make a\n> specific recommendation as to which one to use in what situation,\n> and it might benefit a few additional sentences to help readers\n> based on what you said, i.e. the \"RCS merge\" style that hides the\n> original is succinct and easier to work with when you are familiar\n> with what both sides did, while a more verbose \"diff3\" style helps\n> when you are unfamiliar with what one side (or both sides) did.\n\nI don't get it. Once you have diff3 output, and you want something\nsimpler, you just kill the inner section, right? RCS merge output style\nis simply inferior.\n\nThanks,\n-- Sergey Organov\n"},{"id":"427107","messageId":"60c3684eaf033_3ad920828@natae.notmuch","threadId":"55890","inReplyTo":"xmqq5yyl5buz.fsf@gitster.g","subject":"Re: [PATCH 5/7] xdiff: rename XDL_MERGE_STYLE_DIFF3","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-11T13:42:38Z","receivedAt":"2021-06-11T13:43:56Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n> > The subject would make more sense as 'xdiff: rename XDL_MERGE_DIFF3 to\n> > XDL_MERGE_STYLE_DIFF3' rather than using the new name of the constant\n> > alone.\n> \n> True.\n\nBut why? When we look back in history few people would care what the\nprevious name of XDL_MERGE_STYLE_DIFF3 was, and if they do, they don't\nnecessarily need it in the title.\n\n> >> If we don't specify we are talking about a style, XDL_MERGE_MINIMAL\n> >> could be confused with a valid value instead of XDL_MERGE_DIFF3, which\n> >> it isn't.\n> >\n> > I don't object to the rename but what is the source of the confusion\n> > with XDL_MERGE_MINIMAL?\n> \n> I do not see any confusion, either, but the current XDL_MERGE_DIFF3\n> being a boolean\n\nBut it's not a boolean: git_xmerge_style is currently -1 by default.\n\n> (i.e. if false, use the output style of the 'merge'\n> command) and our lack of an enumeration constant for 'merge' means\n> that a future addition of the third output style would require us to\n> add XDL_MERGE_$STYLE for both the new style and the traditional\n> 'merge' style. And If we would end up with XDL_MERGE_DIFF3,\n> XDL_MERGE_MERGE and XDL_MERGE_FOO for that third output style.\n\nBut can you put XDL_MERGE_FOO in xmp.level? Or XDL_MERGE_BAR in\nxmp.style?\n\n> The 'merge' one simply looks strange in that context. And from that\n> point of view, this change might be a good way to futureproof the\n> codebase.\n\nYes.\n\n-- \nFelipe Contreras\n"},{"id":"427109","messageId":"60c36e8112417_3ad9208d8@natae.notmuch","threadId":"55890","inReplyTo":"xmqqh7i5ci3t.fsf@gitster.g","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-11T14:09:05Z","receivedAt":"2021-06-11T14:09:32Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n\n> A different way to say it is that until we improve the way the\n> conflicted inner merge is shown, diff3 style is not something we can\n> recommend to new users as a default, I would think.\n\nI am not sure about that. Does anybody know any newcommer that you would\nrecommend diff2 (aka. merge) style to?\n\nI wouldn't.\n\nPut yourself in front of a class teaching how to resolve conflicts, I\nwouldn't dream of explaning the particulars of conflict markers, I would\njust fire up Meld, and show them visually.\n\nConflict markers are not for newcomers anyway, so that is a moot point.\n\nMoreover, it is really hard for an expert to put himself in the shoes of\na newcomer... We forget how hard it was at the beginning.\n\nAt least me personally, I remember that long time ago when resolving\nconflicts I constantly had to find the change from the base to the\nremote side in order to see what changes I'm trying to merge, and then\nfrom the base to the local side to see why they were conflicting.\n\nI don't use diff3 because I want to be fancy, I use it because from\nexperience I *need* to see the base more often than not.\n\nPlus, I try to follow the principile of eating your own dogfood [1]. If\nusing diff2 is so awful that basically no git experts use it, why are we\nrecommending it?\n\nIf the purpose of conflict markers is to resolve conflicts **correctly**\nand preferably in an efficient way, then diff3 is just better.\n\nCheers.\n\n[1] https://en.wikipedia.org/wiki/Eating_your_own_dog_food\n\n-- \nFelipe Contreras\n"},{"id":"427110","messageId":"60c3711466f39_3ad920831@natae.notmuch","threadId":"55890","inReplyTo":"638a0500-459a-a25b-afca-904ec0e09866@kdbg.org","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-11T14:20:04Z","receivedAt":"2021-06-11T14:20:10Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Johannes Sixt wrote:\n\n> I understand that diff3 is very useful for an integrator like you who\n> does a lot of merges of code that was not written by yourself.\n> \n> But I would estimate that most conflicts (in absolute number among all\n> developers using Git) arise during rebase operations and cherry-picking,\n> i.e., while one is working on their own code.\n\nThe vast majority of developers don't rebase their code. They only do it\nwhen absolutely necessary, and that is when they have to rebase it on\ntop of another person's code.\n\n> In such sitations, the simpler conflict markup is sufficient, because\n> one knows the background and reason of the conflicts.\n\nI don't know about you, but I often don't know the background of my own\ncode from one month ago, hell, sometimes not even one week ago.\n\nI consider myself from one year ago to be pretty much a different\ndeveloper.\n\n> And then the ability to compact conflicts is a life-saver.\n\nI don't see how shifting one's sight a couple lines below would save\nanybody's life.\n\nOn the other hand opening another tool just to find out was was the\noriginal code is tedious as hell.\n\n> Therefore, I argue that simple conflict style should remain the\n> default even if the presentation of inner conflicts under diff3 style\n> is improved.\n\nFirst let's get rid of all the assumptions you made above.\n\nJust because you personally do X doesn't mean most people do.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427111","messageId":"60c3724de35b4_3ad920839@natae.notmuch","threadId":"55890","inReplyTo":"07ef3a3b-4812-4fa1-c60c-b9085a268bc3@kdbg.org","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-11T14:25:17Z","receivedAt":"2021-06-11T14:25:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Johannes Sixt wrote:\n> Am 11.06.21 um 08:43 schrieb Junio C Hamano:\n\n> > Rebasing to catch up with updated upstream is a different story,\n> > though. The same for cherry-picking an earlier change to an updated\n> > upstream.\n> \n> I've reflected on this a bit more as well. I've forgotten about the\n> \"catch up with someone else\" case. That is certainly helped by diff3\n> style. I retract my opposition and am now neutral to a potential change\n> of default. (I still don't endorse it because it upsets my workflow.)\n\nAll right. Fortunately we have a configuration for that.\n\n> The case that inner conflicts are presented sub-optimally under diff3\n> remains, though.\n\nIndeed, and that's something that we should consider fixing, but it's\nnot necessarily a roadblocker since apparently many developers have been\nable to live with these sub-optimal diff3 inner conflicts.\n\nPersonally I have never experienced what you posted, so maybe there's\nsomething else happening behind the scenes.\n\nMaybe merge-ort changed something.\n\n-- \nFelipe Contreras\n"},{"id":"427112","messageId":"60c3730f6582b_3ad920842@natae.notmuch","threadId":"55890","inReplyTo":"xmqqo8cc3maq.fsf@gitster.g","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-11T14:28:31Z","receivedAt":"2021-06-11T14:29:48Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Junio C Hamano wrote:\n> Johannes Sixt writes:\n> \n> > The case that inner conflicts are presented sub-optimally under diff3\n> > remains, though.\n> \n> I agree that until that happens (necessary but not sufficient\n> condition), it is premature to recommend diff3 style to be the\n> default.\n\nWhy? Most experienced git developers have no issue with diff3. So\npresumably it's good enough as it is.\n\nYes, we should consider improving that issue stated above, but\n*necessary*? I don't think so.\n\n-- \nFelipe Contreras\n"},{"id":"427113","messageId":"60c37480c6ab9_3ad920882@natae.notmuch","threadId":"55890","inReplyTo":"5164b67c-d701-077f-7743-aea4ae13779d@gmail.com","subject":"Re: [PATCH 4/7] checkout: fix merge.conflictstyle handling","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-11T14:34:40Z","receivedAt":"2021-06-11T14:36:00Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Phillip Wood wrote:\n> On 10/06/2021 17:32, Felipe Contreras wrote:\n> > Phillip Wood wrote:\n> >> On 10/06/2021 15:11, Felipe Contreras wrote:\n> >>> Phillip Wood wrote:\n> >>>> On 09/06/2021 20:28, Felipe Contreras wrote:\n> >>>>> Currently both merge.conflictStyle and `git commit --merge\n> >>>>> --conflict=diff3` don't work together, since the former wrongly\n> >>>>> overrides the later.\n> >>>>>\n> >>>>> The way merge configurations are handled is not correct.\n> >>>>> It should be possible to do git_config(merge_recursive_config, ...) just\n> >>>>> like we can with git_diff_basic_config and others.\n> >>>>\n> >>>> It would be helpful to explain what the problem with\n> >>>> merge_recursive_config() actually is rather than just saying \"it should\n> >>>> be possible ...\"\n> >>>\n> >>> The problem is that you can't do this:\n> >>>\n> >>> git_config(merge_recursive_config, NULL);\n> >>>\n> >>> As it was explained.\n> >>\n> >> You do not explain why you cannot do that\n> > \n> > % git grep merge_recursive_config\n> > static void merge_recursive_config(struct merge_options *opt)\n> > \n> > For starters it's a static function.\n> > \n> > Second, clearly the type of functions git_config() receives are not\n> > `void (*)(struct merge_options *)`.\n> > \n> > I mean, I do see value in explaning as much detail as needed in the\n> > commit message, but it shouldn't be a lesson on git's codebase:\n> > git_config() is a standard thing, and it's even mentioned in the user\n> > manual.\n> \n> I'm not asking for a lesson on git's config system, I'm asking you to \n> add a sentence to the commit message to explain what the problem is \n> rather than just saying \"you should be able to do this but you can't\".\n\nYes, but why? What possible reason could there be for this to fail? Can\nyou list two?\n\n git_config(merge_recursive_config, NULL);\n\nBut more importantly, why does it matter? How would it change the patch?\n\n> > https://git-scm.com/docs/user-manual#birdview-on-the-source-code\n> > \n> >>> That is the problem. I don't know how that's not clear.\n> >>>\n> >>>>> Therefore builtins like `git merge` can't call this function at the\n> >>>>> right time.\n> >>>> >\n> >>>>> We shuffle the functions a little bit so at least merge_recursive_config\n> >>>>> doesn't call git_xmerge_config directly and thus override previous\n> >>>>> configurations.\n> >>>>\n> >>>> Rather than papering of the problem, how difficult would it be to add a\n> >>>> field to ll_merge_options and pass the conflict style with that rather\n> >>>> than fiddling with the order that we set a global variable.\n> >>>\n> >>> Probably not that difficult, but then we also need a parser that\n> >>> converts from \"diff3\" to whatever values we decide in that new field. We\n> >>> would need a new parse_config_conflict_style() function.\n> >>> And that function will be only used by `git checkout` and nothing else.\n> >>> So I don't think there's much value in it.\n> >>\n> >> It would allow us to add a --conflict option to all the mergey commands\n> >> in the future and would be much easier to reason about than the approach\n> >> of juggling where we call git_xmerge_config().\n> > \n> > Feel free to write a patch for that.\n> > \n> >> This patch requires us to audit all the code paths that end up in\n> >> merge_recursive_config() to make sure they now call\n> >> git_xmerge_config() themselves. You don't seem to have done that as\n> >> you don't know if am/apply are affected or not.\n> > \n> > That is a separate issue, that I already mentioned...\n> \n> I know you are going to add some tests but the point is that when making \n> a change like this you need to actively audit all the callers of \n> init_merge_options() and ensure that they are now calling \n> git_config(git_xmerge_config) and base you tests on what you find while \n> doing that.\n\nAnd what makes you think I haven't?\n\nIf developers were perfect there would be no need for code review.\n\n> Have you run 'grep init_merge_options()' to see where it is \n> being called?\n\nNow that is insulting.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427114","messageId":"60c375b7a8b90_3ad92084@natae.notmuch","threadId":"55890","inReplyTo":"0975b12c-82e1-344c-e09f-37f568c10514@gmail.com","subject":"Re: [PATCH 1/7] test: add merge style config test","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-11T14:39:51Z","receivedAt":"2021-06-11T14:40:54Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Phillip Wood wrote:\n> On 10/06/2021 17:47, Felipe Contreras wrote:\n> > Phillip Wood wrote:\n> >> On 10/06/2021 14:26, Felipe Contreras wrote:\n> >>> Phillip Wood wrote:\n> >>>> On 09/06/2021 20:28, Felipe Contreras wrote:\n> >>>>> We want to test different combinations of merge.conflictstyle, and a new\n> >>>>> file is the best place to do that.\n> >>>> [...]\n> >>>>> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh\n> >>>>> new file mode 100755\n> >>>\n> >>>>> +test_expect_success 'merge' '\n> >>>>> +\ttest_create_repo merge &&\n> >>>>> +\t(\n> >>>>> +\t\tcd merge &&\n> >>>>> +\n> >>>>> +\t\tfill 1 2 3 >content &&\n> >>>>> +\t\tgit add content &&\n> >>>>> +\t\tgit commit -m base &&\n> >>>>> +\n> >>>>> +\t\tgit checkout -b r &&\n> >>>>> +\t\techo six >>content &&\n> >>>>> +\t\tgit commit -a -m right &&\n> >>>>> +\n> >>>>> +\t\tgit checkout master &&\n> >>>>> +\t\techo 7 >>content &&\n> >>>>> +\t\tgit commit -a -m left &&\n> >>>>> +\n> >>>>> +\t\ttest_must_fail git merge r &&\n> >>>>> +\t\t! grep -E \"\\|+\" content &&\n> >>>>\n> >>>> ! grep \"|\" would be simpler and just as effective.\n> >>>\n> >>> But that would fail if there's a \"command1 | command2\".\n> >>\n> >> I don't understand. What are you expecting content to contain?\n> > \n> > Not a sequence of |.\n> > \n> >> Why doesn't \"\\|+\" fail in that case?\n> > \n> > It would, perhaps \"\\|\\|+\" would be better, or maybe \"\\|{2,}\".\n> \n> The point of my original comment was that you do not need an ERE - 'grep \n> \"||\"' matches the same set of lines as 'grep -E \"\\|\\|+\"'. As it is \n> testing for conflict markers anchoring the pattern to the beginning of a \n> line would probably be a good idea.\n\nRight, I didn't add that because I saw some tests not doing ^ when they\nclearly should, so I thought perhaps there was a compatibility issue,\nbut now I see that ^ is already used in many tests, therefore \"^\\|\\|+\"\nmakes sense.\n\n-- \nFelipe Contreras\n"},{"id":"427115","messageId":"60c38205cdbf8_3ad92084@natae.notmuch","threadId":"55890","inReplyTo":"875yykipq2.fsf@osv.gnss.ru","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-11T15:32:21Z","receivedAt":"2021-06-11T15:33:43Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Sergey Organov wrote:\n> Junio C Hamano writes:\n\n> > I notice that \"git merge --help\" tells what each part separated by\n> > conflict markers mean in both output styles, but does not make a\n> > specific recommendation as to which one to use in what situation,\n> > and it might benefit a few additional sentences to help readers\n> > based on what you said, i.e. the \"RCS merge\" style that hides the\n> > original is succinct and easier to work with when you are familiar\n> > with what both sides did, while a more verbose \"diff3\" style helps\n> > when you are unfamiliar with what one side (or both sides) did.\n> \n> I don't get it. Once you have diff3 output, and you want something\n> simpler, you just kill the inner section, right? RCS merge output style\n> is simply inferior.\n\nThe issue here is not a mere inner section, it's a nested inner section\ndue to a recursive merge.\n\nPersonally I've never encountered these in real life, but you can\ntrigger them with the following synthetic example, and the output with\ndiff3 is:\n\n--- a/content\n+++ b/content\n@@@ -1,1 -1,1 +1,13 @@@\n++<<<<<<< HEAD\n +D\n++||||||| merged common ancestors\n++<<<<<<<<< Temporary merge branch 1\n++B\n++||||||||| 2c9519d\n++1\n++=========\n++A\n++>>>>>>>>> Temporary merge branch 2\n++=======\n+ C\n++>>>>>>> C\n\nWhile with merge is:\n\n--- a/content\n+++ b/content\n@@@ -1,1 -1,1 +1,5 @@@\n++<<<<<<< HEAD\n +D\n++=======\n+ C\n++>>>>>>> C\n\nI don't see why diff3 triggers the output of this temporary merge, that\nis a bug in my book.\n\nI would expect the output to simply be:\n\n--- a/content\n+++ b/content\n@@@ -1,1 -1,1 +1,13 @@@\n++<<<<<<< HEAD\n +D\n++||||||| 2c9519d\n++1\n++=======\n+ C\n++>>>>>>> C\n\nCheers.\n\n git init repo &&\n cd repo &&\n\n echo 1 > content &&\n git add content &&\n git commit -m 1 content &&\n\n git checkout -b A master &&\n echo A > content &&\n git commit -m A content &&\n\n git checkout -b B master &&\n echo B > content &&\n git commit -m B content &&\n\n git checkout -b C A &&\n git rev-parse B >.git/MERGE_HEAD &&\n echo C > content &&\n git commit -m C -a &&\n\n git checkout -b D A &&\n git rev-parse B >.git/MERGE_HEAD &&\n echo D > content &&\n git commit -m D -a &&\n\n git -c merge.conflictstyle=diff3 merge -m final C &&\n cat content\n\n-- \nFelipe Contreras\n"},{"id":"427120","messageId":"87a6nwflez.fsf@osv.gnss.ru","threadId":"55890","inReplyTo":"60c38205cdbf8_3ad92084@natae.notmuch","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-06-11T15:52:52Z","receivedAt":"2021-06-11T15:54:14Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Felipe Contreras writes:\n\n> Sergey Organov wrote:\n>> Junio C Hamano writes:\n>\n>> > I notice that \"git merge --help\" tells what each part separated by\n>> > conflict markers mean in both output styles, but does not make a\n>> > specific recommendation as to which one to use in what situation,\n>> > and it might benefit a few additional sentences to help readers\n>> > based on what you said, i.e. the \"RCS merge\" style that hides the\n>> > original is succinct and easier to work with when you are familiar\n>> > with what both sides did, while a more verbose \"diff3\" style helps\n>> > when you are unfamiliar with what one side (or both sides) did.\n>> \n>> I don't get it. Once you have diff3 output, and you want something\n>> simpler, you just kill the inner section, right? RCS merge output style\n>> is simply inferior.\n>\n> The issue here is not a mere inner section, it's a nested inner section\n> due to a recursive merge.\n\nNo, this one is just generic suggestion by Junio to improve\ndocumentation, unrelated to particular problematic contents of the inner\nsection of diff3.\n\nThanks\n-- Sergey Organov\n"},{"id":"427124","messageId":"60c3911d48f3c_85c8208f6@natae.notmuch","threadId":"55890","inReplyTo":"87a6nwflez.fsf@osv.gnss.ru","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-11T16:36:45Z","receivedAt":"2021-06-11T16:36:48Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Sergey Organov wrote:\n> Felipe Contreras writes:\n> \n> > Sergey Organov wrote:\n> >> Junio C Hamano writes:\n> >\n> >> > I notice that \"git merge --help\" tells what each part separated by\n> >> > conflict markers mean in both output styles, but does not make a\n> >> > specific recommendation as to which one to use in what situation,\n> >> > and it might benefit a few additional sentences to help readers\n> >> > based on what you said, i.e. the \"RCS merge\" style that hides the\n> >> > original is succinct and easier to work with when you are familiar\n> >> > with what both sides did, while a more verbose \"diff3\" style helps\n> >> > when you are unfamiliar with what one side (or both sides) did.\n> >> \n> >> I don't get it. Once you have diff3 output, and you want something\n> >> simpler, you just kill the inner section, right? RCS merge output style\n> >> is simply inferior.\n> >\n> > The issue here is not a mere inner section, it's a nested inner section\n> > due to a recursive merge.\n> \n> No, this one is just generic suggestion by Junio to improve\n> documentation, unrelated to particular problematic contents of the inner\n> section of diff3.\n\nOK, but diff3 is not always just merge minus some stuff.\n\nIt would be nice if it was, which is what triggered the proposal of\nzdiff3:\n\nhttps://lore.kernel.org/git/20130306150548.GC15375@pengutronix.de/\n\n-- \nFelipe Contreras\n"},{"id":"427125","messageId":"dd5a1b6c-f0d3-f135-d03d-5d1f9205503d@kdbg.org","threadId":"55890","inReplyTo":"875yykipq2.fsf@osv.gnss.ru","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-06-11T16:41:09Z","receivedAt":"2021-06-11T16:41:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.06.21 um 13:51 schrieb Sergey Organov:\n> I don't get it. Once you have diff3 output, and you want something\n> simpler, you just kill the inner section, right? RCS merge output style\n> is simply inferior.\n\nThere is an important case where RCS style is not inferior: When the base is\n\n 123456\n\nthen our side makes it\n\n 12ABC56\n\nand their side makes it\n\n 12AXC56\n\nthen diff3 must display the conflict as\n\n 1256\n\nto be technically correct. RCS style can coalesce A and C outside of the\nconflict and display it as\n\n 12AC34\n\nand *that* is the helpful part of this simpler style. You encounter\nthese kinds of conflicts *a lot* when you juggle fixups in a rebase\n--interactive session.\n\nThe thread mentioned earlier upthread explores whether diff3 can do a\nsimilar simplification.\n\n-- Hannes\n"},{"id":"427126","messageId":"763be21c-c87e-6b02-8ca4-256c3c804995@kdbg.org","threadId":"55890","inReplyTo":"60c3724de35b4_3ad920839@natae.notmuch","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2021-06-11T16:53:39Z","receivedAt":"2021-06-11T16:53:47Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.06.21 um 16:25 schrieb Felipe Contreras:\n> Personally I have never experienced what you posted, so maybe there's\n> something else happening behind the scenes.\n\nI have to do a lot of criss-cross merges lately.\n\n> Maybe merge-ort changed something.\n\nIt produces the same result.\n\n-- Hannes\n"},{"id":"427131","messageId":"60c39bb3b5cc4_8d0f20817@natae.notmuch","threadId":"55890","inReplyTo":"dd5a1b6c-f0d3-f135-d03d-5d1f9205503d@kdbg.org","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-11T17:21:55Z","receivedAt":"2021-06-11T17:23:19Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Johannes Sixt wrote:\n> then diff3 must display the conflict as\n> \n> 1256\n> \n> to be technically correct. RCS style can coalesce A and C outside of the\n> conflict and display it as\n> \n> 12AC34\n> \n> and *that* is the helpful part of this simpler style.\n\nI have trouble translating the above to what I'm familiar with in my\nmind, so...\n\ndiff2:\n\n 1\n 2\n A\n <<<<<<< l\n B\n =======\n X\n >>>>>>> r\n C\n 5\n 6\n\ndiff3:\n\n 1\n 2\n <<<<<<< l\n A\n B\n C\n ||||||| b\n 3\n 4\n =======\n A\n X\n C\n >>>>>>> r\n 5\n 6\n\nI personally don't mind at all having a few extra lines in order to\nvisualize what actually happened.\n\nBut of course there's zdiff3:\n\n 1\n 2\n A\n <<<<<<< l\n B\n ||||||| b\n 3\n 4\n =======\n X\n >>>>>>> r\n C\n 5\n 6\n\nWhich is the best of both worlds, even if not technically accurate.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427133","messageId":"60c39e1c52c04_8d0f208b2@natae.notmuch","threadId":"55890","inReplyTo":"CABPp-BH0aRiSUw03nSK6jHRNQ+zcpUzr6WjeJ5GpdUCqCKxbag@mail.gmail.com","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-11T17:32:12Z","receivedAt":"2021-06-11T17:32:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren wrote:\n> On Fri, Jun 11, 2021 at 7:25 AM Felipe Contreras \n> wrote:\n\n> > Personally I have never experienced what you posted, so maybe there's\n> > something else happening behind the scenes.\n> >\n> > Maybe merge-ort changed something.\n> \n> merge-ort made no changes relative to content merges or choice of merge\n> bases. (In fact, merge ort doesn't even handle content merges; that's the\n> xdiff layer.) Even if merge-ort had made changes in this area, merge-ort\n> is not the default and I didn't see the necessary config tweaks in your\n> list of config options. (I would have recommended against people using\n> merge ort until 7bec8e7fa6 (\"Merge branch 'en/ort-readiness'\", 2021-04-16),\n> which only made it into a release last week with 2.32. I probably won't be\n> recommending it as the default at least until the optimization work is\n> merged and it's hard to predict how many more months that will take.)\n\nIndeed, I tested on v2.25 and found the same output.\n\nI thought of merge-ort because 1) I've never seen such kind of output\nbefore, and 2) grepping the code I thought I saw merge-ort being the\ndefault of something, but now I seem to be unable to find where.\n\n> It's more likely that the codebases you work with just don't have\n> criss-cross merges.\n\nYes, that's it.\n\nI don't see why people in these kinds of codebases would like diff3\ndoing that by default.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427135","messageId":"87bl8ce1ug.fsf@osv.gnss.ru","threadId":"55890","inReplyTo":"60c39bb3b5cc4_8d0f20817@natae.notmuch","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-06-11T17:40:55Z","receivedAt":"2021-06-11T17:41:14Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Felipe Contreras writes:\n\n> Johannes Sixt wrote:\n>> then diff3 must display the conflict as\n>> \n>> 1256\n>> \n>> to be technically correct. RCS style can coalesce A and C outside of the\n>> conflict and display it as\n>> \n>> 12AC34\n>> \n>> and *that* is the helpful part of this simpler style.\n>\n> I have trouble translating the above to what I'm familiar with in my\n> mind, so...\n>\n> diff2:\n>\n> 1\n> 2\n> A\n> <<<<<<< l\n> B\n> =======\n> X\n> >>>>>>> r\n> C\n> 5\n> 6\n>\n> diff3:\n>\n> 1\n> 2\n> <<<<<<< l\n> A\n> B\n> C\n> ||||||| b\n> 3\n> 4\n> =======\n> A\n> X\n> C\n> >>>>>>> r\n> 5\n> 6\n>\n> I personally don't mind at all having a few extra lines in order to\n> visualize what actually happened.\n\nPlus a good tool should have an option to quickly show a diff between any\n2 of 3 parts, making analysis even simpler.\n\n>\n> But of course there's zdiff3:\n>\n> 1\n> 2\n> A\n> <<<<<<< l\n> B\n> ||||||| b\n> 3\n> 4\n> =======\n> X\n> >>>>>>> r\n> C\n> 5\n> 6\n>\n> Which is the best of both worlds, even if not technically accurate.\n\nYeah, now I see, thank you both for explanations!\n\nThat said, to me it seems that for any of 3 formats one can find a case\nwhere it's better than the other 2. I'm sure I got a few occasions where\nleaving common part(s) out of conflicts resulted in a confusion and\nmis-merge.\n\nThanks,\n-- Sergey Organov\n"},{"id":"427138","messageId":"60c3a41bd25e3_8d0f2089e@natae.notmuch","threadId":"55890","inReplyTo":"CABPp-BHRQSF2_aYTBfpfnW4Bh3Hz7vLFj_QNGj8R4WeCS6_utw@mail.gmail.com","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-11T17:57:47Z","receivedAt":"2021-06-11T17:58:07Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren wrote:\n> On Fri, Jun 11, 2021 at 8:32 AM Felipe Contreras \n> wrote:\n> > Sergey Organov wrote:\n> > > Junio C Hamano writes:\n\n> > git init repo &&\n> > cd repo &&\n> >\n> > echo 1 > content &&\n> > git add content &&\n> > git commit -m 1 content &&\n> >\n> > git checkout -b A master &&\n> > echo A > content &&\n> > git commit -m A content &&\n> >\n> > git checkout -b B master &&\n> > echo B > content &&\n> > git commit -m B content &&\n> >\n> > git checkout -b C A &&\n> > git rev-parse B >.git/MERGE_HEAD &&\n> > echo C > content &&\n> > git commit -m C -a &&\n> >\n> > git checkout -b D A &&\n> > git rev-parse B >.git/MERGE_HEAD &&\n> > echo D > content &&\n> > git commit -m D -a &&\n> >\n> > git -c merge.conflictstyle=diff3 merge -m final C &&\n> > cat content\n> \n> Right, here you do not have a unique merge base; you have two of them: A &\n> B (it's possible to have three or more as well). To do a three-way merge,\n> you need a single base commit. So, whenever you have more than one merge\n> base, both merge-recursive and merge-ort will merge the merge bases to get\n> a virtual merge base. There's always a risk that the merge bases don't\n> have a unique merge base either, forcing the algorithm to recurse. This\n> behavior is where the merge algorithm 'recursive' got its name from (and\n> which also appears in ort's name -- \"Ostensibly Recursive's Twin\").\n> \n> You could decide to just pick one of the merge-bases at random, and yield a\n> different set of surprises including silently merging in favor of one side\n> when the two sides did things differently. That's problematic.\n> \n> Instead of using a merge base (a recent-as-possible common commit), you\n> could decide to instead just try to find a unique common base, regardless\n> of how ancient it is. Using ancient commits as the base is a step towards\n> just doing a two-way merge (treating the histories as completely\n> independent and throwing merge conflicts whenever any files aren't\n> identical on the two sides). Sure, it's not as bad, but it does yield\n> massive amounts of useless conflicts. So this is problematic too.\n> \n> The alternative to the above two options was the\n> make-a-virtual-merge-base-by-merging-merge-bases strategy. It apparently\n> was very successful.\n\nOK. That makes sense.\n\n> But it does mean that merge bases can have conflict markers in them.\n\nBut why? And even if they do, why do they have to be diff3 conflict\nmarkers?\n\nThis would be more human-friendly:\n\n <<<<<<< HEAD\n D\n ||||||| merged common ancestors\n <<<<<<<<< Temporary merge branch 1\n B\n =========\n A\n >>>>>>>>> Temporary merge branch 2\n =======\n C\n >>>>>>> C\n\nOr just put a stub conflict marker:\n\n <<<<<<< HEAD\n D\n ||||||| merged common ancestors\n <<<<<<<<< Temporary merge >>>>>>>>>\n =======\n C\n >>>>>>> C\n\nOr just use the base of the virtual merge:\n\n <<<<<<< HEAD\n D\n ||||||| merged common ancestors\n 1\n =======\n C\n >>>>>>> C\n\nWe don't have to use diff3 all the way.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427139","messageId":"CABPp-BGtYnVPijg2OWoDBM915-PPFXk1O3H=BMf_itc4dNjAxQ@mail.gmail.com","threadId":"55890","inReplyTo":"60c39e1c52c04_8d0f208b2@natae.notmuch","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-11T17:57:24Z","receivedAt":"2021-06-11T17:58:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jun 11, 2021 at 10:32 AM Felipe Contreras\n wrote:\n>\n> Elijah Newren wrote:\n> > On Fri, Jun 11, 2021 at 7:25 AM Felipe Contreras \n> > wrote:\n>\n> > > Personally I have never experienced what you posted, so maybe there's\n> > > something else happening behind the scenes.\n> > >\n> > > Maybe merge-ort changed something.\n> >\n> > merge-ort made no changes relative to content merges or choice of merge\n> > bases. (In fact, merge ort doesn't even handle content merges; that's the\n> > xdiff layer.) Even if merge-ort had made changes in this area, merge-ort\n> > is not the default and I didn't see the necessary config tweaks in your\n> > list of config options. (I would have recommended against people using\n> > merge ort until 7bec8e7fa6 (\"Merge branch 'en/ort-readiness'\", 2021-04-16),\n> > which only made it into a release last week with 2.32. I probably won't be\n> > recommending it as the default at least until the optimization work is\n> > merged and it's hard to predict how many more months that will take.)\n>\n> Indeed, I tested on v2.25 and found the same output.\n>\n> I thought of merge-ort because 1) I've never seen such kind of output\n> before, and 2) grepping the code I thought I saw merge-ort being the\n> default of something, but now I seem to be unable to find where.\n\nWe have briefly discussed multiple times what things might be needed\nto eventually make merge-ort the default (though it's not even\ncomplete yet; I'm five months into upstreaming the optimization\npatches with an unknown number of months left). There were also a\ncouple patches to make the _tests_ default to using merge-ort on most\nplatforms, while still keeping one suite that tests merge-recursive to\nensure we don't add breakage. Perhaps one of those is what you're\nthinking of?\n\n> > It's more likely that the codebases you work with just don't have\n> > criss-cross merges.\n>\n> Yes, that's it.\n>\n> I don't see why people in these kinds of codebases would like diff3\n> doing that by default.\n\nI suspect they don't[1]. What's the alternative, though? Not using\ndiff3? Picking a different base to avoid the occasional nested\nconflict in the inner merge region, but which overall has much worse\nother side effects? I think Junio was addressing this when he\nrecently said elsewhere in this thread that \"Rejecting diff3 style\noutput because of the way a conflicted part in the inner merge appears\nas a common ancestor version may be throwing the baby out with the\nbathwater\"[2]. Sure, it's an annoyance, but diff3 is still a good\noption and there is no current solution to the annoying nested merge\ndisplay.\n\nAlso, not sure whether it matters or not, but just for completeness I\nshould point out that you can get nested conflict markers without\ncriss-cross merges and without merge.conflictStyle=diff3. It's just\nmuch more rare.\n\n[1] https://lore.kernel.org/git/CAPc5daUC+6cHpexXTO24p4mG_5eL1JmxrYm8h3UfdTh_FMka=w@mail.gmail.com/\n[2] https://lore.kernel.org/git/xmqqh7i5ci3t.fsf@gitster.g/\n"},{"id":"427140","messageId":"60c3a7058af24_8d0f20833@natae.notmuch","threadId":"55890","inReplyTo":"87bl8ce1ug.fsf@osv.gnss.ru","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-11T18:10:13Z","receivedAt":"2021-06-11T18:10:17Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Sergey Organov wrote:\n> Felipe Contreras writes:\n> \n> > Johannes Sixt wrote:\n> >> then diff3 must display the conflict as\n> >> \n> >> 1256\n> >> \n> >> to be technically correct. RCS style can coalesce A and C outside of the\n> >> conflict and display it as\n> >> \n> >> 12AC34\n> >> \n> >> and *that* is the helpful part of this simpler style.\n> >\n> > I have trouble translating the above to what I'm familiar with in my\n> > mind, so...\n> >\n> > diff2:\n> >\n> > 1\n> > 2\n> > A\n> > <<<<<<< l\n> > B\n> > =======\n> > X\n> > >>>>>>> r\n> > C\n> > 5\n> > 6\n> >\n> > diff3:\n> >\n> > 1\n> > 2\n> > <<<<<<< l\n> > A\n> > B\n> > C\n> > ||||||| b\n> > 3\n> > 4\n> > =======\n> > A\n> > X\n> > C\n> > >>>>>>> r\n> > 5\n> > 6\n> >\n> > I personally don't mind at all having a few extra lines in order to\n> > visualize what actually happened.\n> \n> Plus a good tool should have an option to quickly show a diff between any\n> 2 of 3 parts, making analysis even simpler.\n\nIndeed, it depends on the mergetool, but personally I use vimdiff3\n(which I authored). All I need are diff3 conflict markers plus some\ncolors.\n\n> > But of course there's zdiff3:\n> >\n> > 1\n> > 2\n> > A\n> > <<<<<<< l\n> > B\n> > ||||||| b\n> > 3\n> > 4\n> > =======\n> > X\n> > >>>>>>> r\n> > C\n> > 5\n> > 6\n> >\n> > Which is the best of both worlds, even if not technically accurate.\n> \n> Yeah, now I see, thank you both for explanations!\n> \n> That said, to me it seems that for any of 3 formats one can find a case\n> where it's better than the other 2. I'm sure I got a few occasions where\n> leaving common part(s) out of conflicts resulted in a confusion and\n> mis-merge.\n\nYes, and I found it curious that all this sprang up from my suggestion\nto get out of our comfort zones.\n\nSince I don't have my beloved `g mt` alias, I've been forced to do\n`g mergetool --tool=gvimdiff3`, once there, I decided to use gvimdiff2 a\ncouple of times, and I even gave meld a try.\n\nNow, I didn't dare to remove merge.conflictstyle from my config, but if\nI did, I would I have immediately noticed that:\n\n git merge --\n\nThere's nothing to easily switch conflict styles.\n\nSeems like there's many areas of opportunity.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427141","messageId":"87fsxob6s0.fsf@osv.gnss.ru","threadId":"55890","inReplyTo":"60c3a7058af24_8d0f20833@natae.notmuch","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2021-06-11T18:22:39Z","receivedAt":"2021-06-11T18:24:00Z","isPatch":true,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Felipe Contreras writes:\n\n> Sergey Organov wrote:\n>> Felipe Contreras writes:\n>> \n>> > Johannes Sixt wrote:\n>> >> then diff3 must display the conflict as\n>> >> \n>> >> 1256\n>> >> \n>> >> to be technically correct. RCS style can coalesce A and C outside of the\n>> >> conflict and display it as\n>> >> \n>> >> 12AC34\n>> >> \n>> >> and *that* is the helpful part of this simpler style.\n>> >\n>> > I have trouble translating the above to what I'm familiar with in my\n>> > mind, so...\n>> >\n>> > diff2:\n>> >\n>> > 1\n>> > 2\n>> > A\n>> > <<<<<<< l\n>> > B\n>> > =======\n>> > X\n>> > >>>>>>> r\n>> > C\n>> > 5\n>> > 6\n>> >\n>> > diff3:\n>> >\n>> > 1\n>> > 2\n>> > <<<<<<< l\n>> > A\n>> > B\n>> > C\n>> > ||||||| b\n>> > 3\n>> > 4\n>> > =======\n>> > A\n>> > X\n>> > C\n>> > >>>>>>> r\n>> > 5\n>> > 6\n>> >\n>> > I personally don't mind at all having a few extra lines in order to\n>> > visualize what actually happened.\n>> \n>> Plus a good tool should have an option to quickly show a diff between any\n>> 2 of 3 parts, making analysis even simpler.\n>\n> Indeed, it depends on the mergetool, but personally I use vimdiff3\n> (which I authored). All I need are diff3 conflict markers plus some\n> colors.\n\nEmacs's smerge-mode works fine too. In fact that was what made me switch\nto diff3 in git config.\n\nThanks,\n-- Sergey Organov\n"},{"id":"427142","messageId":"60c3ab5e62dbf_8d0f20887@natae.notmuch","threadId":"55890","inReplyTo":"CABPp-BGtYnVPijg2OWoDBM915-PPFXk1O3H=BMf_itc4dNjAxQ@mail.gmail.com","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-11T18:28:46Z","receivedAt":"2021-06-11T18:30:10Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren wrote:\n> On Fri, Jun 11, 2021 at 10:32 AM Felipe Contreras\n> wrote:\n> >\n> > Elijah Newren wrote:\n> > > On Fri, Jun 11, 2021 at 7:25 AM Felipe Contreras \n> > > wrote:\n> >\n> > > > Personally I have never experienced what you posted, so maybe there's\n> > > > something else happening behind the scenes.\n> > > >\n> > > > Maybe merge-ort changed something.\n> > >\n> > > merge-ort made no changes relative to content merges or choice of merge\n> > > bases. (In fact, merge ort doesn't even handle content merges; that's the\n> > > xdiff layer.) Even if merge-ort had made changes in this area, merge-ort\n> > > is not the default and I didn't see the necessary config tweaks in your\n> > > list of config options. (I would have recommended against people using\n> > > merge ort until 7bec8e7fa6 (\"Merge branch 'en/ort-readiness'\", 2021-04-16),\n> > > which only made it into a release last week with 2.32. I probably won't be\n> > > recommending it as the default at least until the optimization work is\n> > > merged and it's hard to predict how many more months that will take.)\n> >\n> > Indeed, I tested on v2.25 and found the same output.\n> >\n> > I thought of merge-ort because 1) I've never seen such kind of output\n> > before, and 2) grepping the code I thought I saw merge-ort being the\n> > default of something, but now I seem to be unable to find where.\n> \n> We have briefly discussed multiple times what things might be needed\n> to eventually make merge-ort the default (though it's not even\n> complete yet; I'm five months into upstreaming the optimization\n> patches with an unknown number of months left). There were also a\n> couple patches to make the _tests_ default to using merge-ort on most\n> platforms, while still keeping one suite that tests merge-recursive to\n> ensure we don't add breakage. Perhaps one of those is what you're\n> thinking of?\n\nNah, I think I just read something wrong.\n\n> > > It's more likely that the codebases you work with just don't have\n> > > criss-cross merges.\n> >\n> > Yes, that's it.\n> >\n> > I don't see why people in these kinds of codebases would like diff3\n> > doing that by default.\n> \n> I suspect they don't[1]. What's the alternative, though? Not using\n> diff3? Picking a different base to avoid the occasional nested\n> conflict in the inner merge region, but which overall has much worse\n> other side effects?\n\nPicking a different conflict style for the inner merge.\n\n> I think Junio was addressing this when he\n> recently said elsewhere in this thread that \"Rejecting diff3 style\n> output because of the way a conflicted part in the inner merge appears\n> as a common ancestor version may be throwing the baby out with the\n> bathwater\"[2]. Sure, it's an annoyance, but diff3 is still a good\n> option and there is no current solution to the annoying nested merge\n> display.\n\nYes, but he proceeded to say we could not recommend diff3 to new users,\nso I see the baby out on the floor.\n\nI don't think it's likely new users will experience a problem with diff3\ninner conflict markers (in 15 years of using git I've never encountered\nthem myself [or I blocked them out my mind]).\n\nEven with this problem, experienced developers seem to be unable to live\nwithout diff3, so I don't know if there's a valid argument against\nmaking it the default.\n\nYes, improving inner conflict markers would make life better for new\nusers, but also everyone else. Software can always be better.\n\nLet's not make perfect be the enemy of the good; diff3 is more than good\nenough already.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427145","messageId":"20210611190235.1970106-1-newren@gmail.com","threadId":"55890","inReplyTo":"60c3a41bd25e3_8d0f2089e@natae.notmuch","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-11T19:02:35Z","receivedAt":"2021-06-11T19:04:25Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"\nOn Fri, Jun 11, 2021 at 10:57 AM Felipe Contreras wrote:\n\n> Elijah Newren wrote:\n> > On Fri, Jun 11, 2021 at 8:32 AM Felipe Contreras \n> > wrote:\n...\n> > The alternative to the above two options was the\n> > make-a-virtual-merge-base-by-merging-merge-bases strategy. It apparently\n> > was very successful.\n> \n> OK. That makes sense.\n> \n> > But it does mean that merge bases can have conflict markers in them.\n> \n> But why? And even if they do, why do they have to be diff3 conflict\n> markers?\n\nThis could be changed; I suspect it just was a natural consequence of how\nthe code was built. (Recursive means there's not a separate code-path for\nmerging the merge-bases, so they get the same merge style by default.)\n\n> This would be more human-friendly:\n> \n> <<<<<<< HEAD\n> D\n> ||||||| merged common ancestors\n> <<<<<<<<< Temporary merge branch 1\n> B\n> =========\n> A\n> >>>>>>>>> Temporary merge branch 2\n> =======\n> C\n> >>>>>>> C\n\nI suspect that would be as easy as this (not compiled or tested):\n\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 095a4d820e..bdd129cbd6 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -131,7 +131,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n \txmp.level = XDL_MERGE_ZEALOUS;\n \txmp.favor = opts->variant;\n \txmp.xpp.flags = opts->xdl_opts;\n-\tif (git_xmerge_style >= 0)\n+\tif (git_xmerge_style >= 0 && !opts->virtual_ancestor)\n \t\txmp.style = git_xmerge_style;\n \tif (marker_size > 0)\n \t\txmp.marker_size = marker_size;\n\n\n> Or just put a stub conflict marker:\n> \n> <<<<<<< HEAD\n> D\n> ||||||| merged common ancestors\n> <<<<<<<<< Temporary merge >>>>>>>>>\n> =======\n> C\n> >>>>>>> C\n\nI don't know what would be involved to do this one; I think it wouldn't\nbe too hard, but I don't think we'd want to pursue this option.\n\n> Or just use the base of the virtual merge:\n> \n> <<<<<<< HEAD\n> D\n> ||||||| merged common ancestors\n> 1\n> =======\n> C\n> >>>>>>> C\n\nI think that implementing this choice would look like this (again, not\ncompiled or tested and I'm not familiar with xdiff so take it with a\nbig grain of salt):\n\n\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 095a4d820e..dbc7f76951 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -130,6 +130,8 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n \tmemset(&xmp, 0, sizeof(xmp));\n \txmp.level = XDL_MERGE_ZEALOUS;\n \txmp.favor = opts->variant;\n+\tif (git_xmerge_style >= 0 && opts->virtual_ancestor)\n+\t\txmp.favor = XDL_MERGE_FAVOR_BASE;\n \txmp.xpp.flags = opts->xdl_opts;\n \tif (git_xmerge_style >= 0)\n \t\txmp.style = git_xmerge_style;\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 8629ae287c..b8d1a536c2 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -62,6 +62,7 @@ extern \"C\" {\n #define XDL_MERGE_FAVOR_OURS 1\n #define XDL_MERGE_FAVOR_THEIRS 2\n #define XDL_MERGE_FAVOR_UNION 3\n+#define XDL_MERGE_FAVOR_BASE 4\n \n /* merge output styles */\n #define XDL_MERGE_DIFF3 1\ndiff --git a/xdiff/xmerge.c b/xdiff/xmerge.c\nindex 95871a0b6e..a8dc42595a 100644\n--- a/xdiff/xmerge.c\n+++ b/xdiff/xmerge.c\n@@ -313,6 +313,9 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,\n \t\t\tif (m->mode & 2)\n \t\t\t\tsize += xdl_recs_copy(xe2, m->i2, m->chg2, 0, 0,\n \t\t\t\t\t\t dest ? dest + size : NULL);\n+\t\t} else if (m->mode == 4) {\n+\t\t\tsize += xdl_orig_copy(xe1, m->i0, m->chg0, needs_cr, 0,\n+\t\t\t\t\t dest ? dest + size : NULL);\n \t\t} else\n \t\t\tcontinue;\n \t\ti = m->i1 + m->chg1;\n\n> We don't have to use diff3 all the way.\n\nRight, thus my mention in the other email to consider adding a\nXDL_MERGE_FAVOR_BASE -- which you then also mention here in your third\noption, and which I've now given at least a partial patch for. Not\nsure if it's a crazy idea or a great idea, since I don't do very many\ncriss-cross merges myself.\n\n\n\nHope that helps,\nElijah\n"},{"id":"427151","messageId":"60c3d0296f869_8d0f208ec@natae.notmuch","threadId":"55890","inReplyTo":"20210611190235.1970106-1-newren@gmail.com","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-11T21:05:45Z","receivedAt":"2021-06-11T21:05:50Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren wrote:\n> On Fri, Jun 11, 2021 at 10:57 AM Felipe Contreras wrote:\n\n> > Or just use the base of the virtual merge:\n> > \n> > <<<<<<< HEAD\n> > D\n> > ||||||| merged common ancestors\n> > 1\n> > =======\n> > C\n> > >>>>>>> C\n> \n> I think that implementing this choice would look like this (again, not\n> compiled or tested and I'm not familiar with xdiff so take it with a\n> big grain of salt):\n> \n> \n> diff --git a/ll-merge.c b/ll-merge.c\n> index 095a4d820e..dbc7f76951 100644\n> --- a/ll-merge.c\n> +++ b/ll-merge.c\n> @@ -130,6 +130,8 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n> \tmemset(&xmp, 0, sizeof(xmp));\n> \txmp.level = XDL_MERGE_ZEALOUS;\n> \txmp.favor = opts->variant;\n> +\tif (git_xmerge_style >= 0 && opts->virtual_ancestor)\n> +\t\txmp.favor = XDL_MERGE_FAVOR_BASE;\n\nThe only time git_xmerge_style isn't >= 0 is when no merge style has\nbeen configured by the user.\n\nI don't see why this:\n\n git -c merge.conflictstyle=merge merge\n\nShould have a different behavior than this:\n\n git merge\n\nIn fact, I don't see why any style should change that desired behavior.\nIf you said there's issues with the \"merge\" style too, perhaps the above\nwill help for those cases too.\n\n> > We don't have to use diff3 all the way.\n> \n> Right, thus my mention in the other email to consider adding a\n> XDL_MERGE_FAVOR_BASE -- which you then also mention here in your third\n> option, and which I've now given at least a partial patch for. Not\n> sure if it's a crazy idea or a great idea, since I don't do very many\n> criss-cross merges myself.\n\nI thought you meant as a separate configurable flag, not something done\nby default.\n\nNow that I understand what you meant I think it could be a great idea.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427153","messageId":"CABPp-BGhaSJmqXFKpCVcUKexNXaa8sLEms-6q-qno7uETzGQ=A@mail.gmail.com","threadId":"55890","inReplyTo":"60c3d0296f869_8d0f208ec@natae.notmuch","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-11T21:40:32Z","receivedAt":"2021-06-11T21:42:01Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jun 11, 2021 at 2:05 PM Felipe Contreras\n wrote:\n>\n> Elijah Newren wrote:\n> > On Fri, Jun 11, 2021 at 10:57 AM Felipe Contreras wrote:\n>\n> > > Or just use the base of the virtual merge:\n> > >\n> > > <<<<<<< HEAD\n> > > D\n> > > ||||||| merged common ancestors\n> > > 1\n> > > =======\n> > > C\n> > > >>>>>>> C\n> >\n> > I think that implementing this choice would look like this (again, not\n> > compiled or tested and I'm not familiar with xdiff so take it with a\n> > big grain of salt):\n> >\n> >\n> > diff --git a/ll-merge.c b/ll-merge.c\n> > index 095a4d820e..dbc7f76951 100644\n> > --- a/ll-merge.c\n> > +++ b/ll-merge.c\n> > @@ -130,6 +130,8 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n> > memset(&xmp, 0, sizeof(xmp));\n> > xmp.level = XDL_MERGE_ZEALOUS;\n> > xmp.favor = opts->variant;\n> > + if (git_xmerge_style >= 0 && opts->virtual_ancestor)\n> > + xmp.favor = XDL_MERGE_FAVOR_BASE;\n>\n> The only time git_xmerge_style isn't >= 0 is when no merge style has\n> been configured by the user.\n\nYep, probably should have just been\n\n+ if (opts->virtual_ancestor)\n+ xmp.favor = XDL_MERGE_FAVOR_BASE;\n\nThough the difference doesn't matter a lot. Since\nmerge.conflictStyle=merge (which is the current default) doesn't\ndisplay the contents from the merge base in a three-way content merge,\nsetting xmp.favor to XDL_MERGE_FAVOR_BASE vs. leaving it as 0 for the\nrecursive/intermediate merges won't generally end up affecting the end\nresult. It'd only matter for diff3 and zdiff3 users.\n\n\nGoing on a slight tangent, I think there's actually a related bug\nhere. We probably should not honor XDL_MERGE_FAVOR_{OURS,THEIRS} when\nopts->virtual_ancestor is true; that's just asking for trouble. I\nthink it'd paradoxically result in reversing the desired behavior\n(e.g. users would see what they'd consider XDL_MERGE_FAVOR_THEIRS\nbehavior when they asked for XDL_MERGE_FAVOR_OURS) in some cases as a\nresult.\n\n> In fact, I don't see why any style should change that desired behavior.\n> If you said there's issues with the \"merge\" style too, perhaps the above\n> will help for those cases too.\n>\n> > > We don't have to use diff3 all the way.\n> >\n> > Right, thus my mention in the other email to consider adding a\n> > XDL_MERGE_FAVOR_BASE -- which you then also mention here in your third\n> > option, and which I've now given at least a partial patch for. Not\n> > sure if it's a crazy idea or a great idea, since I don't do very many\n> > criss-cross merges myself.\n>\n> I thought you meant as a separate configurable flag, not something done\n> by default.\n>\n> Now that I understand what you meant I think it could be a great idea.\n\nIf someone that does lots of criss-cross merges can comment on the\nidea, and agree that it's worth a shot, I can try to turn it into real\npatches.\n\n(I might even try to investigate the zdiff3 stuff too, which sounds\nlike something I've wanted many times...but I'd really rather\nconcentrate on merge-ort until its upstreaming is finished.)\n"},{"id":"427235","messageId":"60c6175ca6840_41f2b208fc@natae.notmuch","threadId":"55890","inReplyTo":"CABPp-BGhaSJmqXFKpCVcUKexNXaa8sLEms-6q-qno7uETzGQ=A@mail.gmail.com","subject":"Re: [PATCH 7/7] xdiff: make diff3 the default conflictStyle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-13T14:34:04Z","receivedAt":"2021-06-13T14:36:27Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Elijah Newren wrote:\n> On Fri, Jun 11, 2021 at 2:05 PM Felipe Contreras\n> wrote:\n> >\n> > Elijah Newren wrote:\n> > > On Fri, Jun 11, 2021 at 10:57 AM Felipe Contreras wrote:\n> >\n> > > > Or just use the base of the virtual merge:\n> > > >\n> > > > <<<<<<< HEAD\n> > > > D\n> > > > ||||||| merged common ancestors\n> > > > 1\n> > > > =======\n> > > > C\n> > > > >>>>>>> C\n> > >\n> > > I think that implementing this choice would look like this (again, not\n> > > compiled or tested and I'm not familiar with xdiff so take it with a\n> > > big grain of salt):\n> > >\n> > >\n> > > diff --git a/ll-merge.c b/ll-merge.c\n> > > index 095a4d820e..dbc7f76951 100644\n> > > --- a/ll-merge.c\n> > > +++ b/ll-merge.c\n> > > @@ -130,6 +130,8 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,\n> > > memset(&xmp, 0, sizeof(xmp));\n> > > xmp.level = XDL_MERGE_ZEALOUS;\n> > > xmp.favor = opts->variant;\n> > > + if (git_xmerge_style >= 0 && opts->virtual_ancestor)\n> > > + xmp.favor = XDL_MERGE_FAVOR_BASE;\n> >\n> > The only time git_xmerge_style isn't >= 0 is when no merge style has\n> > been configured by the user.\n> \n> Yep, probably should have just been\n> \n> + if (opts->virtual_ancestor)\n> + xmp.favor = XDL_MERGE_FAVOR_BASE;\n> \n> Though the difference doesn't matter a lot. Since\n> merge.conflictStyle=merge (which is the current default) doesn't\n> display the contents from the merge base in a three-way content merge,\n> setting xmp.favor to XDL_MERGE_FAVOR_BASE vs. leaving it as 0 for the\n> recursive/intermediate merges won't generally end up affecting the end\n> result. It'd only matter for diff3 and zdiff3 users.\n\nOK, so:\n\n if (git_xmerge_style > 0 && opts->virtual_ancestor)\n\n> Going on a slight tangent, I think there's actually a related bug\n> here. We probably should not honor XDL_MERGE_FAVOR_{OURS,THEIRS} when\n> opts->virtual_ancestor is true; that's just asking for trouble. I\n> think it'd paradoxically result in reversing the desired behavior\n> (e.g. users would see what they'd consider XDL_MERGE_FAVOR_THEIRS\n> behavior when they asked for XDL_MERGE_FAVOR_OURS) in some cases as a\n> result.\n\nMaybe write a test-case and find out?\n\n> > In fact, I don't see why any style should change that desired behavior.\n> > If you said there's issues with the \"merge\" style too, perhaps the above\n> > will help for those cases too.\n> >\n> > > > We don't have to use diff3 all the way.\n> > >\n> > > Right, thus my mention in the other email to consider adding a\n> > > XDL_MERGE_FAVOR_BASE -- which you then also mention here in your third\n> > > option, and which I've now given at least a partial patch for. Not\n> > > sure if it's a crazy idea or a great idea, since I don't do very many\n> > > criss-cross merges myself.\n> >\n> > I thought you meant as a separate configurable flag, not something done\n> > by default.\n> >\n> > Now that I understand what you meant I think it could be a great idea.\n> \n> If someone that does lots of criss-cross merges can comment on the\n> idea, and agree that it's worth a shot, I can try to turn it into real\n> patches.\n\nI would flip the conditional: if nobody that does lots of criss-crosses\nobjects...\n\nIf we waited for a tiny minority of users to speak up before doing\nsomething we migth wait forever.\n\n> (I might even try to investigate the zdiff3 stuff too, which sounds\n> like something I've wanted many times...but I'd really rather\n> concentrate on merge-ort until its upstreaming is finished.)\n\nWell, I just re-sent the patch:\n\nhttps://lore.kernel.org/git/20210613143155.836591-1-felipe.contreras@gmail.com/\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"427781","messageId":"5c27a8ac-f6b1-3551-0af3-4e242437005d@gmail.com","threadId":"55890","inReplyTo":"20210609192842.696646-1-felipe.contreras@gmail.com","subject":"Re: [PATCH 0/7] Make diff3 the default conflict style","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2021-06-17T17:40:14Z","receivedAt":"2021-06-17T17:40:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 09/06/2021 20:28, Felipe Contreras wrote:\n> This patch series turned out much more complicated that simply flipping\n> the switch and dealing with the consequences. Apparently some commands\n> are completely ignoring the configuration (notes and merge-tree), and\n> others are handling it wrong (checkout).\n> \n> So in preparation I created a new test to make sure these rowdy\n> commands handle the configuration correctly, and then step by step I fix\n> them.\n> \n> Once all the commands are fixed I proceed to cleanup xdiff-interface in\n> preparation for the switch.\n> \n> And finally once the switch is flipped the documnetation is updated, and\n> funch of test scripts receive a temporary configuration that returns\n> them to the old \"merge\" (diff2) behavior so they pass with minimum\n> changes.\n> \n> I have already written patches to update the tests so no configuration\n> is needed and they parse the diff3 style directly, but the series is\n> already quite verbose as it is.\n> \n> One salient thorn is that from my point of view merge_recursive_config()\n> is implemtend wrongly and thus can't be called as other configuration\n> functions, like git_diff_basic_config(). It seems there's a huge area of\n> opportunity there to clean all that up, but that's for another series.\n\nRegrettably I shall not be commenting further on these or any future \npatches from you. I do not feel it would be a productive use of my time \nas you have not entered into the kind of constructive discussion that is \nthe expected norm on this list.\n\nBest Wishes\n\nPhillip\n\n> Felipe Contreras (7):\n> test: add merge style config test\n> merge-tree: fix merge.conflictstyle handling\n> notes: fix merge.conflictstyle handling\n> checkout: fix merge.conflictstyle handling\n> xdiff: rename XDL_MERGE_STYLE_DIFF3\n> xdiff: simplify style assignments\n> xdiff: make diff3 the default conflictStyle\n> \n> Documentation/config/merge.txt | 12 +--\n> Documentation/git-merge-file.txt | 2 +\n> Documentation/git-merge.txt | 28 ++----\n> Documentation/git-rerere.txt | 2 +-\n> Documentation/gitattributes.txt | 6 +-\n> Documentation/technical/rerere.txt | 3 +-\n> Documentation/user-manual.txt | 6 +-\n> builtin/merge-file.c | 5 +-\n> builtin/merge-recursive.c | 3 +\n> builtin/merge-tree.c | 4 +\n> builtin/merge.c | 4 +\n> builtin/notes.c | 3 +-\n> ll-merge.c | 3 +-\n> merge-recursive.c | 2 +-\n> sequencer.c | 5 +\n> t/t2023-checkout-m.sh | 2 +\n> t/t3310-notes-merge-manual-resolve.sh | 2 +\n> t/t3311-notes-merge-fanout.sh | 2 +\n> t/t3404-rebase-interactive.sh | 2 +\n> t/t3507-cherry-pick-conflict.sh | 2 +\n> t/t4017-diff-retval.sh | 2 +\n> t/t4048-diff-combined-binary.sh | 2 +\n> t/t4200-rerere.sh | 2 +\n> t/t4300-merge-tree.sh | 2 +\n> t/t6402-merge-rename.sh | 2 +\n> t/t6403-merge-file.sh | 2 +\n> t/t6404-recursive-merge.sh | 2 +\n> t/t6416-recursive-corner-cases.sh | 2 +\n> t/t6417-merge-ours-theirs.sh | 2 +\n> t/t6418-merge-text-auto.sh | 2 +\n> t/t6422-merge-rename-corner-cases.sh | 2 +\n> t/t6423-merge-rename-directories.sh | 1 +\n> t/t6428-merge-conflicts-sparse.sh | 1 +\n> t/t6432-merge-recursive-space-options.sh | 2 +\n> t/t6440-config-conflict-markers.sh | 123 +++++++++++++++++++++++\n> t/t7201-co.sh | 2 +\n> t/t7506-status-submodule.sh | 1 +\n> xdiff-interface.c | 6 +-\n> xdiff/xdiff.h | 3 +-\n> xdiff/xmerge.c | 4 +-\n> 40 files changed, 217 insertions(+), 46 deletions(-)\n> create mode 100755 t/t6440-config-conflict-markers.sh\n> \n\n"},{"id":"427783","messageId":"60cb9347e9d2e_150282085c@natae.notmuch","threadId":"55890","inReplyTo":"5c27a8ac-f6b1-3551-0af3-4e242437005d@gmail.com","subject":"Re: [PATCH 0/7] Make diff3 the default conflict style","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-06-17T18:24:07Z","receivedAt":"2021-06-17T18:24:53Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Phillip Wood wrote:\n> Regrettably I shall not be commenting further on these or any future \n> patches from you. I do not feel it would be a productive use of my time \n> as you have not entered into the kind of constructive discussion that is \n> the expected norm on this list.\n\nPlease let me know where exactly I have ignored your feedback or engaged\nin any kind of unconstructive discussion with you.\n\nWithout any actual proof I don't think the above is an accurate\nassessment.\n\nAlso, in my opinion it's bad manners to say \"I don't like you and I'm\ngoing to mute you\". Just mute.\n\nCheers.\n\n-- \nFelipe Contreras\n"}]}