threads / patch / 54842

v3mergetool: add automerge configuration

Subject: [PATCH v3] mergetool: add automerge configuration

## tl;dr

7 messages between Dec 17, 2020 and Dec 18, 2020. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Felipe Contreras· Dec 17, 2020, 05:45 UTC · lore

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(+)
Show changes to 3 files +31 −0

Documentation/config/mergetool.txt, git-mergetool.sh, t/t7610-mergetool.sh

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
Johannes Sixt· Dec 17, 2020, 07:37 UTC · re: Felipe Contreras · lore

Re: [PATCH v3] mergetool: add automerge configuration

Am 17.12.20 um 06:45 schrieb Felipe Contreras:
Show 31 quoted lines
> 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.

Show 56 quoted lines
> 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
>
Felipe Contreras· Dec 17, 2020, 08:31 UTC · re: Johannes Sixt · lore

Re: [PATCH v3] mergetool: add automerge configuration

Johannes Sixt wrote:
Show 36 quoted lines
> 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
Junio C Hamano· Dec 18, 2020, 00:47 UTC · re: Felipe Contreras · lore

Re: [PATCH v3] mergetool: add automerge configuration

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 7 quoted lines
> 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.

Show 22 quoted lines
> +		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.
Show 14 quoted lines
> +	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
Felipe Contreras· Dec 18, 2020, 02:40 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] mergetool: add automerge configuration

Junio C Hamano wrote:
Show 15 quoted lines
> 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.

Show 44 quoted lines
> > +		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.
Show 5 quoted lines
> 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
Junio C Hamano· Dec 18, 2020, 10:34 UTC · re: Felipe Contreras · lore

Re: [PATCH v3] mergetool: add automerge configuration

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.

Felipe Contreras· Dec 18, 2020, 12:13 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] mergetool: add automerge configuration

Junio C Hamano wrote:
Show 7 quoted lines
> 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

← back to recent threads