[RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Charles Bailey]

Currently git mergetool is restricted to a set of commands defined
in the script. You can subvert the mergetool.<tool>.path to force
git mergetool to use a different command, but if you have a command
whose invocation syntax does not match one of the current tools then
you would have to write a wrapper script for it.

This patch adds three git config variable patterns which allow a more
flexible choice of merge tool.

If you run git mergetool with -t/--tool or the merge.tool config
variable set to an unrecognized tool then git mergetool will query the
mergetool.<tool>.cmd config variable. If this variable exists, then
git mergetool will treat the specified tool as a custom command and
will use a shell eval to run the command so that the appropriate shell
variables can be used to find the merge temporary files.

mergetool.<tool>.trustExitCode can be used to indicate that the exit
code of the custom command can be used to determine the success of the
merge. mergetool.<tool>.keepBackup can be used to specify whether the
original pre-merge file with conflict markers should be kept as a
'.orig' file after the merge tool completes.
---

This is a preliminary patch which aims to make it easier to use
a.n.other 3-way merge tool with git-mergetool without either hacking
the source or writing a wrapper script for the tool.

It follows filter-branch's 'eval a user shell snippet' philosophy to
provide the flexibility and here in lies an ugliness. It exposes
git-mergetool.sh's private variables to the user script. The variables
are BASE, REMOTE, LOCAL and path.

My feeling is that we should give this consistent and documented
names, perhaps GIT_BASE, GIT_REMOTE, GIT_LOCAL, GIT_MERGED or similar.

Also, does anyone know of any reason why the temporary files should
not be cleaned up after an unsuccessful merge? As it is, every time
you abort a mergetool run you end up with another set of .$$.BASE
.$$.REMOTE and .$$.LOCAL files.

A further enhancement that I have thought about is providing a config
option like mergetool.<tool>.cmdNoBase for the case where there is no
base file, as in many cases it might be easier for the user to provide
a second command than to have shell conditionals in their config.

Just for reference I've tried this patch in the windows world with
things like:

git config --global mergetool.tortoise.cmd 'TortoiseMerge.exe \
/base:"$BASE" /theirs:"$REMOTE" /mine:"$LOCAL" /merged:"$path"'

git config --global mergetool.p4.cmd 'p4merge.exe "$BASE" "$REMOTE" \
"$LOCAL" "$path"'

git config --global mergetool.p4win.cmd 'P4WinMrg "$BASE" "$REMOTE" \
"$LOCAL" "$path"'

 Documentation/config.txt |   30 ++++++++++++++++++++++++++++--
 git-mergetool.sh         |   32 ++++++++++++++++++++++++++++++--
 2 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f9bdb16..e26c4b2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -689,8 +689,10 @@ merge.summary::
 
 merge.tool::
 	Controls which merge resolution program is used by
-	linkgit:git-mergetool[1].  Valid values are: "kdiff3", "tkdiff",
-	"meld", "xxdiff", "emerge", "vimdiff", "gvimdiff", and "opendiff".
+	linkgit:git-mergetool[1].  Valid built-in values are: "kdiff3",
+	"tkdiff", "meld", "xxdiff", "emerge", "vimdiff", "gvimdiff", and
+	"opendiff".  Any other value is treated is custom merge tool
+	and there must be a corresponing mergetool.<tool>.cmd option.
 
 merge.verbosity::
 	Controls the amount of output shown by the recursive merge
@@ -717,6 +719,30 @@ 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
+	variables available: 'BASE' is the name of a temporary file
+	containing the common base of the files to be merged, if available;
+	'LOCAL' is the name of a temporary file containing the contents of
+	the file on the current branch; 'REMOTE' is the name of a temporary
+	file containing the contents of the file from the branch being
+	merged; 'path' contains the name of the file to which the merge
+	tool should write the results of a successful merge.
+
+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
+	successful.  If this is not set to true then the merge target file
+	timestamp is checked and the merge assumed to have been successful
+	if the file has been updated, otherwise the user is prompted to
+	indicate the success of the merge.
+
+mergetool.<tool>.keepBackup::
+	After performing a merge, the original file with conflict markers
+	can be saved as a file with a `.orig` extension.  If this variable
+	is set to `false` then this file is not preserved.
+
 pack.window::
 	The size of the window used by linkgit:git-pack-objects[1] when no
 	window size is given on the command line. Defaults to 10.
diff --git a/git-mergetool.sh b/git-mergetool.sh
index cbbb707..78c73ca 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -271,6 +271,21 @@ merge_file () {
 	    status=$?
 	    save_backup
 	    ;;
+	*)
+	    if test -n "$merge_tool_cmd"; then
+		touch "$BACKUP"
+		eval "$merge_tool_cmd"
+		status=$?
+		if test "$merge_tool_trust_exit_code" = "false"; then
+		    check_unchanged
+		fi
+		if test "$merge_tool_keep_backup" = "true"; then
+		    save_backup
+		else
+		    remove_backup
+		fi
+	    fi
+	    ;;
     esac
     if test "$status" -ne 0; then
 	echo "merge of $path failed" 1>&2
@@ -309,12 +324,20 @@ do
     shift
 done
 
+valid_custom_tool()
+{
+    merge_tool_cmd="$(git config mergetool.$1.cmd)"
+    test -n "$merge_tool_cmd"
+}
+
 valid_tool() {
 	case "$1" in
 		kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | ecmerge)
 			;; # happy
 		*)
-			return 1
+			if ! valid_custom_tool "$1"; then
+				return 1
+			fi
 			;;
 	esac
 }
@@ -380,10 +403,15 @@ else
 
     init_merge_tool_path "$merge_tool"
 
-    if ! type "$merge_tool_path" > /dev/null 2>&1; then
+    if test -z "$merge_tool_cmd" && ! type "$merge_tool_path" > /dev/null 2>&1; then
         echo "The merge tool $merge_tool is not available as '$merge_tool_path'"
         exit 1
     fi
+
+    if ! test -z "$merge_tool_cmd"; then
+        merge_tool_trust_exit_code="$(git config --bool merge.$merge_tool.trustExitCode || echo false)"
+        merge_tool_keep_backup="$(git config --bool merge.$merge_tool.keepBackup || echo true)"
+    fi
 fi
 
 
-- 
1.5.4.1.144.gb4758f

Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Junio C Hamano]

Charles Bailey <charles@hashpling.org> writes:

> It follows filter-branch's 'eval a user shell snippet' philosophy to
> provide the flexibility and here in lies an ugliness. It exposes
> git-mergetool.sh's private variables to the user script. The variables
> are BASE, REMOTE, LOCAL and path.
>
> My feeling is that we should give this consistent and documented
> names, perhaps GIT_BASE, GIT_REMOTE, GIT_LOCAL, GIT_MERGED or similar.

I like the general idea and you are right that the external
interface should not expose the variable names the current
implementation happens to use.  I do not think it is necessary
to rename them to GIT_BASE etc., but I do think we need to list
the repertoire of variables that can be expected to be usable by
custom script in any future version of mergetool (even after it
is rewritten in C).  Anybody who uses a variable that is not in
the documented set that the current implementation uses can be
broken ;-).

Also perhaps we would want to spawn the eval in a subprocess to
make it clear that the custom script cannot affect the caller's
variables.

> Also, does anyone know of any reason why the temporary files should
> not be cleaned up after an unsuccessful merge?

I do not use mergetool myself, but presumably it would be to
make the manual inspection easier.

> +mergetool.<tool>.keepBackup::
> +	After performing a merge, the original file with conflict markers
> +	can be saved as a file with a `.orig` extension.  If this variable
> +	is set to `false` then this file is not preserved.

I doubt this belongs to this patch, so does ...

> +		if test "$merge_tool_keep_backup" = "true"; then
> +		    save_backup
> +		else
> +		    remove_backup
> +		fi
> +	    fi

... this part and anything else that deals with the backup
files in the patch.

Shouldn't the handling of back-up files be the same across
backends?

If the answer is yes, it makes mergetool.<tool>.keepBackup
configuration a quite bogus variable, as it is not something you
would configure per backend.

In the existing code, I see kdiff3 arm calls remove_backup while
tkdiff arm and others call save_backup, which seems quite
inconsistent.  Perhaps mergetool needs a command line option
(and perhaps a single configuration variable independent from
which backend is used) to tell what to do about them after a
conflicting merge is resolved and/or resolution is aborted.

Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Charles Bailey]

On Sat, Feb 16, 2008 at 12:04:26PM -0800, Junio C Hamano wrote:
> Charles Bailey <charles@hashpling.org> writes:
> 
> > It follows filter-branch's 'eval a user shell snippet' philosophy to
> > provide the flexibility and here in lies an ugliness. It exposes
> > git-mergetool.sh's private variables to the user script. The variables
> > are BASE, REMOTE, LOCAL and path.
> >
> > My feeling is that we should give this consistent and documented
> > names, perhaps GIT_BASE, GIT_REMOTE, GIT_LOCAL, GIT_MERGED or similar.
> 
> I like the general idea and you are right that the external
> interface should not expose the variable names the current
> implementation happens to use.  I do not think it is necessary
> to rename them to GIT_BASE etc., but I do think we need to list
> the repertoire of variables that can be expected to be usable by
> custom script in any future version of mergetool (even after it
> is rewritten in C).  Anybody who uses a variable that is not in
> the documented set that the current implementation uses can be
> broken ;-).

OK, I'm fine with this, although I think we should use something other
than path (in lower case) for the output.  Either MERGED, RESULT or
OUTPUT, say, I've no strong preferences.

> Also perhaps we would want to spawn the eval in a subprocess to
> make it clear that the custom script cannot affect the caller's
> variables.

Sub-shell, good plan.

> > Also, does anyone know of any reason why the temporary files should
> > not be cleaned up after an unsuccessful merge?
> 
> I do not use mergetool myself, but presumably it would be to
> make the manual inspection easier.

I don't use it a lot (yet) either, but I think that this sort of
change well help git get wider acceptance in the "but does it work
with my favourite 3-way merge tool" community.

As far as it goes, though, I find that if I fire up a merge tool and
it looks complex, I quite often want to abort and look at the easy
files first.  The .BASE/.LOCAL/.REMOTE files don't contain anything
that isn't in the index anyway (the merge tool certainly should be
updating them with intermediate state).

It's a separate issue to the main thrust of this patch, though.

> > +mergetool.<tool>.keepBackup::
> 
--- snip ---

> Shouldn't the handling of back-up files be the same across
> backends?
> 
> If the answer is yes, it makes mergetool.<tool>.keepBackup
> configuration a quite bogus variable, as it is not something you
> would configure per backend.
> 
> In the existing code, I see kdiff3 arm calls remove_backup while
> tkdiff arm and others call save_backup, which seems quite
> inconsistent.  Perhaps mergetool needs a command line option
> (and perhaps a single configuration variable independent from
> which backend is used) to tell what to do about them after a
> conflicting merge is resolved and/or resolution is aborted.

Yes, I think that you're right. I couldn't work out why kdiff3 was
special, but I took the safe/wrong path of assuming that it should be
configurable per backend. I think that it might be something that
should be configurable, but the variable should probably me
mergetool.keepBackup and the patch should be separate from this one.

Charles.

Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Jakub Narebski]

Charles Bailey <charles@hashpling.org> writes:

> Currently git mergetool is restricted to a set of commands defined
> in the script. You can subvert the mergetool.<tool>.path to force
> git mergetool to use a different command, but if you have a command
> whose invocation syntax does not match one of the current tools then
> you would have to write a wrapper script for it.
> 
> This patch adds three git config variable patterns which allow a more
> flexible choice of merge tool.

[...]

> It follows filter-branch's 'eval a user shell snippet' philosophy to
> provide the flexibility and here in lies an ugliness. It exposes
> git-mergetool.sh's private variables to the user script. The variables
> are BASE, REMOTE, LOCAL and path.

Another solution would be to use StGit merger / i2merge / i3merge
format, similar to git-for-each-ref format, namely to expand
%(branch1), %(branch2), %(ancestor), %(output) (well, StGit uses
for some reason %(ancestor)s etc.; git-for-each-ref doesn't).

Although for this would be better if git-mergetool was rewritten
in C, in Perl (or even in Python).

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Steffen Prohaska]

On Feb 16, 2008, at 7:53 PM, Charles Bailey wrote:

> Currently git mergetool is restricted to a set of commands defined
> in the script. You can subvert the mergetool.<tool>.path to force
> git mergetool to use a different command, but if you have a command
> whose invocation syntax does not match one of the current tools then
> you would have to write a wrapper script for it.
>
> [...]
>
> This is a preliminary patch which aims to make it easier to use
> a.n.other 3-way merge tool with git-mergetool without either hacking
> the source or writing a wrapper script for the tool.

Why not just add the tools you have in mind to git mergetool?  If
everyone did that eventually we would have direct support for a
rather long list of tools.  This would be the easiest solution
for the end user: He could just choose the preferred tool and use
it.  The invocation of each merge tool would be coded in
mergetool.  The exact command line could be fine tuned and would
be reviewed by other git developers.

...

> git config --global mergetool.tortoise.cmd 'TortoiseMerge.exe \
> /base:"$BASE" /theirs:"$REMOTE" /mine:"$LOCAL" /merged:"$path"'
>
> git config --global mergetool.p4.cmd 'p4merge.exe "$BASE" "$REMOTE" \
> "$LOCAL" "$path"'
>
> git config --global mergetool.p4win.cmd 'P4WinMrg "$BASE" "$REMOTE" \
> "$LOCAL" "$path"'


If you just integrated these tools directly I could use them
right away instead of first reading the documentation of the
custom mechanism and then looking up how to configure the right
command line for the tool I want to use.  If the tools were
directly integrated I could for example just say "git mergetool
--tool=p4".

That does not mean I am opposed to adding a mechanism for
handling custom tools.  But easier for the end user would be
if the tools were directly integrated.

		Steffen

Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Charles Bailey]

On Sat, Feb 16, 2008 at 11:37:31PM +0100, Steffen Prohaska wrote:
> 
> On Feb 16, 2008, at 7:53 PM, Charles Bailey wrote:
> 
> >Currently git mergetool is restricted to a set of commands defined
> >in the script. You can subvert the mergetool.<tool>.path to force
> >git mergetool to use a different command, but if you have a command
> >whose invocation syntax does not match one of the current tools then
> >you would have to write a wrapper script for it.
> >
> >[...]
> >
> >This is a preliminary patch which aims to make it easier to use
> >a.n.other 3-way merge tool with git-mergetool without either hacking
> >the source or writing a wrapper script for the tool.
> 
> Why not just add the tools you have in mind to git mergetool?  If
> everyone did that eventually we would have direct support for a
> rather long list of tools.  This would be the easiest solution
> for the end user: He could just choose the preferred tool and use
> it.  The invocation of each merge tool would be coded in
> mergetool.  The exact command line could be fine tuned and would
> be reviewed by other git developers.
> 

I have to disagree with this approach. I didn't actually have these
tools in mind when I wrote the patch, I searched around for tools that
I've used in the past and tools that I know that other people use. I
had in mine the questions that I'd want to be able to answer when
people ask me about git. It's not that the list will be large, it's
that it will never be complete because there will always be tools that
aren't globally available.

When I'm asked "can I use xyz visual merge tool" with git there will
always be some xyz for which the tool isn't yet in the list. It's a
lot more difficult to sell "patch the tool, submit the patch so it
doesn't get lost in the next upgrade of git, oh and support the patch
so that it does get integrated" than it is to sell "take the command
line format, massage it and put it in your system/global config".

> If you just integrated these tools directly I could use them
> right away instead of first reading the documentation of the
> custom mechanism and then looking up how to configure the right
> command line for the tool I want to use.  If the tools were
> directly integrated I could for example just say "git mergetool
> --tool=p4".
> 
> That does not mean I am opposed to adding a mechanism for
> handling custom tools.  But easier for the end user would be
> if the tools were directly integrated.
> 
> 		Steffen

Sure, it's nice when the tool you want is already integrated but the
list is never going to be perfect. This patch was designed to support
the fallback position: OK, my tool isn't natively supported, how
easily can I get it working?

I don't believe that git installs a system config by default, but one
idea I had was to rip out all of the native tools support in git
mergetool and replace it with a list of predefined custom tools
configs. This would put all merge tools on an equal footing and should
make extra tool support patches simpler and easier to integrate. This
doesn't have any legs without a system default config, though.

Charles.

Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Johannes Schindelin]

Hi,

On Sun, 17 Feb 2008, Charles Bailey wrote:

> On Sat, Feb 16, 2008 at 11:37:31PM +0100, Steffen Prohaska wrote:
> 
> > Why not just add the tools you have in mind to git mergetool?  If 
> > everyone did that eventually we would have direct support for a rather 
> > long list of tools.  This would be the easiest solution for the end 
> > user: He could just choose the preferred tool and use it.  The 
> > invocation of each merge tool would be coded in mergetool.  The exact 
> > command line could be fine tuned and would be reviewed by other git 
> > developers.
> > 
> 
> I have to disagree with this approach.

So you'd rather have the end users do the same work for the same tool over 
and over again?

Ciao,
Dscho

Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Charles Bailey]

On Sun, Feb 17, 2008 at 12:46:15AM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 17 Feb 2008, Charles Bailey wrote:
> 
> > On Sat, Feb 16, 2008 at 11:37:31PM +0100, Steffen Prohaska wrote:
> > 
> > > Why not just add the tools you have in mind to git mergetool?  If 
> > > everyone did that eventually we would have direct support for a rather 
> > > long list of tools.  This would be the easiest solution for the end 
> > > user: He could just choose the preferred tool and use it.  The 
> > > invocation of each merge tool would be coded in mergetool.  The exact 
> > > command line could be fine tuned and would be reviewed by other git 
> > > developers.
> > > 
> > 
> > I have to disagree with this approach.
> 
> So you'd rather have the end users do the same work for the same tool over 
> and over again?
> 

I'm sorry, I should have made myself clearer. I disagree that the
approach of adding new tool support to the source code as and when they
are encountered is optimal. I believe that it is preferable to have a
solution that allows users to configure, rather then code, support for
their own tools that do not to have native support.

I do not disagree that there is benefit to having a wide range of
tools that are supported natively.

I thought I made a reasonable argument for this in the rest of my
email that you took the headline from, but evidently I came across as
muddled.

Charles.

Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Junio C Hamano]

Charles Bailey <charles@hashpling.org> writes:

> On Sun, Feb 17, 2008 at 12:46:15AM +0000, Johannes Schindelin wrote:
>> 
>> So you'd rather have the end users do the same work for the same tool over 
>> and over again?
>
> I'm sorry, I should have made myself clearer. I disagree that the
> approach of adding new tool support to the source code as and when they
> are encountered is optimal. I believe that it is preferable to have a
> solution that allows users to configure, rather then code, support for
> their own tools that do not to have native support.
>
> I do not disagree that there is benefit to having a wide range of
> tools that are supported natively.
>
> I thought I made a reasonable argument for this in the rest of my
> email that you took the headline from, but evidently I came across as
> muddled.

I do not understand why people are so upset about this.  I think
the approach Charles's patch takes is reasonable, with example
configurations to coax a few of his tools to be driven by
mergetool as backends, that demonstrate the customizing
framework works well.

It of course would also be good to throw in the native support
for the tools he used as examples but I'd say that they are
topics of separate patches.

Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Steffen Prohaska]

On Feb 17, 2008, at 2:15 AM, Junio C Hamano wrote:

> Charles Bailey <charles@hashpling.org> writes:
>
>> On Sun, Feb 17, 2008 at 12:46:15AM +0000, Johannes Schindelin wrote:
>>>
>>> So you'd rather have the end users do the same work for the same  
>>> tool over
>>> and over again?
>>
>> I'm sorry, I should have made myself clearer. I disagree that the
>> approach of adding new tool support to the source code as and when  
>> they
>> are encountered is optimal. I believe that it is preferable to have a
>> solution that allows users to configure, rather then code, support  
>> for
>> their own tools that do not to have native support.
>>
>> I do not disagree that there is benefit to having a wide range of
>> tools that are supported natively.
>>
>> I thought I made a reasonable argument for this in the rest of my
>> email that you took the headline from, but evidently I came across as
>> muddled.
>
> I do not understand why people are so upset about this.  I think
> the approach Charles's patch takes is reasonable, with example
> configurations to coax a few of his tools to be driven by
> mergetool as backends, that demonstrate the customizing
> framework works well.

I am not upset at all and I really appreciate Charles work for
adding a generic mechanism.  I was just wondering why not taking
the direct way of adding tools to git-mergetool one by one until
we eventually have a rather complete list of supported tools.
This would be the easiest solution for end users if their
preferred tool is supported.  It is also easier to add support
for a specific tool than a generic mechanism.

Maybe it is sufficient to refactor "git mergetool" to make it
really easy to add another tool.  Our users are developers who
should know how to add a new tool directly to git mergetool if
they find some guidance in the source.

I see two benefits of the direct approach

  - the source code of git mergetool could be kept simpler without
    a generic configuration mechanism for unknown tools.

  - users would be forced to integrate their tool into git mergetool
    and hopefully they would send patches and eventually we'd have
    rather complete arge number of tools supported.
    (I know at least one case where this pressure helps and I expect
     to see patches that we would not see if a generic mechanism
     was available.)


> It of course would also be good to throw in the native support
> for the tools he used as examples but I'd say that they are
> topics of separate patches.


I believe this is more important than a generic mechanism.

However, I am not opposed to a generic mechanism, ...


On Feb 17, 2008, at 1:20 AM, Charles Bailey wrote:
>>
>> I don't believe that git installs a system config by default, but one
>> idea I had was to rip out all of the native tools support in git
>> mergetool and replace it with a list of predefined custom tools
>> configs. This would put all merge tools on an equal footing and  
>> should
>> make extra tool support patches simpler and easier to integrate. This
>> doesn't have any legs without a system default config, though.

... but I am slightly opposed to this idea.  Note that at least
in one case there is a trick needed to launch the tool.  Such a
trick can easily be coded if the tool is directly added in
"git mergetool"; but it would be much harder to capture by a
generic mechanism via config variables.  The example I mean is
opendiff that needs to be piped to cat (opendiff ... | cat).
Otherwise opendiff detaches FileMerge and returns immediately
without waiting for the user to complete the merge.

		Steffen

Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Charles Bailey]

On Sun, Feb 17, 2008 at 08:59:31AM +0100, Steffen Prohaska wrote:
> On Feb 17, 2008, at 1:20 AM, Charles Bailey wrote:
> >>
> >>I don't believe that git installs a system config by default, but one
> >>idea I had was to rip out all of the native tools support in git
> >>mergetool and replace it with a list of predefined custom tools
> >>configs. This would put all merge tools on an equal footing and  
> >>should
> >>make extra tool support patches simpler and easier to integrate. This
> >>doesn't have any legs without a system default config, though.
> 
> ... but I am slightly opposed to this idea.  Note that at least
> in one case there is a trick needed to launch the tool.  Such a
> trick can easily be coded if the tool is directly added in
> "git mergetool"; but it would be much harder to capture by a
> generic mechanism via config variables.  The example I mean is
> opendiff that needs to be piped to cat (opendiff ... | cat).
> Otherwise opendiff detaches FileMerge and returns immediately
> without waiting for the user to complete the merge.

I just wanted to say that in my patch, as the configuration is a
sub-shell, that it is perfectly possible to do this in a custom
mergetool as well.

I have different reasons for not liking a default system config file,
namely the whole customize vs. updgrade conflict issues.

An alternative that I have considered is to include a
$(sharedir)/gitcore/mergetools.gitconfig which could contain the
default native mergetool configs (both the commands and the
'trustExitCode' settings.

Submitting a new merge tool patch becomes a simple matter of: "I've
used and tested this in my system (or global) gitconfig, please add to
the git distribution mergetool.gitconfig."

I know that most git developers are just as at ease (if not more so)
editing a git shell script as they are at using git config but I still
believe that there is a significant group of users and potential users
for whom there is an import barrier between configuring software and
'having' to hack it to get it to work.

Charles.

Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Theodore Tso]

On Sun, Feb 17, 2008 at 08:59:31AM +0100, Steffen Prohaska wrote:
>
> I am not upset at all and I really appreciate Charles work for
> adding a generic mechanism.  I was just wondering why not taking
> the direct way of adding tools to git-mergetool one by one until
> we eventually have a rather complete list of supported tools.
> This would be the easiest solution for end users if their
> preferred tool is supported.  It is also easier to add support
> for a specific tool than a generic mechanism.

I have no objection to a generic mechanism, but I don't see the value
of Charles suggestion to rip out support for the existing tools
supported by git-mergetool.

I think it *would* be better to use %(foo) extrapolation that
environment variables, so that it's not required for users to write
shell scripts unless absolutely necessary.

When we get around to rewriting git-mergetool in C, it might make
sense to put the tool support in the various shell scripts that are
installed in the git helper binary directory (i.e.,
git-mergetool-kdiff3, git-mergetool-meld, etc.)  That would make it
easier for users to create new shell scripts to support new tools if
necessary.

					- Ted

Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Charles Bailey]

On Sun, Feb 17, 2008 at 04:49:42PM -0500, Theodore Tso wrote:
> I think it *would* be better to use %(foo) extrapolation that
> environment variables, so that it's not required for users to write
> shell scripts unless absolutely necessary.

I understand that %(foo) is neater, but this would make the shell
implementation significantly more complex, and also potentially less
flexible (e.g. if you needed to do the opendiff style |cat trick or
some similar shell trickery).

The reason that I first posted a patch for a different config variable
for baseless merges was because I was that a significant number of
merge tools have significantly different syntaxes for two and three
way merges and I thought that it might be easier for users to do
something like:

git config mergetool.mymerge.cmd 'mytool --3way "$BASE" "$LOCAL" \
"$REMOTE" "$MERGED"'

git config mergetool.mymerge.cmdNoBase 'mytool --2way "$LOCAL" \
"$REMOTE" "$MERGED"'

than it would be to do something like (totally untested):

git config mergetool.mymerge.cmd 'if test -f "$BASE"; then;\
 mytool --3way "$BASE" "$LOCAL" "$REMOTE" "$MERGED"; else; \
 mytool --2way "$LOCAL" "$REMOTE" "$MERGED"; fi'

So in the former case you are still using shell syntax, but a simple
subset of shell should suffice for most needs.  The later case
requires more wizardry.

> When we get around to rewriting git-mergetool in C, it might make
> sense to put the tool support in the various shell scripts that are
> installed in the git helper binary directory (i.e.,
> git-mergetool-kdiff3, git-mergetool-meld, etc.)  That would make it
> easier for users to create new shell scripts to support new tools if
> necessary.
> 
> 					- Ted

This makes sense to me, although I have to say that I'm not really
sure I see the value of turning git-mergetool into C.  It seems to
make a lot of sense as a shell helper, but is it a general principle
that all of git's commands should eventually be converted to C?

Charles.

Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Charles Bailey]

On Sun, Feb 17, 2008 at 04:49:42PM -0500, Theodore Tso wrote:
> I have no objection to a generic mechanism, but I don't see the value
> of Charles suggestion to rip out support for the existing tools
> supported by git-mergetool.

Apologies for the multiple replies, I just remembered that I didn't
comment on this part.

My suggestion was really just me thinking aloud ("one idea I had...").
I was only stating that it would be possible to do this, there's no
value in actually doing this on its own, but the thought exercise
helped me validate my patch (at least to myself). If my patch were
flexible enough to handle all of the current built-in tools in a
generic fashion then it is a good sign that it should be able to cope
with a good portion of (as yet) unknown merge tools which is, after
all, the main point of my patch.

Charles.

Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Junio C Hamano]

Theodore Tso <tytso@MIT.EDU> writes:

> I have no objection to a generic mechanism, but I don't see the value
> of Charles suggestion to rip out support for the existing tools
> supported by git-mergetool.

I missed that suggestion but I agree removing existing support
would not make much sense.

> I think it *would* be better to use %(foo) extrapolation that
> environment variables, so that it's not required for users to write
> shell scripts unless absolutely necessary.

Hmm, although I do not have strong opinions either way, I think
the necessary interface is narrow enough that we could use
environment variables here.  Charles's implementation does
"eval" but it is easy to replace it to run the custom command
after exporting the necessary variables, isn't it?

> When we get around to rewriting git-mergetool in C, it might make
> sense to put the tool support in the various shell scripts that are
> installed in the git helper binary directory (i.e.,
> git-mergetool-kdiff3, git-mergetool-meld, etc.)  That would make it
> easier for users to create new shell scripts to support new tools if
> necessary.

Yup.

Re: [RFC/PATCH] Teach git mergetool to use custom commands defined at config time

[Charles Bailey]

On Sun, Feb 17, 2008 at 04:30:43PM -0800, Junio C Hamano wrote:
> Theodore Tso <tytso@MIT.EDU> writes:
> 
> > I have no objection to a generic mechanism, but I don't see the value
> > of Charles suggestion to rip out support for the existing tools
> > supported by git-mergetool.
> 
> I missed that suggestion but I agree removing existing support
> would not make much sense.

As I said in an earlier reply this was really more of a thought
exercise about my patch than a serious suggestion for integration.

> > I think it *would* be better to use %(foo) extrapolation that
> > environment variables, so that it's not required for users to write
> > shell scripts unless absolutely necessary.
> 
> Hmm, although I do not have strong opinions either way, I think
> the necessary interface is narrow enough that we could use
> environment variables here.  Charles's implementation does
> "eval" but it is easy to replace it to run the custom command
> after exporting the necessary variables, isn't it?

Do you mean instead of:

( eval $tool )

something like:
BASE="$BASE" LOCAL="$LOCAL" REMOTE="$REMOTE" MERGED="$MERGED" $tool

In this case we can skip the whole s/path/MERGED/ patch as it is
unnecessary, and should we now GIT_ prefix the variables as they will
intrude on the environment of the spawned command (not just a specific
sub-shell)? 

Let me know what you think and I can integrate it into my next patch
version.

Charles.