Phillip Wood wrote: > Hi Felipe > > On 18/12/2020 12:49, Felipe Contreras wrote: > > It doesn't make sense to display lines without conflicts in the > > different views of all mergetools. > > > > Only the lines that warrant conflict markers should be displayed. > > > > Most people would want this behavior on, but in case some don't; add a > > new configuration: mergetool.autoMerge. > > > > See Seth House's blog post [1] for the idea, and the rationale. > > > > [1] https://www.eseth.org/2020/mergetools.html > > I would be good to have a summary of the idea in this commit message so > people do not have to go and find a blog post which may well disappear > in the future I thought I did, in the paragraphs above. How about adding this explanation: When merging, not all lines with changes are considered conflicts, for example: cat >BASE <<EOF Patagraph 1 Paragraph 2 EOF cat >LOCAL <<EOF Paragraph 1 Paragraph 2 EOF cat >REMOTE <<EOF Patagraph 1. Paragraph 2. EOF In this case the first paragraph does have a conflict because there are two changes (in LOCAL and REMOTE), that the user must resolve. However, the second paragraph doesn't have a conflict; it's straightforward to decide that we want the only change present (in REMOTE). In fact, if it were not for the first paragraph with a conflict, git wouldn't have bothered the user since the automatic merge would have succeeded. So it doesn't make sense to display these unconflicted lines to the user inside the mergetool; it only creates noise. We can fix that by propagating the final version of the file with the automatic merge to all the panes of the mergetool (BASE, LOCAL, and REMOTE), and only make them differ on the places where the are actual conflicts (and they are demarcated with conflict markers). (this is mostly my explanation though, not Seth's, who used visual examples) > > Original-idea-by: Seth House <seth@xxxxxxxxx> > > Signed-off-by: Felipe Contreras <felipe.contreras@xxxxxxxxx> > > --- > > Documentation/config/mergetool.txt | 3 +++ > > git-mergetool.sh | 17 +++++++++++++++++ > > t/t7610-mergetool.sh | 18 ++++++++++++++++++ > > 3 files changed, 38 insertions(+) > > > > diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt > > index 16a27443a3..7ce6d0d3ac 100644 > > --- a/Documentation/config/mergetool.txt > > +++ b/Documentation/config/mergetool.txt > > @@ -61,3 +61,6 @@ mergetool.writeToTemp:: > > > > mergetool.prompt:: > > Prompt before each invocation of the merge resolution program. > > + > > +mergetool.autoMerge:: > > + Remove lines without conflicts from all the files. Defaults to `true`. > > diff --git a/git-mergetool.sh b/git-mergetool.sh > > index e3f6d543fb..f4db0cac8d 100755 > > --- a/git-mergetool.sh > > +++ b/git-mergetool.sh > > @@ -239,6 +239,17 @@ checkout_staged_file () { > > fi > > } > > > > +auto_merge () { > > + git merge-file --diff3 --marker-size=7 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3" > > I've been wondering if we want to recreate the merge or just get the > merged BASE LOCAL and REMOTE from the merged file in the working tree. > If the user wants to resolve the conflicts in stages, or opens the file > in a editor and fixes some conflicts and then realizes they want to use > a merge tool that work is thrown away if we recreate the merge. They can > always use `checkout --merge` to throw away their changes and start > again with a mergetool. It would mean checking the size of the conflict > markers and using > '/^<{$conflict_marker_size}/,^|{$conflict_marker_size}/' for sed. > Getting the merged BASE would be tricky if the user does not have diff3 > conflicts enabled, I'm not sure if we can safely get BASE from `git > merge-file ...` and LOCAL and REMOTE from the working tree. That's a good point. However, their work is not thrown away; MERGED is not touched by this. It's only for visualization purposes that some already-fixed conflicts would be shown in the mergetool, which yeah; it's not ideal. That's an improvement that can be done later, on top of this patch. The bulk of improvements are already enabled by this, and the marginal gains can be added later. > > + if test -s "$DIFF3" > > + then > > + sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE" > > + sed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' "$DIFF3" >"$LOCAL" > > + sed -e '/^<<<<<<< /,/^=======\r\?$/d' -e '/^>>>>>>> /d' "$DIFF3" >"$REMOTE" > > + fi > > + rm -- "$DIFF3" > > +} > > + > > merge_file () { > > MERGED="$1" > > > > @@ -274,6 +285,7 @@ merge_file () { > > BASE=${BASE##*/} > > fi > > > > + DIFF3="$MERGETOOL_TMPDIR/${BASE}_DIFF3_$$$ext" > > BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext" > > LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext" > > REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext" > > @@ -322,6 +334,11 @@ merge_file () { > > checkout_staged_file 2 "$MERGED" "$LOCAL" > > checkout_staged_file 3 "$MERGED" "$REMOTE" > > > > + if test "$(git config --bool mergetool.autoMerge)" != "false" > > If I run `git config --bool mergetool.autoMerge` it returns an empty > string so I think you need to test it is actually equal to "true". Yeah, this would evaluate to positive: test "" != "false" It's enabled by default since I heard Junio mention it would make sense. > I also share the view that this should be per tool. Your demand that > someone comes up with an example that breaks assumes that we have access > to all the tools that users are using. It's not a demand. It's a fact that unless we have an example (even if hypothetical), the burden of proof has not been met. The default position is that we don't know if such configuration would make sense or not. > Seth has done a great job of > surveying the popular tools but given the size of git's user-base and > the diversity of uses it is very likely that there will be people using > in-house or proprietary tools that no one on the list has access to. Yes, they can just turn off the flag. > I would much prefer to avoid breaking them rather than waiting for a > bug report before implementing a per-tool setting. Even with a per-tool configuration they would be broken (until the user configures otherwise). > It is quite possible people are using different tools for different > files in the same way as they use different merge drivers for > different files and want the setting disabled for a tool that does > semantic merging but enabled textual merges. I think your definition of what's possible and mine are very different. But this is actually what I was asking: an example. You are bringing a hypothetical "semantic mergetool" that would somehow benefit from having unconflicted lines. Can you explain how it would benefit? Also, neither Seth nor Junio responded to my example, can you? Do you agree there is no conflict here? echo Hello > BASE echo Hello > LOCAL echo Hello. > REMOTE git merge-file -p LOCAL BASE REMOTE Cheers. -- Felipe Contreras
