[PATCH v3] mergetool: add automerge configuration

[PATCH v3] mergetool: add automerge configuration

[Felipe Contreras]

It doesn't make sense to display easily-solvable conflicts in the
different views of all mergetools.

Only the chunks that warrant conflict markers should be displayed.

In order to unobtrusively do this, 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

Original-idea-by: Seth House <seth@eseth.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/config/mergetool.txt |  3 +++
 git-mergetool.sh                   | 10 ++++++++++
 t/t7610-mergetool.sh               | 18 ++++++++++++++++++
 3 files changed, 31 insertions(+)

diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 16a27443a3..43af7a96f9 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::
+	Automatically resolve conflicts that don't require user intervention.
diff --git a/git-mergetool.sh b/git-mergetool.sh
index e3f6d543fb..6e86d3b492 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -274,6 +274,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 +323,15 @@ merge_file () {
 	checkout_staged_file 2 "$MERGED" "$LOCAL"
 	checkout_staged_file 3 "$MERGED" "$REMOTE"
 
+	if test "$(git config --bool mergetool.autoMerge)" = "true"
+	then
+		git merge-file --diff3 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3"
+		sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE"
+		sed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' "$DIFF3" >"$LOCAL"
+		sed -e '/^<<<<<<< /,/^=======\r\?$/d' -e '/^>>>>>>> /d' "$DIFF3" >"$REMOTE"
+		rm -- "$DIFF3"
+	fi
+
 	if test -z "$local_mode" || test -z "$remote_mode"
 	then
 		echo "Deleted merge conflict for '$MERGED':"
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 70afdd06fa..b75c91199b 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -828,4 +828,22 @@ test_expect_success 'mergetool -Oorder-file is honored' '
 	test_cmp expect actual
 '
 
+test_expect_success 'mergetool automerge' '
+	test_config mergetool.automerge true &&
+	test_when_finished "git reset --hard" &&
+	git checkout -b test${test_count}_b master &&
+	echo -e "base\n\na" >file1 &&
+	git commit -a -m "base" &&
+	echo -e "base\n\nc" >file1 &&
+	git commit -a -m "remote update" &&
+	git checkout -b test${test_count}_a HEAD~ &&
+	echo -e "local\n\nb" >file1 &&
+	git commit -a -m "local update" &&
+	test_must_fail git merge test${test_count}_b &&
+	yes "" | git mergetool file1 &&
+	echo -e "local\n\nc" >expect &&
+	test_cmp expect file1 &&
+	git commit -m "test resolved with mergetool"
+'
+
 test_done
-- 
2.30.0.rc0

Re: [PATCH v3] mergetool: add automerge configuration

[Johannes Sixt]

Am 17.12.20 um 06:45 schrieb Felipe Contreras:
> It doesn't make sense to display easily-solvable conflicts in the
> different views of all mergetools.
> 
> Only the chunks that warrant conflict markers should be displayed.
> 
> In order to unobtrusively do this, 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
> 
> Original-idea-by: Seth House <seth@eseth.com>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/config/mergetool.txt |  3 +++
>  git-mergetool.sh                   | 10 ++++++++++
>  t/t7610-mergetool.sh               | 18 ++++++++++++++++++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
> index 16a27443a3..43af7a96f9 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::
> +	Automatically resolve conflicts that don't require user intervention.

As I hinted in an earlier message, some tools do know how to deal with
unconflicted changes. This should be a setting in the tool driver, not a
user-visible setting.

> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index e3f6d543fb..6e86d3b492 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -274,6 +274,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 +323,15 @@ merge_file () {
>  	checkout_staged_file 2 "$MERGED" "$LOCAL"
>  	checkout_staged_file 3 "$MERGED" "$REMOTE"
>  
> +	if test "$(git config --bool mergetool.autoMerge)" = "true"
> +	then
> +		git merge-file --diff3 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3"
> +		sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE"
> +		sed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' "$DIFF3" >"$LOCAL"
> +		sed -e '/^<<<<<<< /,/^=======\r\?$/d' -e '/^>>>>>>> /d' "$DIFF3" >"$REMOTE"
> +		rm -- "$DIFF3"
> +	fi
> +
>  	if test -z "$local_mode" || test -z "$remote_mode"
>  	then
>  		echo "Deleted merge conflict for '$MERGED':"
> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> index 70afdd06fa..b75c91199b 100755
> --- a/t/t7610-mergetool.sh
> +++ b/t/t7610-mergetool.sh
> @@ -828,4 +828,22 @@ test_expect_success 'mergetool -Oorder-file is honored' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'mergetool automerge' '
> +	test_config mergetool.automerge true &&
> +	test_when_finished "git reset --hard" &&
> +	git checkout -b test${test_count}_b master &&
> +	echo -e "base\n\na" >file1 &&
> +	git commit -a -m "base" &&
> +	echo -e "base\n\nc" >file1 &&
> +	git commit -a -m "remote update" &&
> +	git checkout -b test${test_count}_a HEAD~ &&
> +	echo -e "local\n\nb" >file1 &&
> +	git commit -a -m "local update" &&
> +	test_must_fail git merge test${test_count}_b &&
> +	yes "" | git mergetool file1 &&
> +	echo -e "local\n\nc" >expect &&
> +	test_cmp expect file1 &&
> +	git commit -m "test resolved with mergetool"
> +'
> +
>  test_done
> 

Re: [PATCH v3] mergetool: add automerge configuration

[Felipe Contreras]

Johannes Sixt wrote:
> Am 17.12.20 um 06:45 schrieb Felipe Contreras:
> > It doesn't make sense to display easily-solvable conflicts in the
> > different views of all mergetools.
> > 
> > Only the chunks that warrant conflict markers should be displayed.
> > 
> > In order to unobtrusively do this, 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
> > 
> > Original-idea-by: Seth House <seth@eseth.com>
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  Documentation/config/mergetool.txt |  3 +++
> >  git-mergetool.sh                   | 10 ++++++++++
> >  t/t7610-mergetool.sh               | 18 ++++++++++++++++++
> >  3 files changed, 31 insertions(+)
> > 
> > diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
> > index 16a27443a3..43af7a96f9 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::
> > +	Automatically resolve conflicts that don't require user intervention.
> 
> As I hinted in an earlier message, some tools do know how to deal with
> unconflicted changes. This should be a setting in the tool driver, not a
> user-visible setting.

It could be done in both. We first should find a mergetool that is
negatively affected by this, which WinMerge doesn't seem to be one of
them:

 1. Before: https://snipboard.io/8JA5Oz.jpg
 2. After: https://snipboard.io/HUXnOg.jpg

That's a positive effect after the patch.

-- 
Felipe Contreras

Re: [PATCH v3] mergetool: add automerge configuration

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> It doesn't make sense to display easily-solvable conflicts in the
> different views of all mergetools.
>
> Only the chunks that warrant conflict markers should be displayed.
>
> In order to unobtrusively do this, add a new configuration:
> mergetool.autoMerge.

As pointed out by others, I think it makes more sense to have
mergetool.$tool.autoMerge, with optionally mergetool.autoMerge
that can be used as a fallback, and enable it by default.  If
we can make the default of enabling/disabling per tool, that
would be ideal (see my other reply on how to do these).

> +		git merge-file --diff3 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3"

Adding "--marker-size 7" to the command line would make the sed
scripts below more robust.

> +		sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE"
> +		sed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' "$DIFF3" >"$LOCAL"
> +		sed -e '/^<<<<<<< /,/^=======\r\?$/d' -e '/^>>>>>>> /d' "$DIFF3" >"$REMOTE"
> +		rm -- "$DIFF3"
> +	fi
> +
>  	if test -z "$local_mode" || test -z "$remote_mode"
>  	then
>  		echo "Deleted merge conflict for '$MERGED':"
> diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> index 70afdd06fa..b75c91199b 100755
> --- a/t/t7610-mergetool.sh
> +++ b/t/t7610-mergetool.sh
> @@ -828,4 +828,22 @@ test_expect_success 'mergetool -Oorder-file is honored' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'mergetool automerge' '
> +	test_config mergetool.automerge true &&
> +	test_when_finished "git reset --hard" &&
> +	git checkout -b test${test_count}_b master &&
> +	echo -e "base\n\na" >file1 &&

I think test-lint-shell-syntax should have complained about the use
of "echo -e" here (that's the reason why I queued the patch in 'seen'
initially but it does not appear in what finally got pushed out).

You can either use the plain-vanilla

	cat >file1 <<-\EOF &&
	...
	EOF

because there is nothing gained by saving number of lines with
reduced readability, or can use

	test_write_lines >file1 base "" "" a &&
	...
	test_write_lines >file1 base "" "" c &&
	...
	test_write_lines >file1 local "" "" b &&
	...
	test_write_lines >file1 local "" "" c &&
	...

which would both save number of lines while making it a bit clearer
which 'line' corresponds to which other 'line' in different
preparation of the same "file1".

Will expect a reroll.  Thanks.

> +	git commit -a -m "base" &&
> +	echo -e "base\n\nc" >file1 &&
> +	git commit -a -m "remote update" &&
> +	git checkout -b test${test_count}_a HEAD~ &&
> +	echo -e "local\n\nb" >file1 &&
> +	git commit -a -m "local update" &&
> +	test_must_fail git merge test${test_count}_b &&
> +	yes "" | git mergetool file1 &&
> +	echo -e "local\n\nc" >expect &&
> +	test_cmp expect file1 &&
> +	git commit -m "test resolved with mergetool"
> +'
> +
>  test_done

Re: [PATCH v3] mergetool: add automerge configuration

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > It doesn't make sense to display easily-solvable conflicts in the
> > different views of all mergetools.
> >
> > Only the chunks that warrant conflict markers should be displayed.
> >
> > In order to unobtrusively do this, add a new configuration:
> > mergetool.autoMerge.
> 
> As pointed out by others, I think it makes more sense to have
> mergetool.$tool.autoMerge, with optionally mergetool.autoMerge
> that can be used as a fallback, and enable it by default.  If
> we can make the default of enabling/disabling per tool, that
> would be ideal (see my other reply on how to do these).

I don't think that's the case. There is no tool the user wouldn't want
this enabled on.

> > +		git merge-file --diff3 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3"
> 
> Adding "--marker-size 7" to the command line would make the sed
> scripts below more robust.

Right, althought the user can't configure otherwise, and the default 7,
it can't hurt to add it.

> > +		sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE"
> > +		sed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' "$DIFF3" >"$LOCAL"
> > +		sed -e '/^<<<<<<< /,/^=======\r\?$/d' -e '/^>>>>>>> /d' "$DIFF3" >"$REMOTE"
> > +		rm -- "$DIFF3"
> > +	fi
> > +
> >  	if test -z "$local_mode" || test -z "$remote_mode"
> >  	then
> >  		echo "Deleted merge conflict for '$MERGED':"
> > diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
> > index 70afdd06fa..b75c91199b 100755
> > --- a/t/t7610-mergetool.sh
> > +++ b/t/t7610-mergetool.sh
> > @@ -828,4 +828,22 @@ test_expect_success 'mergetool -Oorder-file is honored' '
> >  	test_cmp expect actual
> >  '
> >  
> > +test_expect_success 'mergetool automerge' '
> > +	test_config mergetool.automerge true &&
> > +	test_when_finished "git reset --hard" &&
> > +	git checkout -b test${test_count}_b master &&
> > +	echo -e "base\n\na" >file1 &&
> 
> I think test-lint-shell-syntax should have complained about the use
> of "echo -e" here (that's the reason why I queued the patch in 'seen'
> initially but it does not appear in what finally got pushed out).
> 
> You can either use the plain-vanilla
> 
> 	cat >file1 <<-\EOF &&
> 	...
> 	EOF
> 
> because there is nothing gained by saving number of lines with
> reduced readability, or can use
> 
> 	test_write_lines >file1 base "" "" a &&
> 	...
> 	test_write_lines >file1 base "" "" c &&
> 	...
> 	test_write_lines >file1 local "" "" b &&
> 	...
> 	test_write_lines >file1 local "" "" c &&
> 	...

It's only one "", but OK.

> which would both save number of lines while making it a bit clearer
> which 'line' corresponds to which other 'line' in different
> preparation of the same "file1".
> 
> Will expect a reroll.  Thanks.

I have the next version ready, I'll wait for feedback from Seth
regarding tools that might want this turned off (which in my opinion are
zero).

Cheers.

-- 
Felipe Contreras

Re: [PATCH v3] mergetool: add automerge configuration

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> Right, althought the user can't configure otherwise, and the default 7,
> it can't hurt to add it.

Yes, that would make it certain that the code would survive the
change in the underlying xmerge part of the system.

I am still not sure what the right plan to deal with conflicts
recorded in the virtual ancestor.  Do we just close our eyes and
ignore it?

An easy way out may be to give an command line override so that
users can easily re-run mergetool with the feature disabled only for
a single invocation that went wrong, which would make it less likely
that users disable the feature permanently upon seeing just one case
of the new logic getting confused.

Re: [PATCH v3] mergetool: add automerge configuration

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > Right, althought the user can't configure otherwise, and the default 7,
> > it can't hurt to add it.
> 
> Yes, that would make it certain that the code would survive the
> change in the underlying xmerge part of the system.

Yes.

> I am still not sure what the right plan to deal with conflicts
> recorded in the virtual ancestor.  Do we just close our eyes and
> ignore it?

This is what we already do.

When a file has changes that don't bump into each other they are not
considered conflicts, for example:

 BASE:   sentence
 LOCAL:  sentence
 REMOTE: sentence.

Is that a conflict?

Whether you call these "trivial conflicts", or "non-conflicts" doesn't
matter, if the file only has changes of this kind, git considers the
file merged automatically.

If the merge has *all* changes of this kind, then the it is
automatically completed. No mergetool necessary.

> An easy way out may be to give an command line override so that
> users can easily re-run mergetool with the feature disabled only for
> a single invocation that went wrong,

Why would it go wrong?

Git has already determined there's no conflict.

Does it make sense to run "git mergetool
--consider-all-changes-as-conflicts" and go back in time before the
commit merge that had no conflicts was done, and run mergetool on all
the files that contain changes, even if git initially considered the
file as merged (since they contained no "true" conflicts)?

When does it make sense to open an instance of the mergetool on a file
that contains *zero* conflict markers?

-- 
Felipe Contreras