All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 0/1] mergetool: remove unconflicted lines
@ 2020-12-23  4:53 Felipe Contreras
  2020-12-23  4:53 ` [PATCH v5 1/1] mergetool: add automerge configuration Felipe Contreras
                   ` (2 more replies)
  0 siblings, 3 replies; 80+ messages in thread
From: Felipe Contreras @ 2020-12-23  4:53 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, David Aguilar, Johannes Sixt, Seth House,
	Felipe Contreras

There's not much to say other that what the commit message of the patch says.

Note: no feedback has been ignored; I replied to all the feedback, I didn't hear anything back.

Changes since v4:

 * Improved commit message with suggestions from Phillip Wood.

Felipe Contreras (1):
  mergetool: add automerge configuration

 Documentation/config/mergetool.txt |  3 +++
 git-mergetool.sh                   | 17 +++++++++++++++++
 t/t7610-mergetool.sh               | 18 ++++++++++++++++++
 3 files changed, 38 insertions(+)

Range-diff:
1:  776c1fbb97 ! 1:  2dc53f4dda mergetool: add automerge configuration
    @@ Metadata
      ## Commit message ##
         mergetool: add automerge configuration
     
    -    It doesn't make sense to display lines without conflicts in the
    -    different views of all mergetools.
    +    The purpose of mergetools is to resolve conflicts when git cannot
    +    automatically do so.
     
    -    Only the lines that warrant conflict markers should be displayed.
    +    In order to do that git has added markers in the specific areas that
    +    need resolving, which the user must manually fix. The tool is supposed
    +    to help with that.
     
    -    Most people would want this behavior on, but in case some don't; add a
    -    new configuration: mergetool.autoMerge.
    +    However, by passing the original BASE, LOCAL, and REMOTE files, many
    +    changes without conflict are presented to the user when in fact nothing
    +    needs to be done for those.
    +
    +    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 there are actual
    +    conflicts.
    +
    +    As most people will want the new behavior, we enable it by default.
    +    Users that do not want the new behavior can set the new configuration
    +    mergetool.autoMerge to false.
     
         See Seth House's blog post [1] for the idea, and the rationale.
     
-- 
2.30.0.rc1


^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH v5 1/1] mergetool: add automerge configuration
  2020-12-23  4:53 [PATCH v5 0/1] mergetool: remove unconflicted lines Felipe Contreras
@ 2020-12-23  4:53 ` Felipe Contreras
  2020-12-23 13:34   ` Junio C Hamano
  2020-12-24  9:24 ` [PATCH v5 0/1] mergetool: remove unconflicted lines Junio C Hamano
  2020-12-27 20:58 ` [PATCH v6 0/1] mergetool: add automerge configuration Seth House
  2 siblings, 1 reply; 80+ messages in thread
From: Felipe Contreras @ 2020-12-23  4:53 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, David Aguilar, Johannes Sixt, Seth House,
	Felipe Contreras

The purpose of mergetools is to resolve conflicts when git cannot
automatically do so.

In order to do that git has added markers in the specific areas that
need resolving, which the user must manually fix. The tool is supposed
to help with that.

However, by passing the original BASE, LOCAL, and REMOTE files, many
changes without conflict are presented to the user when in fact nothing
needs to be done for those.

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 there are actual
conflicts.

As most people will want the new behavior, we enable it by default.
Users that do not want the new behavior can set the new configuration
mergetool.autoMerge to false.

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                   | 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"
+	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"
+	then
+		auto_merge
+	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..ccabd04823 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 &&
+	test_write_lines >file1 base "" a &&
+	git commit -a -m "base" &&
+	test_write_lines >file1 base "" c &&
+	git commit -a -m "remote update" &&
+	git checkout -b test${test_count}_a HEAD~ &&
+	test_write_lines >file1 local "" b &&
+	git commit -a -m "local update" &&
+	test_must_fail git merge test${test_count}_b &&
+	yes "" | git mergetool file1 &&
+	test_write_lines >expect local "" c &&
+	test_cmp expect file1 &&
+	git commit -m "test resolved with mergetool"
+'
+
 test_done
-- 
2.30.0.rc1


^ permalink raw reply related	[flat|nested] 80+ messages in thread

* Re: [PATCH v5 1/1] mergetool: add automerge configuration
  2020-12-23  4:53 ` [PATCH v5 1/1] mergetool: add automerge configuration Felipe Contreras
@ 2020-12-23 13:34   ` Junio C Hamano
  2020-12-23 14:23     ` Felipe Contreras
  0 siblings, 1 reply; 80+ messages in thread
From: Junio C Hamano @ 2020-12-23 13:34 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git, David Aguilar, Johannes Sixt, Seth House

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

> +auto_merge () {
> +	git merge-file --diff3 --marker-size=7 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3"
> +	if test -s "$DIFF3"
> +	then

We do not want to ignore the exit status from the command.  IOW, I
think the above wants to be rather

	if git merge-file ... >"$DIFF3" &&
	   test -s "$DIFF3"
	then
		...

to catch a merge-file that writes halfway and then crashes (doing
the same check in different ways are probably possible, of course)

> +		sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE"

Does everybody's sed take "\?" and interprets it as zero-or-one?
POSIX uses BRE and it doesn't like \? as far as I recall, and "-E"
to force ERE is a GNUism.

> +		sed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' "$DIFF3" >"$LOCAL"
> +		sed -e '/^<<<<<<< /,/^=======\r\?$/d' -e '/^>>>>>>> /d' "$DIFF3" >"$REMOTE"

I'd feel safer if these resulting $BASE, $LOCAL and $REMOTE are
validated to be conflict-marker free (i.e. '^\([<|=>]\)\1\1\1\1\1\1'
does not appear) to make sure there was no funny virtual ancestor
that records a conflicted recursive merge result confused our logic.

When we see an unfortunate sign that it happened, we can revert the
automerge and let the tool handle the original input.

> +	fi
> +	rm -- "$DIFF3"
> +}
> +

"$DIFF3" is always created (unless shell redirection into it fails),
so "rm --" would be fine in practice, I guess, but "rm -f --" would
not hurt.

> +	DIFF3="$MERGETOOL_TMPDIR/${BASE}_DIFF3_$$$ext"

$MERGETOOL_TMPDIR is either "mktemp -d -t "git-mergetool-XXXXXX" or
".".  Also, we liberally pass "$DIFF3" to "sed" as an argument and
assume that the command would take it as a filename and not an
option.

For the above reason, "rm --", while it is not wrong per-se, can be
just a simple "rm", as there is no funny leading letters in "$DIFF3"
that requires disambiguation.


^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v5 1/1] mergetool: add automerge configuration
  2020-12-23 13:34   ` Junio C Hamano
@ 2020-12-23 14:23     ` Felipe Contreras
  2020-12-23 20:21       ` Junio C Hamano
  0 siblings, 1 reply; 80+ messages in thread
From: Felipe Contreras @ 2020-12-23 14:23 UTC (permalink / raw)
  To: Junio C Hamano, Felipe Contreras
  Cc: git, David Aguilar, Johannes Sixt, Seth House

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > +auto_merge () {
> > +	git merge-file --diff3 --marker-size=7 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3"
> > +	if test -s "$DIFF3"
> > +	then
> 
> We do not want to ignore the exit status from the command.  IOW, I
> think the above wants to be rather
> 
> 	if git merge-file ... >"$DIFF3" &&
> 	   test -s "$DIFF3"
> 	then
> 		...

That doesn't work.

"git merge-file" always returns non-zero status when it succeeds (it's
the number of conflicts generated).

> > +		sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE"
> 
> Does everybody's sed take "\?" and interprets it as zero-or-one?

I don't know.

> POSIX uses BRE and it doesn't like \? as far as I recall, and "-E"
> to force ERE is a GNUism.

Another possibility is \s\*. It's less specific though.

> > +		sed -e '/^||||||| /,/^>>>>>>> /d' -e '/^<<<<<<< /d' "$DIFF3" >"$LOCAL"
> > +		sed -e '/^<<<<<<< /,/^=======\r\?$/d' -e '/^>>>>>>> /d' "$DIFF3" >"$REMOTE"
> 
> I'd feel safer if these resulting $BASE, $LOCAL and $REMOTE are
> validated to be conflict-marker free (i.e. '^\([<|=>]\)\1\1\1\1\1\1'
> does not appear) to make sure there was no funny virtual ancestor
> that records a conflicted recursive merge result confused our logic.
> 
> When we see an unfortunate sign that it happened, we can revert the
> automerge and let the tool handle the original input.

What if the original file does have these markers?

Which is probably something we should be checking beforehand and not
attempt an automerge in those cases.

Or we could add the --base option to "git merge-file" so we don't have
to do that work by hand.

> > +	fi
> > +	rm -- "$DIFF3"
> > +}
> > +
> 
> "$DIFF3" is always created (unless shell redirection into it fails),
> so "rm --" would be fine in practice, I guess, but "rm -f --" would
> not hurt.

I just did the same as below:

  rm -- "$BACKUP"

> > +	DIFF3="$MERGETOOL_TMPDIR/${BASE}_DIFF3_$$$ext"
> 
> $MERGETOOL_TMPDIR is either "mktemp -d -t "git-mergetool-XXXXXX" or
> ".".  Also, we liberally pass "$DIFF3" to "sed" as an argument and
> assume that the command would take it as a filename and not an
> option.
> 
> For the above reason, "rm --", while it is not wrong per-se, can be
> just a simple "rm", as there is no funny leading letters in "$DIFF3"
> that requires disambiguation.

Other parts of the file do this:

  rm -f -- "$LOCAL" "$REMOTE" "$BASE" "$BACKUP"

I'm just following what the script already does.

Cheers.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v5 1/1] mergetool: add automerge configuration
  2020-12-23 14:23     ` Felipe Contreras
@ 2020-12-23 20:21       ` Junio C Hamano
  2020-12-24  0:14         ` Felipe Contreras
  0 siblings, 1 reply; 80+ messages in thread
From: Junio C Hamano @ 2020-12-23 20:21 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git, David Aguilar, Johannes Sixt, Seth House

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

> Junio C Hamano wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>> 
>> > +auto_merge () {
>> > +	git merge-file --diff3 --marker-size=7 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3"
>> > +	if test -s "$DIFF3"
>> > +	then
>> 
>> We do not want to ignore the exit status from the command.  IOW, I
>> think the above wants to be rather
>> 
>> 	if git merge-file ... >"$DIFF3" &&
>> 	   test -s "$DIFF3"
>> 	then
>> 		...
>
> That doesn't work.
>
> "git merge-file" always returns non-zero status when it succeeds (it's
> the number of conflicts generated).

Ah, I forgot about that one.  I think "the number of conflicts" was
a UI mistake (the original that it mimics is "merge" from RCS suite,
which uses 1 and 2 for "conflicts" and "trouble") but we know we
will get conflicts, so it is wrong to expect success from the
command.  Deliberately ignoring the return status is the right thing
to do.

> What if the original file does have these markers?
>
> Which is probably something we should be checking beforehand and not
> attempt an automerge in those cases.

Yes, that is a much better approach to avoid unnecessary work.

When we made the conflict marker length configurable, we were hoping
that we no longer have to worry about the cases where payload files
(original or ours or theirs) have lines that are confusingly similar
to the conflict markers, but because we are interfacing external tools
that are unaware of the facility, it probably would not help us in
this case all that much.

FWIW, we use a fiarly large size for our own files in t/ and
Documentation/ directories ourselves, and it does help topic branch
merges somewhat frequently.

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v5 1/1] mergetool: add automerge configuration
  2020-12-23 20:21       ` Junio C Hamano
@ 2020-12-24  0:14         ` Felipe Contreras
  2020-12-24  0:32           ` Junio C Hamano
  0 siblings, 1 reply; 80+ messages in thread
From: Felipe Contreras @ 2020-12-24  0:14 UTC (permalink / raw)
  To: Junio C Hamano, Felipe Contreras
  Cc: git, David Aguilar, Johannes Sixt, Seth House

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > Junio C Hamano wrote:
> >> Felipe Contreras <felipe.contreras@gmail.com> writes:
> >> 
> >> > +auto_merge () {
> >> > +	git merge-file --diff3 --marker-size=7 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3"
> >> > +	if test -s "$DIFF3"
> >> > +	then
> >> 
> >> We do not want to ignore the exit status from the command.  IOW, I
> >> think the above wants to be rather
> >> 
> >> 	if git merge-file ... >"$DIFF3" &&
> >> 	   test -s "$DIFF3"
> >> 	then
> >> 		...
> >
> > That doesn't work.
> >
> > "git merge-file" always returns non-zero status when it succeeds (it's
> > the number of conflicts generated).
> 
> Ah, I forgot about that one.  I think "the number of conflicts" was
> a UI mistake (the original that it mimics is "merge" from RCS suite,
> which uses 1 and 2 for "conflicts" and "trouble") but we know we
> will get conflicts, so it is wrong to expect success from the
> command.  Deliberately ignoring the return status is the right thing
> to do.

I agree. My bet is that nobody is checking the return status of "git
merge-file" to find out the number of conflicts. Plus, how can you check
the difference between 255 conflicts and error -1?

But that's the situation we are in now.

> > What if the original file does have these markers?
> >
> > Which is probably something we should be checking beforehand and not
> > attempt an automerge in those cases.
> 
> Yes, that is a much better approach to avoid unnecessary work.
> 
> When we made the conflict marker length configurable, we were hoping
> that we no longer have to worry about the cases where payload files
> (original or ours or theirs) have lines that are confusingly similar
> to the conflict markers, but because we are interfacing external tools
> that are unaware of the facility, it probably would not help us in
> this case all that much.
> 
> FWIW, we use a fiarly large size for our own files in t/ and
> Documentation/ directories ourselves, and it does help topic branch
> merges somewhat frequently.

We could do something like --marker-size=13 to minimize the chances of
that happening.

In that case I would prefer '/^<\{13\} /' (to avoid too many
characters). I see those regexes used elsewhere in git, but I don't know
how portable that is.

If we wanted to make sure none of those markers remain it's not enough
to check for '^[<|=>]{13}', what follows up should be a space, or some
delimiter, not another < for example. So maybe '^[<|=>]{13}[^<|=>]'?

So, do we want those three things?

 1. A non-standard marker-size
 2. Check beforehand the existence of those markers and disable
    automerge
 3. Check afterwards the existence of those markers and disable
    automerge

Cheers.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v5 1/1] mergetool: add automerge configuration
  2020-12-24  0:14         ` Felipe Contreras
@ 2020-12-24  0:32           ` Junio C Hamano
  2020-12-24  1:36             ` Felipe Contreras
  0 siblings, 1 reply; 80+ messages in thread
From: Junio C Hamano @ 2020-12-24  0:32 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git, David Aguilar, Johannes Sixt, Seth House

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

>> Ah, I forgot about that one.  I think "the number of conflicts" was
>> a UI mistake (the original that it mimics is "merge" from RCS suite,
>> which uses 1 and 2 for "conflicts" and "trouble") but we know we
>> will get conflicts, so it is wrong to expect success from the
>> command.  Deliberately ignoring the return status is the right thing
>> to do.
>
> I agree. My bet is that nobody is checking the return status of "git
> merge-file" to find out the number of conflicts. Plus, how can you check
> the difference between 255 conflicts and error -1?

Yup, I already mentioned UI mistake so you do not have to repeat it
to consume more bandwidth.  We're in agreement already.

> We could do something like --marker-size=13 to minimize the chances of
> that happening.
>
> In that case I would prefer '/^<\{13\} /' (to avoid too many
> characters). I see those regexes used elsewhere in git, but I don't know
> how portable that is.

If it is used elsewhere with "sed", then that would be OK, but if it
is not with "sed" but with "grep", that's quite a different story.

> So, do we want those three things?
>
>  1. A non-standard marker-size
>  2. Check beforehand the existence of those markers and disable
>     automerge
>  3. Check afterwards the existence of those markers and disable
>     automerge

I do not think 3 is needed if we do 2 and I do not think 1 would
particularly be useful *UNLESS* the code consults with the attribute
system to see what marker size the path uses to avoid crashing with
the non-standard marker-size the path already uses.

So the easiest would be not to do anything for now, with a note
about known limitations in the doc.  The second easiest would be to
do 2. alone.  We could do 1. to be more complete but I tend to think
that it is better to leave it as #leftoverbits.




^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v5 1/1] mergetool: add automerge configuration
  2020-12-24  0:32           ` Junio C Hamano
@ 2020-12-24  1:36             ` Felipe Contreras
  2020-12-24  6:17               ` Junio C Hamano
  0 siblings, 1 reply; 80+ messages in thread
From: Felipe Contreras @ 2020-12-24  1:36 UTC (permalink / raw)
  To: Junio C Hamano, Felipe Contreras
  Cc: git, David Aguilar, Johannes Sixt, Seth House

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> >> Ah, I forgot about that one.  I think "the number of conflicts" was
> >> a UI mistake (the original that it mimics is "merge" from RCS suite,
> >> which uses 1 and 2 for "conflicts" and "trouble") but we know we
> >> will get conflicts, so it is wrong to expect success from the
> >> command.  Deliberately ignoring the return status is the right thing
> >> to do.
> >
> > I agree. My bet is that nobody is checking the return status of "git
> > merge-file" to find out the number of conflicts. Plus, how can you check
> > the difference between 255 conflicts and error -1?
> 
> Yup, I already mentioned UI mistake so you do not have to repeat

You said it was a UI mistake, not me. I am a different mind than yours.

This [1] is the first time *you* communicated it was a UI mistake.

This [2] is the first time *I* communicated it was a UI mistake.

I communicated that fact after you, so I did not repeat anything,
because I hadn't said that before. *You* did, not *me*.

> it to consume more bandwidth.

This is what is consuming bandwidth.

Not me stating *for the first time* that I agree what you just stated.

You could have skipped what I said *for the first time*, if you didn't
find it particularly interesting, and that would have saved bandwidth.

> > We could do something like --marker-size=13 to minimize the chances of
> > that happening.
> >
> > In that case I would prefer '/^<\{13\} /' (to avoid too many
> > characters). I see those regexes used elsewhere in git, but I don't know
> > how portable that is.
> 
> If it is used elsewhere with "sed", then that would be OK, but if it
> is not with "sed" but with "grep", that's quite a different story.

In t/t3427-rebase-subtree.sh there is:

  sed -e "s%\([0-9a-f]\{40\} \)files_subtree/%\1%"

Not sure if that counts. There's other places in the tests.

However, I don't see the point if the marker-size is a low enough number, like 7.

> > So, do we want those three things?
> >
> >  1. A non-standard marker-size
> >  2. Check beforehand the existence of those markers and disable
> >     automerge
> >  3. Check afterwards the existence of those markers and disable
> >     automerge
> 
> I do not think 3 is needed if we do 2 and I do not think 1 would
> particularly be useful *UNLESS* the code consults with the attribute
> system to see what marker size the path uses to avoid crashing with
> the non-standard marker-size the path already uses.

But what is more likely? a) That the marker-size is 7 (the default), or
b) that the marker-size is not the default, but that there's a
marker-size attribute *and* the value is precisely 13?

I think a) is way more likely than b).

> So the easiest would be not to do anything for now, with a note
> about known limitations in the doc.  The second easiest would be to
> do 2. alone.  We could do 1. to be more complete but I tend to think
> that it is better to leave it as #leftoverbits.

OK. I think 1. is low-hanging fruit, but I'm fine with not doing
anything, or trying 2.

I don't think 2. would be that hard, so I will try that before
re-rolling the series.

(unless somebody replies to my other pending arguments)

Cheers.

[1] https://lore.kernel.org/git/xmqqblek8e94.fsf@gitster.c.googlers.com/
[2] https://lore.kernel.org/git/5fe3dd62e12f8_7855a2081f@natae.notmuch/

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v5 1/1] mergetool: add automerge configuration
  2020-12-24  1:36             ` Felipe Contreras
@ 2020-12-24  6:17               ` Junio C Hamano
  2020-12-24 15:59                 ` Felipe Contreras
  0 siblings, 1 reply; 80+ messages in thread
From: Junio C Hamano @ 2020-12-24  6:17 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git, David Aguilar, Johannes Sixt, Seth House

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

>> Yup, I already mentioned UI mistake so you do not have to repeat
>
> You said it was a UI mistake, not me. I am a different mind than yours.

Yes, but the point is that I do not need to nor particularly want to
hear your opinion on the behaviour of "git merge-file".  I know (and
others reading the thread on the list also know) that the exit code
of the command is misdesigned already.

> I communicated that fact after you, so I did not repeat anything,
> because I hadn't said that before. *You* did, not *me*.

Again, please realize that on list discussion is a team effort to
come up together a better design of a shared solution.  And if you
already know that (I don't read your mind ;-), please act like you
do, too.

Thanks.


^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v5 0/1] mergetool: remove unconflicted lines
  2020-12-23  4:53 [PATCH v5 0/1] mergetool: remove unconflicted lines Felipe Contreras
  2020-12-23  4:53 ` [PATCH v5 1/1] mergetool: add automerge configuration Felipe Contreras
@ 2020-12-24  9:24 ` Junio C Hamano
  2020-12-24 16:16   ` Felipe Contreras
  2020-12-30  5:47   ` Johannes Schindelin
  2020-12-27 20:58 ` [PATCH v6 0/1] mergetool: add automerge configuration Seth House
  2 siblings, 2 replies; 80+ messages in thread
From: Junio C Hamano @ 2020-12-24  9:24 UTC (permalink / raw)
  To: Felipe Contreras, Johannes Schindelin
  Cc: git, David Aguilar, Johannes Sixt, Seth House

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

> There's not much to say other that what the commit message of the patch says.
>
> Note: no feedback has been ignored; I replied to all the feedback, I didn't hear anything back.
>
> Changes since v4:
>
>  * Improved commit message with suggestions from Phillip Wood.
>
> Felipe Contreras (1):
>   mergetool: add automerge configuration

This breakage is possibly a fallout from either this patch or
1e2ae142 (t7[5-9]*: adjust the references to the default branch name
"main", 2020-11-18).

  https://github.com/git/git/runs/1602803804#step:7:10358

I cannot quite tell how the two strings compared with 'test' on
output line 10355 are different in the output, though.

Thanks.

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v5 1/1] mergetool: add automerge configuration
  2020-12-24  6:17               ` Junio C Hamano
@ 2020-12-24 15:59                 ` Felipe Contreras
  2020-12-24 22:32                   ` Junio C Hamano
  0 siblings, 1 reply; 80+ messages in thread
From: Felipe Contreras @ 2020-12-24 15:59 UTC (permalink / raw)
  To: Junio C Hamano, Felipe Contreras
  Cc: git, David Aguilar, Johannes Sixt, Seth House

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> >> Yup, I already mentioned UI mistake so you do not have to repeat
> >
> > You said it was a UI mistake, not me. I am a different mind than yours.
> 
> Yes, but the point is that I do not need to nor particularly want to
> hear your opinion on the behaviour of "git merge-file".

Then disregard the comment.

> I know (and others reading the thread on the list also know) that the
> exit code of the command is misdesigned already.

Unless you can read minds, you don't know that.

And even if you do, I don't know what you know. I can't read your mind.

Plus, they can disregard the comment as well.

> > I communicated that fact after you, so I did not repeat anything,
> > because I hadn't said that before. *You* did, not *me*.
> 
> Again, please realize that on list discussion is a team effort to
> come up together a better design of a shared solution.

Which is why agreement in a team with different minds and different
viewpoints is important.

Just to show a few instances of Jeff King telling you he agrees with
you:

 1. "I agree it's not all that useful in that example" [1]. 16 Dec 2020
 2. "I agree with the current definition" [2]. 18 Dec 2020 (same thread)
 3. "I agree the two should behave the same" [3]. 18 Dec 2020 (same
    mail)

The fact that you value Jeff King's agreement and don't care what some
other members in the community think, is a personal vale judgement, and
doesn't necessarily mean the viewpoints of such community members are
objectively worthless.

Cheers.

[1] https://lore.kernel.org/git/X9pUc2HXUr3+WHbR@coredump.intra.peff.net/
[2] https://lore.kernel.org/git/X9xJ6BHM9VY0%2FyLs@coredump.intra.peff.net/
[3] https://lore.kernel.org/git/X9xJ6BHM9VY0%2FyLs@coredump.intra.peff.net/

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v5 0/1] mergetool: remove unconflicted lines
  2020-12-24  9:24 ` [PATCH v5 0/1] mergetool: remove unconflicted lines Junio C Hamano
@ 2020-12-24 16:16   ` Felipe Contreras
  2020-12-30  5:47   ` Johannes Schindelin
  1 sibling, 0 replies; 80+ messages in thread
From: Felipe Contreras @ 2020-12-24 16:16 UTC (permalink / raw)
  To: Junio C Hamano, Felipe Contreras, Johannes Schindelin
  Cc: git, David Aguilar, Johannes Sixt, Seth House

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > There's not much to say other that what the commit message of the patch says.
> >
> > Note: no feedback has been ignored; I replied to all the feedback, I didn't hear anything back.
> >
> > Changes since v4:
> >
> >  * Improved commit message with suggestions from Phillip Wood.
> >
> > Felipe Contreras (1):
> >   mergetool: add automerge configuration
> 
> This breakage is possibly a fallout from either this patch or
> 1e2ae142 (t7[5-9]*: adjust the references to the default branch name
> "main", 2020-11-18).
> 
>   https://github.com/git/git/runs/1602803804#step:7:10358

It seems likely it's the mergetool patch.

This regex '/^=======\r\?$/' is supposed to handle the crlf situation.

I can't imagine what would be different in Windows regarding that
situation.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v5 1/1] mergetool: add automerge configuration
  2020-12-24 15:59                 ` Felipe Contreras
@ 2020-12-24 22:32                   ` Junio C Hamano
  2020-12-27 18:01                     ` Felipe Contreras
  0 siblings, 1 reply; 80+ messages in thread
From: Junio C Hamano @ 2020-12-24 22:32 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git, David Aguilar, Johannes Sixt, Seth House

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

> Junio C Hamano wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>> 
>> >> Yup, I already mentioned UI mistake so you do not have to repeat
>> >
>> > You said it was a UI mistake, not me. I am a different mind than yours.
>> 
>> Yes, but the point is that I do not need to nor particularly want to
>> hear your opinion on the behaviour of "git merge-file".
>
>> I know (and others reading the thread on the list also know) that the
>> exit code of the command is misdesigned already.
>
> Unless you can read minds, you don't know that.

Actually I do, because they heard from me already ;-).  If this were
the case where our messages crossed, perhaps, but in this case yours
was a response to my message.

>> Again, please realize that on list discussion is a team effort to
>> come up together a better design of a shared solution.
>
> Which is why agreement in a team with different minds and different
> viewpoints is important.

It is not like opinions on all points are important.  Whether the
exit code from merge-file is or is not a UI mistake does NOT have
any influence on what we were discussing.  My opinion is that exit
code from merge-file is a UI mistake, but even if you disagree with
that, that would not change the conclusion we already reached that
the code should ignore its exit status, like you originally wrote.

I am already trying to ignore your opinions on things that do not
matter in the context of this project, as you told me earlier ;-)
But just like patches, messages are written only once but read by
many people, so I'd always aim for reducing noise at the source.

Anyway, happy holidays and pleasant new year to you and to
everybody.

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v5 1/1] mergetool: add automerge configuration
  2020-12-24 22:32                   ` Junio C Hamano
@ 2020-12-27 18:01                     ` Felipe Contreras
  0 siblings, 0 replies; 80+ messages in thread
From: Felipe Contreras @ 2020-12-27 18:01 UTC (permalink / raw)
  To: Junio C Hamano, Felipe Contreras
  Cc: git, David Aguilar, Johannes Sixt, Seth House

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > Junio C Hamano wrote:
> >> Felipe Contreras <felipe.contreras@gmail.com> writes:
> >> 
> >> >> Yup, I already mentioned UI mistake so you do not have to repeat
> >> >
> >> > You said it was a UI mistake, not me. I am a different mind than yours.
> >> 
> >> Yes, but the point is that I do not need to nor particularly want to
> >> hear your opinion on the behaviour of "git merge-file".
> >
> >> I know (and others reading the thread on the list also know) that the
> >> exit code of the command is misdesigned already.
> >
> > Unless you can read minds, you don't know that.
> 
> Actually I do, because they heard from me already ;-).

They heard that you *think* it's a UI mistake.

The fact that you think something is a mistake doesn't necessarily mean
it's actually a mistake, and other community members might think
otherwise.

You do not dictate what others on the list know.

> >> Again, please realize that on list discussion is a team effort to
> >> come up together a better design of a shared solution.
> >
> > Which is why agreement in a team with different minds and different
> > viewpoints is important.
> 
> It is not like opinions on all points are important.  Whether the
> exit code from merge-file is or is not a UI mistake does NOT have
> any influence on what we were discussing.

Which is why I initially did not express such an opinion.

But you did, presumably you had some reason to do so, so I simply
did the same and expressed mine.

> I am already trying to ignore your opinions on things that do not
> matter in the context of this project, as you told me earlier ;-)
> But just like patches, messages are written only once but read by
> many people, so I'd always aim for reducing noise at the source.

What you consider noise others might not.

Good writers say you should not assume what your readers know.

Yes, some readers might think exactly like you do, and they don't need
what you consider obvious information. But for every person that
thinks exactly like you, there are dozens that don't, and it's those you
should keep in mind.

Most people err on the side of not providing enough information to the
minds dissimilar to theirs.

This is called the curse of knowledge [1].

I try not to do that.

> Anyway, happy holidays and pleasant new year to you and to
> everybody.

Same to you.

Cheers.

[1] https://en.wikipedia.org/wiki/Curse_of_knowledge

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH v6 0/1] mergetool: add automerge configuration
  2020-12-23  4:53 [PATCH v5 0/1] mergetool: remove unconflicted lines Felipe Contreras
  2020-12-23  4:53 ` [PATCH v5 1/1] mergetool: add automerge configuration Felipe Contreras
  2020-12-24  9:24 ` [PATCH v5 0/1] mergetool: remove unconflicted lines Junio C Hamano
@ 2020-12-27 20:58 ` Seth House
  2020-12-27 20:58   ` [PATCH v6 1/2] " Seth House
                     ` (3 more replies)
  2 siblings, 4 replies; 80+ messages in thread
From: Seth House @ 2020-12-27 20:58 UTC (permalink / raw)
  To: git
  Cc: Seth House, Junio C Hamano, David Aguilar, Johannes Sixt,
	Felipe Contreras

Sorry for the slow turnaround on this. I haven't used Git via email
patches before now so it took me quite a few hours to read through
tutorials, configure git-send-email and fight missing Perl libs. Please
let me know if I did anything incorrectly! I should be able to
contribute more quickly from now on.

Changes since v5:

 * Add per-tool configuration that Felipe has a "deep philosophical"
   opposition to adding.

Felipe Contreras (1):
  mergetool: add automerge configuration

Seth House (1):
  mergetool: Add per-tool support for the autoMerge flag

 Documentation/config/mergetool.txt |  6 ++++++
 git-mergetool.sh                   | 13 +++++++++++++
 t/t7610-mergetool.sh               | 18 ++++++++++++++++++
 3 files changed, 37 insertions(+)

-- 
2.29.2


^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH v6 1/2] mergetool: add automerge configuration
  2020-12-27 20:58 ` [PATCH v6 0/1] mergetool: add automerge configuration Seth House
@ 2020-12-27 20:58   ` Seth House
  2020-12-27 22:06     ` Junio C Hamano
  2020-12-27 20:58   ` [PATCH v6 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 80+ messages in thread
From: Seth House @ 2020-12-27 20:58 UTC (permalink / raw)
  To: git; +Cc: Felipe Contreras, Seth House

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

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.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* [PATCH v6 2/2] mergetool: Add per-tool support for the autoMerge flag
  2020-12-27 20:58 ` [PATCH v6 0/1] mergetool: add automerge configuration Seth House
  2020-12-27 20:58   ` [PATCH v6 1/2] " Seth House
@ 2020-12-27 20:58   ` Seth House
  2020-12-27 22:31     ` Junio C Hamano
  2020-12-28  0:41   ` [PATCH v7 0/2] mergetool: add automerge configuration Seth House
  2020-12-28  1:02   ` [PATCH v6 0/1] " Felipe Contreras
  3 siblings, 1 reply; 80+ messages in thread
From: Seth House @ 2020-12-27 20:58 UTC (permalink / raw)
  To: git; +Cc: Seth House

Keep the global mergetool flag and add a per-tool override flag so that
users may enable the flag for one tool and disable it for another.

Signed-off-by: Seth House <seth@eseth.com>
---
 Documentation/config/mergetool.txt | 3 +++
 git-mergetool.sh                   | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 43af7a96f9..7f32281a61 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -21,6 +21,9 @@ mergetool.<tool>.trustExitCode::
 	if the file has been updated, otherwise the user is prompted to
 	indicate the success of the merge.
 
+mergetool.<tool>.autoMerge::
+	Automatically resolve conflicts that don't require user intervention.
+
 mergetool.meld.hasOutput::
 	Older versions of `meld` do not support the `--output` option.
 	Git will attempt to detect whether `meld` supports `--output`
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 6e86d3b492..81df301734 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -323,7 +323,10 @@ merge_file () {
 	checkout_staged_file 2 "$MERGED" "$LOCAL"
 	checkout_staged_file 3 "$MERGED" "$REMOTE"
 
-	if test "$(git config --bool mergetool.autoMerge)" = "true"
+	if test "$(
+		git config --get --bool "mergetool.$merge_tool.automerge" ||
+		git config --get --bool "mergetool.automerge" ||
+		echo true)" = true
 	then
 		git merge-file --diff3 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3"
 		sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE"
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* Re: [PATCH v6 1/2] mergetool: add automerge configuration
  2020-12-27 20:58   ` [PATCH v6 1/2] " Seth House
@ 2020-12-27 22:06     ` Junio C Hamano
  2020-12-27 22:29       ` Seth House
  0 siblings, 1 reply; 80+ messages in thread
From: Junio C Hamano @ 2020-12-27 22:06 UTC (permalink / raw)
  To: Seth House; +Cc: git, Felipe Contreras

Seth House <seth@eseth.com> writes:

> ...
> 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>
> ---

Missing Sign-off as a relayer.

> 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 &&

These do not seem to be taken from the version that has been
improved by reviwer comments after v3.

> +	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

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v6 1/2] mergetool: add automerge configuration
  2020-12-27 22:06     ` Junio C Hamano
@ 2020-12-27 22:29       ` Seth House
  2020-12-27 22:59         ` Junio C Hamano
  0 siblings, 1 reply; 80+ messages in thread
From: Seth House @ 2020-12-27 22:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Felipe Contreras

On Sun, Dec 27, 2020 at 02:06:58PM -0800, Junio C Hamano wrote:
> Missing Sign-off as a relayer.

I haven't come across that in the docs on contributing to Git and my
Google searches aren't helping. Do you mind pointing me to what to add?

> These do not seem to be taken from the version that has been
> improved by reviwer comments after v3.

Whoops! Thanks for the catch. Seems I fished the wrong version out of my
email. Created a new v7 based off the correct v5.


^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v6 2/2] mergetool: Add per-tool support for the autoMerge flag
  2020-12-27 20:58   ` [PATCH v6 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House
@ 2020-12-27 22:31     ` Junio C Hamano
  0 siblings, 0 replies; 80+ messages in thread
From: Junio C Hamano @ 2020-12-27 22:31 UTC (permalink / raw)
  To: Seth House; +Cc: git

Seth House <seth@eseth.com> writes:

> Keep the global mergetool flag and add a per-tool override flag so that
> users may enable the flag for one tool and disable it for another.
>
> Signed-off-by: Seth House <seth@eseth.com>
> ---
>  Documentation/config/mergetool.txt | 3 +++
>  git-mergetool.sh                   | 5 ++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
> index 43af7a96f9..7f32281a61 100644
> --- a/Documentation/config/mergetool.txt
> +++ b/Documentation/config/mergetool.txt
> @@ -21,6 +21,9 @@ mergetool.<tool>.trustExitCode::
>  	if the file has been updated, otherwise the user is prompted to
>  	indicate the success of the merge.
>  
> +mergetool.<tool>.autoMerge::
> +	Automatically resolve conflicts that don't require user intervention.
> +
>  mergetool.meld.hasOutput::
>  	Older versions of `meld` do not support the `--output` option.
>  	Git will attempt to detect whether `meld` supports `--output`
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 6e86d3b492..81df301734 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -323,7 +323,10 @@ merge_file () {
>  	checkout_staged_file 2 "$MERGED" "$LOCAL"
>  	checkout_staged_file 3 "$MERGED" "$REMOTE"
>  
> -	if test "$(git config --bool mergetool.autoMerge)" = "true"
> +	if test "$(
> +		git config --get --bool "mergetool.$merge_tool.automerge" ||
> +		git config --get --bool "mergetool.automerge" ||
> +		echo true)" = true

Your [v6 1/2] that you build this step on does not enable the
feature by default, but this step does; it deserves to be documented
and mentioned in the proposed log message.

But I think you'd want to build this step on top of newer one, if
only to take the portability fix to the tests, and that patch
enables the feature by default, so ...

>  	then
>  		git merge-file --diff3 -q -p "$LOCAL" "$BASE" "$REMOTE" >"$DIFF3"
>  		sed -e '/^<<<<<<< /,/^||||||| /d' -e '/^=======\r\?$/,/^>>>>>>> /d' "$DIFF3" >"$BASE"

Thanks.

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v6 1/2] mergetool: add automerge configuration
  2020-12-27 22:29       ` Seth House
@ 2020-12-27 22:59         ` Junio C Hamano
  0 siblings, 0 replies; 80+ messages in thread
From: Junio C Hamano @ 2020-12-27 22:59 UTC (permalink / raw)
  To: Seth House; +Cc: git, Felipe Contreras

Seth House <seth@eseth.com> writes:

> On Sun, Dec 27, 2020 at 02:06:58PM -0800, Junio C Hamano wrote:
>> Missing Sign-off as a relayer.
>
> I haven't come across that in the docs on contributing to Git and my
> Google searches aren't helping. Do you mind pointing me to what to add?

Documentation/SubmittingPatches#sign-off

    === Certify your work by adding your `Signed-off-by` trailer

    To improve tracking of who did what, we ask you to certify that you
    wrote the patch or have the right to pass it on under the same license
    as ours, by "signing off" your patch.  Without sign-off, we cannot
    accept your patches.

    If you can certify the below D-C-O:

    [[dco]]
    .Developer's Certificate of Origin 1.1
    ____
    By making a contribution to this project, I certify that:

    a. The contribution was created in whole or in part by me and I
       have the right to submit it under the open source license
       indicated in the file; or

    b. The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

    c. The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

    d. I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.
    ____

    you add a "Signed-off-by" trailer to your commit, that looks like
    this:

    ....
            Signed-off-by: Random J Developer <random@developer.example.org>
    ....


So, you'd add your own signed-off-by trailer at the end of the
trailer list.

https://lore.kernel.org/git/pull.805.git.1607091741254.gitgitgadget@gmail.com/

for an example where Johannes Schindelin picked up a patch written
by Dennis Ameling and relayed it to the list.

Thanks.

^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH v7 0/2] mergetool: add automerge configuration
  2020-12-27 20:58 ` [PATCH v6 0/1] mergetool: add automerge configuration Seth House
  2020-12-27 20:58   ` [PATCH v6 1/2] " Seth House
  2020-12-27 20:58   ` [PATCH v6 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House
@ 2020-12-28  0:41   ` Seth House
  2020-12-28  0:41     ` [PATCH v7 1/2] " Seth House
                       ` (3 more replies)
  2020-12-28  1:02   ` [PATCH v6 0/1] " Felipe Contreras
  3 siblings, 4 replies; 80+ messages in thread
From: Seth House @ 2020-12-28  0:41 UTC (permalink / raw)
  To: git
  Cc: Seth House, Junio C Hamano, David Aguilar, Johannes Sixt,
	Felipe Contreras

Changes since v6:

 * Incorporated Junio's help and advice:

   * Rebased v7 off of the correct v5 version.
   * Signed off on Felipe's commit. (Although I have minor qualms with
     Felipe's various wording and even the name of the flag it is
     decidedly not worth burdening the list with bike-shedding.)

Changes since v5:

 * Add per-tool configuration that Felipe has a "deep philosophical"
   opposition to adding.

Felipe Contreras (1):
  mergetool: add automerge configuration

Seth House (1):
  mergetool: Add per-tool support for the autoMerge flag

 Documentation/config/mergetool.txt |  6 ++++++
 git-mergetool.sh                   | 20 ++++++++++++++++++++
 t/t7610-mergetool.sh               | 18 ++++++++++++++++++
 3 files changed, 44 insertions(+)

-- 
2.29.2


^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH v7 1/2] mergetool: add automerge configuration
  2020-12-28  0:41   ` [PATCH v7 0/2] mergetool: add automerge configuration Seth House
@ 2020-12-28  0:41     ` Seth House
  2020-12-28  0:41     ` [PATCH v7 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 80+ messages in thread
From: Seth House @ 2020-12-28  0:41 UTC (permalink / raw)
  To: git; +Cc: Felipe Contreras, Seth House

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

The purpose of mergetools is to resolve conflicts when git cannot
automatically do so.

In order to do that git has added markers in the specific areas that
need resolving, which the user must manually fix. The tool is supposed
to help with that.

However, by passing the original BASE, LOCAL, and REMOTE files, many
changes without conflict are presented to the user when in fact nothing
needs to be done for those.

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 there are actual
conflicts.

As most people will want the new behavior, we enable it by default.
Users that do not want the new behavior can set the new configuration
mergetool.autoMerge to false.

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>
Signed-off-by: Seth House <seth@eseth.com>
---
 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"
+	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"
+	then
+		auto_merge
+	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..ccabd04823 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 &&
+	test_write_lines >file1 base "" a &&
+	git commit -a -m "base" &&
+	test_write_lines >file1 base "" c &&
+	git commit -a -m "remote update" &&
+	git checkout -b test${test_count}_a HEAD~ &&
+	test_write_lines >file1 local "" b &&
+	git commit -a -m "local update" &&
+	test_must_fail git merge test${test_count}_b &&
+	yes "" | git mergetool file1 &&
+	test_write_lines >expect local "" c &&
+	test_cmp expect file1 &&
+	git commit -m "test resolved with mergetool"
+'
+
 test_done
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* [PATCH v7 2/2] mergetool: Add per-tool support for the autoMerge flag
  2020-12-28  0:41   ` [PATCH v7 0/2] mergetool: add automerge configuration Seth House
  2020-12-28  0:41     ` [PATCH v7 1/2] " Seth House
@ 2020-12-28  0:41     ` Seth House
  2020-12-28  1:18       ` Felipe Contreras
  2020-12-28  4:54     ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House
  2020-12-28 10:29     ` [PATCH v7 0/2] mergetool: add automerge configuration Junio C Hamano
  3 siblings, 1 reply; 80+ messages in thread
From: Seth House @ 2020-12-28  0:41 UTC (permalink / raw)
  To: git; +Cc: Seth House

Keep the global mergetool flag and add a per-tool override flag so that
users may enable the flag for one tool and disable it for another.

Signed-off-by: Seth House <seth@eseth.com>
---
 Documentation/config/mergetool.txt | 3 +++
 git-mergetool.sh                   | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 7ce6d0d3ac..ef147fc118 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -21,6 +21,9 @@ mergetool.<tool>.trustExitCode::
 	if the file has been updated, otherwise the user is prompted to
 	indicate the success of the merge.
 
+mergetool.<tool>.autoMerge::
+	Remove lines without conflicts from all the files. Defaults to `true`.
+
 mergetool.meld.hasOutput::
 	Older versions of `meld` do not support the `--output` option.
 	Git will attempt to detect whether `meld` supports `--output`
diff --git a/git-mergetool.sh b/git-mergetool.sh
index f4db0cac8d..e3c7d78d1d 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -334,7 +334,10 @@ merge_file () {
 	checkout_staged_file 2 "$MERGED" "$LOCAL"
 	checkout_staged_file 3 "$MERGED" "$REMOTE"
 
-	if test "$(git config --bool mergetool.autoMerge)" != "false"
+	if test "$(
+		git config --get --bool "mergetool.$merge_tool.automerge" ||
+		git config --get --bool "mergetool.automerge" ||
+		echo true)" = true
 	then
 		auto_merge
 	fi
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* RE: [PATCH v6 0/1] mergetool: add automerge configuration
  2020-12-27 20:58 ` [PATCH v6 0/1] mergetool: add automerge configuration Seth House
                     ` (2 preceding siblings ...)
  2020-12-28  0:41   ` [PATCH v7 0/2] mergetool: add automerge configuration Seth House
@ 2020-12-28  1:02   ` Felipe Contreras
  3 siblings, 0 replies; 80+ messages in thread
From: Felipe Contreras @ 2020-12-28  1:02 UTC (permalink / raw)
  To: Seth House, git
  Cc: Seth House, Junio C Hamano, David Aguilar, Johannes Sixt,
	Felipe Contreras

Seth House wrote:
> Sorry for the slow turnaround on this. I haven't used Git via email
> patches before now so it took me quite a few hours to read through
> tutorials, configure git-send-email and fight missing Perl libs.

What distribution are you using?

> Changes since v5:

This is not v6 of my patch series; it's v1 of yours, which I think
should have a different title.

What happens when I want to do v6?

Other than that (and the fact that you initially used the wrong version
as a baseline), I'm fine with your approach of doing a patch on top of
mine.

Cheers.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 80+ messages in thread

* RE: [PATCH v7 2/2] mergetool: Add per-tool support for the autoMerge flag
  2020-12-28  0:41     ` [PATCH v7 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House
@ 2020-12-28  1:18       ` Felipe Contreras
  0 siblings, 0 replies; 80+ messages in thread
From: Felipe Contreras @ 2020-12-28  1:18 UTC (permalink / raw)
  To: Seth House, git; +Cc: Seth House

Seth House wrote:
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index f4db0cac8d..e3c7d78d1d 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -334,7 +334,10 @@ merge_file () {
>  	checkout_staged_file 2 "$MERGED" "$LOCAL"
>  	checkout_staged_file 3 "$MERGED" "$REMOTE"
>  
> -	if test "$(git config --bool mergetool.autoMerge)" != "false"
> +	if test "$(
> +		git config --get --bool "mergetool.$merge_tool.automerge" ||
> +		git config --get --bool "mergetool.automerge" ||
> +		echo true)" = true

This is a per-tool user configuration.

Wasn't your argument that some tools would want to disable this flag?
That is; the tool, not the user.

For example, the author of diffconflicts might want to disable this flag
for all its users, or at least disable it by default.

How can the winmerge difftool disable this flag?

>  	then
>  		auto_merge
>  	fi

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH v8 0/4] mergetool: add automerge configuration
  2020-12-28  0:41   ` [PATCH v7 0/2] mergetool: add automerge configuration Seth House
  2020-12-28  0:41     ` [PATCH v7 1/2] " Seth House
  2020-12-28  0:41     ` [PATCH v7 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House
@ 2020-12-28  4:54     ` Seth House
  2020-12-28  4:54       ` [PATCH v8 1/4] " Seth House
                         ` (4 more replies)
  2020-12-28 10:29     ` [PATCH v7 0/2] mergetool: add automerge configuration Junio C Hamano
  3 siblings, 5 replies; 80+ messages in thread
From: Seth House @ 2020-12-28  4:54 UTC (permalink / raw)
  To: git
  Cc: Seth House, Junio C Hamano, David Aguilar, Johannes Sixt,
	Felipe Contreras

Changes since v7:

 * Add a tool-specific override function to setup_tool based on Junio's
   original patch feedback.

   The implementation of initialize_merge_tool is very much not set in
   stone. Suggestions are very welcome for alternate approaches that are
   less invasive.

Felipe Contreras (1):
  mergetool: add automerge configuration

Seth House (3):
  mergetool: Add per-tool support for the autoMerge flag
  mergetool: Break setup_tool out into separate initialization function
  mergetool: Add automerge_enabled tool-specific override function

 Documentation/config/mergetool.txt   |  6 ++++++
 Documentation/git-mergetool--lib.txt |  4 ++++
 git-difftool--helper.sh              |  2 ++
 git-mergetool--lib.sh                | 11 ++++++++---
 git-mergetool.sh                     | 22 ++++++++++++++++++++++
 t/t7610-mergetool.sh                 | 18 ++++++++++++++++++
 6 files changed, 60 insertions(+), 3 deletions(-)

-- 
2.29.2



^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH v8 1/4] mergetool: add automerge configuration
  2020-12-28  4:54     ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House
@ 2020-12-28  4:54       ` Seth House
  2020-12-28 11:30         ` Johannes Sixt
  2020-12-28  4:54       ` [PATCH v8 2/4] mergetool: Add per-tool support for the autoMerge flag Seth House
                         ` (3 subsequent siblings)
  4 siblings, 1 reply; 80+ messages in thread
From: Seth House @ 2020-12-28  4:54 UTC (permalink / raw)
  To: git; +Cc: Felipe Contreras, Seth House

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

The purpose of mergetools is to resolve conflicts when git cannot
automatically do so.

In order to do that git has added markers in the specific areas that
need resolving, which the user must manually fix. The tool is supposed
to help with that.

However, by passing the original BASE, LOCAL, and REMOTE files, many
changes without conflict are presented to the user when in fact nothing
needs to be done for those.

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 there are actual
conflicts.

As most people will want the new behavior, we enable it by default.
Users that do not want the new behavior can set the new configuration
mergetool.autoMerge to false.

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>
Signed-off-by: Seth House <seth@eseth.com>
---
 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"
+	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"
+	then
+		auto_merge
+	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..ccabd04823 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 &&
+	test_write_lines >file1 base "" a &&
+	git commit -a -m "base" &&
+	test_write_lines >file1 base "" c &&
+	git commit -a -m "remote update" &&
+	git checkout -b test${test_count}_a HEAD~ &&
+	test_write_lines >file1 local "" b &&
+	git commit -a -m "local update" &&
+	test_must_fail git merge test${test_count}_b &&
+	yes "" | git mergetool file1 &&
+	test_write_lines >expect local "" c &&
+	test_cmp expect file1 &&
+	git commit -m "test resolved with mergetool"
+'
+
 test_done
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* [PATCH v8 2/4] mergetool: Add per-tool support for the autoMerge flag
  2020-12-28  4:54     ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House
  2020-12-28  4:54       ` [PATCH v8 1/4] " Seth House
@ 2020-12-28  4:54       ` Seth House
  2020-12-28 13:09         ` Junio C Hamano
  2020-12-28  4:54       ` [PATCH v8 3/4] mergetool: Break setup_tool out into separate initialization function Seth House
                         ` (2 subsequent siblings)
  4 siblings, 1 reply; 80+ messages in thread
From: Seth House @ 2020-12-28  4:54 UTC (permalink / raw)
  To: git; +Cc: Seth House

Keep the global mergetool flag and add a per-tool override flag so that
users may enable the flag for one tool and disable it for another.

Signed-off-by: Seth House <seth@eseth.com>
---
 Documentation/config/mergetool.txt | 3 +++
 git-mergetool.sh                   | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 7ce6d0d3ac..ef147fc118 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -21,6 +21,9 @@ mergetool.<tool>.trustExitCode::
 	if the file has been updated, otherwise the user is prompted to
 	indicate the success of the merge.
 
+mergetool.<tool>.autoMerge::
+	Remove lines without conflicts from all the files. Defaults to `true`.
+
 mergetool.meld.hasOutput::
 	Older versions of `meld` do not support the `--output` option.
 	Git will attempt to detect whether `meld` supports `--output`
diff --git a/git-mergetool.sh b/git-mergetool.sh
index f4db0cac8d..e3c7d78d1d 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -334,7 +334,10 @@ merge_file () {
 	checkout_staged_file 2 "$MERGED" "$LOCAL"
 	checkout_staged_file 3 "$MERGED" "$REMOTE"
 
-	if test "$(git config --bool mergetool.autoMerge)" != "false"
+	if test "$(
+		git config --get --bool "mergetool.$merge_tool.automerge" ||
+		git config --get --bool "mergetool.automerge" ||
+		echo true)" = true
 	then
 		auto_merge
 	fi
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* [PATCH v8 3/4] mergetool: Break setup_tool out into separate initialization function
  2020-12-28  4:54     ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House
  2020-12-28  4:54       ` [PATCH v8 1/4] " Seth House
  2020-12-28  4:54       ` [PATCH v8 2/4] mergetool: Add per-tool support for the autoMerge flag Seth House
@ 2020-12-28  4:54       ` Seth House
  2020-12-28 11:48         ` Johannes Sixt
  2020-12-28  4:54       ` [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function Seth House
  2020-12-28 19:29       ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House
  4 siblings, 1 reply; 80+ messages in thread
From: Seth House @ 2020-12-28  4:54 UTC (permalink / raw)
  To: git; +Cc: Seth House

The tool-specific functions are sometimes needed in scope earlier than
when run_merge_tool is called.

Signed-off-by: Seth House <seth@eseth.com>
---
 Documentation/git-mergetool--lib.txt | 4 ++++
 git-difftool--helper.sh              | 2 ++
 git-mergetool--lib.sh                | 7 ++++---
 git-mergetool.sh                     | 2 ++
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt
index 4da9d24096..3e8f59ac0e 100644
--- a/Documentation/git-mergetool--lib.txt
+++ b/Documentation/git-mergetool--lib.txt
@@ -38,6 +38,10 @@ get_merge_tool_cmd::
 get_merge_tool_path::
 	returns the custom path for a merge tool.
 
+initialize_merge_tool::
+	bring merge tool specific functions into scope so they can be used or
+	overridden.
+
 run_merge_tool::
 	launches a merge tool given the tool name and a true/false
 	flag to indicate whether a merge base is present.
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 46af3e60b7..c47a6d4253 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -61,6 +61,7 @@ launch_merge_tool () {
 		export BASE
 		eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'
 	else
+		initialize_merge_tool "$merge_tool"
 		run_merge_tool "$merge_tool"
 	fi
 }
@@ -79,6 +80,7 @@ if test -n "$GIT_DIFFTOOL_DIRDIFF"
 then
 	LOCAL="$1"
 	REMOTE="$2"
+	initialize_merge_tool "$merge_tool"
 	run_merge_tool "$merge_tool" false
 else
 	# Launch the merge tool on each path provided by 'git diff'
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 7225abd811..e059b3559e 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -248,6 +248,10 @@ trust_exit_code () {
 	fi
 }
 
+initialize_merge_tool () {
+	# Bring tool-specific functions into scope
+	setup_tool "$1" || return 1
+}
 
 # Entry point for running tools
 run_merge_tool () {
@@ -259,9 +263,6 @@ run_merge_tool () {
 	merge_tool_path=$(get_merge_tool_path "$1") || exit
 	base_present="$2"
 
-	# Bring tool-specific functions into scope
-	setup_tool "$1" || return 1
-
 	if merge_mode
 	then
 		run_merge_cmd "$1"
diff --git a/git-mergetool.sh b/git-mergetool.sh
index e3c7d78d1d..929192d0f8 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -334,6 +334,8 @@ merge_file () {
 	checkout_staged_file 2 "$MERGED" "$LOCAL"
 	checkout_staged_file 3 "$MERGED" "$REMOTE"
 
+	initialize_merge_tool "$merge_tool"
+
 	if test "$(
 		git config --get --bool "mergetool.$merge_tool.automerge" ||
 		git config --get --bool "mergetool.automerge" ||
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function
  2020-12-28  4:54     ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House
                         ` (2 preceding siblings ...)
  2020-12-28  4:54       ` [PATCH v8 3/4] mergetool: Break setup_tool out into separate initialization function Seth House
@ 2020-12-28  4:54       ` Seth House
  2020-12-28 11:57         ` Johannes Sixt
  2020-12-28 13:19         ` Junio C Hamano
  2020-12-28 19:29       ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House
  4 siblings, 2 replies; 80+ messages in thread
From: Seth House @ 2020-12-28  4:54 UTC (permalink / raw)
  To: git; +Cc: Seth House

Hat-tip to Junio C Hamano for the implementation.

Signed-off-by: Seth House <seth@eseth.com>
---
 git-mergetool--lib.sh | 4 ++++
 git-mergetool.sh      | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index e059b3559e..5084ceffeb 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -164,6 +164,10 @@ setup_tool () {
 		return 1
 	}
 
+	automerge_enabled () {
+		true
+	}
+
 	translate_merge_tool_path () {
 		echo "$1"
 	}
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 929192d0f8..a44afd3822 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -336,7 +336,7 @@ merge_file () {
 
 	initialize_merge_tool "$merge_tool"
 
-	if test "$(
+	if automerge_enabled && test "$(
 		git config --get --bool "mergetool.$merge_tool.automerge" ||
 		git config --get --bool "mergetool.automerge" ||
 		echo true)" = true
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* Re: [PATCH v7 0/2] mergetool: add automerge configuration
  2020-12-28  0:41   ` [PATCH v7 0/2] mergetool: add automerge configuration Seth House
                       ` (2 preceding siblings ...)
  2020-12-28  4:54     ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House
@ 2020-12-28 10:29     ` Junio C Hamano
  2020-12-28 14:40       ` Felipe Contreras
  3 siblings, 1 reply; 80+ messages in thread
From: Junio C Hamano @ 2020-12-28 10:29 UTC (permalink / raw)
  To: Seth House; +Cc: git, David Aguilar, Johannes Sixt, Felipe Contreras

Seth House <seth@eseth.com> writes:

>    * Signed off on Felipe's commit. (Although I have minor qualms with
>      Felipe's various wording and even the name of the flag it is
>      decidedly not worth burdening the list with bike-shedding.)

Even when the original is a horrible patch in your opinion that is
laden with bugs, as long as the original author signed it off
(which means that the original author certifies that it can be
included in and distributed by the project under our licensing
terms, and agrees to the fact that the original author did so will
be recorded in perpetuity), you can relay such a patch as-is, and
you are required (i.e. SubmittingPatches is pretty clear that
without your sign-off we cannot accept) to sign it off to record
the provenance of the code.

The other side of the above coin is that you are not endorsing or
vounching for the patch when you sign it off, so your name is not
smudged by wording and flag name chosen in a way that you may
consider poor.  So "Although..." part is not a good objection
against signing it off.

In other words, sign-off is not about assuring quality.

Also, instead of relaying as-is, you can relay a patch with your
improvements rolled into the same patch (i.e. not as follow-up
fixes).  Some (or major) parts of the original patch may still
remain in the edited result and you'd need to keep original author's
sign-off as-is [*1*].

In this topic's case, 2/2 would be a feature enhancement on top of
1/2, so relaying 1/2 as-is would be OK, but in a case where an
promising patch was sent with sign-off and bugs, then gets abandoned
by the original author, fixing the bug in the patch you relay in
place (i.e. not as follow-up patches) may even be necessary to keep
bisectability.  When you do so, you'd typically do:

	Subject: [PATCH] title of the patch

	... original author's log message, possibly copyedited
	... by you

+	<Comment on what you did on top of the original can come here>

	Signed-off-by: Original Author <ori@ginal.au.thor>
+	[or brief comment here]
+	Signed-off-by: Your Name <you@your.do.main>

 (1) add your sign-off at the end
 (2) explain what you changed relative to the original, either
     inside [] on the line before your sign-off, or at the end
     of the log message proper.

to indicate that it is not relayed as-is; this allows you to take
responsibility of an unintended breakage your "fixes" might have
caused.


[Footnote]

*1* The result may become something that no longer aligns the
original author's opinion, but that is OK.  The sign-off by the
original author just says that the original author has the right to
contribute (the remaining part of) the patch and the original author
agrees that the record of author's involvement in the patch
(including sign-off) will be kept.  

It is not about assuring quality of the final work by the original
author, either.

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v8 1/4] mergetool: add automerge configuration
  2020-12-28  4:54       ` [PATCH v8 1/4] " Seth House
@ 2020-12-28 11:30         ` Johannes Sixt
  0 siblings, 0 replies; 80+ messages in thread
From: Johannes Sixt @ 2020-12-28 11:30 UTC (permalink / raw)
  To: Seth House; +Cc: Felipe Contreras, git

Am 28.12.20 um 05:54 schrieb Seth House:
> 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`.

This text, starting with "Remove lines", sounds alarming. Isn't it more
along the lines of:

	Consolidate non-conflicting parts, so that only conflicting
	parts are presented to the merge tool. Defaults to `true`.

It would be great to keep the list of config entries sorted. I know that
mergetool.prompt is not at the correct location, but that shouldn't be
an excuse to make the situation worse.

-- Hannes

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v8 3/4] mergetool: Break setup_tool out into separate initialization function
  2020-12-28  4:54       ` [PATCH v8 3/4] mergetool: Break setup_tool out into separate initialization function Seth House
@ 2020-12-28 11:48         ` Johannes Sixt
  0 siblings, 0 replies; 80+ messages in thread
From: Johannes Sixt @ 2020-12-28 11:48 UTC (permalink / raw)
  To: Seth House; +Cc: git

Am 28.12.20 um 05:54 schrieb Seth House:
> The tool-specific functions are sometimes needed in scope earlier than
> when run_merge_tool is called.

You should answer why this change is needed. "are sometimes needed in
scope earlier" cannot be true, else we would have a bug that is fixed by
this change. But this isn't a bug fix, is it?

It would be ok to say "We are going to add another thing that we will
need before run_merge_tool; this is a preparation" or something.

Which brings me to another point: I do not see that something is added
to initialize_merge_tool in a later patch. You are only replacing
setup_tool calls by initialize_merge_tool, which forwards to setup_tool.
Why do we need a new function?

> 
> Signed-off-by: Seth House <seth@eseth.com>
> ---
>  Documentation/git-mergetool--lib.txt | 4 ++++
>  git-difftool--helper.sh              | 2 ++
>  git-mergetool--lib.sh                | 7 ++++---
>  git-mergetool.sh                     | 2 ++
>  4 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt
> index 4da9d24096..3e8f59ac0e 100644
> --- a/Documentation/git-mergetool--lib.txt
> +++ b/Documentation/git-mergetool--lib.txt
> @@ -38,6 +38,10 @@ get_merge_tool_cmd::
>  get_merge_tool_path::
>  	returns the custom path for a merge tool.
>  
> +initialize_merge_tool::
> +	bring merge tool specific functions into scope so they can be used or
> +	overridden.
> +
>  run_merge_tool::
>  	launches a merge tool given the tool name and a true/false
>  	flag to indicate whether a merge base is present.

[ swapped hunks for better sentence structure ]

> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 7225abd811..e059b3559e 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -248,6 +248,10 @@ trust_exit_code () {
>  	fi
>  }
>
> +initialize_merge_tool () {
> +	# Bring tool-specific functions into scope
> +	setup_tool "$1" || return 1
> +}
>
>  # Entry point for running tools
>  run_merge_tool () {
> @@ -259,9 +263,6 @@ run_merge_tool () {
>  	merge_tool_path=$(get_merge_tool_path "$1") || exit
>  	base_present="$2"
>
> -	# Bring tool-specific functions into scope
> -	setup_tool "$1" || return 1
> -

Before this change, run_merge_tool would exit here when there was an
error during setup_tool. But...

> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
> index 46af3e60b7..c47a6d4253 100755
> --- a/git-difftool--helper.sh
> +++ b/git-difftool--helper.sh
> @@ -61,6 +61,7 @@ launch_merge_tool () {
>  		export BASE
>  		eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'
>  	else
> +		initialize_merge_tool "$merge_tool"
>  		run_merge_tool "$merge_tool"
>  	fi

... after the change we do not exit anymore. Does it matter?

Perhaps

		initialize_merge_tool "$merge_tool" &&
  		run_merge_tool "$merge_tool"

>  }
> @@ -79,6 +80,7 @@ if test -n "$GIT_DIFFTOOL_DIRDIFF"
>  then
>  	LOCAL="$1"
>  	REMOTE="$2"
> +	initialize_merge_tool "$merge_tool"
>  	run_merge_tool "$merge_tool" false
>  else
>  	# Launch the merge tool on each path provided by 'git diff'


> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index e3c7d78d1d..929192d0f8 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -334,6 +334,8 @@ merge_file () {
>  	checkout_staged_file 2 "$MERGED" "$LOCAL"
>  	checkout_staged_file 3 "$MERGED" "$REMOTE"
>  
> +	initialize_merge_tool "$merge_tool"
> +
>  	if test "$(
>  		git config --get --bool "mergetool.$merge_tool.automerge" ||
>  		git config --get --bool "mergetool.automerge" ||
> 


^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function
  2020-12-28  4:54       ` [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function Seth House
@ 2020-12-28 11:57         ` Johannes Sixt
  2020-12-28 13:19         ` Junio C Hamano
  1 sibling, 0 replies; 80+ messages in thread
From: Johannes Sixt @ 2020-12-28 11:57 UTC (permalink / raw)
  To: Seth House; +Cc: git

Am 28.12.20 um 05:54 schrieb Seth House:
> Hat-tip to Junio C Hamano for the implementation.

We usually write this as

Helped-by: Junio C Hamano <gitster@pobox.com>

> 
> Signed-off-by: Seth House <seth@eseth.com>
> ---
>  git-mergetool--lib.sh | 4 ++++
>  git-mergetool.sh      | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index e059b3559e..5084ceffeb 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -164,6 +164,10 @@ setup_tool () {
>  		return 1
>  	}
>  
> +	automerge_enabled () {
> +		true

I would have written this as `return 0` instead of `true` like some of
the functions above this hunk.

> +	}
> +
>  	translate_merge_tool_path () {
>  		echo "$1"
>  	}
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 929192d0f8..a44afd3822 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -336,7 +336,7 @@ merge_file () {
>  
>  	initialize_merge_tool "$merge_tool"
>  
> -	if test "$(
> +	if automerge_enabled && test "$(
>  		git config --get --bool "mergetool.$merge_tool.automerge" ||
>  		git config --get --bool "mergetool.automerge" ||
>  		echo true)" = true
> 

-- Hannes

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v8 2/4] mergetool: Add per-tool support for the autoMerge flag
  2020-12-28  4:54       ` [PATCH v8 2/4] mergetool: Add per-tool support for the autoMerge flag Seth House
@ 2020-12-28 13:09         ` Junio C Hamano
  0 siblings, 0 replies; 80+ messages in thread
From: Junio C Hamano @ 2020-12-28 13:09 UTC (permalink / raw)
  To: Seth House; +Cc: git


> Subject: Re: [PATCH v8 2/4] mergetool: Add per-tool support for the autoMerge flag

"git shortlog --no-merges --since=2.months" may tell you this but our
convention is not to capitalize the word after "<area>:" on the
title.

> Keep the global mergetool flag and add a per-tool override flag so that
> users may enable the flag for one tool and disable it for another.
>
> Signed-off-by: Seth House <seth@eseth.com>
> ---
>  Documentation/config/mergetool.txt | 3 +++
>  git-mergetool.sh                   | 5 ++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
> index 7ce6d0d3ac..ef147fc118 100644
> --- a/Documentation/config/mergetool.txt
> +++ b/Documentation/config/mergetool.txt
> @@ -21,6 +21,9 @@ mergetool.<tool>.trustExitCode::
>  	if the file has been updated, otherwise the user is prompted to
>  	indicate the success of the merge.
>  
> +mergetool.<tool>.autoMerge::
> +	Remove lines without conflicts from all the files. Defaults to `true`.
> +

This entry needs to mention how it relates to the big red button
mergetool.autoMerge and vice versa.  E.g.

 mergetool.autoMerge::
-	Remove lines without conflicts from all the files. Defaults to `true`.
+	Remove lines without conflicts from all the files. Can be
+	overriden per-tool via `mergetool.<tool>.autoMerge` configuration
+	variable. Defaults to `true`.

would be a good update to the documentation introduced by the
previous step.  It is somewhat misleading for the per-tool entry
added in this patch to say "Defaults to `true`", as the value of the
big red button configuration would be the real default.

	Remove ... the files when the mergetool '<tool>' is in use.
	See also `mergetool.autoMerge`.

or something like that, perhaps.

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function
  2020-12-28  4:54       ` [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function Seth House
  2020-12-28 11:57         ` Johannes Sixt
@ 2020-12-28 13:19         ` Junio C Hamano
  1 sibling, 0 replies; 80+ messages in thread
From: Junio C Hamano @ 2020-12-28 13:19 UTC (permalink / raw)
  To: Seth House; +Cc: git

Seth House <seth@eseth.com> writes:

> Hat-tip to Junio C Hamano for the implementation.

That is the least interesting thing the log message for this commit
can talk about.  Instead readers should be able to learn things like
these from the log message (I am not saying the log message should
have these in an enumerated list; I am just enumerating these as
samples):

 - Why does this exist?  

 - What does a tool author want to use the mechanism for, and how
   does the tool author use it?  

 - This mechanism allows tool authors to say "never allow autoMerge
   for this tool", but there is no provision to let them say "always
   use autoMerge without allowing users to turn it off".

It also needs a bit of documentation update to mention that
individual mergetool backend can choose not to trigger the feature
at all, even if the user configures it with mergetool.autoMerge and
mergetool.<tool>.autoMerge options.

Thanks.

> Signed-off-by: Seth House <seth@eseth.com>
> ---
>  git-mergetool--lib.sh | 4 ++++
>  git-mergetool.sh      | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index e059b3559e..5084ceffeb 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -164,6 +164,10 @@ setup_tool () {
>  		return 1
>  	}
>  
> +	automerge_enabled () {
> +		true
> +	}
> +
>  	translate_merge_tool_path () {
>  		echo "$1"
>  	}
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 929192d0f8..a44afd3822 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -336,7 +336,7 @@ merge_file () {
>  
>  	initialize_merge_tool "$merge_tool"
>  
> -	if test "$(
> +	if automerge_enabled && test "$(
>  		git config --get --bool "mergetool.$merge_tool.automerge" ||
>  		git config --get --bool "mergetool.automerge" ||
>  		echo true)" = true

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v7 0/2] mergetool: add automerge configuration
  2020-12-28 10:29     ` [PATCH v7 0/2] mergetool: add automerge configuration Junio C Hamano
@ 2020-12-28 14:40       ` Felipe Contreras
  0 siblings, 0 replies; 80+ messages in thread
From: Felipe Contreras @ 2020-12-28 14:40 UTC (permalink / raw)
  To: Junio C Hamano, Seth House
  Cc: git, David Aguilar, Johannes Sixt, Felipe Contreras

Junio C Hamano wrote:
> Also, instead of relaying as-is, you can relay a patch with your
> improvements rolled into the same patch (i.e. not as follow-up
> fixes).  Some (or major) parts of the original patch may still
> remain in the edited result and you'd need to keep original author's
> sign-off as-is [*1*].

Yes, you *can*, but doing so in this case would be against the author's
wishes, and a violation of the Developer Certificate of Origin.

> In this topic's case, 2/2 would be a feature enhancement on top of
> 1/2, so relaying 1/2 as-is would be OK, but in a case where an
> promising patch was sent with sign-off and bugs, then gets abandoned
> by the original author, fixing the bug in the patch you relay in
> place (i.e. not as follow-up patches) may even be necessary to keep
> bisectability.  When you do so, you'd typically do:

If the author doesn't object (which is usually the case), this makes
sense.

But if the author objects, you would be violating clause (d) of the DCO.

Just because the only way to do X is to violate laws, terms, or
agreements doesn't mean that's what you should do. You can simply not do
X.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH v9 0/5] mergetool: add automerge configuration
  2020-12-28  4:54     ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House
                         ` (3 preceding siblings ...)
  2020-12-28  4:54       ` [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function Seth House
@ 2020-12-28 19:29       ` Seth House
  2020-12-28 19:29         ` [PATCH v9 1/5] " Seth House
                           ` (5 more replies)
  4 siblings, 6 replies; 80+ messages in thread
From: Seth House @ 2020-12-28 19:29 UTC (permalink / raw)
  To: git
  Cc: Seth House, Junio C Hamano, David Aguilar, Johannes Sixt,
	Felipe Contreras

Thank you Junio and Johannes for patiently pointing out all my newbie
mistakes.

Changes since v8:

- Improve documentation of what the autoMerge flag does.
- Add documentation note for the `automerge_enabled` override.

  - Junio, is there another place this should also live?

- Cross-reference merge.autoMerge and merge.<tool>.autoMerge docs.
- Sort the list of mergetool options. Added as a standalone commit in
  case this change doesn't belong as part of this patchset. I'd be happy
  to move this to a standalone patch if that's preferable.
- Improve all commit messages with full explanations and rationale:

  - Johannes, I didn't directly respond to your `initialize_merge_tool`
    question because you're right that explanation should be part of the
    commit message. Please let me know if that now answers your question
    and if not we can discuss further in a thread.

- Fix omitted exit codes when running `initialize_merge_tool`.
- Fix `automerge_enabled` return value for consistency.
- Update commit message capitalization to conform to repo norms.
- Rephrase commit message 'thanks' as Helped-by markers.

Felipe Contreras (1):
  mergetool: add automerge configuration

Seth House (4):
  mergetool: alphabetize the mergetool config docs
  mergetool: add per-tool support for the autoMerge flag
  mergetool: break setup_tool out into separate initialization function
  mergetool: add automerge_enabled tool-specific override function

 Documentation/config/mergetool.txt   | 28 ++++++++++++++++++++++------
 Documentation/git-mergetool--lib.txt |  4 ++++
 git-difftool--helper.sh              |  2 ++
 git-mergetool--lib.sh                | 11 ++++++++---
 git-mergetool.sh                     | 22 ++++++++++++++++++++++
 t/t7610-mergetool.sh                 | 18 ++++++++++++++++++
 6 files changed, 76 insertions(+), 9 deletions(-)

-- 
2.29.2



^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH v9 1/5] mergetool: add automerge configuration
  2020-12-28 19:29       ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House
@ 2020-12-28 19:29         ` Seth House
  2020-12-28 19:29         ` [PATCH v9 2/5] mergetool: alphabetize the mergetool config docs Seth House
                           ` (4 subsequent siblings)
  5 siblings, 0 replies; 80+ messages in thread
From: Seth House @ 2020-12-28 19:29 UTC (permalink / raw)
  To: git; +Cc: Felipe Contreras, Seth House

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

The purpose of mergetools is to resolve conflicts when git cannot
automatically do so.

In order to do that git has added markers in the specific areas that
need resolving, which the user must manually fix. The tool is supposed
to help with that.

However, by passing the original BASE, LOCAL, and REMOTE files, many
changes without conflict are presented to the user when in fact nothing
needs to be done for those.

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 there are actual
conflicts.

As most people will want the new behavior, we enable it by default.
Users that do not want the new behavior can set the new configuration
mergetool.autoMerge to false.

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>
Signed-off-by: Seth House <seth@eseth.com>
---
 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"
+	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"
+	then
+		auto_merge
+	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..ccabd04823 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 &&
+	test_write_lines >file1 base "" a &&
+	git commit -a -m "base" &&
+	test_write_lines >file1 base "" c &&
+	git commit -a -m "remote update" &&
+	git checkout -b test${test_count}_a HEAD~ &&
+	test_write_lines >file1 local "" b &&
+	git commit -a -m "local update" &&
+	test_must_fail git merge test${test_count}_b &&
+	yes "" | git mergetool file1 &&
+	test_write_lines >expect local "" c &&
+	test_cmp expect file1 &&
+	git commit -m "test resolved with mergetool"
+'
+
 test_done
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* [PATCH v9 2/5] mergetool: alphabetize the mergetool config docs
  2020-12-28 19:29       ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House
  2020-12-28 19:29         ` [PATCH v9 1/5] " Seth House
@ 2020-12-28 19:29         ` Seth House
  2020-12-28 19:29         ` [PATCH v9 3/5] mergetool: add per-tool support for the autoMerge flag Seth House
                           ` (3 subsequent siblings)
  5 siblings, 0 replies; 80+ messages in thread
From: Seth House @ 2020-12-28 19:29 UTC (permalink / raw)
  To: git; +Cc: Seth House

The ordering in this file has drifted a little. Let's make things better
while we're adding new entres. :)

Signed-off-by: Seth House <seth@eseth.com>
---
 Documentation/config/mergetool.txt | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 7ce6d0d3ac..3291fa7102 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -1,7 +1,3 @@
-mergetool.<tool>.path::
-	Override the path for the given tool.  This is useful in case
-	your tool is not in the PATH.
-
 mergetool.<tool>.cmd::
 	Specify the command to invoke the specified merge tool.  The
 	specified command is evaluated in shell with the following
@@ -13,6 +9,10 @@ mergetool.<tool>.cmd::
 	merged; 'MERGED' contains the name of the file to which the merge
 	tool should write the results of a successful merge.
 
+mergetool.<tool>.path::
+	Override the path for the given tool.  This is useful in case
+	your tool is not in the PATH.
+
 mergetool.<tool>.trustExitCode::
 	For a custom merge command, specify whether the exit code of
 	the merge command can be used to determine whether the merge was
@@ -40,6 +40,9 @@ mergetool.meld.useAutoMerge::
 	value of `false` avoids using `--auto-merge` altogether, and is the
 	default value.
 
+mergetool.autoMerge::
+	Remove lines without conflicts from all the files. Defaults to `true`.
+
 mergetool.keepBackup::
 	After performing a merge, the original file with conflict markers
 	can be saved as a file with a `.orig` extension.  If this variable
@@ -53,14 +56,11 @@ mergetool.keepTemporaries::
 	preserved, otherwise they will be removed after the tool has
 	exited. Defaults to `false`.
 
+mergetool.prompt::
+	Prompt before each invocation of the merge resolution program.
+
 mergetool.writeToTemp::
 	Git writes temporary 'BASE', 'LOCAL', and 'REMOTE' versions of
 	conflicting files in the worktree by default.  Git will attempt
 	to use a temporary directory for these files when set `true`.
 	Defaults to `false`.
-
-mergetool.prompt::
-	Prompt before each invocation of the merge resolution program.
-
-mergetool.autoMerge::
-	Remove lines without conflicts from all the files. Defaults to `true`.
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* [PATCH v9 3/5] mergetool: add per-tool support for the autoMerge flag
  2020-12-28 19:29       ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House
  2020-12-28 19:29         ` [PATCH v9 1/5] " Seth House
  2020-12-28 19:29         ` [PATCH v9 2/5] mergetool: alphabetize the mergetool config docs Seth House
@ 2020-12-28 19:29         ` Seth House
  2020-12-28 19:29         ` [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function Seth House
                           ` (2 subsequent siblings)
  5 siblings, 0 replies; 80+ messages in thread
From: Seth House @ 2020-12-28 19:29 UTC (permalink / raw)
  To: git; +Cc: Seth House, Junio C Hamano, Johannes Sixt

Keep the global mergetool flag and add a per-tool override flag so that
users may enable the flag for one tool and disable it for another.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Seth House <seth@eseth.com>
---
 Documentation/config/mergetool.txt | 15 ++++++++++++++-
 git-mergetool.sh                   |  5 ++++-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 3291fa7102..bde472d49a 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -1,3 +1,9 @@
+mergetool.<tool>.autoMerge::
+	A mergetool-specific override for the global `mergetool.autoMerge`
+	configuration flag. This allows individual mergetools to enable or
+	disable the flag regardless of the global setting. See
+	`mergetool.autoMerge` for the full description.
+
 mergetool.<tool>.cmd::
 	Specify the command to invoke the specified merge tool.  The
 	specified command is evaluated in shell with the following
@@ -41,7 +47,14 @@ mergetool.meld.useAutoMerge::
 	default value.
 
 mergetool.autoMerge::
-	Remove lines without conflicts from all the files. Defaults to `true`.
+	During a merge Git will automatically resolve as many conflicts as
+	possible and then wrap conflict markers around any conflicts that it
+	cannot resolve. This flag consolidates the non-conflicting parts into
+	the corresponding 'LOCAL' and 'REMOTE' files so that only the
+	unresolved conflicts are presented to the merge tool. Can be overriden
+	per-tool via the `mergetool.<tool>.autoMerge` configuration variable.
+	Note: individual mergetool scripts can elect to ignore user preferences
+	entirely. Defaults to `true`.
 
 mergetool.keepBackup::
 	After performing a merge, the original file with conflict markers
diff --git a/git-mergetool.sh b/git-mergetool.sh
index f4db0cac8d..e3c7d78d1d 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -334,7 +334,10 @@ merge_file () {
 	checkout_staged_file 2 "$MERGED" "$LOCAL"
 	checkout_staged_file 3 "$MERGED" "$REMOTE"
 
-	if test "$(git config --bool mergetool.autoMerge)" != "false"
+	if test "$(
+		git config --get --bool "mergetool.$merge_tool.automerge" ||
+		git config --get --bool "mergetool.automerge" ||
+		echo true)" = true
 	then
 		auto_merge
 	fi
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function
  2020-12-28 19:29       ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House
                           ` (2 preceding siblings ...)
  2020-12-28 19:29         ` [PATCH v9 3/5] mergetool: add per-tool support for the autoMerge flag Seth House
@ 2020-12-28 19:29         ` Seth House
  2020-12-29  8:50           ` Johannes Sixt
  2020-12-28 19:29         ` [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function Seth House
  2021-01-30  5:46         ` [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
  5 siblings, 1 reply; 80+ messages in thread
From: Seth House @ 2020-12-28 19:29 UTC (permalink / raw)
  To: git; +Cc: Seth House

This is preparation for the following commit where we need to source the
mergetool shell script to look for overrides before `run_merge_tool` is
called. Previously `run_merge_tool` both sourced that script and invoked
the mergetool.

In the case of the following commit, we need the result of the
`automerge_enabled` override, if it exists, well before we actually run
`run_merge_tool`.

A new function `initialize_merge_tool` was chosen for consistency with
`run_merge_tool` since `setup_tool` and `setup_user_tool` are not
exposed or called directly.

Signed-off-by: Seth House <seth@eseth.com>
---
 Documentation/git-mergetool--lib.txt | 4 ++++
 git-difftool--helper.sh              | 2 ++
 git-mergetool--lib.sh                | 7 ++++---
 git-mergetool.sh                     | 2 ++
 4 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt
index 4da9d24096..3e8f59ac0e 100644
--- a/Documentation/git-mergetool--lib.txt
+++ b/Documentation/git-mergetool--lib.txt
@@ -38,6 +38,10 @@ get_merge_tool_cmd::
 get_merge_tool_path::
 	returns the custom path for a merge tool.
 
+initialize_merge_tool::
+	bring merge tool specific functions into scope so they can be used or
+	overridden.
+
 run_merge_tool::
 	launches a merge tool given the tool name and a true/false
 	flag to indicate whether a merge base is present.
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 46af3e60b7..234dd6944e 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -61,6 +61,7 @@ launch_merge_tool () {
 		export BASE
 		eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'
 	else
+		initialize_merge_tool "$merge_tool" &&
 		run_merge_tool "$merge_tool"
 	fi
 }
@@ -79,6 +80,7 @@ if test -n "$GIT_DIFFTOOL_DIRDIFF"
 then
 	LOCAL="$1"
 	REMOTE="$2"
+	initialize_merge_tool "$merge_tool" &&
 	run_merge_tool "$merge_tool" false
 else
 	# Launch the merge tool on each path provided by 'git diff'
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 7225abd811..e059b3559e 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -248,6 +248,10 @@ trust_exit_code () {
 	fi
 }
 
+initialize_merge_tool () {
+	# Bring tool-specific functions into scope
+	setup_tool "$1" || return 1
+}
 
 # Entry point for running tools
 run_merge_tool () {
@@ -259,9 +263,6 @@ run_merge_tool () {
 	merge_tool_path=$(get_merge_tool_path "$1") || exit
 	base_present="$2"
 
-	# Bring tool-specific functions into scope
-	setup_tool "$1" || return 1
-
 	if merge_mode
 	then
 		run_merge_cmd "$1"
diff --git a/git-mergetool.sh b/git-mergetool.sh
index e3c7d78d1d..929192d0f8 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -334,6 +334,8 @@ merge_file () {
 	checkout_staged_file 2 "$MERGED" "$LOCAL"
 	checkout_staged_file 3 "$MERGED" "$REMOTE"
 
+	initialize_merge_tool "$merge_tool"
+
 	if test "$(
 		git config --get --bool "mergetool.$merge_tool.automerge" ||
 		git config --get --bool "mergetool.automerge" ||
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function
  2020-12-28 19:29       ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House
                           ` (3 preceding siblings ...)
  2020-12-28 19:29         ` [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function Seth House
@ 2020-12-28 19:29         ` Seth House
  2020-12-29  2:01           ` Felipe Contreras
  2021-01-06  5:55           ` Junio C Hamano
  2021-01-30  5:46         ` [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
  5 siblings, 2 replies; 80+ messages in thread
From: Seth House @ 2020-12-28 19:29 UTC (permalink / raw)
  To: git; +Cc: Seth House, Junio C Hamano

The author or maintainer of a mergetool may optionally elect disable (or
enable) the `autoMerge` feature for that mergetool even if the user has
chosen differently using the `mergetool.autoMerge` and
`mergetool.<tool>.autoMerge` options.

To add a tool-specific override, edit the `mergetools/<tool>` shell
script for that tool and add an `automerge_enabled` function:

    automerge_enabled () {
        return 1
    }

Disabling may be desirable if the mergetool wants or needs access to the
original, unmodified 'LOCAL', 'REMOTE', and 'BASE' versions of the
conflicted file. For example:

- A tool may use a custom conflict resolution algorithm and prefer to
  ignore the results of Git's conflict resolution.
- A tool may want to visually compare/constrast the version of the file
  from before the merge (saved to 'LOCAL', 'REMOTE', and 'BASE') with
  Git's conflict resolution results (saved to 'MERGED').
- A student or researcher working on a new algorithm may want to
  directly compare the result of that algorithm with the result of Git's
  algorithm.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Seth House <seth@eseth.com>
---
 git-mergetool--lib.sh | 4 ++++
 git-mergetool.sh      | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index e059b3559e..567991abbc 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -164,6 +164,10 @@ setup_tool () {
 		return 1
 	}
 
+	automerge_enabled () {
+		return 0
+	}
+
 	translate_merge_tool_path () {
 		echo "$1"
 	}
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 929192d0f8..a44afd3822 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -336,7 +336,7 @@ merge_file () {
 
 	initialize_merge_tool "$merge_tool"
 
-	if test "$(
+	if automerge_enabled && test "$(
 		git config --get --bool "mergetool.$merge_tool.automerge" ||
 		git config --get --bool "mergetool.automerge" ||
 		echo true)" = true
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* RE: [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function
  2020-12-28 19:29         ` [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function Seth House
@ 2020-12-29  2:01           ` Felipe Contreras
  2021-01-06  5:55           ` Junio C Hamano
  1 sibling, 0 replies; 80+ messages in thread
From: Felipe Contreras @ 2020-12-29  2:01 UTC (permalink / raw)
  To: Seth House, git; +Cc: Seth House, Junio C Hamano

Seth House wrote:
> Disabling may be desirable if the mergetool wants or needs access to the
> original, unmodified 'LOCAL', 'REMOTE', and 'BASE' versions of the
> conflicted file. For example:
> 
> - A tool may use a custom conflict resolution algorithm and prefer to
>   ignore the results of Git's conflict resolution.

If git's conflict resolution decides there are no conflicts, how would
such tool "ignore" that?

> - A tool may want to visually compare/constrast the version of the file
>   from before the merge (saved to 'LOCAL', 'REMOTE', and 'BASE') with
>   Git's conflict resolution results (saved to 'MERGED').

Can't such tool use "git checkout-index" for that?

> - A student or researcher working on a new algorithm may want to
>   directly compare the result of that algorithm with the result of Git's
>   algorithm.

 1. If git's algorithm decides there are no conflicts, and the new
    algorithm decides there are conflicts, how would such researcher
    find that out?

 2. Can't such researcher simply do:
    git -c mergetool.automerge=false mergetool?

Cheers.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function
  2020-12-28 19:29         ` [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function Seth House
@ 2020-12-29  8:50           ` Johannes Sixt
  2020-12-29 17:23             ` Seth House
  0 siblings, 1 reply; 80+ messages in thread
From: Johannes Sixt @ 2020-12-29  8:50 UTC (permalink / raw)
  To: Seth House; +Cc: git

Am 28.12.20 um 20:29 schrieb Seth House:
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index e3c7d78d1d..929192d0f8 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -334,6 +334,8 @@ merge_file () {
>  	checkout_staged_file 2 "$MERGED" "$LOCAL"
>  	checkout_staged_file 3 "$MERGED" "$REMOTE"
>  
> +	initialize_merge_tool "$merge_tool"

In my earlier review, I was not explicit about the lack of error
handling of this invocation, because I hoped that you would notice it
yourself. `initialize_merge_tool` does have a few failure modes via
`setup_tool` that are not really unlikely; ignoring errors would be
wrong, I think.

Before this change, the errors would be handled as part of the failing
`run_merge_tool` call at the end of function `merge_file`. But now
errors are ignored. Just appending `|| return` would not be appropriate
at this point because a lot has already happened before the call that
has to be rewound. Would it be possible to move the call above the
`mergetool_tmpdir_init` call, so that nothing has to be rewound?

> +
>  	if test "$(
>  		git config --get --bool "mergetool.$merge_tool.automerge" ||
>  		git config --get --bool "mergetool.automerge" ||
> 

-- Hannes

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function
  2020-12-29  8:50           ` Johannes Sixt
@ 2020-12-29 17:23             ` Seth House
  0 siblings, 0 replies; 80+ messages in thread
From: Seth House @ 2020-12-29 17:23 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: git

On Tue, Dec 29, 2020 at 09:50:44AM +0100, Johannes Sixt wrote:
> Would it be possible to move the call above the
> `mergetool_tmpdir_init` call, so that nothing has to be rewound?

Ah, I see. Good suggestion. Yes, moving that call higher works just
fine. E.g.:

diff --git a/git-mergetool.sh b/git-mergetool.sh
index a44afd3822..36c1920dd6 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -276,6 +276,8 @@ merge_file () {
 		ext=
 	esac
 
+	initialize_merge_tool "$merge_tool" || return
+
 	mergetool_tmpdir_init
 
 	if test "$MERGETOOL_TMPDIR" != "."
@@ -334,8 +336,6 @@ merge_file () {
 	checkout_staged_file 2 "$MERGED" "$LOCAL"
 	checkout_staged_file 3 "$MERGED" "$REMOTE"
 
-	initialize_merge_tool "$merge_tool"
-
 	if automerge_enabled && test "$(
 		git config --get --bool "mergetool.$merge_tool.automerge" ||
 		git config --get --bool "mergetool.automerge" ||

Thanks. I'll roll that change into a v10 patch series later today or
tomorrow to give a little more time for any other feedback.


^ permalink raw reply related	[flat|nested] 80+ messages in thread

* Re: [PATCH v5 0/1] mergetool: remove unconflicted lines
  2020-12-24  9:24 ` [PATCH v5 0/1] mergetool: remove unconflicted lines Junio C Hamano
  2020-12-24 16:16   ` Felipe Contreras
@ 2020-12-30  5:47   ` Johannes Schindelin
  2020-12-30 22:33     ` Felipe Contreras
  1 sibling, 1 reply; 80+ messages in thread
From: Johannes Schindelin @ 2020-12-30  5:47 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Felipe Contreras, git, David Aguilar, Johannes Sixt, Seth House

Hi Junio,

On Thu, 24 Dec 2020, Junio C Hamano wrote:

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > There's not much to say other that what the commit message of the patch says.
> >
> > Note: no feedback has been ignored; I replied to all the feedback, I didn't hear anything back.
> >
> > Changes since v4:
> >
> >  * Improved commit message with suggestions from Phillip Wood.
> >
> > Felipe Contreras (1):
> >   mergetool: add automerge configuration
>
> This breakage is possibly a fallout from either this patch or
> 1e2ae142 (t7[5-9]*: adjust the references to the default branch name
> "main", 2020-11-18).
>
>   https://github.com/git/git/runs/1602803804#step:7:10358
>
> I cannot quite tell how the two strings compared with 'test' on
> output line 10355 are different in the output, though.

I spent more time than I cared to spend on this, and still have not quite
figured out what is the fault, but I can state with conviction that the
problem is not even introduced by any merge into `seen`. The
`fc/mergetool-automerge` branch itself is already broken:
https://github.com/gitgitgadget/git/actions/runs/441233234

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v5 0/1] mergetool: remove unconflicted lines
  2020-12-30  5:47   ` Johannes Schindelin
@ 2020-12-30 22:33     ` Felipe Contreras
  0 siblings, 0 replies; 80+ messages in thread
From: Felipe Contreras @ 2020-12-30 22:33 UTC (permalink / raw)
  To: Johannes Schindelin, Junio C Hamano
  Cc: Felipe Contreras, git, David Aguilar, Johannes Sixt, Seth House

Johannes Schindelin wrote:
> On Thu, 24 Dec 2020, Junio C Hamano wrote:
> > This breakage is possibly a fallout from either this patch or
> > 1e2ae142 (t7[5-9]*: adjust the references to the default branch name
> > "main", 2020-11-18).
> >
> >   https://github.com/git/git/runs/1602803804#step:7:10358
> >
> > I cannot quite tell how the two strings compared with 'test' on
> > output line 10355 are different in the output, though.
> 
> I spent more time than I cared to spend on this, and still have not quite
> figured out what is the fault, but I can state with conviction that the
> problem is not even introduced by any merge into `seen`. The
> `fc/mergetool-automerge` branch itself is already broken:
> https://github.com/gitgitgadget/git/actions/runs/441233234

Yes, if you didn't have me blocked and read what I said a week ago in
[1], you would have saved yourself that time.

Seth House has claimed the patch series though.

[1] https://lore.kernel.org/git/5fe4bec2da21a_19c92085f@natae.notmuch/

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function
  2020-12-28 19:29         ` [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function Seth House
  2020-12-29  2:01           ` Felipe Contreras
@ 2021-01-06  5:55           ` Junio C Hamano
  2021-01-07  3:58             ` Seth House
  1 sibling, 1 reply; 80+ messages in thread
From: Junio C Hamano @ 2021-01-06  5:55 UTC (permalink / raw)
  To: Seth House; +Cc: git

Seth House <seth@eseth.com> writes:

> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 929192d0f8..a44afd3822 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -336,7 +336,7 @@ merge_file () {
>  
>  	initialize_merge_tool "$merge_tool"
>  
> -	if test "$(
> +	if automerge_enabled && test "$(
>  		git config --get --bool "mergetool.$merge_tool.automerge" ||
>  		git config --get --bool "mergetool.automerge" ||
>  		echo true)" = true

This allows the tool author to say "nobody ever is allowed to use my
tool with the automerge feature".  I know I may have suggested
something like that, but I am not sure if we want to be all that
draconian.

If the user explicitly says "I want the new behaviour enabled for
this particular merge tool", we are better off letting the user use
it and take responsibility for the possible breakage.

My preference would probably be

 - if "mergetool.$merge_tool.automerge" is set to 'true' or 'false',
   that's final.

 - Your automerge_enabled helper that is by default 'true' (but
   allows individual merge_tool to return 'false') is asked, and if
   it says 'false', that's final.  But 'true' from automerge_enabled
   is not final at this step.

 - if "mergetool.automerge" is set to 'true' or 'false', that's
   final.

 - otherwise, your automerge_enabled helper's answer (either 'true'
   or 'false') gives the final answer.

That way, those who use a broad "mergetool.automerge = true/false"
would still honor what automerge_enabled yields (which is "enabled
by default but individual merge_tool can set its default to be
disabled"), while individual mergetool.$merge_tool.automerge
configuration would always win.

Hmm?

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function
  2021-01-06  5:55           ` Junio C Hamano
@ 2021-01-07  3:58             ` Seth House
  2021-01-07  6:38               ` Junio C Hamano
  0 siblings, 1 reply; 80+ messages in thread
From: Seth House @ 2021-01-07  3:58 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Tue, Jan 05, 2021 at 09:55:26PM -0800, Junio C Hamano wrote:
> If the user explicitly says "I want the new behaviour enabled for
> this particular merge tool", we are better off letting the user use
> it and take responsibility for the possible breakage.

Good suggestion. Agreed on all counts. I'll roll that preference
hierarchy into the v10 patch set.


^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function
  2021-01-07  3:58             ` Seth House
@ 2021-01-07  6:38               ` Junio C Hamano
  2021-01-07  9:27                 ` Seth House
  0 siblings, 1 reply; 80+ messages in thread
From: Junio C Hamano @ 2021-01-07  6:38 UTC (permalink / raw)
  To: Seth House; +Cc: git

Seth House <seth@eseth.com> writes:

> On Tue, Jan 05, 2021 at 09:55:26PM -0800, Junio C Hamano wrote:
>> If the user explicitly says "I want the new behaviour enabled for
>> this particular merge tool", we are better off letting the user use
>> it and take responsibility for the possible breakage.
>
> Good suggestion. Agreed on all counts. I'll roll that preference
> hierarchy into the v10 patch set.

By the way, do you have any idea why we see test breakages only on
macos when this topic is merged to 'seen'?

https://github.com/git/git/runs/1659807735?check_suite_focus=true#step:4:1641
https://github.com/git/git/runs/1659807735?check_suite_focus=true#step:5:2641

Thanks.

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function
  2021-01-07  6:38               ` Junio C Hamano
@ 2021-01-07  9:27                 ` Seth House
  2021-01-07 21:26                   ` Junio C Hamano
  0 siblings, 1 reply; 80+ messages in thread
From: Seth House @ 2021-01-07  9:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Wed, Jan 06, 2021 at 10:38:09PM -0800, Junio C Hamano wrote:
> By the way, do you have any idea why we see test breakages only on
> macos when this topic is merged to 'seen'?

Thanks for those links. I have an OSX machine nearby and will
investigate tomorrow.

Related: are the Windows tests affected by this patch? I wanted to check
for myself but I've been struggling with getting Git-for-Windows
installed in a VM.


^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function
  2021-01-07  9:27                 ` Seth House
@ 2021-01-07 21:26                   ` Junio C Hamano
  2021-01-08 15:04                     ` Johannes Schindelin
  0 siblings, 1 reply; 80+ messages in thread
From: Junio C Hamano @ 2021-01-07 21:26 UTC (permalink / raw)
  To: Seth House; +Cc: git, Johannes Schindelin

Seth House <seth@eseth.com> writes:

> On Wed, Jan 06, 2021 at 10:38:09PM -0800, Junio C Hamano wrote:
>> By the way, do you have any idea why we see test breakages only on
>> macos when this topic is merged to 'seen'?
>
> Thanks for those links. I have an OSX machine nearby and will
> investigate tomorrow.
>
> Related: are the Windows tests affected by this patch? I wanted to check
> for myself but I've been struggling with getting Git-for-Windows
> installed in a VM.

On the left hand side of the page I gave the links to, it shows that
'windows-build' job is failing (and windows-test jobs are not run as
a consequence).  I am not sure why it failed, but I have a feeling
that the build machinery hasn't even seen the code being built when
it errored out.

  cf. https://github.com/git/git/runs/1659807855?check_suite_focus=true#step:3:40

So we cannot tell (yet).

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function
  2021-01-07 21:26                   ` Junio C Hamano
@ 2021-01-08 15:04                     ` Johannes Schindelin
  0 siblings, 0 replies; 80+ messages in thread
From: Johannes Schindelin @ 2021-01-08 15:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Seth House, git

Hi Junio,

On Thu, 7 Jan 2021, Junio C Hamano wrote:

> Seth House <seth@eseth.com> writes:
>
> > On Wed, Jan 06, 2021 at 10:38:09PM -0800, Junio C Hamano wrote:
> >> By the way, do you have any idea why we see test breakages only on
> >> macos when this topic is merged to 'seen'?
> >
> > Thanks for those links. I have an OSX machine nearby and will
> > investigate tomorrow.
> >
> > Related: are the Windows tests affected by this patch? I wanted to check
> > for myself but I've been struggling with getting Git-for-Windows
> > installed in a VM.
>
> On the left hand side of the page I gave the links to, it shows that
> 'windows-build' job is failing (and windows-test jobs are not run as
> a consequence).  I am not sure why it failed, but I have a feeling
> that the build machinery hasn't even seen the code being built when
> it errored out.
>
>   cf. https://github.com/git/git/runs/1659807855?check_suite_focus=true#step:3:40
>
> So we cannot tell (yet).

There are unfortunately intermittent failures while downloading
git-sdk-64-minimal; That's what this job is seeing. I restarted that build:
https://github.com/git/git/runs/1659807855?check_suite_focus=true#step:3:40

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH v10 0/3]  mergetool: add hideResolved configuration (was automerge)
  2020-12-28 19:29       ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House
                           ` (4 preceding siblings ...)
  2020-12-28 19:29         ` [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function Seth House
@ 2021-01-30  5:46         ` Seth House
  2021-01-30  5:46           ` [PATCH v10 1/3] mergetool: add hideResolved configuration Seth House
                             ` (3 more replies)
  5 siblings, 4 replies; 80+ messages in thread
From: Seth House @ 2021-01-30  5:46 UTC (permalink / raw)
  To: git
  Cc: Seth House, Junio C Hamano, David Aguilar, Johannes Sixt,
	Felipe Contreras

Changes since v9:

- Rename automerge to hideResolved.

  Several mergetools have a feature that they call "automerge", "auto
  merge", or "auto solve" and Git has a `mergetool.meld.useAutoMerge`
  flag so I think it's better to avoid potential confusion. Plus Git
  already performed the merge and this doesn't do any additional
  merging, instead it just "hides" conflicts that Git already resolved.

- Reworked and consolidated commits and commit messages.

- Add preference hierarchy.

  Followed Junio's suggestions:
  https://lore.kernel.org/git/xmqqpn2ivcc1.fsf@gitster.c.googlers.com/

- Switch from sed to two merge-file calls.

  Thanks to everyone who helped with all the suggestions and fixup!s to
  get sed working cross-platform. Unfortunately there's not a great,
  portable method to preserve carriage returns when using both autocrlf
  and MSYS2: https://lore.kernel.org/git/20210120232447.GA35105@ellen/

  Although calling merge-file twice is (much) less efficient than sed
  it's still fairly quick for small files. For large files it's likely
  opening those files in a mergetool will have a higher overhead than
  the merge-file invocations:
  https://lore.kernel.org/git/20210122010902.GA48178@ellen/

  A potential future optimisation could be to augment the
  C implementation (xmerge.c ?) with a flag to write two files as the
  merge is being performed instead of writing conflict markers.

- Kept `initialize_merge_tool` wrapper.

  I updated the commit message where `initialize_merge_tool` is
  introduced to try and better explain my thinking for not simply
  exposing `setup_tool` instead. I'm happy to switch that if anyone
  still feels it should be switched.

Seth House (3):
  mergetool: add hideResolved configuration
  mergetool: break setup_tool out into separate initialization function
  mergetool: add per-tool support and overrides for the hideResolved
    flag

 Documentation/config/mergetool.txt   | 15 +++++++++++++++
 Documentation/git-mergetool--lib.txt |  4 ++++
 git-difftool--helper.sh              |  6 ++++++
 git-mergetool--lib.sh                | 11 ++++++++---
 git-mergetool.sh                     | 26 ++++++++++++++++++++++++++
 t/t7610-mergetool.sh                 | 18 ++++++++++++++++++
 6 files changed, 77 insertions(+), 3 deletions(-)

-- 
2.29.2



^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH v10 1/3] mergetool: add hideResolved configuration
  2021-01-30  5:46         ` [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
@ 2021-01-30  5:46           ` Seth House
  2021-01-30  8:08             ` Junio C Hamano
  2021-01-30  5:46           ` [PATCH v10 2/3] mergetool: break setup_tool out into separate initialization function Seth House
                             ` (2 subsequent siblings)
  3 siblings, 1 reply; 80+ messages in thread
From: Seth House @ 2021-01-30  5:46 UTC (permalink / raw)
  To: git; +Cc: Seth House, Felipe Contreras

The purpose of a mergetool is to help the user resolve any conflicts
that Git cannot automatically resolve. If there is a conflict that must
be resolved manually Git will write a file named MERGED which contains
everything Git was able to resolve by itself and also everything that it
was not able to resolve wrapped in conflict markers.

One way to think of MERGED is as a two- or three-way diff. If each
"side" of the conflict markers is separately extracted an external tool
can represent those conflicts as a side-by-side diff.

However many mergetools instead diff LOCAL and REMOTE both of which
contain versions of the file from before the merge. Since the conflicts
Git resolved automatically are not present it forces the user to
manually re-resolve those conflicts. Some mergetools also show MERGED
but often only for reference and not as the focal point to resolve the
conflicts.

This adds a `mergetool.hideResolved` flag that will overwrite LOCAL and
REMOTE with each corresponding "side" of a conflicted file and thus hide
all conflicts that Git was able to resolve itself. Overwriting these
files will immediately benefit any mergetool that uses them without
requiring any changes.

No adverse effects were noted in a small survey of popular mergetools[1]
so this behavior defaults to `true`. However it can be globally disabled
by setting `mergetool.hideResolved` to `false`.

[1] https://www.eseth.org/2020/mergetools.html
    https://github.com/whiteinge/eseth/blob/c884424769fffb05d87afb33b2cf80cecb4044c3/2020/mergetools.md

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

diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 16a27443a3..3171bacf91 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -40,6 +40,15 @@ mergetool.meld.useAutoMerge::
 	value of `false` avoids using `--auto-merge` altogether, and is the
 	default value.
 
+mergetool.hideResolved::
+	During a merge Git will automatically resolve as many conflicts as
+	possible and then wrap conflict markers around any conflicts that it
+	cannot resolve. This flag writes the non-conflicting parts into the
+	corresponding 'LOCAL' and 'REMOTE' files so that only the unresolved
+	conflicts are presented to the merge tool. Can be overriden per-tool
+	via the `mergetool.<tool>.hideResolved` configuration variable.
+	Defaults to `true`.
+
 mergetool.keepBackup::
 	After performing a merge, the original file with conflict markers
 	can be saved as a file with a `.orig` extension.  If this variable
diff --git a/git-mergetool.sh b/git-mergetool.sh
index e3f6d543fb..5b0d15ed89 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -239,6 +239,13 @@ checkout_staged_file () {
 	fi
 }
 
+hide_resolved () {
+	git merge-file --ours -q -p "$LOCAL" "$BASE" "$REMOTE" >"$LCONFL"
+	git merge-file --theirs -q -p "$LOCAL" "$BASE" "$REMOTE" >"$RCONFL"
+	mv -- "$LCONFL" "$LOCAL"
+	mv -- "$RCONFL" "$REMOTE"
+}
+
 merge_file () {
 	MERGED="$1"
 
@@ -276,7 +283,9 @@ merge_file () {
 
 	BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext"
 	LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
+	LCONFL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_LCONFL_$$$ext"
 	REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext"
+	RCONFL="$MERGETOOL_TMPDIR/${BASE}_REMOTE_RCONFL_$$$ext"
 	BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext"
 
 	base_mode= local_mode= remote_mode=
@@ -322,6 +331,11 @@ merge_file () {
 	checkout_staged_file 2 "$MERGED" "$LOCAL"
 	checkout_staged_file 3 "$MERGED" "$REMOTE"
 
+	if test "$(git config --get mergetool.hideResolved)" != "false"
+	then
+		hide_resolved
+	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..0e34b87e37 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 hideResolved' '
+	test_config mergetool.hideResolved true &&
+	test_when_finished "git reset --hard" &&
+	git checkout -b test${test_count}_b master &&
+	test_write_lines >file1 base "" a &&
+	git commit -a -m "base" &&
+	test_write_lines >file1 base "" c &&
+	git commit -a -m "remote update" &&
+	git checkout -b test${test_count}_a HEAD~ &&
+	test_write_lines >file1 local "" b &&
+	git commit -a -m "local update" &&
+	test_must_fail git merge test${test_count}_b &&
+	yes "" | git mergetool file1 &&
+	test_write_lines >expect local "" c &&
+	test_cmp expect file1 &&
+	git commit -m "test resolved with mergetool"
+'
+
 test_done
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* [PATCH v10 2/3] mergetool: break setup_tool out into separate initialization function
  2021-01-30  5:46         ` [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
  2021-01-30  5:46           ` [PATCH v10 1/3] mergetool: add hideResolved configuration Seth House
@ 2021-01-30  5:46           ` Seth House
  2021-01-30  5:46           ` [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House
  2021-02-09 20:07           ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
  3 siblings, 0 replies; 80+ messages in thread
From: Seth House @ 2021-01-30  5:46 UTC (permalink / raw)
  To: git; +Cc: Seth House

This is preparation for the following commit where we need to source the
mergetool shell script to look for overrides before `run_merge_tool` is
called. Previously `run_merge_tool` both sourced that script and invoked
the mergetool.

In the case of the following commit, we need the result of the
`hide_resolved` override, if present, before we actually run
`run_merge_tool`.

The new `initialize_merge_tool` wrapper is exposed and documented as
a public interface for consistency with the existing `run_merge_tool`
which is also public. Although `setup_tool` could instead be exposed
directly, the related `setup_user_tool` would probably also want to be
elevated to match and this felt the cleanest to me.

Signed-off-by: Seth House <seth@eseth.com>
---
 Documentation/git-mergetool--lib.txt | 4 ++++
 git-difftool--helper.sh              | 6 ++++++
 git-mergetool--lib.sh                | 7 ++++---
 git-mergetool.sh                     | 2 ++
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt
index 4da9d24096..3e8f59ac0e 100644
--- a/Documentation/git-mergetool--lib.txt
+++ b/Documentation/git-mergetool--lib.txt
@@ -38,6 +38,10 @@ get_merge_tool_cmd::
 get_merge_tool_path::
 	returns the custom path for a merge tool.
 
+initialize_merge_tool::
+	bring merge tool specific functions into scope so they can be used or
+	overridden.
+
 run_merge_tool::
 	launches a merge tool given the tool name and a true/false
 	flag to indicate whether a merge base is present.
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 46af3e60b7..992124cc67 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -61,6 +61,9 @@ launch_merge_tool () {
 		export BASE
 		eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'
 	else
+		initialize_merge_tool "$merge_tool"
+		# ignore the error from the above --- run_merge_tool
+		# will diagnose unusable tool by itself
 		run_merge_tool "$merge_tool"
 	fi
 }
@@ -79,6 +82,9 @@ if test -n "$GIT_DIFFTOOL_DIRDIFF"
 then
 	LOCAL="$1"
 	REMOTE="$2"
+	initialize_merge_tool "$merge_tool"
+	# ignore the error from the above --- run_merge_tool
+	# will diagnose unusable tool by itself
 	run_merge_tool "$merge_tool" false
 else
 	# Launch the merge tool on each path provided by 'git diff'
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 7225abd811..e059b3559e 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -248,6 +248,10 @@ trust_exit_code () {
 	fi
 }
 
+initialize_merge_tool () {
+	# Bring tool-specific functions into scope
+	setup_tool "$1" || return 1
+}
 
 # Entry point for running tools
 run_merge_tool () {
@@ -259,9 +263,6 @@ run_merge_tool () {
 	merge_tool_path=$(get_merge_tool_path "$1") || exit
 	base_present="$2"
 
-	# Bring tool-specific functions into scope
-	setup_tool "$1" || return 1
-
 	if merge_mode
 	then
 		run_merge_cmd "$1"
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 5b0d15ed89..865f12551a 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -272,6 +272,8 @@ merge_file () {
 		ext=
 	esac
 
+	initialize_merge_tool "$merge_tool" || return
+
 	mergetool_tmpdir_init
 
 	if test "$MERGETOOL_TMPDIR" != "."
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag
  2021-01-30  5:46         ` [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
  2021-01-30  5:46           ` [PATCH v10 1/3] mergetool: add hideResolved configuration Seth House
  2021-01-30  5:46           ` [PATCH v10 2/3] mergetool: break setup_tool out into separate initialization function Seth House
@ 2021-01-30  5:46           ` Seth House
  2021-01-30  8:08             ` Junio C Hamano
  2021-02-09 20:07           ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
  3 siblings, 1 reply; 80+ messages in thread
From: Seth House @ 2021-01-30  5:46 UTC (permalink / raw)
  To: git; +Cc: Seth House, Johannes Sixt, Junio C Hamano

Keep the global mergetool flag and add a per-tool override flag so that
users may enable the flag for one tool and disable it for another. In
addition, the author or maintainer of a mergetool may optionally elect
to set the default `hideResolved` value for that mergetool.

To disable the feature for a specific tool, edit the `mergetools/<tool>`
shell script for that tool and add a `hide_resolved_enabled` function:

    hide_resolved_enabled () {
        return 1
    }

Disabling may be desirable if the mergetool wants or needs access to the
original, unmodified 'LOCAL' and 'REMOTE' versions of the conflicted
file. For example:

- A tool may use a custom conflict resolution algorithm and prefer to
  ignore the results of Git's conflict resolution.
- A tool may want to visually compare/constrast the version of the file
  from before the merge (saved to 'LOCAL', 'REMOTE', and 'BASE') with
  Git's conflict resolution results (saved to 'MERGED').

Helped-by: Johannes Sixt <j6t@kdbg.org>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Seth House <seth@eseth.com>
---
 Documentation/config/mergetool.txt |  6 ++++++
 git-mergetool--lib.sh              |  4 ++++
 git-mergetool.sh                   | 14 ++++++++++++--
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 3171bacf91..046816fb07 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -13,6 +13,12 @@ mergetool.<tool>.cmd::
 	merged; 'MERGED' contains the name of the file to which the merge
 	tool should write the results of a successful merge.
 
+mergetool.<tool>.hideResolved::
+	A mergetool-specific override for the global `mergetool.hideResolved`
+	configuration flag. This allows individual mergetools to enable or
+	disable the flag regardless of the global setting. See
+	`mergetool.hideResolved` for the full description.
+
 mergetool.<tool>.trustExitCode::
 	For a custom merge command, specify whether the exit code of
 	the merge command can be used to determine whether the merge was
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index e059b3559e..11f00dde41 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -164,6 +164,10 @@ setup_tool () {
 		return 1
 	}
 
+	hide_resolved_enabled () {
+		return 0
+	}
+
 	translate_merge_tool_path () {
 		echo "$1"
 	}
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 865f12551a..6cf3884277 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -333,9 +333,19 @@ merge_file () {
 	checkout_staged_file 2 "$MERGED" "$LOCAL"
 	checkout_staged_file 3 "$MERGED" "$REMOTE"
 
-	if test "$(git config --get mergetool.hideResolved)" != "false"
+	# hideResolved preferences hierarchy:
+	# First respect user's tool-specific configuration if exists.
+	if test "$(git config --get "mergetool.$merge_tool.hideResolved")" != "false"
 	then
-		hide_resolved
+		# Next respect tool-specified configuration.
+		if hide_resolved_enabled
+		then
+			# Finally respect if user has a global disable.
+			if test "$(git config --get "mergetool.hideResolved")" != "false"
+			then
+				hide_resolved
+			fi
+		fi
 	fi
 
 	if test -z "$local_mode" || test -z "$remote_mode"
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* Re: [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag
  2021-01-30  5:46           ` [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House
@ 2021-01-30  8:08             ` Junio C Hamano
  0 siblings, 0 replies; 80+ messages in thread
From: Junio C Hamano @ 2021-01-30  8:08 UTC (permalink / raw)
  To: Seth House; +Cc: git, Johannes Sixt

Seth House <seth@eseth.com> writes:

> Keep the global mergetool flag and add a per-tool override flag so that
> users may enable the flag for one tool and disable it for another. In
> addition, the author or maintainer of a mergetool may optionally elect
> to set the default `hideResolved` value for that mergetool.

OK.

> To disable the feature for a specific tool, edit the `mergetools/<tool>`
> shell script for that tool and add a `hide_resolved_enabled` function:
>
>     hide_resolved_enabled () {
>         return 1
>     }
>
> Disabling may be desirable if the mergetool wants or needs access to the
> original, unmodified 'LOCAL' and 'REMOTE' versions of the conflicted
> file.

The above sounds as if it is a hint/help for end users, but it is
unreasonable to expect all end users of a particular <tool> to edit
part of their Git installation.  I suspect that you didn't mean it
that way, and instead it is meant to advise (new) tool authors who
will add mergetools/<tool> for their own tool, and when read with
that in mind, it does make sort-of sense (except that when you are
author of this thing, you won't "edit" as if you are modifying
something that already exists---you'd be the one who is adding the
<tool> under mergetools/ directory).

For an end-user, to disable the feature for a tool, you'd just
configure mergetool.<tool>.hideResolved to 'false', right?

> For example:
>
> - A tool may use a custom conflict resolution algorithm and prefer to
>   ignore the results of Git's conflict resolution.
> - A tool may want to visually compare/constrast the version of the file
>   from before the merge (saved to 'LOCAL', 'REMOTE', and 'BASE') with
>   Git's conflict resolution results (saved to 'MERGED').
>
> Helped-by: Johannes Sixt <j6t@kdbg.org>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Seth House <seth@eseth.com>
> ---
>  Documentation/config/mergetool.txt |  6 ++++++
>  git-mergetool--lib.sh              |  4 ++++
>  git-mergetool.sh                   | 14 ++++++++++++--
>  3 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
> index 3171bacf91..046816fb07 100644
> --- a/Documentation/config/mergetool.txt
> +++ b/Documentation/config/mergetool.txt
> @@ -13,6 +13,12 @@ mergetool.<tool>.cmd::
>  	merged; 'MERGED' contains the name of the file to which the merge
>  	tool should write the results of a successful merge.
>  
> +mergetool.<tool>.hideResolved::
> +	A mergetool-specific override for the global `mergetool.hideResolved`
> +	configuration flag. This allows individual mergetools to enable or
> +	disable the flag regardless of the global setting. See
> +	`mergetool.hideResolved` for the full description.

This description is iffy.  

The configuration allows "users" to enable or disable the feature
for individual mergetools, overriding the 'mergetool.hideResolved'
global setting, no?  The above paragraph reads as if the tool author
is enabling/disabling it.

> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index e059b3559e..11f00dde41 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -164,6 +164,10 @@ setup_tool () {
>  		return 1
>  	}
>  
> +	hide_resolved_enabled () {
> +		return 0
> +	}
> +
>  	translate_merge_tool_path () {
>  		echo "$1"
>  	}
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 865f12551a..6cf3884277 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -333,9 +333,19 @@ merge_file () {
>  	checkout_staged_file 2 "$MERGED" "$LOCAL"
>  	checkout_staged_file 3 "$MERGED" "$REMOTE"
>  
> -	if test "$(git config --get mergetool.hideResolved)" != "false"
> +	# hideResolved preferences hierarchy:
> +	# First respect user's tool-specific configuration if exists.
> +	if test "$(git config --get "mergetool.$merge_tool.hideResolved")" != "false"

The same "--type=bool" comment applies to this step, too.

>  	then
> +		# Next respect tool-specified configuration.
> +		if hide_resolved_enabled
> +		then
> +			# Finally respect if user has a global disable.
> +			if test "$(git config --get "mergetool.hideResolved")" != "false"
> +			then
> +				hide_resolved
> +			fi
> +		fi
>  	fi

I am not sure if I understand this logic.

If the user says "for the tool <tool>, set hideresolved to
true/false" explicitly, I think it should be final.  Even if the
tool's author expresses that s/he prefers not to have to work on a
pre-munged input by setting hide_resolved_enabled to false, if the
end-user says s/he wants to use it on that tool, we do not want to
help the tool to override the user's wish.

If the user says "use hideresolved feature, as I like it in general"
by setting mergetool.hideResolved, on the other hand, it may be also
reasonable to heed "no, I recommend against it for this tool" for
individual tool whose hide_resolved_enabled returns false.  And if
the global is set to 'false', the user says "I do not want it", and
it may be iffy to let individual tool to countermand it.

WHen dealing with either of these variables, therefore, you'd need
to know if the variable is not set at all, or if the variable is set
to true, or to false.  Even if we default to enabled, we need to be
able to tell if the user didn't say anything (and we enabled the
feature for the user because of our default choice), or if the user
explicitly said s/he wants it.

In other words, you'd need to treat mergetool.hideResolved and
mergetool.$merge_tool.hideResolved as tristates.

Here is how "git config --type=bool" can be used to normalize
various ways to spell true/false and tell between "not set" and "set
to some value":

    $ git -c a.b config --type=bool a.b; echo $?
    true
    0
    $ git -c a.b=yes config --type=bool a.b; echo $?
    true
    0
    $ git -c a.b=0 config --type=bool a.b; echo $?
    false
    0
    $ git config --type=bool a.b; echo $?
    1

IOW, if "git config --type=bool" fails, the user does not have the
variable set.  If it succeeds, you'd get normalized 'true/false'
string on its standard output.

Using that technique, here is my attempt to rewrite the above logic,
with commentary.

    global_config=mergetool.hideResolved
    tool_config=mergetool.$merge_tool.hideResolved

    if enabled=$(git config --type=bool "$tool_config")
    then
	# The user explicitly says true or false, so there
	# is no point in asking any other source of preferences
	;
    elif enabled=$(git config --type=bool "$global_config")
    then
	# There is a blanket preference for all tools, and 'true'
	# means "I like the hide-resolved in general, so use it
	# when appropriate" by the user.  We can let the tool
	# author to override and disable, though.
	#
	# On the other hand, when set to 'false', it is "I really
	# don't like the feature in general, so do not use it
	# anywhere", which we take it as final, without letting
	# the tool override it.
        if test "$enabled" = true && hide_resolved_enabled
	then
		enabled=true
	else
		enabled=false
	fi
    else
	# The user does not have preference.  Ask the tool
	if hide_resolved_enabled
	then
		enabled=true
	else
		enabled=false
	fi
    fi

    # Now we know if the feature should be used.
    if test "$enabled" = true
    then
	hide_resolved
    fi


^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v10 1/3] mergetool: add hideResolved configuration
  2021-01-30  5:46           ` [PATCH v10 1/3] mergetool: add hideResolved configuration Seth House
@ 2021-01-30  8:08             ` Junio C Hamano
  0 siblings, 0 replies; 80+ messages in thread
From: Junio C Hamano @ 2021-01-30  8:08 UTC (permalink / raw)
  To: Seth House; +Cc: git, Felipe Contreras

Seth House <seth@eseth.com> writes:

> +mergetool.hideResolved::
> +	During a merge Git will automatically resolve as many conflicts as
> +	possible and then wrap conflict markers around any conflicts that it
> +	cannot resolve. This flag writes the non-conflicting parts into the
> +	corresponding 'LOCAL' and 'REMOTE' files so that only the unresolved
> +	conflicts are presented to the merge tool. Can be overriden per-tool
> +	via the `mergetool.<tool>.hideResolved` configuration variable.
> +	Defaults to `true`.

This description makes the readers expect that the configuration
variable is a boolean, and setting it to 'no' would disable the
feature, but ...

> @@ -322,6 +331,11 @@ merge_file () {
>  	checkout_staged_file 2 "$MERGED" "$LOCAL"
>  	checkout_staged_file 3 "$MERGED" "$REMOTE"
>  
> +	if test "$(git config --get mergetool.hideResolved)" != "false"

... without --type=bool, any boolean 'false' value that is not
exactly spelled 'false' won't be normalized and fail this test.

I haven't read the remaining 2 patches, so I cannot yet tell if I
can just insert "--type=bool" here and everything would be fine,
or if there are other fallouts for doing so.

> +	then
> +		hide_resolved
> +	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..0e34b87e37 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 hideResolved' '
> +	test_config mergetool.hideResolved true &&
> +	test_when_finished "git reset --hard" &&
> +	git checkout -b test${test_count}_b master &&

As a new feature, this should work with the tip of 'master', but I
think the t7610 test forces the initial branch name to be 'main'.

I'll tweak locally while queuing.

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

^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH v11 0/3]  mergetool: add hideResolved configuration (was automerge)
  2021-01-30  5:46         ` [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
                             ` (2 preceding siblings ...)
  2021-01-30  5:46           ` [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House
@ 2021-02-09 20:07           ` Seth House
  2021-02-09 20:07             ` [PATCH v11 1/3] mergetool: add hideResolved configuration Seth House
                               ` (3 more replies)
  3 siblings, 4 replies; 80+ messages in thread
From: Seth House @ 2021-02-09 20:07 UTC (permalink / raw)
  To: git
  Cc: Seth House, Junio C Hamano, David Aguilar, Johannes Sixt,
	Felipe Contreras

Changes since v10:

- Update the calls to `git config` to return normalized strings.

  Junio, thank you for explaining the existence/omission and normalized
  strings tristate. I missed that in the docs and that's perfect.

- Adopt Junio's replacement preference hierarchy conditionals to respect
  opt-ins and not just opt-outs.

  Your suggested code worked out-of-box in all the scenarios I could
  think to test.  \o/

- Tweak the mergetool.hideResolved docs to call out the role of LOCAL
  and REMOTE.

- Reword commit messages and docs to better differentiate between config
  flags users set and code that merge tool maintainers write.

Seth House (3):
  mergetool: add hideResolved configuration
  mergetool: break setup_tool out into separate initialization function
  mergetool: add per-tool support and overrides for the hideResolved
    flag

 Documentation/config/mergetool.txt   | 14 ++++++++
 Documentation/git-mergetool--lib.txt |  4 +++
 git-difftool--helper.sh              |  6 ++++
 git-mergetool--lib.sh                | 11 ++++--
 git-mergetool.sh                     | 52 ++++++++++++++++++++++++++++
 t/t7610-mergetool.sh                 | 18 ++++++++++
 6 files changed, 102 insertions(+), 3 deletions(-)

-- 
2.29.2



^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH v11 1/3] mergetool: add hideResolved configuration
  2021-02-09 20:07           ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
@ 2021-02-09 20:07             ` Seth House
  2021-03-09  2:29               ` [PATCH] mergetool: do not enable hideResolved by default Jonathan Nieder
  2021-02-09 20:07             ` [PATCH v11 2/3] mergetool: break setup_tool out into separate initialization function Seth House
                               ` (2 subsequent siblings)
  3 siblings, 1 reply; 80+ messages in thread
From: Seth House @ 2021-02-09 20:07 UTC (permalink / raw)
  To: git; +Cc: Seth House, Felipe Contreras

The purpose of a mergetool is to help the user resolve any conflicts
that Git cannot automatically resolve. If there is a conflict that must
be resolved manually Git will write a file named MERGED which contains
everything Git was able to resolve by itself and also everything that it
was not able to resolve wrapped in conflict markers.

One way to think of MERGED is as a two- or three-way diff. If each
"side" of the conflict markers is separately extracted an external tool
can represent those conflicts as a side-by-side diff.

However many mergetools instead diff LOCAL and REMOTE both of which
contain versions of the file from before the merge. Since the conflicts
Git resolved automatically are not present it forces the user to
manually re-resolve those conflicts. Some mergetools also show MERGED
but often only for reference and not as the focal point to resolve the
conflicts.

This adds a `mergetool.hideResolved` flag that will overwrite LOCAL and
REMOTE with each corresponding "side" of a conflicted file and thus hide
all conflicts that Git was able to resolve itself. Overwriting these
files will immediately benefit any mergetool that uses them without
requiring any changes to the tool.

No adverse effects were noted in a small survey of popular mergetools[1]
so this behavior defaults to `true`. However it can be globally disabled
by setting `mergetool.hideResolved` to `false`.

[1] https://www.eseth.org/2020/mergetools.html
    https://github.com/whiteinge/eseth/blob/c884424769fffb05d87afb33b2cf80cecb4044c3/2020/mergetools.md

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

diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 16a27443a3..b858191970 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -40,6 +40,16 @@ mergetool.meld.useAutoMerge::
 	value of `false` avoids using `--auto-merge` altogether, and is the
 	default value.
 
+mergetool.hideResolved::
+	During a merge Git will automatically resolve as many conflicts as
+	possible and write the 'MERGED' file containing conflict markers around
+	any conflicts that it cannot resolve; 'LOCAL' and 'REMOTE' normally
+	represent the versions of the file from before Git's conflict
+	resolution. This flag causes 'LOCAL' and 'REMOTE' to be overwriten so
+	that only the unresolved conflicts are presented to the merge tool. Can
+	be configured per-tool via the `mergetool.<tool>.hideResolved`
+	configuration variable. Defaults to `true`.
+
 mergetool.keepBackup::
 	After performing a merge, the original file with conflict markers
 	can be saved as a file with a `.orig` extension.  If this variable
diff --git a/git-mergetool.sh b/git-mergetool.sh
index e3f6d543fb..40a103443d 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -239,6 +239,13 @@ checkout_staged_file () {
 	fi
 }
 
+hide_resolved () {
+	git merge-file --ours -q -p "$LOCAL" "$BASE" "$REMOTE" >"$LCONFL"
+	git merge-file --theirs -q -p "$LOCAL" "$BASE" "$REMOTE" >"$RCONFL"
+	mv -- "$LCONFL" "$LOCAL"
+	mv -- "$RCONFL" "$REMOTE"
+}
+
 merge_file () {
 	MERGED="$1"
 
@@ -276,7 +283,9 @@ merge_file () {
 
 	BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext"
 	LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
+	LCONFL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_LCONFL_$$$ext"
 	REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext"
+	RCONFL="$MERGETOOL_TMPDIR/${BASE}_REMOTE_RCONFL_$$$ext"
 	BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext"
 
 	base_mode= local_mode= remote_mode=
@@ -322,6 +331,11 @@ merge_file () {
 	checkout_staged_file 2 "$MERGED" "$LOCAL"
 	checkout_staged_file 3 "$MERGED" "$REMOTE"
 
+	if test "$(git config --type=bool mergetool.hideResolved)" != "false"
+	then
+		hide_resolved
+	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 04b0095072..cec4a860ef 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -842,4 +842,22 @@ test_expect_success 'mergetool --tool-help shows recognized tools' '
 	grep meld mergetools
 '
 
+test_expect_success 'mergetool hideResolved' '
+	test_config mergetool.hideResolved true &&
+	test_when_finished "git reset --hard" &&
+	git checkout -b test${test_count}_b master &&
+	test_write_lines >file1 base "" a &&
+	git commit -a -m "base" &&
+	test_write_lines >file1 base "" c &&
+	git commit -a -m "remote update" &&
+	git checkout -b test${test_count}_a HEAD~ &&
+	test_write_lines >file1 local "" b &&
+	git commit -a -m "local update" &&
+	test_must_fail git merge test${test_count}_b &&
+	yes "" | git mergetool file1 &&
+	test_write_lines >expect local "" c &&
+	test_cmp expect file1 &&
+	git commit -m "test resolved with mergetool"
+'
+
 test_done
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* [PATCH v11 2/3] mergetool: break setup_tool out into separate initialization function
  2021-02-09 20:07           ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
  2021-02-09 20:07             ` [PATCH v11 1/3] mergetool: add hideResolved configuration Seth House
@ 2021-02-09 20:07             ` Seth House
  2021-02-09 20:07             ` [PATCH v11 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House
  2021-02-09 22:11             ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Junio C Hamano
  3 siblings, 0 replies; 80+ messages in thread
From: Seth House @ 2021-02-09 20:07 UTC (permalink / raw)
  To: git; +Cc: Seth House

This is preparation for the following commit where we need to source the
mergetool shell script to look for overrides before `run_merge_tool` is
called. Previously `run_merge_tool` both sourced that script and invoked
the mergetool.

In the case of the following commit, we need the result of the
`hide_resolved` override, if present, before we actually run
`run_merge_tool`.

The new `initialize_merge_tool` wrapper is exposed and documented as
a public interface for consistency with the existing `run_merge_tool`
which is also public. Although `setup_tool` could instead be exposed
directly, the related `setup_user_tool` would probably also want to be
elevated to match and this felt the cleanest to me.

Signed-off-by: Seth House <seth@eseth.com>
---
 Documentation/git-mergetool--lib.txt | 4 ++++
 git-difftool--helper.sh              | 6 ++++++
 git-mergetool--lib.sh                | 7 ++++---
 git-mergetool.sh                     | 2 ++
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-mergetool--lib.txt b/Documentation/git-mergetool--lib.txt
index 4da9d24096..3e8f59ac0e 100644
--- a/Documentation/git-mergetool--lib.txt
+++ b/Documentation/git-mergetool--lib.txt
@@ -38,6 +38,10 @@ get_merge_tool_cmd::
 get_merge_tool_path::
 	returns the custom path for a merge tool.
 
+initialize_merge_tool::
+	bring merge tool specific functions into scope so they can be used or
+	overridden.
+
 run_merge_tool::
 	launches a merge tool given the tool name and a true/false
 	flag to indicate whether a merge base is present.
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 46af3e60b7..992124cc67 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -61,6 +61,9 @@ launch_merge_tool () {
 		export BASE
 		eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'
 	else
+		initialize_merge_tool "$merge_tool"
+		# ignore the error from the above --- run_merge_tool
+		# will diagnose unusable tool by itself
 		run_merge_tool "$merge_tool"
 	fi
 }
@@ -79,6 +82,9 @@ if test -n "$GIT_DIFFTOOL_DIRDIFF"
 then
 	LOCAL="$1"
 	REMOTE="$2"
+	initialize_merge_tool "$merge_tool"
+	# ignore the error from the above --- run_merge_tool
+	# will diagnose unusable tool by itself
 	run_merge_tool "$merge_tool" false
 else
 	# Launch the merge tool on each path provided by 'git diff'
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 78f3647ed9..4a8e36c792 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -250,6 +250,10 @@ trust_exit_code () {
 	fi
 }
 
+initialize_merge_tool () {
+	# Bring tool-specific functions into scope
+	setup_tool "$1" || return 1
+}
 
 # Entry point for running tools
 run_merge_tool () {
@@ -261,9 +265,6 @@ run_merge_tool () {
 	merge_tool_path=$(get_merge_tool_path "$1") || exit
 	base_present="$2"
 
-	# Bring tool-specific functions into scope
-	setup_tool "$1" || return 1
-
 	if merge_mode
 	then
 		run_merge_cmd "$1"
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 40a103443d..e5eac935f3 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -272,6 +272,8 @@ merge_file () {
 		ext=
 	esac
 
+	initialize_merge_tool "$merge_tool" || return
+
 	mergetool_tmpdir_init
 
 	if test "$MERGETOOL_TMPDIR" != "."
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* [PATCH v11 3/3] mergetool: add per-tool support and overrides for the hideResolved flag
  2021-02-09 20:07           ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
  2021-02-09 20:07             ` [PATCH v11 1/3] mergetool: add hideResolved configuration Seth House
  2021-02-09 20:07             ` [PATCH v11 2/3] mergetool: break setup_tool out into separate initialization function Seth House
@ 2021-02-09 20:07             ` Seth House
  2021-02-09 22:11             ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Junio C Hamano
  3 siblings, 0 replies; 80+ messages in thread
From: Seth House @ 2021-02-09 20:07 UTC (permalink / raw)
  To: git; +Cc: Seth House, Johannes Sixt, Junio C Hamano

Add a per-tool override flag so that users may enable the flag for one
tool and disable it for another by setting
`mergetool.<tool>.hideResolved` to `false`.

In addition, the author or maintainer of a mergetool may optionally
override the default `hideResolved` value for that mergetool. If the
`mergetools/<tool>` shell script contains a `hide_resolved_enabled`
function it will be called when the mergetool is invoked and the return
value will be used as the default for the `hideResolved` flag.

    hide_resolved_enabled () {
        return 1
    }

Disabling may be desirable if the mergetool wants or needs access to the
original, unmodified 'LOCAL' and 'REMOTE' versions of the conflicted
file. For example:

- A tool may use a custom conflict resolution algorithm and prefer to
  ignore the results of Git's conflict resolution.
- A tool may want to visually compare/constrast the version of the file
  from before the merge (saved to 'LOCAL', 'REMOTE', and 'BASE') with
  Git's conflict resolution results (saved to 'MERGED').

Helped-by: Johannes Sixt <j6t@kdbg.org>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Seth House <seth@eseth.com>
---
 Documentation/config/mergetool.txt |  5 +++++
 git-mergetool--lib.sh              |  4 ++++
 git-mergetool.sh                   | 36 +++++++++++++++++++++++++++++-
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index b858191970..90f76f5b9b 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -13,6 +13,11 @@ mergetool.<tool>.cmd::
 	merged; 'MERGED' contains the name of the file to which the merge
 	tool should write the results of a successful merge.
 
+mergetool.<tool>.hideResolved::
+	Allows the user to override the global `mergetool.hideResolved` value
+	for a specific tool. See `mergetool.hideResolved` for the full
+	description.
+
 mergetool.<tool>.trustExitCode::
 	For a custom merge command, specify whether the exit code of
 	the merge command can be used to determine whether the merge was
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 4a8e36c792..542a6a75eb 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -166,6 +166,10 @@ setup_tool () {
 		return 1
 	}
 
+	hide_resolved_enabled () {
+		return 0
+	}
+
 	translate_merge_tool_path () {
 		echo "$1"
 	}
diff --git a/git-mergetool.sh b/git-mergetool.sh
index e5eac935f3..911470a5b2 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -333,7 +333,41 @@ merge_file () {
 	checkout_staged_file 2 "$MERGED" "$LOCAL"
 	checkout_staged_file 3 "$MERGED" "$REMOTE"
 
-	if test "$(git config --type=bool mergetool.hideResolved)" != "false"
+	# hideResolved preferences hierarchy.
+	global_config="mergetool.hideResolved"
+	tool_config="mergetool.${merge_tool}.hideResolved"
+
+	if enabled=$(git config --type=bool "$tool_config")
+	then
+		# The user has a specific preference for a specific tool and no
+		# other preferences should override that.
+		: ;
+	elif enabled=$(git config --type=bool "$global_config")
+	then
+		# The user has a general preference for all tools.
+		#
+		# 'true' means the user likes the feature so we should use it
+		# where possible but tool authors can still override.
+		#
+		# 'false' means the user doesn't like the feature so we should
+		# not use it anywhere.
+		if test "$enabled" = true && hide_resolved_enabled
+		then
+		    enabled=true
+		else
+		    enabled=false
+		fi
+	else
+		# The user does not have a preference. Ask the tool.
+		if hide_resolved_enabled
+		then
+		    enabled=true
+		else
+		    enabled=false
+		fi
+	fi
+
+	if test "$enabled" = true
 	then
 		hide_resolved
 	fi
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 80+ messages in thread

* Re: [PATCH v11 0/3]  mergetool: add hideResolved configuration (was automerge)
  2021-02-09 20:07           ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
                               ` (2 preceding siblings ...)
  2021-02-09 20:07             ` [PATCH v11 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House
@ 2021-02-09 22:11             ` Junio C Hamano
  2021-02-09 23:27               ` Seth House
  3 siblings, 1 reply; 80+ messages in thread
From: Junio C Hamano @ 2021-02-09 22:11 UTC (permalink / raw)
  To: Seth House; +Cc: git, David Aguilar, Johannes Sixt, Felipe Contreras

Seth House <seth@eseth.com> writes:

> Changes since v10:
> Seth House (3):
>   mergetool: add hideResolved configuration
>   mergetool: break setup_tool out into separate initialization function
>   mergetool: add per-tool support and overrides for the hideResolved
>     flag

Thanks for all these iterations.  The resuling series looks good to
me.

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH v11 0/3]  mergetool: add hideResolved configuration (was automerge)
  2021-02-09 22:11             ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Junio C Hamano
@ 2021-02-09 23:27               ` Seth House
  0 siblings, 0 replies; 80+ messages in thread
From: Seth House @ 2021-02-09 23:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, David Aguilar, Johannes Sixt, Felipe Contreras

On Tue, Feb 09, 2021 at 02:11:05PM -0800, Junio C Hamano wrote:
> Thanks for all these iterations.  The resuling series looks good to
> me.

Woot! Thanks for all the great feedback and for walking me through the
process. I'm really happy with where this ended up.


^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH] mergetool: do not enable hideResolved by default
  2021-02-09 20:07             ` [PATCH v11 1/3] mergetool: add hideResolved configuration Seth House
@ 2021-03-09  2:29               ` Jonathan Nieder
  2021-03-09  5:42                 ` Seth House
  2021-03-10  8:06                 ` Junio C Hamano
  0 siblings, 2 replies; 80+ messages in thread
From: Jonathan Nieder @ 2021-03-09  2:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom

A typical mergetool uses four panes, showing the content of the file
being resolved from MERGE_BASE ('BASE'), HEAD ('LOCAL'), MERGE_HEAD
('REMOTE'), and the working copy.  This allows understanding the
conflicts in context: by seeing the entire content of the file from
MERGE_HEAD, say, we can see the full intent of the code we are pulling
in and understand what they were trying to do that conflicted with our
own changes.

Sometimes, though, the exact content of these three competing versions
of a file is not so important.  Especially if the mergetool supports
folding unchanged lines, the new 'mergetool.hideResolved' feature can
be helpful for allowing a person resolving a merge to focus on the
portion with conflicts.  For sections of the file where BASE matched
LOCAL or REMOTE, this feature makes all three versions match the
resolved version, so that the user resolving can focus exclusively on
the portions with conflicts.  In other words, hideResolved makes a
multi-pane merge tool show a similar amount of information to the file
with conflict markers with conflictstyle=diff3, saving the operator
from having to pay attention to parts that resolved cleanly.

98ea309b3f (mergetool: add hideResolved configuration, 2021-02-09)
which introduced this setting enabled it by default, explaining:

    No adverse effects were noted in a small survey of popular mergetools[1]
    so this behavior defaults to `true`. However it can be globally disabled
    by setting `mergetool.hideResolved` to `false`.

In practice, however, this has proved confusing for users.  No
indication is shown in the UI that the base, local, and remote
versions shown have been modified by additional resolution.
Especially in cases where conflicts involve elements beyond textual
conflict, it has resulted in incorrect resolutions and wasted work to
figure out what happened.  Flip the default back to the traditional
behavior of `false`: although the old behavior involves slightly
slower merges in the only-textual-conflicts case, it prevents this
kind of painful moment of betrayal by one's tools, which is more
important.

Should we want to migrate to hideResolved=true in the future, we still
can.  It just requires a more careful migration, including a period
where "git mergetool" shows a warning or errors out in affected cases.

Reported-by: Dana Dahlstrom <dahlstrom@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi,

Seth House wrote:

> No adverse effects were noted in a small survey of popular mergetools[1]
> so this behavior defaults to `true`. However it can be globally disabled
> by setting `mergetool.hideResolved` to `false`.

Thanks much for protecting this by a flag.  We tried this out
internally at Google when it hit "next" and not too long later
realized that the new default of "true" is not workable for us.  I
don't believe it's the right default for Git, either, hence this
patch.

Thanks for working on the merge resolution workflow; it's much
appreciated.

Sincerely,
Jonathan

 Documentation/config/mergetool.txt | 2 +-
 git-mergetool.sh                   | 9 ++-------
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 90f76f5b9b..cafbbef46a 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -53,7 +53,7 @@ mergetool.hideResolved::
 	resolution. This flag causes 'LOCAL' and 'REMOTE' to be overwriten so
 	that only the unresolved conflicts are presented to the merge tool. Can
 	be configured per-tool via the `mergetool.<tool>.hideResolved`
-	configuration variable. Defaults to `true`.
+	configuration variable. Defaults to `false`.
 
 mergetool.keepBackup::
 	After performing a merge, the original file with conflict markers
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 911470a5b2..f751d9cfe2 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -358,13 +358,8 @@ merge_file () {
 		    enabled=false
 		fi
 	else
-		# The user does not have a preference. Ask the tool.
-		if hide_resolved_enabled
-		then
-		    enabled=true
-		else
-		    enabled=false
-		fi
+		# The user does not have a preference. Default to disabled.
+		enabled=false
 	fi
 
 	if test "$enabled" = true
-- 
2.31.0.rc1.246.gcd05c9c855


^ permalink raw reply related	[flat|nested] 80+ messages in thread

* Re: [PATCH] mergetool: do not enable hideResolved by default
  2021-03-09  2:29               ` [PATCH] mergetool: do not enable hideResolved by default Jonathan Nieder
@ 2021-03-09  5:42                 ` Seth House
  2021-03-10  1:23                   ` Jonathan Nieder
  2021-03-10  8:06                 ` Junio C Hamano
  1 sibling, 1 reply; 80+ messages in thread
From: Seth House @ 2021-03-09  5:42 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Junio C Hamano, git, Felipe Contreras, Dana Dahlstrom

On Mon, Mar 08, 2021 at 06:29:35PM -0800, Jonathan Nieder wrote:
> A typical mergetool uses four panes, showing the content of the file
> being resolved from MERGE_BASE ('BASE'), HEAD ('LOCAL'), MERGE_HEAD
> ('REMOTE'), and the working copy.  This allows understanding the
> conflicts in context: by seeing the entire content of the file from
> MERGE_HEAD, say, we can see the full intent of the code we are pulling
> in and understand what they were trying to do that conflicted with our
> own changes.

Well said. Agreed on all counts.

The very early days of these patch sets touched on this exact discussion
point. (I'd link to it but that early discussion was a tad...unfocused.)
I make semi-frequent reference of those versions of the conflicted file
in the way you describe and have disabled hideResolved for a merge tool
I maintain for that reason.

>     No adverse effects were noted in a small survey of popular mergetools[1]
>     so this behavior defaults to `true`. However it can be globally disabled
>     by setting `mergetool.hideResolved` to `false`.
> 
> In practice, however, this has proved confusing for users.  No
> indication is shown in the UI that the base, local, and remote
> versions shown have been modified by additional resolution.

Compelling point. This flag drastically changes what LOCAL and REMOTE
represent with little to no explanation.

There are three options to achieve the same end-goal of hideResolved
that I've thought of:

1.  Individual merge tools should do this work, not Git.

    A merge tool already has all the information needed to hide
    already-resolved conflicts since that is what MERGED represents.
    Conflict markers *are* a two-way diff and a merge tool should
    display them as such, rather than display the textual markers
    verbatim.

    In many ways this is the ideal approach -- all merge tools could be
    doing this with existing Git right now but none have seemingly
    thought of doing so yet.

2.  Git could pass six versions of the conflicted file to a merge tool,
    rather than the current four.

    Merge tools could accept LOCAL, REMOTE, BASE, MERGED (as most
    currently do), and also LCONFL and RCONFL files. The latter two
    being copies of MERGED but "pre split" by Git into the left
    conflicts and the right conflicts.

    This would spare the merge tool the work of splitting MERGED. It may
    encourage them to continue displaying LOCAL and REMOTE as useful
    context but also make it easy to diff LCONFL with RCONFL and use
    that diff to actually resolve the conflict. It could also make
    things worse, as many tools simply diff _every_ file Git gives them
    regardless if that makes sense or not (>_<).

3.  Git could overwrite LOCAL and REMOTE to display only unresolved
    conflicts.

    (The current hideResolved addition.) This has the pragmatic benefit
    of requiring the least amount of change for all merge tools, but to
    your point above, *destroys* valuable data -- the additional context
    to help understand where the conflicts came from -- and that data
    can't be viewd without running additional Git commands to fetch it.

Defaulting hideResolved to off is a fine change IMO. We don't have a way
to communicate to the end-user that LOCAL and REMOTE represent something
markedly different than what they have traditionally represented, so
having this be an opt-in will force the user to read the docs and
understand the ramifications.

I really appreciate your thoughts that accompanied this patch. Sorry for
the long response but your email made me want to ask the question:

Does the need to default hideResolved to off mean that it is the wrong
approach?

Thinking through an end-user's workflow: would a user want to configure
two copies of the same merge tool -- one with hideResolved and one
without? An easy conflict could benefit from the former but if it's
a tricky conflict the user would have to exit the tool and reopen the
same tool without the flag. That sounds like an annoying workflow, and
although the user would now have that extra, valuable context it would
also put them squarely back into the current state of viewing
already-resolved conflicts.

I know the Option 3, hideResolved, is merged and has that momentum and
this patch looks good to me -- but perhaps Option 2 is more "correct",
or Option 1, or yet another option I haven't thought of. Thoughts?


^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH] mergetool: do not enable hideResolved by default
  2021-03-09  5:42                 ` Seth House
@ 2021-03-10  1:23                   ` Jonathan Nieder
  0 siblings, 0 replies; 80+ messages in thread
From: Jonathan Nieder @ 2021-03-10  1:23 UTC (permalink / raw)
  To: Seth House; +Cc: Junio C Hamano, git, Felipe Contreras, Dana Dahlstrom

Hi,

Seth House wrote:

> The very early days of these patch sets touched on this exact discussion
> point. (I'd link to it but that early discussion was a tad...unfocused.)
> I make semi-frequent reference of those versions of the conflicted file
> in the way you describe and have disabled hideResolved for a merge tool
> I maintain for that reason.

Thanks.  Do you have a public example of a merge that was produced in
such a way?  It might help focus the discussion.

For concreteness' sake: in the repository that Dana mentioned, one can
see some merges from before hideResolved at
https://android.googlesource.com/platform/tools/idea/+log/mirror-goog-studio-master-dev/build.txt.

The xml files there (I'm not sure these are the right ones for me to
focus on, just commenting as I observe) remind me of other routine
conflicts with xml I've had to resolve in the past, e.g. at
https://git.eclipse.org/r/c/jgit/jgit/+/134451/3.  Having information
from each side of the merge and not a mixture can be very helpful in
this kind of case.  That's especially true when the three-way merge
algorithm didn't end up lining up the files correctly, which has
happened from time to time to me in files with repetitive structure.

[...]
> There are three options to achieve the same end-goal of hideResolved
> that I've thought of:
>
> 1.  Individual merge tools should do this work, not Git.
>
>     A merge tool already has all the information needed to hide
>     already-resolved conflicts since that is what MERGED represents.
>     Conflict markers *are* a two-way diff and a merge tool should
>     display them as such, rather than display the textual markers
>     verbatim.
>
>     In many ways this is the ideal approach -- all merge tools could be
>     doing this with existing Git right now but none have seemingly
>     thought of doing so yet.

One obstacle to this is that a merge tool can't count on the file in
the worktree containing pristine conflict markers, because the user
may have already started to work on the merge resolution.

> 2.  Git could pass six versions of the conflicted file to a merge tool,
>     rather than the current four.
>
>     Merge tools could accept LOCAL, REMOTE, BASE, MERGED (as most
>     currently do), and also LCONFL and RCONFL files. The latter two
>     being copies of MERGED but "pre split" by Git into the left
>     conflicts and the right conflicts.
>
>     This would spare the merge tool the work of splitting MERGED. It may
>     encourage them to continue displaying LOCAL and REMOTE as useful
>     context but also make it easy to diff LCONFL with RCONFL and use
>     that diff to actually resolve the conflict. It could also make
>     things worse, as many tools simply diff _every_ file Git gives them
>     regardless if that makes sense or not (>_<).

Interesting!  I kind of like this, especially if it were something the
tool could opt in to.  That said, I'm not the best person to ask, since
I never ended up finding a good workflow using mergetool for my own use;
instead, I tend to do the work of a merge tool "by hand":

- gradually resolving the merge in each diff3-style conflict hunk by
  removing common lines from base+local and base+remote until there is
  nothing left in base

- in harder cases, making the worktree match the local version,
  putting the diff from base to remote in a temporary file, and then
  hunk by hunk applying it

- in even harder cases, using git-imerge
  <https://github.com/mhagger/git-imerge>

[...]
> 3.  Git could overwrite LOCAL and REMOTE to display only unresolved
>     conflicts.
>
>     (The current hideResolved addition.) This has the pragmatic benefit
>     of requiring the least amount of change for all merge tools,

That's a good argument for having the option available, *as long as
the user explicitly turns it on*.

[...]
> Does the need to default hideResolved to off mean that it is the wrong
> approach?

One disadvantage relative to (1) is that the mergetool has no way to
visually distinguish the automatically resolved portion.  For that
reason, I suspect this will never be something we can make the
default.  But in principle I'm not against it existing.

The implementation is concise and maintainable.  The documentation
adds a little user-facing complexity; I think as long as we're able
to keep it clear and well maintained, that should be okay.

git-mergetool.txt probably ought to mention the hideResolved setting.
Otherwise, users can have a confusing experience if they set the
config once and forget about it later.

[...]
> Thinking through an end-user's workflow: would a user want to configure
> two copies of the same merge tool -- one with hideResolved and one
> without? An easy conflict could benefit from the former but if it's
> a tricky conflict the user would have to exit the tool and reopen the
> same tool without the flag. That sounds like an annoying workflow, and
> although the user would now have that extra, valuable context it would
> also put them squarely back into the current state of viewing
> already-resolved conflicts.
>
> I know the Option 3, hideResolved, is merged and has that momentum and
> this patch looks good to me -- but perhaps Option 2 is more "correct",
> or Option 1, or yet another option I haven't thought of. Thoughts?

I suspect option 1 is indeed more correct.  Dana mentions that some
mergetools (p4merge?) use different colors to highlight the
'automatically resolved' portions, something that isn't possible using
option 3.

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH] mergetool: do not enable hideResolved by default
  2021-03-09  2:29               ` [PATCH] mergetool: do not enable hideResolved by default Jonathan Nieder
  2021-03-09  5:42                 ` Seth House
@ 2021-03-10  8:06                 ` Junio C Hamano
  2021-03-11  1:57                   ` Junio C Hamano
  1 sibling, 1 reply; 80+ messages in thread
From: Junio C Hamano @ 2021-03-10  8:06 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom

Jonathan Nieder <jrnieder@gmail.com> writes:

> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 911470a5b2..f751d9cfe2 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -358,13 +358,8 @@ merge_file () {
>  		    enabled=false
>  		fi
>  	else
> -		# The user does not have a preference. Ask the tool.
> -		if hide_resolved_enabled
> -		then
> -		    enabled=true
> -		else
> -		    enabled=false
> -		fi
> +		# The user does not have a preference. Default to disabled.
> +		enabled=false

OK.  So the logic used to be

 - If the user has preference for a specific backend, use it;

 - If the user says the feature is unwanted, that is final;

 - If the user says it generally is OK to use the feature, let each
   backend set the preference;

 - If there is no preference, let each backend set the preference.

As we want to disable the feature for any backend when the user does
not explicitly say the feature is wanted (either in general, or for
a specific backend), the change in the above hunk is exactly want we
want to see.

Looking good.  Let's not revert the series and disable by default.

Should I expect an updated log message, though?  What was in the
proposed log message sounded more unsubstantiated complaint than
giving readable reasons why the feature is unwanted, but both the
response by Seth and your response to Seth's response had material
that made it more convincing why we would want to disable this by
default, e.g. "with little to no explanation", "We don't have a way
to communicate to the end-user" (both by Seth), "when ... didn't end
up lining up the files correctly", "no way to visually distinguish"
(yours) are all good ingredients to explain why this feature is
prone to subtly and silently give wrong information to the
end-users.

Thanks.

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH] mergetool: do not enable hideResolved by default
  2021-03-10  8:06                 ` Junio C Hamano
@ 2021-03-11  1:57                   ` Junio C Hamano
  2021-03-12 23:12                     ` Junio C Hamano
  0 siblings, 1 reply; 80+ messages in thread
From: Junio C Hamano @ 2021-03-11  1:57 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom

Junio C Hamano <gitster@pobox.com> writes:

> As we want to disable the feature for any backend when the user does
> not explicitly say the feature is wanted (either in general, or for
> a specific backend), the change in the above hunk is exactly want we
> want to see.
>
> Looking good.  Let's not revert the series and disable by default.
>
> Should I expect an updated log message, though?  What was in the
> proposed log message sounded more unsubstantiated complaint than
> giving readable reasons why the feature is unwanted, but both the
> response by Seth and your response to Seth's response had material
> that made it more convincing why we would want to disable this by
> default, e.g. "with little to no explanation", "We don't have a way
> to communicate to the end-user" (both by Seth), "when ... didn't end
> up lining up the files correctly", "no way to visually distinguish"
> (yours) are all good ingredients to explain why this feature is
> prone to subtly and silently give wrong information to the
> end-users.

For tonight's pushout, I'll use the patch as-is and merge it in
'seen'.

Thanks.

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH] mergetool: do not enable hideResolved by default
  2021-03-11  1:57                   ` Junio C Hamano
@ 2021-03-12 23:12                     ` Junio C Hamano
  2021-03-12 23:29                       ` Jonathan Nieder
  0 siblings, 1 reply; 80+ messages in thread
From: Junio C Hamano @ 2021-03-12 23:12 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> As we want to disable the feature for any backend when the user does
>> not explicitly say the feature is wanted (either in general, or for
>> a specific backend), the change in the above hunk is exactly want we
>> want to see.
>>
>> Looking good.  Let's not revert the series and disable by default.
>>
>> Should I expect an updated log message, though?  What was in the
>> proposed log message sounded more unsubstantiated complaint than
>> giving readable reasons why the feature is unwanted, but both the
>> response by Seth and your response to Seth's response had material
>> that made it more convincing why we would want to disable this by
>> default, e.g. "with little to no explanation", "We don't have a way
>> to communicate to the end-user" (both by Seth), "when ... didn't end
>> up lining up the files correctly", "no way to visually distinguish"
>> (yours) are all good ingredients to explain why this feature is
>> prone to subtly and silently give wrong information to the
>> end-users.
>
> For tonight's pushout, I'll use the patch as-is and merge it in
> 'seen'.

Any progress here?

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH] mergetool: do not enable hideResolved by default
  2021-03-12 23:12                     ` Junio C Hamano
@ 2021-03-12 23:29                       ` Jonathan Nieder
  2021-03-12 23:36                         ` Junio C Hamano
  0 siblings, 1 reply; 80+ messages in thread
From: Jonathan Nieder @ 2021-03-12 23:29 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom

Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> > Junio C Hamano <gitster@pobox.com> writes:

>>> As we want to disable the feature for any backend when the user does
>>> not explicitly say the feature is wanted (either in general, or for
>>> a specific backend), the change in the above hunk is exactly want we
>>> want to see.
>>>
>>> Looking good.  Let's not revert the series and disable by default.
>>>
>>> Should I expect an updated log message, though?
[...]
>> For tonight's pushout, I'll use the patch as-is and merge it in
>> 'seen'.
>
> Any progress here?

Sorry for the delay.  I should be able to send out an improved log
message (more concise and summarizing the supporting info from this
thread) later this afternoon.

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH] mergetool: do not enable hideResolved by default
  2021-03-12 23:29                       ` Jonathan Nieder
@ 2021-03-12 23:36                         ` Junio C Hamano
  2021-03-13  8:36                           ` [PATCH v2 0/2] " Jonathan Nieder
  0 siblings, 1 reply; 80+ messages in thread
From: Junio C Hamano @ 2021-03-12 23:36 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom

Jonathan Nieder <jrnieder@gmail.com> writes:

>> Any progress here?
>
> Sorry for the delay.  I should be able to send out an improved log
> message (more concise and summarizing the supporting info from this
> thread) later this afternoon.

Thanks.  I think this is the last known regression in the -rc, and
an update before the final happens on coming Monday is very much
appreciated.

^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH v2 0/2] mergetool: do not enable hideResolved by default
  2021-03-12 23:36                         ` Junio C Hamano
@ 2021-03-13  8:36                           ` Jonathan Nieder
  2021-03-13  8:38                             ` [PATCH 1/2] " Jonathan Nieder
  2021-03-13  8:41                             ` [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1) Jonathan Nieder
  0 siblings, 2 replies; 80+ messages in thread
From: Jonathan Nieder @ 2021-03-13  8:36 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>>                       I should be able to send out an improved log
>> message (more concise and summarizing the supporting info from this
>> thread) later this afternoon.
>
> Thanks.  I think this is the last known regression in the -rc, and
> an update before the final happens on coming Monday is very much
> appreciated.

A little late, but here it is.  Thoughts of all kinds welcome, as
always.

Jonathan Nieder (2):
  mergetool: do not enable hideResolved by default
  doc: describe mergetool configuration in git-mergetool page

 Documentation/config/mergetool.txt | 2 +-
 Documentation/git-mergetool.txt    | 4 ++++
 git-mergetool.sh                   | 9 ++-------
 3 files changed, 7 insertions(+), 8 deletions(-)

^ permalink raw reply	[flat|nested] 80+ messages in thread

* [PATCH 1/2] mergetool: do not enable hideResolved by default
  2021-03-13  8:36                           ` [PATCH v2 0/2] " Jonathan Nieder
@ 2021-03-13  8:38                             ` Jonathan Nieder
  2021-03-13  8:41                             ` [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1) Jonathan Nieder
  1 sibling, 0 replies; 80+ messages in thread
From: Jonathan Nieder @ 2021-03-13  8:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom

When 98ea309b3f (mergetool: add hideResolved configuration,
2021-02-09) introduced the mergetool.hideResolved setting to reduce
the clutter in viewing non-conflicted sections of files in a
mergetool, it enabled it by default, explaining:

    No adverse effects were noted in a small survey of popular mergetools[1]
    so this behavior defaults to `true`.

In practice, alas, adverse effects do appear.  A few issues:

1. No indication is shown in the UI that the base, local, and remote
   versions shown have been modified by additional resolution.  This
   is inherent in the design: the idea of mergetool.hideResolved is to
   convince a mergetool that expects pristine local, base, and remote
   files to show partially resolved verisons of those files instead;
   there is no additional source of information accessible to the
   mergetool to see where the resolution has happened.

   (By contrast, a mergetool generating the partial resolution from
   conflict markers for itself would be able to hilight the resolved
   sections with a different color.)

   A user accustomed to seeing the files without partial resolution
   gets no indication that this behavior has changed when they upgrade
   Git.

2. If the computed merge did not line up the files correctly (for
   example due to repeated sections in the file), the partially
   resolved files can be misleading and do not have enough information
   to reconstruct what happened and compute the correct merge result.

3. Resolving a conflict can involve information beyond the textual
   conflict.  For example, if the local and remote versions added
   overlapping functionality in different ways, seeing the full
   unresolved versions of each alongside the base gives information
   about each side's intent that makes it possible to come up with a
   resolution that combines those two intents.  By contrast, when
   starting with partially resolved versions of those files, one can
   produce a subtly wrong resolution that includes redundant extra
   code added by one side that is not needed in the approach taken
   on the other.

All that said, a user wanting to focus on textual conflicts with
reduced clutter can still benefit from mergetool.hideResolved=true as
a way to deemphasize sections of the code that resolve cleanly without
requiring any changes to the invoked mergetool.  The caveats described
above are reduced when the user has explicitly turned this on, because
then the user is aware of them.

Flip the default to 'false'.

Reported-by: Dana Dahlstrom <dahlstrom@google.com>
Helped-by: Seth House <seth@eseth.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Only difference from v1 is the commit message.

 Documentation/config/mergetool.txt | 2 +-
 git-mergetool.sh                   | 9 ++-------
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/Documentation/config/mergetool.txt b/Documentation/config/mergetool.txt
index 90f76f5b9ba..cafbbef46ae 100644
--- a/Documentation/config/mergetool.txt
+++ b/Documentation/config/mergetool.txt
@@ -53,7 +53,7 @@ mergetool.hideResolved::
 	resolution. This flag causes 'LOCAL' and 'REMOTE' to be overwriten so
 	that only the unresolved conflicts are presented to the merge tool. Can
 	be configured per-tool via the `mergetool.<tool>.hideResolved`
-	configuration variable. Defaults to `true`.
+	configuration variable. Defaults to `false`.
 
 mergetool.keepBackup::
 	After performing a merge, the original file with conflict markers
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 911470a5b2c..f751d9cfe20 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -358,13 +358,8 @@ merge_file () {
 		    enabled=false
 		fi
 	else
-		# The user does not have a preference. Ask the tool.
-		if hide_resolved_enabled
-		then
-		    enabled=true
-		else
-		    enabled=false
-		fi
+		# The user does not have a preference. Default to disabled.
+		enabled=false
 	fi
 
 	if test "$enabled" = true
-- 
2.31.0.rc2.261.g7f71774620


^ permalink raw reply related	[flat|nested] 80+ messages in thread

* [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1)
  2021-03-13  8:36                           ` [PATCH v2 0/2] " Jonathan Nieder
  2021-03-13  8:38                             ` [PATCH 1/2] " Jonathan Nieder
@ 2021-03-13  8:41                             ` Jonathan Nieder
  2021-03-13 23:34                               ` Junio C Hamano
  2021-03-13 23:37                               ` Junio C Hamano
  1 sibling, 2 replies; 80+ messages in thread
From: Jonathan Nieder @ 2021-03-13  8:41 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom

In particular, this describes mergetool.hideResolved, which can help
users discover this setting (either because it may be useful to them
or in order to understand mergetool's behavior if they have forgotten
setting it in the past).

Tested by running

	make -C Documentation git-mergetool.1
	man Documentation/git-mergetool.1

and reading through the page.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/git-mergetool.txt | 4 ++++
 1 file changed, 4 insertions(+)

Thanks for reading.

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 6b14702e784..e587c7763a7 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -99,6 +99,10 @@ success of the resolution after the custom tool has exited.
 	(see linkgit:git-config[1]).  To cancel `diff.orderFile`,
 	use `-O/dev/null`.
 
+CONFIGURATION
+-------------
+include::config/mergetool.txt[]
+
 TEMPORARY FILES
 ---------------
 `git mergetool` creates `*.orig` backup files while resolving merges.
-- 
2.31.0.rc2.261.g7f71774620


^ permalink raw reply related	[flat|nested] 80+ messages in thread

* Re: [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1)
  2021-03-13  8:41                             ` [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1) Jonathan Nieder
@ 2021-03-13 23:34                               ` Junio C Hamano
  2021-03-13 23:37                               ` Junio C Hamano
  1 sibling, 0 replies; 80+ messages in thread
From: Junio C Hamano @ 2021-03-13 23:34 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom

Jonathan Nieder <jrnieder@gmail.com> writes:
> diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
> index 6b14702e784..e587c7763a7 100644
> --- a/Documentation/git-mergetool.txt
> +++ b/Documentation/git-mergetool.txt
> @@ -99,6 +99,10 @@ success of the resolution after the custom tool has exited.
>  	(see linkgit:git-config[1]).  To cancel `diff.orderFile`,
>  	use `-O/dev/null`.
>  
> +CONFIGURATION
> +-------------
> +include::config/mergetool.txt[]
> +

It is a nice touch.  We don't have much explanation other than the
description below ...

mergetool.hideResolved::
	During a merge Git will automatically resolve as many conflicts as
	possible and write the 'MERGED' file containing conflict markers around
	any conflicts that it cannot resolve; 'LOCAL' and 'REMOTE' normally
	represent the versions of the file from before Git's conflict
	resolution. This flag causes 'LOCAL' and 'REMOTE' to be overwriten so
	that only the unresolved conflicts are presented to the merge tool. Can
	be configured per-tool via the `mergetool.<tool>.hideResolved`
	configuration variable. Defaults to `false`.

... which appears in the included file on the feature.

Thanks.

^ permalink raw reply	[flat|nested] 80+ messages in thread

* Re: [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1)
  2021-03-13  8:41                             ` [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1) Jonathan Nieder
  2021-03-13 23:34                               ` Junio C Hamano
@ 2021-03-13 23:37                               ` Junio C Hamano
  1 sibling, 0 replies; 80+ messages in thread
From: Junio C Hamano @ 2021-03-13 23:37 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git, Felipe Contreras, Seth House, Dana Dahlstrom

Jonathan Nieder <jrnieder@gmail.com> writes:

> Tested by running
>
> 	make -C Documentation git-mergetool.1
> 	man Documentation/git-mergetool.1
>
> and reading through the page.

Nice.  Also applying this step and running

	cd Documentation && ./doc-diff HEAD^ HEAD

would was a trivial way to see the change ;-)

^ permalink raw reply	[flat|nested] 80+ messages in thread

end of thread, other threads:[~2021-03-13 23:38 UTC | newest]

Thread overview: 80+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-23  4:53 [PATCH v5 0/1] mergetool: remove unconflicted lines Felipe Contreras
2020-12-23  4:53 ` [PATCH v5 1/1] mergetool: add automerge configuration Felipe Contreras
2020-12-23 13:34   ` Junio C Hamano
2020-12-23 14:23     ` Felipe Contreras
2020-12-23 20:21       ` Junio C Hamano
2020-12-24  0:14         ` Felipe Contreras
2020-12-24  0:32           ` Junio C Hamano
2020-12-24  1:36             ` Felipe Contreras
2020-12-24  6:17               ` Junio C Hamano
2020-12-24 15:59                 ` Felipe Contreras
2020-12-24 22:32                   ` Junio C Hamano
2020-12-27 18:01                     ` Felipe Contreras
2020-12-24  9:24 ` [PATCH v5 0/1] mergetool: remove unconflicted lines Junio C Hamano
2020-12-24 16:16   ` Felipe Contreras
2020-12-30  5:47   ` Johannes Schindelin
2020-12-30 22:33     ` Felipe Contreras
2020-12-27 20:58 ` [PATCH v6 0/1] mergetool: add automerge configuration Seth House
2020-12-27 20:58   ` [PATCH v6 1/2] " Seth House
2020-12-27 22:06     ` Junio C Hamano
2020-12-27 22:29       ` Seth House
2020-12-27 22:59         ` Junio C Hamano
2020-12-27 20:58   ` [PATCH v6 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House
2020-12-27 22:31     ` Junio C Hamano
2020-12-28  0:41   ` [PATCH v7 0/2] mergetool: add automerge configuration Seth House
2020-12-28  0:41     ` [PATCH v7 1/2] " Seth House
2020-12-28  0:41     ` [PATCH v7 2/2] mergetool: Add per-tool support for the autoMerge flag Seth House
2020-12-28  1:18       ` Felipe Contreras
2020-12-28  4:54     ` [PATCH v8 0/4] mergetool: add automerge configuration Seth House
2020-12-28  4:54       ` [PATCH v8 1/4] " Seth House
2020-12-28 11:30         ` Johannes Sixt
2020-12-28  4:54       ` [PATCH v8 2/4] mergetool: Add per-tool support for the autoMerge flag Seth House
2020-12-28 13:09         ` Junio C Hamano
2020-12-28  4:54       ` [PATCH v8 3/4] mergetool: Break setup_tool out into separate initialization function Seth House
2020-12-28 11:48         ` Johannes Sixt
2020-12-28  4:54       ` [PATCH v8 4/4] mergetool: Add automerge_enabled tool-specific override function Seth House
2020-12-28 11:57         ` Johannes Sixt
2020-12-28 13:19         ` Junio C Hamano
2020-12-28 19:29       ` [PATCH v9 0/5] mergetool: add automerge configuration Seth House
2020-12-28 19:29         ` [PATCH v9 1/5] " Seth House
2020-12-28 19:29         ` [PATCH v9 2/5] mergetool: alphabetize the mergetool config docs Seth House
2020-12-28 19:29         ` [PATCH v9 3/5] mergetool: add per-tool support for the autoMerge flag Seth House
2020-12-28 19:29         ` [PATCH v9 4/5] mergetool: break setup_tool out into separate initialization function Seth House
2020-12-29  8:50           ` Johannes Sixt
2020-12-29 17:23             ` Seth House
2020-12-28 19:29         ` [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function Seth House
2020-12-29  2:01           ` Felipe Contreras
2021-01-06  5:55           ` Junio C Hamano
2021-01-07  3:58             ` Seth House
2021-01-07  6:38               ` Junio C Hamano
2021-01-07  9:27                 ` Seth House
2021-01-07 21:26                   ` Junio C Hamano
2021-01-08 15:04                     ` Johannes Schindelin
2021-01-30  5:46         ` [PATCH v10 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
2021-01-30  5:46           ` [PATCH v10 1/3] mergetool: add hideResolved configuration Seth House
2021-01-30  8:08             ` Junio C Hamano
2021-01-30  5:46           ` [PATCH v10 2/3] mergetool: break setup_tool out into separate initialization function Seth House
2021-01-30  5:46           ` [PATCH v10 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House
2021-01-30  8:08             ` Junio C Hamano
2021-02-09 20:07           ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Seth House
2021-02-09 20:07             ` [PATCH v11 1/3] mergetool: add hideResolved configuration Seth House
2021-03-09  2:29               ` [PATCH] mergetool: do not enable hideResolved by default Jonathan Nieder
2021-03-09  5:42                 ` Seth House
2021-03-10  1:23                   ` Jonathan Nieder
2021-03-10  8:06                 ` Junio C Hamano
2021-03-11  1:57                   ` Junio C Hamano
2021-03-12 23:12                     ` Junio C Hamano
2021-03-12 23:29                       ` Jonathan Nieder
2021-03-12 23:36                         ` Junio C Hamano
2021-03-13  8:36                           ` [PATCH v2 0/2] " Jonathan Nieder
2021-03-13  8:38                             ` [PATCH 1/2] " Jonathan Nieder
2021-03-13  8:41                             ` [PATCH 2/2] doc: describe mergetool configuration in git-mergetool(1) Jonathan Nieder
2021-03-13 23:34                               ` Junio C Hamano
2021-03-13 23:37                               ` Junio C Hamano
2021-02-09 20:07             ` [PATCH v11 2/3] mergetool: break setup_tool out into separate initialization function Seth House
2021-02-09 20:07             ` [PATCH v11 3/3] mergetool: add per-tool support and overrides for the hideResolved flag Seth House
2021-02-09 22:11             ` [PATCH v11 0/3] mergetool: add hideResolved configuration (was automerge) Junio C Hamano
2021-02-09 23:27               ` Seth House
2020-12-28 10:29     ` [PATCH v7 0/2] mergetool: add automerge configuration Junio C Hamano
2020-12-28 14:40       ` Felipe Contreras
2020-12-28  1:02   ` [PATCH v6 0/1] " Felipe Contreras

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.