Git Mailing List Archive on lore.kernel.org
 help / color / 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; 55+ 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] 55+ 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; 55+ 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	[flat|nested] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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	[flat|nested] 55+ 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; 55+ 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	[flat|nested] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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	[flat|nested] 55+ 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; 55+ 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	[flat|nested] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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	[flat|nested] 55+ 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; 55+ 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	[flat|nested] 55+ 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; 55+ 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	[flat|nested] 55+ 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; 55+ 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	[flat|nested] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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
                           ` (4 more replies)
  4 siblings, 5 replies; 55+ 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] 55+ 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
                           ` (3 subsequent siblings)
  4 siblings, 0 replies; 55+ 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	[flat|nested] 55+ 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
                           ` (2 subsequent siblings)
  4 siblings, 0 replies; 55+ 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	[flat|nested] 55+ 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
  2020-12-28 19:29         ` [PATCH v9 5/5] mergetool: add automerge_enabled tool-specific override function Seth House
  4 siblings, 0 replies; 55+ 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	[flat|nested] 55+ 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
  4 siblings, 1 reply; 55+ 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	[flat|nested] 55+ 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
  4 siblings, 2 replies; 55+ 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	[flat|nested] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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	[flat|nested] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ 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; 55+ 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] 55+ messages in thread

end of thread, back to index

Thread overview: 55+ 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
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

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git