| Message ID | 20210609192842.696646-8-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make diff3 the default conflict style | expand |
Am 09.06.21 um 21:28 schrieb Felipe Contreras: > Virtually everyone is using it, and it's one of the first things we > teach newcomers in order to resolve conflicts efficiently. > > Let's make it the default. I tested diff3 style the VERY FIRST TIME the other day and was greated with the below. Needless to say that this change is a no-go from my POV. Without diff3: <<<<<<< HEAD CClustering ComputeSSLClusters(double threshPercent, const CDataInfo* scale) const override; void ComputeDist(DistFunc distFunc, CDoubleArray& dist, double& minDist, double& maxDist) const; double EstimateNodeDist2() const override; std::vector<double> EstimateNeighborMinDist() const override; ======= CClustering ComputeSSLClusters(double threshPercent, const CDoubleArray& compWeights, const CDataInfo* scale) const override; static void ComputeDist(const CNetNodeHolder& vecs, CDoubleArray& dist, double& minDist, double& maxDist); >>>>>>> no-compweights-in-cnet With diff3: <<<<<<< HEAD CClustering ComputeSSLClusters(double threshPercent, const CDataInfo* scale) const override; void ComputeDist(DistFunc distFunc, CDoubleArray& dist, double& minDist, double& maxDist) const; double EstimateNodeDist2() const override; std::vector<double> EstimateNeighborMinDist() const override; ||||||| merged common ancestors <<<<<<<<< Temporary merge branch 1 CClustering ComputeSSLClusters(double threshPercent, const CDataInfo* scale) const override; void ComputeDist(DistFunc distFunc, CDoubleArray& dist, double& minDist, double& maxDist) const; virtual void ComputeKNearest(int K, const double*, Neighborhood& result) const; ||||||||| d261d9944 CClustering ComputeClusters(const double* dist, double threshold, const CDataInfo* scale) const override; virtual void ComputeDist(DistFunc distFunc, CDoubleArray& dist, double& minDist, double& maxDist); virtual void ComputeUMatrix(); virtual void ComputeKNearest(int K, const double*, Neighborhood& result) const; ========= CClustering ComputeClusters(const double* dist, double threshold, const CDataInfo* scale) const override; virtual void ComputeDist(DistFunc distFunc, CDoubleArray& dist, double& minDist, double& maxDist); virtual void ComputeUMatrix(); ======= CClustering ComputeSSLClusters(double threshPercent, const CDoubleArray& compWeights, const CDataInfo* scale) const override; static void ComputeDist(const CNetNodeHolder& vecs, CDoubleArray& dist, double& minDist, double& maxDist); >>>>>>> no-compweights-in-cnet -- Hannes
On 2021-06-10 08:41:21+0200, Johannes Sixt <j6t@kdbg.org> wrote: > Am 09.06.21 um 21:28 schrieb Felipe Contreras: > > Virtually everyone is using it, and it's one of the first things we > > teach newcomers in order to resolve conflicts efficiently. > > > > Let's make it the default. > > I tested diff3 style the VERY FIRST TIME the other day and was greated > with the below. Needless to say that this change is a no-go from my POV. I agree, despite using 3-way merging (with external tools) to resolve conflicts. I prefer the current conflict style, aka no-diff3 conflict style. -- Danh > > Without diff3: > > <<<<<<< HEAD > CClustering ComputeSSLClusters(double threshPercent, const CDataInfo* scale) const override; > void ComputeDist(DistFunc distFunc, CDoubleArray& dist, > double& minDist, double& maxDist) const; > double EstimateNodeDist2() const override; > std::vector<double> EstimateNeighborMinDist() const override; > ======= > CClustering ComputeSSLClusters(double threshPercent, > const CDoubleArray& compWeights, const CDataInfo* scale) const override; > static void ComputeDist(const CNetNodeHolder& vecs, CDoubleArray& dist, > double& minDist, double& maxDist); > >>>>>>> no-compweights-in-cnet > > With diff3: > > <<<<<<< HEAD > CClustering ComputeSSLClusters(double threshPercent, const CDataInfo* scale) const override; > void ComputeDist(DistFunc distFunc, CDoubleArray& dist, > double& minDist, double& maxDist) const; > double EstimateNodeDist2() const override; > std::vector<double> EstimateNeighborMinDist() const override; > ||||||| merged common ancestors > <<<<<<<<< Temporary merge branch 1 > CClustering ComputeSSLClusters(double threshPercent, const CDataInfo* scale) const override; > void ComputeDist(DistFunc distFunc, CDoubleArray& dist, > double& minDist, double& maxDist) const; > virtual void ComputeKNearest(int K, const double*, > Neighborhood& result) const; > ||||||||| d261d9944 > CClustering ComputeClusters(const double* dist, double threshold, > const CDataInfo* scale) const override; > virtual void ComputeDist(DistFunc distFunc, CDoubleArray& dist, > double& minDist, double& maxDist); > virtual void ComputeUMatrix(); > virtual void ComputeKNearest(int K, const double*, > Neighborhood& result) const; > ========= > CClustering ComputeClusters(const double* dist, double threshold, > const CDataInfo* scale) const override; > virtual void ComputeDist(DistFunc distFunc, CDoubleArray& dist, > double& minDist, double& maxDist); > virtual void ComputeUMatrix(); > ======= > CClustering ComputeSSLClusters(double threshPercent, > const CDoubleArray& compWeights, const CDataInfo* scale) const override; > static void ComputeDist(const CNetNodeHolder& vecs, CDoubleArray& dist, > double& minDist, double& maxDist); > >>>>>>> no-compweights-in-cnet > > -- Hannes
On 09/06/2021 20:28, Felipe Contreras wrote: > Virtually everyone is using it, and it's one of the first things we > teach newcomers in order to resolve conflicts efficiently. Given there are millions of users I'm not sure how you established that virtually everyone is using it. I think that while this change would be useful to some users (though not many if virtually everyone already has it set) it has the potential to annoy a lot of users who are happy with the existing default. I do not think that it is a positive change over all. Had the default been diff3 from early on in git's history then I would not advocate changing it to the current default but I think the time has passed when it can be changed without inconveniencing existing users. The patches up to this point have useful fixes in them which would improve git, thanks for working on them. Best Wishes Phillip > Let's make it the default. > > This generates a ton of changes in the tests. Although we probably will > want to update them to use th new default, override the configuration so > we use the old one for now. > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > Documentation/config/merge.txt | 12 +++++----- > Documentation/git-merge-file.txt | 2 ++ > Documentation/git-merge.txt | 28 +++++++----------------- > Documentation/git-rerere.txt | 2 +- > Documentation/gitattributes.txt | 6 ++--- > Documentation/technical/rerere.txt | 3 +-- > Documentation/user-manual.txt | 6 ++++- > t/t2023-checkout-m.sh | 2 ++ > t/t3310-notes-merge-manual-resolve.sh | 2 ++ > t/t3311-notes-merge-fanout.sh | 2 ++ > t/t3404-rebase-interactive.sh | 2 ++ > t/t3507-cherry-pick-conflict.sh | 2 ++ > t/t4017-diff-retval.sh | 2 ++ > t/t4048-diff-combined-binary.sh | 2 ++ > t/t4200-rerere.sh | 2 ++ > t/t4300-merge-tree.sh | 2 ++ > t/t6402-merge-rename.sh | 2 ++ > t/t6403-merge-file.sh | 2 ++ > t/t6404-recursive-merge.sh | 2 ++ > t/t6416-recursive-corner-cases.sh | 2 ++ > t/t6417-merge-ours-theirs.sh | 2 ++ > t/t6418-merge-text-auto.sh | 2 ++ > t/t6422-merge-rename-corner-cases.sh | 2 ++ > t/t6423-merge-rename-directories.sh | 1 + > t/t6428-merge-conflicts-sparse.sh | 1 + > t/t6432-merge-recursive-space-options.sh | 2 ++ > t/t6440-config-conflict-markers.sh | 8 +++---- > t/t7201-co.sh | 2 ++ > t/t7506-status-submodule.sh | 1 + > xdiff-interface.c | 2 +- > 30 files changed, 70 insertions(+), 38 deletions(-) > > diff --git a/Documentation/config/merge.txt b/Documentation/config/merge.txt > index cb2ed58907..2dba937dd0 100644 > --- a/Documentation/config/merge.txt > +++ b/Documentation/config/merge.txt > @@ -1,10 +1,10 @@ > merge.conflictStyle:: > - Specify the style in which conflicted hunks are written out to > - working tree files upon merge. The default is "merge", which > - shows a `<<<<<<<` conflict marker, changes made by one side, > - a `=======` marker, changes made by the other side, and then > - a `>>>>>>>` marker. An alternate style, "diff3", adds a `|||||||` > - marker and the original text before the `=======` marker. > + Specify the style in which conflicted hunks are written out to working > + tree files upon merge. The default is "diff3", which shows a `<<<<<<<` > + conflict marker, changes made by one side, a `|||||||` marker, the > + original text, a `=======` marker, changes made by the other side, and > + then a `>>>>>>>` marker. A simpler mode "merge" omits the `|||||||` > + marker and the original text. > > merge.defaultToUpstream:: > If merge is called without any commit argument, merge the upstream > diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt > index f856032613..7d8e74c872 100644 > --- a/Documentation/git-merge-file.txt > +++ b/Documentation/git-merge-file.txt > @@ -30,6 +30,8 @@ normally outputs a warning and brackets the conflict with lines containing > > <<<<<<< A > lines in file A > + ||||||| > + lines in merge base > ======= > lines in file B > >>>>>>> B > diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt > index 3819fadac1..14dadf2e16 100644 > --- a/Documentation/git-merge.txt > +++ b/Documentation/git-merge.txt > @@ -233,7 +233,7 @@ final result verbatim. When both sides made changes to the same area, > however, Git cannot randomly pick one side over the other, and asks you to > resolve it by leaving what both sides did to that area. > > -By default, Git uses the same style as the one used by the "merge" program > +By default, Git uses a similar style to the one used by the "merge" program > from the RCS suite to present such a conflicted hunk, like this: > > ------------ > @@ -242,6 +242,8 @@ ancestor, or cleanly resolved because only one side changed. > <<<<<<< yours:sample.txt > Conflict resolution is hard; > let's go shopping. > +||||||| > +Originally there's no conflict. > ======= > Git makes conflict resolution easy. > >>>>>>> theirs:sample.txt > @@ -249,17 +251,12 @@ And here is another line that is cleanly resolved or unmodified. > ------------ > > The area where a pair of conflicting changes happened is marked with markers > -`<<<<<<<`, `=======`, and `>>>>>>>`. The part before the `=======` > -is typically your side, and the part afterwards is typically their side. > - > -The default format does not show what the original said in the conflicting > -area. You cannot tell how many lines are deleted and replaced with > -Barbie's remark on your side. The only thing you can tell is that your > -side wants to say it is hard and you'd prefer to go shopping, while the > -other side wants to claim it is easy. > +`<<<<<<<`, `=======`, and `>>>>>>>`. The part before the `|||||||` > +is typically your side, and the part after `=======` is typically their side. > +In-between is the original code. > > -An alternative style can be used by setting the "merge.conflictStyle" > -configuration variable to "diff3". In "diff3" style, the above conflict > +An more basic style can be used by setting the "merge.conflictStyle" > +configuration variable to "merge". In "merge" style, the above conflict > may look like this: > > ------------ > @@ -268,21 +265,12 @@ ancestor, or cleanly resolved because only one side changed. > <<<<<<< yours:sample.txt > Conflict resolution is hard; > let's go shopping. > -||||||| > -Conflict resolution is hard. > ======= > Git makes conflict resolution easy. > >>>>>>> theirs:sample.txt > And here is another line that is cleanly resolved or unmodified. > ------------ > > -In addition to the `<<<<<<<`, `=======`, and `>>>>>>>` markers, it uses > -another `|||||||` marker that is followed by the original text. You can > -tell that the original just stated a fact, and your side simply gave in to > -that statement and gave up, while the other side tried to have a more > -positive attitude. You can sometimes come up with a better resolution by > -viewing the original. > - > > HOW TO RESOLVE CONFLICTS > ------------------------ > diff --git a/Documentation/git-rerere.txt b/Documentation/git-rerere.txt > index 4cfc883378..89b0820995 100644 > --- a/Documentation/git-rerere.txt > +++ b/Documentation/git-rerere.txt > @@ -159,7 +159,7 @@ resolve. > > Running the 'git rerere' command immediately after a conflicted > automerge records the conflicted working tree files, with the > -usual conflict markers `<<<<<<<`, `=======`, and `>>>>>>>` in > +usual conflict markers `<<<<<<<`, `|||||||`, `=======`, and `>>>>>>>` in > them. Later, after you are done resolving the conflicts, > running 'git rerere' again will record the resolved state of these > files. Suppose you did this when you created the test merge of > diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt > index 83fd4e19a4..b767215ac2 100644 > --- a/Documentation/gitattributes.txt > +++ b/Documentation/gitattributes.txt > @@ -1042,10 +1042,10 @@ text:: > > Usual 3-way file level merge for text files. Conflicted > regions are marked with conflict markers `<<<<<<<`, > - `=======` and `>>>>>>>`. The version from your branch > - appears before the `=======` marker, and the version > + `|||||||`, `=======` and `>>>>>>>`. The version from your branch > + appears before the `|||||||` marker, and the version > from the merged branch appears after the `=======` > - marker. > + marker. In-between is the original. > > binary:: > > diff --git a/Documentation/technical/rerere.txt b/Documentation/technical/rerere.txt > index af5f9fc24f..38b44f4430 100644 > --- a/Documentation/technical/rerere.txt > +++ b/Documentation/technical/rerere.txt > @@ -42,8 +42,7 @@ get a conflict like the following: > >>>>>>> AC > > Doing the analogous with AC2 (forking a branch ABAC2 off of branch AB > -and then merging branch AC2 into it), using the diff3 conflict style, > -we get a conflict like the following: > +and then merging branch AC2 into it), we get a conflict like the following: > > <<<<<<< HEAD > B > diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt > index f9e54b8674..3ddde87482 100644 > --- a/Documentation/user-manual.txt > +++ b/Documentation/user-manual.txt > @@ -1243,6 +1243,8 @@ files with conflicts will have conflict markers added, like this: > ------------------------------------------------- > <<<<<<< HEAD:file.txt > Hello world > +||||||| > +Original > ======= > Goodbye > >>>>>>> 77976da35a11db4580b80ae27e8d65caf5208086:file.txt > @@ -1276,9 +1278,11 @@ diff --cc file.txt > index 802992c,2b60207..0000000 > --- a/file.txt > +++ b/file.txt > -@@@ -1,1 -1,1 +1,5 @@@ > +@@@ -1,1 -1,1 +1,7 @@@ > ++<<<<<<< HEAD:file.txt > +Hello world > +++||||||| > +++Original > ++======= > + Goodbye > ++>>>>>>> 77976da35a11db4580b80ae27e8d65caf5208086:file.txt > diff --git a/t/t2023-checkout-m.sh b/t/t2023-checkout-m.sh > index 7b327b7544..219c82532a 100755 > --- a/t/t2023-checkout-m.sh > +++ b/t/t2023-checkout-m.sh > @@ -9,6 +9,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > . ./test-lib.sh > > +git config merge.conflictstyle merge # TODO: use the default > + > test_expect_success setup ' > test_tick && > test_commit both.txt both.txt initial && > diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh > index d3d72e25fe..cbd5d8302e 100755 > --- a/t/t3310-notes-merge-manual-resolve.sh > +++ b/t/t3310-notes-merge-manual-resolve.sh > @@ -7,6 +7,8 @@ test_description='Test notes merging with manual conflict resolution' > > . ./test-lib.sh > > +git config --global merge.conflictstyle merge # TODO: use the default > + > # Set up a notes merge scenario with different kinds of conflicts > test_expect_success 'setup commits' ' > test_commit 1st && > diff --git a/t/t3311-notes-merge-fanout.sh b/t/t3311-notes-merge-fanout.sh > index 5b675417e9..4aeaa05c15 100755 > --- a/t/t3311-notes-merge-fanout.sh > +++ b/t/t3311-notes-merge-fanout.sh > @@ -7,6 +7,8 @@ test_description='Test notes merging at various fanout levels' > > . ./test-lib.sh > > +git config merge.conflictstyle merge # TODO: use the default > + > verify_notes () { > notes_ref="$1" > commit="$2" > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 66bcbbf952..769079a71c 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -32,6 +32,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > . "$TEST_DIRECTORY"/lib-rebase.sh > > +git config merge.conflictstyle merge # TODO: use the default > + > test_expect_success 'setup' ' > git switch -C primary && > test_commit A file1 && > diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh > index 014001b8f3..647a40f314 100755 > --- a/t/t3507-cherry-pick-conflict.sh > +++ b/t/t3507-cherry-pick-conflict.sh > @@ -281,6 +281,7 @@ test_expect_success \ > > test_expect_success 'failed cherry-pick describes conflict in work tree' ' > pristine_detach initial && > + git config merge.conflictstyle merge && # TODO: use the default > cat <<-EOF >expected && > <<<<<<< HEAD > a > @@ -316,6 +317,7 @@ test_expect_success 'diff3 -m style' ' > > test_expect_success 'revert also handles conflicts sanely' ' > git config --unset merge.conflictstyle && > + git config merge.conflictstyle merge && # TODO: use the default > pristine_detach initial && > cat <<-EOF >expected && > <<<<<<< HEAD > diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh > index ed461f481e..04b77af2a4 100755 > --- a/t/t4017-diff-retval.sh > +++ b/t/t4017-diff-retval.sh > @@ -7,6 +7,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > . ./test-lib.sh > > +git config merge.conflictstyle merge # TODO: use the default > + > test_expect_success 'setup' ' > echo "1 " >a && > git add . && > diff --git a/t/t4048-diff-combined-binary.sh b/t/t4048-diff-combined-binary.sh > index 0260cf64f5..49a56731dd 100755 > --- a/t/t4048-diff-combined-binary.sh > +++ b/t/t4048-diff-combined-binary.sh > @@ -6,6 +6,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > . ./test-lib.sh > > +git config merge.conflictstyle merge # TODO: use the default > + > test_expect_success 'setup binary merge conflict' ' > echo oneQ1 | q_to_nul >binary && > git add binary && > diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh > index 9f8c76dffb..e9ae3d6fde 100755 > --- a/t/t4200-rerere.sh > +++ b/t/t4200-rerere.sh > @@ -27,6 +27,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > . ./test-lib.sh > > +git config merge.conflictstyle merge # TODO: use the default > + > test_expect_success 'setup' ' > cat >a1 <<-\EOF && > Some title > diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh > index e59601e5fe..f21ccaf0a6 100755 > --- a/t/t4300-merge-tree.sh > +++ b/t/t4300-merge-tree.sh > @@ -6,6 +6,8 @@ > test_description='git merge-tree' > . ./test-lib.sh > > +git config merge.conflictstyle merge # TODO: use the default > + > test_expect_success setup ' > test_commit "initial" "initial-file" "initial" > ' > diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh > index 425dad97d5..4f01bbc451 100755 > --- a/t/t6402-merge-rename.sh > +++ b/t/t6402-merge-rename.sh > @@ -270,6 +270,7 @@ test_expect_success 'setup for rename + d/f conflicts' ' > git checkout --orphan dir-in-way && > git rm -rf . && > git clean -fdqx && > + git config merge.conflictstyle merge && # TODO: use the default > > mkdir sub && > mkdir dir && > @@ -871,6 +872,7 @@ test_expect_success 'setup for use of extended merge markers' ' > git clean -fdqx && > rm -rf .git && > git init && > + git config merge.conflictstyle merge && # TODO: use the default > > printf "1\n2\n3\n4\n5\n6\n7\n8\n" >original_file && > git add original_file && > diff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh > index 2f421d967a..1428dfb5c6 100755 > --- a/t/t6403-merge-file.sh > +++ b/t/t6403-merge-file.sh > @@ -3,6 +3,8 @@ > test_description='RCS merge replacement: merge-file' > . ./test-lib.sh > > +git config merge.conflictstyle merge # TODO: use the default > + > test_expect_success 'setup' ' > cat >orig.txt <<-\EOF && > Dominus regit me, > diff --git a/t/t6404-recursive-merge.sh b/t/t6404-recursive-merge.sh > index eaf48e941e..a3354b8f9a 100755 > --- a/t/t6404-recursive-merge.sh > +++ b/t/t6404-recursive-merge.sh > @@ -6,6 +6,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > . ./test-lib.sh > > +git config merge.conflictstyle merge # TODO: use the default > + > # This scenario is based on a real-world repository of Shawn Pearce. > > # 1 - A - D - F > diff --git a/t/t6416-recursive-corner-cases.sh b/t/t6416-recursive-corner-cases.sh > index 84f5082366..ac4e69a325 100755 > --- a/t/t6416-recursive-corner-cases.sh > +++ b/t/t6416-recursive-corner-cases.sh > @@ -8,6 +8,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > . ./test-lib.sh > . "$TEST_DIRECTORY"/lib-merge.sh > > +git config --global merge.conflictstyle merge # TODO: use the default > + > # > # L1 L2 > # o---o > diff --git a/t/t6417-merge-ours-theirs.sh b/t/t6417-merge-ours-theirs.sh > index ac9aee9a66..b8208a383b 100755 > --- a/t/t6417-merge-ours-theirs.sh > +++ b/t/t6417-merge-ours-theirs.sh > @@ -6,6 +6,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > . ./test-lib.sh > > +git config merge.conflictstyle merge # TODO: use the default > + > test_expect_success setup ' > for i in 1 2 3 4 5 6 7 8 9 > do > diff --git a/t/t6418-merge-text-auto.sh b/t/t6418-merge-text-auto.sh > index 1e0296dd17..e18f67776c 100755 > --- a/t/t6418-merge-text-auto.sh > +++ b/t/t6418-merge-text-auto.sh > @@ -17,6 +17,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > . ./test-lib.sh > > +git config merge.conflictstyle merge # TODO: use the default > + > test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b > > compare_files () { > diff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh > index bf4ce3c63d..6bb4b6d968 100755 > --- a/t/t6422-merge-rename-corner-cases.sh > +++ b/t/t6422-merge-rename-corner-cases.sh > @@ -9,6 +9,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > . ./test-lib.sh > . "$TEST_DIRECTORY"/lib-merge.sh > > +git config --global merge.conflictstyle merge # TODO: use the default > + > test_setup_rename_delete_untracked () { > test_create_repo rename-delete-untracked && > ( > diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh > index 7134769149..5f6cacd064 100755 > --- a/t/t6423-merge-rename-directories.sh > +++ b/t/t6423-merge-rename-directories.sh > @@ -28,6 +28,7 @@ test_description="recursive merge with directory renames" > . ./test-lib.sh > . "$TEST_DIRECTORY"/lib-merge.sh > > +git config --global merge.conflictstyle merge # TODO: use the default > > ########################################################################### > # SECTION 1: Basic cases we should be able to handle > diff --git a/t/t6428-merge-conflicts-sparse.sh b/t/t6428-merge-conflicts-sparse.sh > index 7e8bf497f8..18975801db 100755 > --- a/t/t6428-merge-conflicts-sparse.sh > +++ b/t/t6428-merge-conflicts-sparse.sh > @@ -25,6 +25,7 @@ test_description="merge cases" > . ./test-lib.sh > . "$TEST_DIRECTORY"/lib-merge.sh > > +git config --global merge.conflictstyle merge # TODO: use the default > > # Testcase basic, conflicting changes in 'numerals' > > diff --git a/t/t6432-merge-recursive-space-options.sh b/t/t6432-merge-recursive-space-options.sh > index db4b77e63d..5cfe8a4fbd 100755 > --- a/t/t6432-merge-recursive-space-options.sh > +++ b/t/t6432-merge-recursive-space-options.sh > @@ -16,6 +16,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > . ./test-lib.sh > > +git config merge.conflictstyle merge # TODO: use the default > + > test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b > if test_have_prereq GREP_STRIPS_CR > then > diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh > index 485ad0eee0..ae0bab37ad 100755 > --- a/t/t6440-config-conflict-markers.sh > +++ b/t/t6440-config-conflict-markers.sh > @@ -29,7 +29,7 @@ test_expect_success 'merge' ' > git commit -a -m left && > > test_must_fail git merge r && > - ! grep -E "\|+" content && > + grep -E "\|+" content && > > git reset --hard && > test_must_fail git -c merge.conflictstyle=diff3 merge r && > @@ -52,7 +52,7 @@ test_expect_success 'merge-tree' ' > test_commit l content l && > > git merge-tree initial r l >actual && > - ! grep -E "\|+" actual && > + grep -E "\|+" actual && > > git -c merge.conflictstyle=diff3 merge-tree initial r l >actual && > grep -E "\|+" actual && > @@ -77,7 +77,7 @@ test_expect_success 'notes' ' > git notes add -f -m l initial && > > test_must_fail git notes merge r && > - ! grep -E "\|+" .git/NOTES_MERGE_WORKTREE/* && > + grep -E "\|+" .git/NOTES_MERGE_WORKTREE/* && > > git notes merge --abort && > test_must_fail git -c merge.conflictstyle=diff3 notes merge r && > @@ -104,7 +104,7 @@ test_expect_success 'checkout' ' > > fill b d >content && > git checkout --merge master && > - ! grep -E "\|+" content && > + grep -E "\|+" content && > > git config merge.conflictstyle merge && > > diff --git a/t/t7201-co.sh b/t/t7201-co.sh > index 7f6e23a4bb..11444d5360 100755 > --- a/t/t7201-co.sh > +++ b/t/t7201-co.sh > @@ -25,6 +25,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > . ./test-lib.sh > > +git config merge.conflictstyle merge # TODO: use the default > + > test_tick > > fill () { > diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh > index 3fcb44767f..90449e80c3 100755 > --- a/t/t7506-status-submodule.sh > +++ b/t/t7506-status-submodule.sh > @@ -253,6 +253,7 @@ test_expect_success 'status with merge conflict in .gitmodules' ' > test_create_repo_with_commit sub2 && > ( > cd super && > + git config merge.conflictstyle merge && # TODO: use the default > prev=$(git rev-parse HEAD) && > git checkout -b add_sub1 && > git submodule add ../sub1 && > diff --git a/xdiff-interface.c b/xdiff-interface.c > index 19a030fbe2..1447771724 100644 > --- a/xdiff-interface.c > +++ b/xdiff-interface.c > @@ -299,7 +299,7 @@ int xdiff_compare_lines(const char *l1, long s1, > return xdl_recmatch(l1, s1, l2, s2, flags); > } > > -int git_xmerge_style = XDL_MERGE_STYLE_MERGE; > +int git_xmerge_style = XDL_MERGE_STYLE_DIFF3; > > int git_xmerge_config(const char *var, const char *value, void *cb) > { >
Johannes Sixt wrote: > Am 09.06.21 um 21:28 schrieb Felipe Contreras: > > Virtually everyone is using it, and it's one of the first things we > > teach newcomers in order to resolve conflicts efficiently. > > > > Let's make it the default. > > I tested diff3 style the VERY FIRST TIME the other day and was greated > with the below. Needless to say that this change is a no-go from my POV. "I found a bug" is not a valid reason not to do approach X. The bug gets fixed and the approach continues. Cheers.
Đoàn Trần Công Danh wrote: > On 2021-06-10 08:41:21+0200, Johannes Sixt <j6t@kdbg.org> wrote: > > Am 09.06.21 um 21:28 schrieb Felipe Contreras: > > > Virtually everyone is using it, and it's one of the first things we > > > teach newcomers in order to resolve conflicts efficiently. > > > > > > Let's make it the default. > > > > I tested diff3 style the VERY FIRST TIME the other day and was greated > > with the below. Needless to say that this change is a no-go from my POV. > > I agree, despite using 3-way merging (with external tools) to resolve conflicts. > > I prefer the current conflict style, aka no-diff3 conflict style. Defaults are not for you, they are for the majority of users.
On Thu, Jun 10, 2021 at 08:41:21AM +0200, Johannes Sixt wrote: > Am 09.06.21 um 21:28 schrieb Felipe Contreras: > > Virtually everyone is using it, and it's one of the first things we > > teach newcomers in order to resolve conflicts efficiently. > > > > Let's make it the default. > > I tested diff3 style the VERY FIRST TIME the other day and was greated > with the below. Needless to say that this change is a no-go from my POV. > [...] I didn't look too deeply at your example, but I suspect it may be related to the fact that diff3 does not try to minimize the conflicts as much (and then the recursive merge on top of that piles on extra layers of confusion). There's a lot more discussion in this old thread: https://lore.kernel.org/git/20130306150548.GC15375@pengutronix.de/ -Peff
Phillip Wood wrote: > On 09/06/2021 20:28, Felipe Contreras wrote: > > Virtually everyone is using it, and it's one of the first things we > > teach newcomers in order to resolve conflicts efficiently. > > Given there are millions of users I'm not sure how you established that > virtually everyone is using it. Because it's the stablished consensus that resolving conflicts with merge.conflictstyle=merge is suboptimal. Even if it's not the majority using it (say 49%), the majority would benefit from using it. > I think that while this change would be useful to some users (though > not many if virtually everyone already has it set) it has the > potential to annoy a lot of users who are happy with the existing > default. Every change has the potential to annoy some users. That's an argument against further development of git, not this particular patch. > I do not think that it is a positive change over all. OK. Others disagree. > Had the default been diff3 from early on in git's history then I would > not advocate changing it to the current default but I think the time has > passed when it can be changed without inconveniencing existing users. git has changed defaults in the past, and it will change defaults in the future. There will always be people pushing back against progress, and that is good. But progress happens regardless. Cheers.
Jeff King wrote: > On Thu, Jun 10, 2021 at 08:41:21AM +0200, Johannes Sixt wrote: > > > Am 09.06.21 um 21:28 schrieb Felipe Contreras: > > > Virtually everyone is using it, and it's one of the first things we > > > teach newcomers in order to resolve conflicts efficiently. > > > > > > Let's make it the default. > > > > I tested diff3 style the VERY FIRST TIME the other day and was greated > > with the below. Needless to say that this change is a no-go from my POV. > > [...] > > I didn't look too deeply at your example, but I suspect it may be > related to the fact that diff3 does not try to minimize the conflicts as > much (and then the recursive merge on top of that piles on extra layers > of confusion). > > There's a lot more discussion in this old thread: > > https://lore.kernel.org/git/20130306150548.GC15375@pengutronix.de/ Geezus. My patches always end up kicking the hornest nest don't they? Maybe it would make sense to revive the zdiff3 patch and attempt to make that the default. That would take a lot of time though, so it wasn't as easy as just flipping a switch from "merge" to "diff3". But there is value in attempting to make the default merge.conflictstyle work for everyone (or last least as many people as possible). It's shinning a light on issues that are already present now. For reference--and since gmane links don't work any more--here are the relevant links for the past discussions: https://lore.kernel.org/git/20130306150548.GC15375@pengutronix.de/ https://lore.kernel.org/git/alpine.LFD.1.10.0808311021120.12958@nehalem.linux-foundation.org/ https://lore.kernel.org/git/1220056963-2352-5-git-send-email-gitster@pobox.com/ Cheers.
On Thu, Jun 10, 2021 at 11:00:59AM -0500, Felipe Contreras wrote: > > I didn't look too deeply at your example, but I suspect it may be > > related to the fact that diff3 does not try to minimize the conflicts as > > much (and then the recursive merge on top of that piles on extra layers > > of confusion). > > > > There's a lot more discussion in this old thread: > > > > https://lore.kernel.org/git/20130306150548.GC15375@pengutronix.de/ > > Geezus. My patches always end up kicking the hornest nest don't they? > > Maybe it would make sense to revive the zdiff3 patch and attempt to make > that the default. That would take a lot of time though, so it wasn't as > easy as just flipping a switch from "merge" to "diff3". I had that patch in my daily build for several years, and I would occasionally trigger it when seeing an ugly conflict. IIRC, it segfaulted on me a few times, but I never tracked down the bug. Just a caution in case anybody wants to resurrect it. -Peff
Jeff King <peff@peff.net> writes: > I didn't look too deeply at your example, but I suspect it may be > related to the fact that diff3 does not try to minimize the conflicts as > much (and then the recursive merge on top of that piles on extra layers > of confusion). > > There's a lot more discussion in this old thread: > > https://lore.kernel.org/git/20130306150548.GC15375@pengutronix.de/ Yes, it does not help that the given sample involves conflicts in the inner merge, which is unfortunately almost unreadable. For less confusing merges, diff3 style is often a lifesaver necessary for avoiding mismerges by showing what the common ancestor looked like, so that the reader/merger/integrator can tell what each side wanted to do to the conflicted section. Rejecting diff3 style output because of the way a conflicted part in the inner merge appears as a common ancestor version may be throwing the baby out with the bathwater. A different way to say it is that until we improve the way the conflicted inner merge is shown, diff3 style is not something we can recommend to new users as a default, I would think.
Am 11.06.21 um 03:20 schrieb Junio C Hamano: > Yes, it does not help that the given sample involves conflicts in > the inner merge, which is unfortunately almost unreadable. For less > confusing merges, diff3 style is often a lifesaver necessary for > avoiding mismerges by showing what the common ancestor looked like, > so that the reader/merger/integrator can tell what each side wanted > to do to the conflicted section. > > Rejecting diff3 style output because of the way a conflicted part in > the inner merge appears as a common ancestor version may be throwing > the baby out with the bathwater. A different way to say it is that > until we improve the way the conflicted inner merge is shown, diff3 > style is not something we can recommend to new users as a default, I > would think. I understand that diff3 is very useful for an integrator like you who does a lot of merges of code that was not written by yourself. But I would estimate that most conflicts (in absolute number among all developers using Git) arise during rebase operations and cherry-picking, i.e., while one is working on their own code. In such sitations, the simpler conflict markup is sufficient, because one knows the background and reason of the conflicts. And then the ability to compact conflicts is a life-saver. Therefore, I argue that simple conflict style should remain the default even if the presentation of inner conflicts under diff3 style is improved. -- Hannes
Johannes Sixt <j6t@kdbg.org> writes: > But I would estimate that most conflicts (in absolute number among all > developers using Git) arise during rebase operations and cherry-picking, > i.e., while one is working on their own code. In such sitations, the > simpler conflict markup is sufficient, because one knows the background > and reason of the conflicts. "rebase -i" to reorganize one's own series would be a prime example of "conflicts you need to resolve in code that is purely your own and nobody else's", and cherry-picking used while reorganizing one's own series falls into the same category. I agree that a simpler markup would be more appropriate in such cases. Rebasing to catch up with updated upstream is a different story, though. The same for cherry-picking an earlier change to an updated upstream.
Am 11.06.21 um 08:43 schrieb Junio C Hamano: > Johannes Sixt <j6t@kdbg.org> writes: > >> But I would estimate that most conflicts (in absolute number among all >> developers using Git) arise during rebase operations and cherry-picking, >> i.e., while one is working on their own code. In such sitations, the >> simpler conflict markup is sufficient, because one knows the background >> and reason of the conflicts. > > "rebase -i" to reorganize one's own series would be a prime example > of "conflicts you need to resolve in code that is purely your own > and nobody else's", and cherry-picking used while reorganizing one's > own series falls into the same category. I agree that a simpler > markup would be more appropriate in such cases. > > Rebasing to catch up with updated upstream is a different story, > though. The same for cherry-picking an earlier change to an updated > upstream. I've reflected on this a bit more as well. I've forgotten about the "catch up with someone else" case. That is certainly helped by diff3 style. I retract my opposition and am now neutral to a potential change of default. (I still don't endorse it because it upsets my workflow.) The case that inner conflicts are presented sub-optimally under diff3 remains, though. -- Hannes
Johannes Sixt <j6t@kdbg.org> writes: > The case that inner conflicts are presented sub-optimally under diff3 > remains, though. I agree that until that happens (necessary but not sufficient condition), it is premature to recommend diff3 style to be the default. I notice that "git merge --help" tells what each part separated by conflict markers mean in both output styles, but does not make a specific recommendation as to which one to use in what situation, and it might benefit a few additional sentences to help readers based on what you said, i.e. the "RCS merge" style that hides the original is succinct and easier to work with when you are familiar with what both sides did, while a more verbose "diff3" style helps when you are unfamiliar with what one side (or both sides) did.
Junio C Hamano <gitster@pobox.com> writes: > Johannes Sixt <j6t@kdbg.org> writes: > >> The case that inner conflicts are presented sub-optimally under diff3 >> remains, though. > > I agree that until that happens (necessary but not sufficient > condition), it is premature to recommend diff3 style to be the > default. Yep. A work-around could be to fix diff3 to rather produce RCS merge style in such situations? > > I notice that "git merge --help" tells what each part separated by > conflict markers mean in both output styles, but does not make a > specific recommendation as to which one to use in what situation, > and it might benefit a few additional sentences to help readers > based on what you said, i.e. the "RCS merge" style that hides the > original is succinct and easier to work with when you are familiar > with what both sides did, while a more verbose "diff3" style helps > when you are unfamiliar with what one side (or both sides) did. I don't get it. Once you have diff3 output, and you want something simpler, you just kill the inner section, right? RCS merge output style is simply inferior. Thanks, -- Sergey Organov
Junio C Hamano wrote: > A different way to say it is that until we improve the way the > conflicted inner merge is shown, diff3 style is not something we can > recommend to new users as a default, I would think. I am not sure about that. Does anybody know any newcommer that you would recommend diff2 (aka. merge) style to? I wouldn't. Put yourself in front of a class teaching how to resolve conflicts, I wouldn't dream of explaning the particulars of conflict markers, I would just fire up Meld, and show them visually. Conflict markers are not for newcomers anyway, so that is a moot point. Moreover, it is really hard for an expert to put himself in the shoes of a newcomer... We forget how hard it was at the beginning. At least me personally, I remember that long time ago when resolving conflicts I constantly had to find the change from the base to the remote side in order to see what changes I'm trying to merge, and then from the base to the local side to see why they were conflicting. I don't use diff3 because I want to be fancy, I use it because from experience I *need* to see the base more often than not. Plus, I try to follow the principile of eating your own dogfood [1]. If using diff2 is so awful that basically no git experts use it, why are we recommending it? If the purpose of conflict markers is to resolve conflicts **correctly** and preferably in an efficient way, then diff3 is just better. Cheers. [1] https://en.wikipedia.org/wiki/Eating_your_own_dog_food
Johannes Sixt wrote: > I understand that diff3 is very useful for an integrator like you who > does a lot of merges of code that was not written by yourself. > > But I would estimate that most conflicts (in absolute number among all > developers using Git) arise during rebase operations and cherry-picking, > i.e., while one is working on their own code. The vast majority of developers don't rebase their code. They only do it when absolutely necessary, and that is when they have to rebase it on top of another person's code. > In such sitations, the simpler conflict markup is sufficient, because > one knows the background and reason of the conflicts. I don't know about you, but I often don't know the background of my own code from one month ago, hell, sometimes not even one week ago. I consider myself from one year ago to be pretty much a different developer. > And then the ability to compact conflicts is a life-saver. I don't see how shifting one's sight a couple lines below would save anybody's life. On the other hand opening another tool just to find out was was the original code is tedious as hell. > Therefore, I argue that simple conflict style should remain the > default even if the presentation of inner conflicts under diff3 style > is improved. First let's get rid of all the assumptions you made above. Just because you personally do X doesn't mean most people do. Cheers.
Johannes Sixt wrote: > Am 11.06.21 um 08:43 schrieb Junio C Hamano: > > Rebasing to catch up with updated upstream is a different story, > > though. The same for cherry-picking an earlier change to an updated > > upstream. > > I've reflected on this a bit more as well. I've forgotten about the > "catch up with someone else" case. That is certainly helped by diff3 > style. I retract my opposition and am now neutral to a potential change > of default. (I still don't endorse it because it upsets my workflow.) All right. Fortunately we have a configuration for that. > The case that inner conflicts are presented sub-optimally under diff3 > remains, though. Indeed, and that's something that we should consider fixing, but it's not necessarily a roadblocker since apparently many developers have been able to live with these sub-optimal diff3 inner conflicts. Personally I have never experienced what you posted, so maybe there's something else happening behind the scenes. Maybe merge-ort changed something.
Junio C Hamano wrote: > Johannes Sixt <j6t@kdbg.org> writes: > > > The case that inner conflicts are presented sub-optimally under diff3 > > remains, though. > > I agree that until that happens (necessary but not sufficient > condition), it is premature to recommend diff3 style to be the > default. Why? Most experienced git developers have no issue with diff3. So presumably it's good enough as it is. Yes, we should consider improving that issue stated above, but *necessary*? I don't think so.
Sergey Organov wrote: > Junio C Hamano <gitster@pobox.com> writes: > > I notice that "git merge --help" tells what each part separated by > > conflict markers mean in both output styles, but does not make a > > specific recommendation as to which one to use in what situation, > > and it might benefit a few additional sentences to help readers > > based on what you said, i.e. the "RCS merge" style that hides the > > original is succinct and easier to work with when you are familiar > > with what both sides did, while a more verbose "diff3" style helps > > when you are unfamiliar with what one side (or both sides) did. > > I don't get it. Once you have diff3 output, and you want something > simpler, you just kill the inner section, right? RCS merge output style > is simply inferior. The issue here is not a mere inner section, it's a nested inner section due to a recursive merge. Personally I've never encountered these in real life, but you can trigger them with the following synthetic example, and the output with diff3 is: --- a/content +++ b/content @@@ -1,1 -1,1 +1,13 @@@ ++<<<<<<< HEAD +D ++||||||| merged common ancestors ++<<<<<<<<< Temporary merge branch 1 ++B ++||||||||| 2c9519d ++1 ++========= ++A ++>>>>>>>>> Temporary merge branch 2 ++======= + C ++>>>>>>> C While with merge is: --- a/content +++ b/content @@@ -1,1 -1,1 +1,5 @@@ ++<<<<<<< HEAD +D ++======= + C ++>>>>>>> C I don't see why diff3 triggers the output of this temporary merge, that is a bug in my book. I would expect the output to simply be: --- a/content +++ b/content @@@ -1,1 -1,1 +1,13 @@@ ++<<<<<<< HEAD +D ++||||||| 2c9519d ++1 ++======= + C ++>>>>>>> C Cheers. git init repo && cd repo && echo 1 > content && git add content && git commit -m 1 content && git checkout -b A master && echo A > content && git commit -m A content && git checkout -b B master && echo B > content && git commit -m B content && git checkout -b C A && git rev-parse B >.git/MERGE_HEAD && echo C > content && git commit -m C -a && git checkout -b D A && git rev-parse B >.git/MERGE_HEAD && echo D > content && git commit -m D -a && git -c merge.conflictstyle=diff3 merge -m final C && cat content
Felipe Contreras <felipe.contreras@gmail.com> writes: > Sergey Organov wrote: >> Junio C Hamano <gitster@pobox.com> writes: > >> > I notice that "git merge --help" tells what each part separated by >> > conflict markers mean in both output styles, but does not make a >> > specific recommendation as to which one to use in what situation, >> > and it might benefit a few additional sentences to help readers >> > based on what you said, i.e. the "RCS merge" style that hides the >> > original is succinct and easier to work with when you are familiar >> > with what both sides did, while a more verbose "diff3" style helps >> > when you are unfamiliar with what one side (or both sides) did. >> >> I don't get it. Once you have diff3 output, and you want something >> simpler, you just kill the inner section, right? RCS merge output style >> is simply inferior. > > The issue here is not a mere inner section, it's a nested inner section > due to a recursive merge. No, this one is just generic suggestion by Junio to improve documentation, unrelated to particular problematic contents of the inner section of diff3. Thanks -- Sergey Organov
Sergey Organov wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > Sergey Organov wrote: > >> Junio C Hamano <gitster@pobox.com> writes: > > > >> > I notice that "git merge --help" tells what each part separated by > >> > conflict markers mean in both output styles, but does not make a > >> > specific recommendation as to which one to use in what situation, > >> > and it might benefit a few additional sentences to help readers > >> > based on what you said, i.e. the "RCS merge" style that hides the > >> > original is succinct and easier to work with when you are familiar > >> > with what both sides did, while a more verbose "diff3" style helps > >> > when you are unfamiliar with what one side (or both sides) did. > >> > >> I don't get it. Once you have diff3 output, and you want something > >> simpler, you just kill the inner section, right? RCS merge output style > >> is simply inferior. > > > > The issue here is not a mere inner section, it's a nested inner section > > due to a recursive merge. > > No, this one is just generic suggestion by Junio to improve > documentation, unrelated to particular problematic contents of the inner > section of diff3. OK, but diff3 is not always just merge minus some stuff. It would be nice if it was, which is what triggered the proposal of zdiff3: https://lore.kernel.org/git/20130306150548.GC15375@pengutronix.de/
Am 11.06.21 um 13:51 schrieb Sergey Organov: > I don't get it. Once you have diff3 output, and you want something > simpler, you just kill the inner section, right? RCS merge output style > is simply inferior. There is an important case where RCS style is not inferior: When the base is 123456 then our side makes it 12ABC56 and their side makes it 12AXC56 then diff3 must display the conflict as 12<ABC|34=AXC>56 to be technically correct. RCS style can coalesce A and C outside of the conflict and display it as 12A<B=X>C34 and *that* is the helpful part of this simpler style. You encounter these kinds of conflicts *a lot* when you juggle fixups in a rebase --interactive session. The thread mentioned earlier upthread explores whether diff3 can do a similar simplification. -- Hannes
Am 11.06.21 um 16:25 schrieb Felipe Contreras: > Personally I have never experienced what you posted, so maybe there's > something else happening behind the scenes. I have to do a lot of criss-cross merges lately. > Maybe merge-ort changed something. It produces the same result. -- Hannes
Johannes Sixt wrote: > then diff3 must display the conflict as > > 12<ABC|34=AXC>56 > > to be technically correct. RCS style can coalesce A and C outside of the > conflict and display it as > > 12A<B=X>C34 > > and *that* is the helpful part of this simpler style. I have trouble translating the above to what I'm familiar with in my mind, so... diff2: 1 2 A <<<<<<< l B ======= X >>>>>>> r C 5 6 diff3: 1 2 <<<<<<< l A B C ||||||| b 3 4 ======= A X C >>>>>>> r 5 6 I personally don't mind at all having a few extra lines in order to visualize what actually happened. But of course there's zdiff3: 1 2 A <<<<<<< l B ||||||| b 3 4 ======= X >>>>>>> r C 5 6 Which is the best of both worlds, even if not technically accurate. Cheers.
Elijah Newren wrote: > On Fri, Jun 11, 2021 at 7:25 AM Felipe Contreras <felipe.contreras@gmail.com> > wrote: > > Personally I have never experienced what you posted, so maybe there's > > something else happening behind the scenes. > > > > Maybe merge-ort changed something. > > merge-ort made no changes relative to content merges or choice of merge > bases. (In fact, merge ort doesn't even handle content merges; that's the > xdiff layer.) Even if merge-ort had made changes in this area, merge-ort > is not the default and I didn't see the necessary config tweaks in your > list of config options. (I would have recommended against people using > merge ort until 7bec8e7fa6 ("Merge branch 'en/ort-readiness'", 2021-04-16), > which only made it into a release last week with 2.32. I probably won't be > recommending it as the default at least until the optimization work is > merged and it's hard to predict how many more months that will take.) Indeed, I tested on v2.25 and found the same output. I thought of merge-ort because 1) I've never seen such kind of output before, and 2) grepping the code I thought I saw merge-ort being the default of something, but now I seem to be unable to find where. > It's more likely that the codebases you work with just don't have > criss-cross merges. Yes, that's it. I don't see why people in these kinds of codebases would like diff3 doing that by default. Cheers.
Felipe Contreras <felipe.contreras@gmail.com> writes: > Johannes Sixt wrote: >> then diff3 must display the conflict as >> >> 12<ABC|34=AXC>56 >> >> to be technically correct. RCS style can coalesce A and C outside of the >> conflict and display it as >> >> 12A<B=X>C34 >> >> and *that* is the helpful part of this simpler style. > > I have trouble translating the above to what I'm familiar with in my > mind, so... > > diff2: > > 1 > 2 > A > <<<<<<< l > B > ======= > X > >>>>>>> r > C > 5 > 6 > > diff3: > > 1 > 2 > <<<<<<< l > A > B > C > ||||||| b > 3 > 4 > ======= > A > X > C > >>>>>>> r > 5 > 6 > > I personally don't mind at all having a few extra lines in order to > visualize what actually happened. Plus a good tool should have an option to quickly show a diff between any 2 of 3 parts, making analysis even simpler. > > But of course there's zdiff3: > > 1 > 2 > A > <<<<<<< l > B > ||||||| b > 3 > 4 > ======= > X > >>>>>>> r > C > 5 > 6 > > Which is the best of both worlds, even if not technically accurate. Yeah, now I see, thank you both for explanations! That said, to me it seems that for any of 3 formats one can find a case where it's better than the other 2. I'm sure I got a few occasions where leaving common part(s) out of conflicts resulted in a confusion and mis-merge. Thanks, -- Sergey Organov
On Fri, Jun 11, 2021 at 10:32 AM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > Elijah Newren wrote: > > On Fri, Jun 11, 2021 at 7:25 AM Felipe Contreras <felipe.contreras@gmail.com> > > wrote: > > > > Personally I have never experienced what you posted, so maybe there's > > > something else happening behind the scenes. > > > > > > Maybe merge-ort changed something. > > > > merge-ort made no changes relative to content merges or choice of merge > > bases. (In fact, merge ort doesn't even handle content merges; that's the > > xdiff layer.) Even if merge-ort had made changes in this area, merge-ort > > is not the default and I didn't see the necessary config tweaks in your > > list of config options. (I would have recommended against people using > > merge ort until 7bec8e7fa6 ("Merge branch 'en/ort-readiness'", 2021-04-16), > > which only made it into a release last week with 2.32. I probably won't be > > recommending it as the default at least until the optimization work is > > merged and it's hard to predict how many more months that will take.) > > Indeed, I tested on v2.25 and found the same output. > > I thought of merge-ort because 1) I've never seen such kind of output > before, and 2) grepping the code I thought I saw merge-ort being the > default of something, but now I seem to be unable to find where. We have briefly discussed multiple times what things might be needed to eventually make merge-ort the default (though it's not even complete yet; I'm five months into upstreaming the optimization patches with an unknown number of months left). There were also a couple patches to make the _tests_ default to using merge-ort on most platforms, while still keeping one suite that tests merge-recursive to ensure we don't add breakage. Perhaps one of those is what you're thinking of? > > It's more likely that the codebases you work with just don't have > > criss-cross merges. > > Yes, that's it. > > I don't see why people in these kinds of codebases would like diff3 > doing that by default. I suspect they don't[1]. What's the alternative, though? Not using diff3? Picking a different base to avoid the occasional nested conflict in the inner merge region, but which overall has much worse other side effects? I think Junio was addressing this when he recently said elsewhere in this thread that "Rejecting diff3 style output because of the way a conflicted part in the inner merge appears as a common ancestor version may be throwing the baby out with the bathwater"[2]. Sure, it's an annoyance, but diff3 is still a good option and there is no current solution to the annoying nested merge display. Also, not sure whether it matters or not, but just for completeness I should point out that you can get nested conflict markers without criss-cross merges and without merge.conflictStyle=diff3. It's just much more rare. [1] https://lore.kernel.org/git/CAPc5daUC+6cHpexXTO24p4mG_5eL1JmxrYm8h3UfdTh_FMka=w@mail.gmail.com/ [2] https://lore.kernel.org/git/xmqqh7i5ci3t.fsf@gitster.g/
Elijah Newren wrote: > On Fri, Jun 11, 2021 at 8:32 AM Felipe Contreras <felipe.contreras@gmail.com> > wrote: > > Sergey Organov wrote: > > > Junio C Hamano <gitster@pobox.com> writes: > > git init repo && > > cd repo && > > > > echo 1 > content && > > git add content && > > git commit -m 1 content && > > > > git checkout -b A master && > > echo A > content && > > git commit -m A content && > > > > git checkout -b B master && > > echo B > content && > > git commit -m B content && > > > > git checkout -b C A && > > git rev-parse B >.git/MERGE_HEAD && > > echo C > content && > > git commit -m C -a && > > > > git checkout -b D A && > > git rev-parse B >.git/MERGE_HEAD && > > echo D > content && > > git commit -m D -a && > > > > git -c merge.conflictstyle=diff3 merge -m final C && > > cat content > > Right, here you do not have a unique merge base; you have two of them: A & > B (it's possible to have three or more as well). To do a three-way merge, > you need a single base commit. So, whenever you have more than one merge > base, both merge-recursive and merge-ort will merge the merge bases to get > a virtual merge base. There's always a risk that the merge bases don't > have a unique merge base either, forcing the algorithm to recurse. This > behavior is where the merge algorithm 'recursive' got its name from (and > which also appears in ort's name -- "Ostensibly Recursive's Twin"). > > You could decide to just pick one of the merge-bases at random, and yield a > different set of surprises including silently merging in favor of one side > when the two sides did things differently. That's problematic. > > Instead of using a merge base (a recent-as-possible common commit), you > could decide to instead just try to find a unique common base, regardless > of how ancient it is. Using ancient commits as the base is a step towards > just doing a two-way merge (treating the histories as completely > independent and throwing merge conflicts whenever any files aren't > identical on the two sides). Sure, it's not as bad, but it does yield > massive amounts of useless conflicts. So this is problematic too. > > The alternative to the above two options was the > make-a-virtual-merge-base-by-merging-merge-bases strategy. It apparently > was very successful. OK. That makes sense. > But it does mean that merge bases can have conflict markers in them. But why? And even if they do, why do they have to be diff3 conflict markers? This would be more human-friendly: <<<<<<< HEAD D ||||||| merged common ancestors <<<<<<<<< Temporary merge branch 1 B ========= A >>>>>>>>> Temporary merge branch 2 ======= C >>>>>>> C Or just put a stub conflict marker: <<<<<<< HEAD D ||||||| merged common ancestors <<<<<<<<< Temporary merge >>>>>>>>> ======= C >>>>>>> C Or just use the base of the virtual merge: <<<<<<< HEAD D ||||||| merged common ancestors 1 ======= C >>>>>>> C We don't have to use diff3 all the way. Cheers.
Sergey Organov wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > Johannes Sixt wrote: > >> then diff3 must display the conflict as > >> > >> 12<ABC|34=AXC>56 > >> > >> to be technically correct. RCS style can coalesce A and C outside of the > >> conflict and display it as > >> > >> 12A<B=X>C34 > >> > >> and *that* is the helpful part of this simpler style. > > > > I have trouble translating the above to what I'm familiar with in my > > mind, so... > > > > diff2: > > > > 1 > > 2 > > A > > <<<<<<< l > > B > > ======= > > X > > >>>>>>> r > > C > > 5 > > 6 > > > > diff3: > > > > 1 > > 2 > > <<<<<<< l > > A > > B > > C > > ||||||| b > > 3 > > 4 > > ======= > > A > > X > > C > > >>>>>>> r > > 5 > > 6 > > > > I personally don't mind at all having a few extra lines in order to > > visualize what actually happened. > > Plus a good tool should have an option to quickly show a diff between any > 2 of 3 parts, making analysis even simpler. Indeed, it depends on the mergetool, but personally I use vimdiff3 (which I authored). All I need are diff3 conflict markers plus some colors. > > But of course there's zdiff3: > > > > 1 > > 2 > > A > > <<<<<<< l > > B > > ||||||| b > > 3 > > 4 > > ======= > > X > > >>>>>>> r > > C > > 5 > > 6 > > > > Which is the best of both worlds, even if not technically accurate. > > Yeah, now I see, thank you both for explanations! > > That said, to me it seems that for any of 3 formats one can find a case > where it's better than the other 2. I'm sure I got a few occasions where > leaving common part(s) out of conflicts resulted in a confusion and > mis-merge. Yes, and I found it curious that all this sprang up from my suggestion to get out of our comfort zones. Since I don't have my beloved `g mt` alias, I've been forced to do `g mergetool --tool=gvimdiff3`, once there, I decided to use gvimdiff2 a couple of times, and I even gave meld a try. Now, I didn't dare to remove merge.conflictstyle from my config, but if I did, I would I have immediately noticed that: git merge --<tab> There's nothing to easily switch conflict styles. Seems like there's many areas of opportunity. Cheers.
Felipe Contreras <felipe.contreras@gmail.com> writes: > Sergey Organov wrote: >> Felipe Contreras <felipe.contreras@gmail.com> writes: >> >> > Johannes Sixt wrote: >> >> then diff3 must display the conflict as >> >> >> >> 12<ABC|34=AXC>56 >> >> >> >> to be technically correct. RCS style can coalesce A and C outside of the >> >> conflict and display it as >> >> >> >> 12A<B=X>C34 >> >> >> >> and *that* is the helpful part of this simpler style. >> > >> > I have trouble translating the above to what I'm familiar with in my >> > mind, so... >> > >> > diff2: >> > >> > 1 >> > 2 >> > A >> > <<<<<<< l >> > B >> > ======= >> > X >> > >>>>>>> r >> > C >> > 5 >> > 6 >> > >> > diff3: >> > >> > 1 >> > 2 >> > <<<<<<< l >> > A >> > B >> > C >> > ||||||| b >> > 3 >> > 4 >> > ======= >> > A >> > X >> > C >> > >>>>>>> r >> > 5 >> > 6 >> > >> > I personally don't mind at all having a few extra lines in order to >> > visualize what actually happened. >> >> Plus a good tool should have an option to quickly show a diff between any >> 2 of 3 parts, making analysis even simpler. > > Indeed, it depends on the mergetool, but personally I use vimdiff3 > (which I authored). All I need are diff3 conflict markers plus some > colors. Emacs's smerge-mode works fine too. In fact that was what made me switch to diff3 in git config. Thanks, -- Sergey Organov
Elijah Newren wrote: > On Fri, Jun 11, 2021 at 10:32 AM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > Elijah Newren wrote: > > > On Fri, Jun 11, 2021 at 7:25 AM Felipe Contreras <felipe.contreras@gmail.com> > > > wrote: > > > > > > Personally I have never experienced what you posted, so maybe there's > > > > something else happening behind the scenes. > > > > > > > > Maybe merge-ort changed something. > > > > > > merge-ort made no changes relative to content merges or choice of merge > > > bases. (In fact, merge ort doesn't even handle content merges; that's the > > > xdiff layer.) Even if merge-ort had made changes in this area, merge-ort > > > is not the default and I didn't see the necessary config tweaks in your > > > list of config options. (I would have recommended against people using > > > merge ort until 7bec8e7fa6 ("Merge branch 'en/ort-readiness'", 2021-04-16), > > > which only made it into a release last week with 2.32. I probably won't be > > > recommending it as the default at least until the optimization work is > > > merged and it's hard to predict how many more months that will take.) > > > > Indeed, I tested on v2.25 and found the same output. > > > > I thought of merge-ort because 1) I've never seen such kind of output > > before, and 2) grepping the code I thought I saw merge-ort being the > > default of something, but now I seem to be unable to find where. > > We have briefly discussed multiple times what things might be needed > to eventually make merge-ort the default (though it's not even > complete yet; I'm five months into upstreaming the optimization > patches with an unknown number of months left). There were also a > couple patches to make the _tests_ default to using merge-ort on most > platforms, while still keeping one suite that tests merge-recursive to > ensure we don't add breakage. Perhaps one of those is what you're > thinking of? Nah, I think I just read something wrong. > > > It's more likely that the codebases you work with just don't have > > > criss-cross merges. > > > > Yes, that's it. > > > > I don't see why people in these kinds of codebases would like diff3 > > doing that by default. > > I suspect they don't[1]. What's the alternative, though? Not using > diff3? Picking a different base to avoid the occasional nested > conflict in the inner merge region, but which overall has much worse > other side effects? Picking a different conflict style for the inner merge. > I think Junio was addressing this when he > recently said elsewhere in this thread that "Rejecting diff3 style > output because of the way a conflicted part in the inner merge appears > as a common ancestor version may be throwing the baby out with the > bathwater"[2]. Sure, it's an annoyance, but diff3 is still a good > option and there is no current solution to the annoying nested merge > display. Yes, but he proceeded to say we could not recommend diff3 to new users, so I see the baby out on the floor. I don't think it's likely new users will experience a problem with diff3 inner conflict markers (in 15 years of using git I've never encountered them myself [or I blocked them out my mind]). Even with this problem, experienced developers seem to be unable to live without diff3, so I don't know if there's a valid argument against making it the default. Yes, improving inner conflict markers would make life better for new users, but also everyone else. Software can always be better. Let's not make perfect be the enemy of the good; diff3 is more than good enough already. Cheers.
On Fri, Jun 11, 2021 at 10:57 AM Felipe Contreras <felipe.contreras@gmail.com> wrote: > Elijah Newren wrote: > > On Fri, Jun 11, 2021 at 8:32 AM Felipe Contreras <felipe.contreras@gmail.com> > > wrote: ... > > The alternative to the above two options was the > > make-a-virtual-merge-base-by-merging-merge-bases strategy. It apparently > > was very successful. > > OK. That makes sense. > > > But it does mean that merge bases can have conflict markers in them. > > But why? And even if they do, why do they have to be diff3 conflict > markers? This could be changed; I suspect it just was a natural consequence of how the code was built. (Recursive means there's not a separate code-path for merging the merge-bases, so they get the same merge style by default.) > This would be more human-friendly: > > <<<<<<< HEAD > D > ||||||| merged common ancestors > <<<<<<<<< Temporary merge branch 1 > B > ========= > A > >>>>>>>>> Temporary merge branch 2 > ======= > C > >>>>>>> C I suspect that would be as easy as this (not compiled or tested): diff --git a/ll-merge.c b/ll-merge.c index 095a4d820e..bdd129cbd6 100644 --- a/ll-merge.c +++ b/ll-merge.c @@ -131,7 +131,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused, xmp.level = XDL_MERGE_ZEALOUS; xmp.favor = opts->variant; xmp.xpp.flags = opts->xdl_opts; - if (git_xmerge_style >= 0) + if (git_xmerge_style >= 0 && !opts->virtual_ancestor) xmp.style = git_xmerge_style; if (marker_size > 0) xmp.marker_size = marker_size; > Or just put a stub conflict marker: > > <<<<<<< HEAD > D > ||||||| merged common ancestors > <<<<<<<<< Temporary merge >>>>>>>>> > ======= > C > >>>>>>> C I don't know what would be involved to do this one; I think it wouldn't be too hard, but I don't think we'd want to pursue this option. > Or just use the base of the virtual merge: > > <<<<<<< HEAD > D > ||||||| merged common ancestors > 1 > ======= > C > >>>>>>> C I think that implementing this choice would look like this (again, not compiled or tested and I'm not familiar with xdiff so take it with a big grain of salt): diff --git a/ll-merge.c b/ll-merge.c index 095a4d820e..dbc7f76951 100644 --- a/ll-merge.c +++ b/ll-merge.c @@ -130,6 +130,8 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused, memset(&xmp, 0, sizeof(xmp)); xmp.level = XDL_MERGE_ZEALOUS; xmp.favor = opts->variant; + if (git_xmerge_style >= 0 && opts->virtual_ancestor) + xmp.favor = XDL_MERGE_FAVOR_BASE; xmp.xpp.flags = opts->xdl_opts; if (git_xmerge_style >= 0) xmp.style = git_xmerge_style; diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h index 8629ae287c..b8d1a536c2 100644 --- a/xdiff/xdiff.h +++ b/xdiff/xdiff.h @@ -62,6 +62,7 @@ extern "C" { #define XDL_MERGE_FAVOR_OURS 1 #define XDL_MERGE_FAVOR_THEIRS 2 #define XDL_MERGE_FAVOR_UNION 3 +#define XDL_MERGE_FAVOR_BASE 4 /* merge output styles */ #define XDL_MERGE_DIFF3 1 diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c index 95871a0b6e..a8dc42595a 100644 --- a/xdiff/xmerge.c +++ b/xdiff/xmerge.c @@ -313,6 +313,9 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1, if (m->mode & 2) size += xdl_recs_copy(xe2, m->i2, m->chg2, 0, 0, dest ? dest + size : NULL); + } else if (m->mode == 4) { + size += xdl_orig_copy(xe1, m->i0, m->chg0, needs_cr, 0, + dest ? dest + size : NULL); } else continue; i = m->i1 + m->chg1; > We don't have to use diff3 all the way. Right, thus my mention in the other email to consider adding a XDL_MERGE_FAVOR_BASE -- which you then also mention here in your third option, and which I've now given at least a partial patch for. Not sure if it's a crazy idea or a great idea, since I don't do very many criss-cross merges myself. Hope that helps, Elijah
Elijah Newren wrote: > On Fri, Jun 11, 2021 at 10:57 AM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > Or just use the base of the virtual merge: > > > > <<<<<<< HEAD > > D > > ||||||| merged common ancestors > > 1 > > ======= > > C > > >>>>>>> C > > I think that implementing this choice would look like this (again, not > compiled or tested and I'm not familiar with xdiff so take it with a > big grain of salt): > > > diff --git a/ll-merge.c b/ll-merge.c > index 095a4d820e..dbc7f76951 100644 > --- a/ll-merge.c > +++ b/ll-merge.c > @@ -130,6 +130,8 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused, > memset(&xmp, 0, sizeof(xmp)); > xmp.level = XDL_MERGE_ZEALOUS; > xmp.favor = opts->variant; > + if (git_xmerge_style >= 0 && opts->virtual_ancestor) > + xmp.favor = XDL_MERGE_FAVOR_BASE; The only time git_xmerge_style isn't >= 0 is when no merge style has been configured by the user. I don't see why this: git -c merge.conflictstyle=merge merge Should have a different behavior than this: git merge In fact, I don't see why any style should change that desired behavior. If you said there's issues with the "merge" style too, perhaps the above will help for those cases too. > > We don't have to use diff3 all the way. > > Right, thus my mention in the other email to consider adding a > XDL_MERGE_FAVOR_BASE -- which you then also mention here in your third > option, and which I've now given at least a partial patch for. Not > sure if it's a crazy idea or a great idea, since I don't do very many > criss-cross merges myself. I thought you meant as a separate configurable flag, not something done by default. Now that I understand what you meant I think it could be a great idea. Cheers.
On Fri, Jun 11, 2021 at 2:05 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > Elijah Newren wrote: > > On Fri, Jun 11, 2021 at 10:57 AM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > > > Or just use the base of the virtual merge: > > > > > > <<<<<<< HEAD > > > D > > > ||||||| merged common ancestors > > > 1 > > > ======= > > > C > > > >>>>>>> C > > > > I think that implementing this choice would look like this (again, not > > compiled or tested and I'm not familiar with xdiff so take it with a > > big grain of salt): > > > > > > diff --git a/ll-merge.c b/ll-merge.c > > index 095a4d820e..dbc7f76951 100644 > > --- a/ll-merge.c > > +++ b/ll-merge.c > > @@ -130,6 +130,8 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused, > > memset(&xmp, 0, sizeof(xmp)); > > xmp.level = XDL_MERGE_ZEALOUS; > > xmp.favor = opts->variant; > > + if (git_xmerge_style >= 0 && opts->virtual_ancestor) > > + xmp.favor = XDL_MERGE_FAVOR_BASE; > > The only time git_xmerge_style isn't >= 0 is when no merge style has > been configured by the user. Yep, probably should have just been + if (opts->virtual_ancestor) + xmp.favor = XDL_MERGE_FAVOR_BASE; Though the difference doesn't matter a lot. Since merge.conflictStyle=merge (which is the current default) doesn't display the contents from the merge base in a three-way content merge, setting xmp.favor to XDL_MERGE_FAVOR_BASE vs. leaving it as 0 for the recursive/intermediate merges won't generally end up affecting the end result. It'd only matter for diff3 and zdiff3 users. Going on a slight tangent, I think there's actually a related bug here. We probably should not honor XDL_MERGE_FAVOR_{OURS,THEIRS} when opts->virtual_ancestor is true; that's just asking for trouble. I think it'd paradoxically result in reversing the desired behavior (e.g. users would see what they'd consider XDL_MERGE_FAVOR_THEIRS behavior when they asked for XDL_MERGE_FAVOR_OURS) in some cases as a result. > In fact, I don't see why any style should change that desired behavior. > If you said there's issues with the "merge" style too, perhaps the above > will help for those cases too. > > > > We don't have to use diff3 all the way. > > > > Right, thus my mention in the other email to consider adding a > > XDL_MERGE_FAVOR_BASE -- which you then also mention here in your third > > option, and which I've now given at least a partial patch for. Not > > sure if it's a crazy idea or a great idea, since I don't do very many > > criss-cross merges myself. > > I thought you meant as a separate configurable flag, not something done > by default. > > Now that I understand what you meant I think it could be a great idea. If someone that does lots of criss-cross merges can comment on the idea, and agree that it's worth a shot, I can try to turn it into real patches. (I might even try to investigate the zdiff3 stuff too, which sounds like something I've wanted many times...but I'd really rather concentrate on merge-ort until its upstreaming is finished.)
Elijah Newren wrote: > On Fri, Jun 11, 2021 at 2:05 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > Elijah Newren wrote: > > > On Fri, Jun 11, 2021 at 10:57 AM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > > > > > Or just use the base of the virtual merge: > > > > > > > > <<<<<<< HEAD > > > > D > > > > ||||||| merged common ancestors > > > > 1 > > > > ======= > > > > C > > > > >>>>>>> C > > > > > > I think that implementing this choice would look like this (again, not > > > compiled or tested and I'm not familiar with xdiff so take it with a > > > big grain of salt): > > > > > > > > > diff --git a/ll-merge.c b/ll-merge.c > > > index 095a4d820e..dbc7f76951 100644 > > > --- a/ll-merge.c > > > +++ b/ll-merge.c > > > @@ -130,6 +130,8 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused, > > > memset(&xmp, 0, sizeof(xmp)); > > > xmp.level = XDL_MERGE_ZEALOUS; > > > xmp.favor = opts->variant; > > > + if (git_xmerge_style >= 0 && opts->virtual_ancestor) > > > + xmp.favor = XDL_MERGE_FAVOR_BASE; > > > > The only time git_xmerge_style isn't >= 0 is when no merge style has > > been configured by the user. > > Yep, probably should have just been > > + if (opts->virtual_ancestor) > + xmp.favor = XDL_MERGE_FAVOR_BASE; > > Though the difference doesn't matter a lot. Since > merge.conflictStyle=merge (which is the current default) doesn't > display the contents from the merge base in a three-way content merge, > setting xmp.favor to XDL_MERGE_FAVOR_BASE vs. leaving it as 0 for the > recursive/intermediate merges won't generally end up affecting the end > result. It'd only matter for diff3 and zdiff3 users. OK, so: if (git_xmerge_style > 0 && opts->virtual_ancestor) > Going on a slight tangent, I think there's actually a related bug > here. We probably should not honor XDL_MERGE_FAVOR_{OURS,THEIRS} when > opts->virtual_ancestor is true; that's just asking for trouble. I > think it'd paradoxically result in reversing the desired behavior > (e.g. users would see what they'd consider XDL_MERGE_FAVOR_THEIRS > behavior when they asked for XDL_MERGE_FAVOR_OURS) in some cases as a > result. Maybe write a test-case and find out? > > In fact, I don't see why any style should change that desired behavior. > > If you said there's issues with the "merge" style too, perhaps the above > > will help for those cases too. > > > > > > We don't have to use diff3 all the way. > > > > > > Right, thus my mention in the other email to consider adding a > > > XDL_MERGE_FAVOR_BASE -- which you then also mention here in your third > > > option, and which I've now given at least a partial patch for. Not > > > sure if it's a crazy idea or a great idea, since I don't do very many > > > criss-cross merges myself. > > > > I thought you meant as a separate configurable flag, not something done > > by default. > > > > Now that I understand what you meant I think it could be a great idea. > > If someone that does lots of criss-cross merges can comment on the > idea, and agree that it's worth a shot, I can try to turn it into real > patches. I would flip the conditional: if nobody that does lots of criss-crosses objects... If we waited for a tiny minority of users to speak up before doing something we migth wait forever. > (I might even try to investigate the zdiff3 stuff too, which sounds > like something I've wanted many times...but I'd really rather > concentrate on merge-ort until its upstreaming is finished.) Well, I just re-sent the patch: https://lore.kernel.org/git/20210613143155.836591-1-felipe.contreras@gmail.com/ Cheers.
diff --git a/Documentation/config/merge.txt b/Documentation/config/merge.txt index cb2ed58907..2dba937dd0 100644 --- a/Documentation/config/merge.txt +++ b/Documentation/config/merge.txt @@ -1,10 +1,10 @@ merge.conflictStyle:: - Specify the style in which conflicted hunks are written out to - working tree files upon merge. The default is "merge", which - shows a `<<<<<<<` conflict marker, changes made by one side, - a `=======` marker, changes made by the other side, and then - a `>>>>>>>` marker. An alternate style, "diff3", adds a `|||||||` - marker and the original text before the `=======` marker. + Specify the style in which conflicted hunks are written out to working + tree files upon merge. The default is "diff3", which shows a `<<<<<<<` + conflict marker, changes made by one side, a `|||||||` marker, the + original text, a `=======` marker, changes made by the other side, and + then a `>>>>>>>` marker. A simpler mode "merge" omits the `|||||||` + marker and the original text. merge.defaultToUpstream:: If merge is called without any commit argument, merge the upstream diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt index f856032613..7d8e74c872 100644 --- a/Documentation/git-merge-file.txt +++ b/Documentation/git-merge-file.txt @@ -30,6 +30,8 @@ normally outputs a warning and brackets the conflict with lines containing <<<<<<< A lines in file A + ||||||| + lines in merge base ======= lines in file B >>>>>>> B diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt index 3819fadac1..14dadf2e16 100644 --- a/Documentation/git-merge.txt +++ b/Documentation/git-merge.txt @@ -233,7 +233,7 @@ final result verbatim. When both sides made changes to the same area, however, Git cannot randomly pick one side over the other, and asks you to resolve it by leaving what both sides did to that area. -By default, Git uses the same style as the one used by the "merge" program +By default, Git uses a similar style to the one used by the "merge" program from the RCS suite to present such a conflicted hunk, like this: ------------ @@ -242,6 +242,8 @@ ancestor, or cleanly resolved because only one side changed. <<<<<<< yours:sample.txt Conflict resolution is hard; let's go shopping. +||||||| +Originally there's no conflict. ======= Git makes conflict resolution easy. >>>>>>> theirs:sample.txt @@ -249,17 +251,12 @@ And here is another line that is cleanly resolved or unmodified. ------------ The area where a pair of conflicting changes happened is marked with markers -`<<<<<<<`, `=======`, and `>>>>>>>`. The part before the `=======` -is typically your side, and the part afterwards is typically their side. - -The default format does not show what the original said in the conflicting -area. You cannot tell how many lines are deleted and replaced with -Barbie's remark on your side. The only thing you can tell is that your -side wants to say it is hard and you'd prefer to go shopping, while the -other side wants to claim it is easy. +`<<<<<<<`, `=======`, and `>>>>>>>`. The part before the `|||||||` +is typically your side, and the part after `=======` is typically their side. +In-between is the original code. -An alternative style can be used by setting the "merge.conflictStyle" -configuration variable to "diff3". In "diff3" style, the above conflict +An more basic style can be used by setting the "merge.conflictStyle" +configuration variable to "merge". In "merge" style, the above conflict may look like this: ------------ @@ -268,21 +265,12 @@ ancestor, or cleanly resolved because only one side changed. <<<<<<< yours:sample.txt Conflict resolution is hard; let's go shopping. -||||||| -Conflict resolution is hard. ======= Git makes conflict resolution easy. >>>>>>> theirs:sample.txt And here is another line that is cleanly resolved or unmodified. ------------ -In addition to the `<<<<<<<`, `=======`, and `>>>>>>>` markers, it uses -another `|||||||` marker that is followed by the original text. You can -tell that the original just stated a fact, and your side simply gave in to -that statement and gave up, while the other side tried to have a more -positive attitude. You can sometimes come up with a better resolution by -viewing the original. - HOW TO RESOLVE CONFLICTS ------------------------ diff --git a/Documentation/git-rerere.txt b/Documentation/git-rerere.txt index 4cfc883378..89b0820995 100644 --- a/Documentation/git-rerere.txt +++ b/Documentation/git-rerere.txt @@ -159,7 +159,7 @@ resolve. Running the 'git rerere' command immediately after a conflicted automerge records the conflicted working tree files, with the -usual conflict markers `<<<<<<<`, `=======`, and `>>>>>>>` in +usual conflict markers `<<<<<<<`, `|||||||`, `=======`, and `>>>>>>>` in them. Later, after you are done resolving the conflicts, running 'git rerere' again will record the resolved state of these files. Suppose you did this when you created the test merge of diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt index 83fd4e19a4..b767215ac2 100644 --- a/Documentation/gitattributes.txt +++ b/Documentation/gitattributes.txt @@ -1042,10 +1042,10 @@ text:: Usual 3-way file level merge for text files. Conflicted regions are marked with conflict markers `<<<<<<<`, - `=======` and `>>>>>>>`. The version from your branch - appears before the `=======` marker, and the version + `|||||||`, `=======` and `>>>>>>>`. The version from your branch + appears before the `|||||||` marker, and the version from the merged branch appears after the `=======` - marker. + marker. In-between is the original. binary:: diff --git a/Documentation/technical/rerere.txt b/Documentation/technical/rerere.txt index af5f9fc24f..38b44f4430 100644 --- a/Documentation/technical/rerere.txt +++ b/Documentation/technical/rerere.txt @@ -42,8 +42,7 @@ get a conflict like the following: >>>>>>> AC Doing the analogous with AC2 (forking a branch ABAC2 off of branch AB -and then merging branch AC2 into it), using the diff3 conflict style, -we get a conflict like the following: +and then merging branch AC2 into it), we get a conflict like the following: <<<<<<< HEAD B diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt index f9e54b8674..3ddde87482 100644 --- a/Documentation/user-manual.txt +++ b/Documentation/user-manual.txt @@ -1243,6 +1243,8 @@ files with conflicts will have conflict markers added, like this: ------------------------------------------------- <<<<<<< HEAD:file.txt Hello world +||||||| +Original ======= Goodbye >>>>>>> 77976da35a11db4580b80ae27e8d65caf5208086:file.txt @@ -1276,9 +1278,11 @@ diff --cc file.txt index 802992c,2b60207..0000000 --- a/file.txt +++ b/file.txt -@@@ -1,1 -1,1 +1,5 @@@ +@@@ -1,1 -1,1 +1,7 @@@ ++<<<<<<< HEAD:file.txt +Hello world +++||||||| +++Original ++======= + Goodbye ++>>>>>>> 77976da35a11db4580b80ae27e8d65caf5208086:file.txt diff --git a/t/t2023-checkout-m.sh b/t/t2023-checkout-m.sh index 7b327b7544..219c82532a 100755 --- a/t/t2023-checkout-m.sh +++ b/t/t2023-checkout-m.sh @@ -9,6 +9,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +git config merge.conflictstyle merge # TODO: use the default + test_expect_success setup ' test_tick && test_commit both.txt both.txt initial && diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh index d3d72e25fe..cbd5d8302e 100755 --- a/t/t3310-notes-merge-manual-resolve.sh +++ b/t/t3310-notes-merge-manual-resolve.sh @@ -7,6 +7,8 @@ test_description='Test notes merging with manual conflict resolution' . ./test-lib.sh +git config --global merge.conflictstyle merge # TODO: use the default + # Set up a notes merge scenario with different kinds of conflicts test_expect_success 'setup commits' ' test_commit 1st && diff --git a/t/t3311-notes-merge-fanout.sh b/t/t3311-notes-merge-fanout.sh index 5b675417e9..4aeaa05c15 100755 --- a/t/t3311-notes-merge-fanout.sh +++ b/t/t3311-notes-merge-fanout.sh @@ -7,6 +7,8 @@ test_description='Test notes merging at various fanout levels' . ./test-lib.sh +git config merge.conflictstyle merge # TODO: use the default + verify_notes () { notes_ref="$1" commit="$2" diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 66bcbbf952..769079a71c 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -32,6 +32,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . "$TEST_DIRECTORY"/lib-rebase.sh +git config merge.conflictstyle merge # TODO: use the default + test_expect_success 'setup' ' git switch -C primary && test_commit A file1 && diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh index 014001b8f3..647a40f314 100755 --- a/t/t3507-cherry-pick-conflict.sh +++ b/t/t3507-cherry-pick-conflict.sh @@ -281,6 +281,7 @@ test_expect_success \ test_expect_success 'failed cherry-pick describes conflict in work tree' ' pristine_detach initial && + git config merge.conflictstyle merge && # TODO: use the default cat <<-EOF >expected && <<<<<<< HEAD a @@ -316,6 +317,7 @@ test_expect_success 'diff3 -m style' ' test_expect_success 'revert also handles conflicts sanely' ' git config --unset merge.conflictstyle && + git config merge.conflictstyle merge && # TODO: use the default pristine_detach initial && cat <<-EOF >expected && <<<<<<< HEAD diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh index ed461f481e..04b77af2a4 100755 --- a/t/t4017-diff-retval.sh +++ b/t/t4017-diff-retval.sh @@ -7,6 +7,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +git config merge.conflictstyle merge # TODO: use the default + test_expect_success 'setup' ' echo "1 " >a && git add . && diff --git a/t/t4048-diff-combined-binary.sh b/t/t4048-diff-combined-binary.sh index 0260cf64f5..49a56731dd 100755 --- a/t/t4048-diff-combined-binary.sh +++ b/t/t4048-diff-combined-binary.sh @@ -6,6 +6,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +git config merge.conflictstyle merge # TODO: use the default + test_expect_success 'setup binary merge conflict' ' echo oneQ1 | q_to_nul >binary && git add binary && diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh index 9f8c76dffb..e9ae3d6fde 100755 --- a/t/t4200-rerere.sh +++ b/t/t4200-rerere.sh @@ -27,6 +27,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +git config merge.conflictstyle merge # TODO: use the default + test_expect_success 'setup' ' cat >a1 <<-\EOF && Some title diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh index e59601e5fe..f21ccaf0a6 100755 --- a/t/t4300-merge-tree.sh +++ b/t/t4300-merge-tree.sh @@ -6,6 +6,8 @@ test_description='git merge-tree' . ./test-lib.sh +git config merge.conflictstyle merge # TODO: use the default + test_expect_success setup ' test_commit "initial" "initial-file" "initial" ' diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh index 425dad97d5..4f01bbc451 100755 --- a/t/t6402-merge-rename.sh +++ b/t/t6402-merge-rename.sh @@ -270,6 +270,7 @@ test_expect_success 'setup for rename + d/f conflicts' ' git checkout --orphan dir-in-way && git rm -rf . && git clean -fdqx && + git config merge.conflictstyle merge && # TODO: use the default mkdir sub && mkdir dir && @@ -871,6 +872,7 @@ test_expect_success 'setup for use of extended merge markers' ' git clean -fdqx && rm -rf .git && git init && + git config merge.conflictstyle merge && # TODO: use the default printf "1\n2\n3\n4\n5\n6\n7\n8\n" >original_file && git add original_file && diff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh index 2f421d967a..1428dfb5c6 100755 --- a/t/t6403-merge-file.sh +++ b/t/t6403-merge-file.sh @@ -3,6 +3,8 @@ test_description='RCS merge replacement: merge-file' . ./test-lib.sh +git config merge.conflictstyle merge # TODO: use the default + test_expect_success 'setup' ' cat >orig.txt <<-\EOF && Dominus regit me, diff --git a/t/t6404-recursive-merge.sh b/t/t6404-recursive-merge.sh index eaf48e941e..a3354b8f9a 100755 --- a/t/t6404-recursive-merge.sh +++ b/t/t6404-recursive-merge.sh @@ -6,6 +6,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +git config merge.conflictstyle merge # TODO: use the default + # This scenario is based on a real-world repository of Shawn Pearce. # 1 - A - D - F diff --git a/t/t6416-recursive-corner-cases.sh b/t/t6416-recursive-corner-cases.sh index 84f5082366..ac4e69a325 100755 --- a/t/t6416-recursive-corner-cases.sh +++ b/t/t6416-recursive-corner-cases.sh @@ -8,6 +8,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh . "$TEST_DIRECTORY"/lib-merge.sh +git config --global merge.conflictstyle merge # TODO: use the default + # # L1 L2 # o---o diff --git a/t/t6417-merge-ours-theirs.sh b/t/t6417-merge-ours-theirs.sh index ac9aee9a66..b8208a383b 100755 --- a/t/t6417-merge-ours-theirs.sh +++ b/t/t6417-merge-ours-theirs.sh @@ -6,6 +6,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +git config merge.conflictstyle merge # TODO: use the default + test_expect_success setup ' for i in 1 2 3 4 5 6 7 8 9 do diff --git a/t/t6418-merge-text-auto.sh b/t/t6418-merge-text-auto.sh index 1e0296dd17..e18f67776c 100755 --- a/t/t6418-merge-text-auto.sh +++ b/t/t6418-merge-text-auto.sh @@ -17,6 +17,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +git config merge.conflictstyle merge # TODO: use the default + test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b compare_files () { diff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh index bf4ce3c63d..6bb4b6d968 100755 --- a/t/t6422-merge-rename-corner-cases.sh +++ b/t/t6422-merge-rename-corner-cases.sh @@ -9,6 +9,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh . "$TEST_DIRECTORY"/lib-merge.sh +git config --global merge.conflictstyle merge # TODO: use the default + test_setup_rename_delete_untracked () { test_create_repo rename-delete-untracked && ( diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh index 7134769149..5f6cacd064 100755 --- a/t/t6423-merge-rename-directories.sh +++ b/t/t6423-merge-rename-directories.sh @@ -28,6 +28,7 @@ test_description="recursive merge with directory renames" . ./test-lib.sh . "$TEST_DIRECTORY"/lib-merge.sh +git config --global merge.conflictstyle merge # TODO: use the default ########################################################################### # SECTION 1: Basic cases we should be able to handle diff --git a/t/t6428-merge-conflicts-sparse.sh b/t/t6428-merge-conflicts-sparse.sh index 7e8bf497f8..18975801db 100755 --- a/t/t6428-merge-conflicts-sparse.sh +++ b/t/t6428-merge-conflicts-sparse.sh @@ -25,6 +25,7 @@ test_description="merge cases" . ./test-lib.sh . "$TEST_DIRECTORY"/lib-merge.sh +git config --global merge.conflictstyle merge # TODO: use the default # Testcase basic, conflicting changes in 'numerals' diff --git a/t/t6432-merge-recursive-space-options.sh b/t/t6432-merge-recursive-space-options.sh index db4b77e63d..5cfe8a4fbd 100755 --- a/t/t6432-merge-recursive-space-options.sh +++ b/t/t6432-merge-recursive-space-options.sh @@ -16,6 +16,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +git config merge.conflictstyle merge # TODO: use the default + test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b if test_have_prereq GREP_STRIPS_CR then diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh index 485ad0eee0..ae0bab37ad 100755 --- a/t/t6440-config-conflict-markers.sh +++ b/t/t6440-config-conflict-markers.sh @@ -29,7 +29,7 @@ test_expect_success 'merge' ' git commit -a -m left && test_must_fail git merge r && - ! grep -E "\|+" content && + grep -E "\|+" content && git reset --hard && test_must_fail git -c merge.conflictstyle=diff3 merge r && @@ -52,7 +52,7 @@ test_expect_success 'merge-tree' ' test_commit l content l && git merge-tree initial r l >actual && - ! grep -E "\|+" actual && + grep -E "\|+" actual && git -c merge.conflictstyle=diff3 merge-tree initial r l >actual && grep -E "\|+" actual && @@ -77,7 +77,7 @@ test_expect_success 'notes' ' git notes add -f -m l initial && test_must_fail git notes merge r && - ! grep -E "\|+" .git/NOTES_MERGE_WORKTREE/* && + grep -E "\|+" .git/NOTES_MERGE_WORKTREE/* && git notes merge --abort && test_must_fail git -c merge.conflictstyle=diff3 notes merge r && @@ -104,7 +104,7 @@ test_expect_success 'checkout' ' fill b d >content && git checkout --merge master && - ! grep -E "\|+" content && + grep -E "\|+" content && git config merge.conflictstyle merge && diff --git a/t/t7201-co.sh b/t/t7201-co.sh index 7f6e23a4bb..11444d5360 100755 --- a/t/t7201-co.sh +++ b/t/t7201-co.sh @@ -25,6 +25,8 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +git config merge.conflictstyle merge # TODO: use the default + test_tick fill () { diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh index 3fcb44767f..90449e80c3 100755 --- a/t/t7506-status-submodule.sh +++ b/t/t7506-status-submodule.sh @@ -253,6 +253,7 @@ test_expect_success 'status with merge conflict in .gitmodules' ' test_create_repo_with_commit sub2 && ( cd super && + git config merge.conflictstyle merge && # TODO: use the default prev=$(git rev-parse HEAD) && git checkout -b add_sub1 && git submodule add ../sub1 && diff --git a/xdiff-interface.c b/xdiff-interface.c index 19a030fbe2..1447771724 100644 --- a/xdiff-interface.c +++ b/xdiff-interface.c @@ -299,7 +299,7 @@ int xdiff_compare_lines(const char *l1, long s1, return xdl_recmatch(l1, s1, l2, s2, flags); } -int git_xmerge_style = XDL_MERGE_STYLE_MERGE; +int git_xmerge_style = XDL_MERGE_STYLE_DIFF3; int git_xmerge_config(const char *var, const char *value, void *cb) {
Virtually everyone is using it, and it's one of the first things we teach newcomers in order to resolve conflicts efficiently. Let's make it the default. This generates a ton of changes in the tests. Although we probably will want to update them to use th new default, override the configuration so we use the old one for now. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- Documentation/config/merge.txt | 12 +++++----- Documentation/git-merge-file.txt | 2 ++ Documentation/git-merge.txt | 28 +++++++----------------- Documentation/git-rerere.txt | 2 +- Documentation/gitattributes.txt | 6 ++--- Documentation/technical/rerere.txt | 3 +-- Documentation/user-manual.txt | 6 ++++- t/t2023-checkout-m.sh | 2 ++ t/t3310-notes-merge-manual-resolve.sh | 2 ++ t/t3311-notes-merge-fanout.sh | 2 ++ t/t3404-rebase-interactive.sh | 2 ++ t/t3507-cherry-pick-conflict.sh | 2 ++ t/t4017-diff-retval.sh | 2 ++ t/t4048-diff-combined-binary.sh | 2 ++ t/t4200-rerere.sh | 2 ++ t/t4300-merge-tree.sh | 2 ++ t/t6402-merge-rename.sh | 2 ++ t/t6403-merge-file.sh | 2 ++ t/t6404-recursive-merge.sh | 2 ++ t/t6416-recursive-corner-cases.sh | 2 ++ t/t6417-merge-ours-theirs.sh | 2 ++ t/t6418-merge-text-auto.sh | 2 ++ t/t6422-merge-rename-corner-cases.sh | 2 ++ t/t6423-merge-rename-directories.sh | 1 + t/t6428-merge-conflicts-sparse.sh | 1 + t/t6432-merge-recursive-space-options.sh | 2 ++ t/t6440-config-conflict-markers.sh | 8 +++---- t/t7201-co.sh | 2 ++ t/t7506-status-submodule.sh | 1 + xdiff-interface.c | 2 +- 30 files changed, 70 insertions(+), 38 deletions(-)