[PATCH v2] mergetool: support setting path to tool as config var mergetool.<tool>.path

[PATCH v2] mergetool: support setting path to tool as config var mergetool.<tool>.path

[Steffen Prohaska]

This commit adds a mechanism to provide absolute paths to the
external programs called by 'git mergetool'. A path can be
specified in the configuation variable mergetool.<tool>.path.
The configuration variable is similar to how we name branches
and remotes. It is extensible if we need to specify more details
about a tool.

The mechanism added by this patch is orthogonal to the existing
merge.tool configuration variable:
1) merge.tool selects a tool from a list of known programs for which
command line arguments and the mechanism for returning merge results is know.
2) mergetool.<tool>.path provides details on how to locate the selected
tool in the filesystem. Paths for several tools can be configured simultaneously.

The mechanism is especially useful on Windows, where external
programs are unlikely to be in PATH.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
 Documentation/git-mergetool.txt |    6 ++
 git-mergetool.sh                |   97 +++++++++++++++++++++-----------------
 2 files changed, 60 insertions(+), 43 deletions(-)

This updated patch is based on the the comments and suggestions by
Theodor, Frank, and Johannes.

    Steffen

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 6c32c6d..17a33e7 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -31,6 +31,12 @@ If a merge resolution program is not specified, 'git mergetool'
 will use the configuration variable merge.tool.  If the
 configuration variable merge.tool is not set, 'git mergetool'
 will pick a suitable default.
++
+You can explicitly provide a full path to the tool by setting the
+configuration variable mergetool.<tool>.path. For example, you
+can configure the absolute path to kdiff3 by setting
+mergetool.kdiff3.path. Otherise, 'git mergetool' assumes the tool
+is available in PATH.
 
 Author
 ------
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 9f4f313..1e4583f 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -192,10 +192,10 @@ merge_file () {
     case "$merge_tool" in
 	kdiff3)
 	    if base_present ; then
-		(kdiff3 --auto --L1 "$path (Base)" --L2 "$path (Local)" --L3 "$path (Remote)" \
+		("$merge_tool_path" --auto --L1 "$path (Base)" --L2 "$path (Local)" --L3 "$path (Remote)" \
 		    -o "$path" -- "$BASE" "$LOCAL" "$REMOTE" > /dev/null 2>&1)
 	    else
-		(kdiff3 --auto --L1 "$path (Local)" --L2 "$path (Remote)" \
+		("$merge_tool_path" --auto --L1 "$path (Local)" --L2 "$path (Remote)" \
 		    -o "$path" -- "$LOCAL" "$REMOTE" > /dev/null 2>&1)
 	    fi
 	    status=$?
@@ -203,35 +203,35 @@ merge_file () {
 	    ;;
 	tkdiff)
 	    if base_present ; then
-		tkdiff -a "$BASE" -o "$path" -- "$LOCAL" "$REMOTE"
+		"$merge_tool_path" -a "$BASE" -o "$path" -- "$LOCAL" "$REMOTE"
 	    else
-		tkdiff -o "$path" -- "$LOCAL" "$REMOTE"
+		"$merge_tool_path" -o "$path" -- "$LOCAL" "$REMOTE"
 	    fi
 	    status=$?
 	    save_backup
 	    ;;
 	meld|vimdiff)
 	    touch "$BACKUP"
-	    $merge_tool -- "$LOCAL" "$path" "$REMOTE"
+	    "$merge_tool_path" -- "$LOCAL" "$path" "$REMOTE"
 	    check_unchanged
 	    save_backup
 	    ;;
 	gvimdiff)
 		touch "$BACKUP"
-		gvimdiff -f -- "$LOCAL" "$path" "$REMOTE"
+		"$merge_tool_path" -f -- "$LOCAL" "$path" "$REMOTE"
 		check_unchanged
 		save_backup
 		;;
 	xxdiff)
 	    touch "$BACKUP"
 	    if base_present ; then
-		xxdiff -X --show-merged-pane \
+		"$merge_tool_path" -X --show-merged-pane \
 		    -R 'Accel.SaveAsMerged: "Ctrl-S"' \
 		    -R 'Accel.Search: "Ctrl+F"' \
 		    -R 'Accel.SearchForward: "Ctrl-G"' \
 		    --merged-file "$path" -- "$LOCAL" "$BASE" "$REMOTE"
 	    else
-		xxdiff -X --show-merged-pane \
+		"$merge_tool_path" -X --show-merged-pane \
 		    -R 'Accel.SaveAsMerged: "Ctrl-S"' \
 		    -R 'Accel.Search: "Ctrl+F"' \
 		    -R 'Accel.SearchForward: "Ctrl-G"' \
@@ -243,18 +243,18 @@ merge_file () {
 	opendiff)
 	    touch "$BACKUP"
 	    if base_present; then
-		opendiff "$LOCAL" "$REMOTE" -ancestor "$BASE" -merge "$path" | cat
+		"$merge_tool_path" "$LOCAL" "$REMOTE" -ancestor "$BASE" -merge "$path" | cat
 	    else
-		opendiff "$LOCAL" "$REMOTE" -merge "$path" | cat
+		"$merge_tool_path" "$LOCAL" "$REMOTE" -merge "$path" | cat
 	    fi
 	    check_unchanged
 	    save_backup
 	    ;;
 	emerge)
 	    if base_present ; then
-		emacs -f emerge-files-with-ancestor-command "$LOCAL" "$REMOTE" "$BASE" "$(basename "$path")"
+		"$merge_tool_path" -f emerge-files-with-ancestor-command "$LOCAL" "$REMOTE" "$BASE" "$(basename "$path")"
 	    else
-		emacs -f emerge-files-command "$LOCAL" "$REMOTE" "$(basename "$path")"
+		"$merge_tool_path" -f emerge-files-command "$LOCAL" "$REMOTE" "$(basename "$path")"
 	    fi
 	    status=$?
 	    save_backup
@@ -297,17 +297,38 @@ do
     shift
 done
 
+valid_tool() {
+	case "$1" in
+		kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff)
+			;; # happy
+		*)
+			return 1
+			;;
+	esac
+}
+
+init_merge_tool_path() {
+	merge_tool_path=`git config mergetool.$1.path`
+	if test -z "$merge_tool_path" ; then
+		case "$merge_tool" in
+			emerge)
+				merge_tool_path=emacs
+				;;
+			*)
+				merge_tool_path=$merge_tool
+				;;
+		esac
+	fi
+}
+
+
 if test -z "$merge_tool"; then
     merge_tool=`git config merge.tool`
-    case "$merge_tool" in
-	kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff | gvimdiff | "")
-	    ;; # happy
-	*)
+    test -n "$merge_tool" || valid_tool "$merge_tool" || {
 	    echo >&2 "git config option merge.tool set to unknown tool: $merge_tool"
 	    echo >&2 "Resetting to default..."
 	    unset merge_tool
-	    ;;
-    esac
+    }
 fi
 
 if test -z "$merge_tool" ; then
@@ -329,40 +350,30 @@ if test -z "$merge_tool" ; then
     merge_tool_candidates="$merge_tool_candidates opendiff emerge vimdiff"
     echo "merge tool candidates: $merge_tool_candidates"
     for i in $merge_tool_candidates; do
-        if test $i = emerge ; then
-            cmd=emacs
-        else
-            cmd=$i
-        fi
-        if type $cmd > /dev/null 2>&1; then
+        init_merge_tool_path $i
+        if type "$merge_tool_path" > /dev/null 2>&1; then
             merge_tool=$i
             break
         fi
     done
     if test -z "$merge_tool" ; then
-	echo "No available merge resolution programs available."
+	echo "No known merge resolution program available."
 	exit 1
     fi
+else
+    valid_tool "$merge_tool" || {
+        echo >&2 "Unknown merge_tool $merge_tool"
+        exit 1
+    }
+
+    init_merge_tool_path "$merge_tool"
+
+    if ! 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
 fi
 
-case "$merge_tool" in
-    kdiff3|tkdiff|meld|xxdiff|vimdiff|gvimdiff|opendiff)
-	if ! type "$merge_tool" > /dev/null 2>&1; then
-	    echo "The merge tool $merge_tool is not available"
-	    exit 1
-	fi
-	;;
-    emerge)
-	if ! type "emacs" > /dev/null 2>&1; then
-	    echo "Emacs is not available"
-	    exit 1
-	fi
-	;;
-    *)
-	echo "Unknown merge tool: $merge_tool"
-	exit 1
-	;;
-esac
 
 if test $# -eq 0 ; then
 	files=`git ls-files -u | sed -e 's/^[^	]*	//' | sort -u`
-- 
1.5.3.3.127.g40d17

Re: [PATCH v2] mergetool: support setting path to tool as config var mergetool.<tool>.path

[Johannes Schindelin]

Hi,

On Tue, 9 Oct 2007, Steffen Prohaska wrote:

> This commit adds a mechanism to provide absolute paths to the external 
> programs called by 'git mergetool'. A path can be specified in the 
> configuation variable mergetool.<tool>.path. The configuration variable 
> is similar to how we name branches and remotes. It is extensible if we 
> need to specify more details about a tool.

Okay, let's step back a bit.

What does mergetool do?  It calls different merge helpers, each with its 
own convention how to call it.  For example, tkdiff is called either as

		tkdiff -a "$BASE" -o "$path" -- "$LOCAL" "$REMOTE"

or as

		tkdiff -o "$path" -- "$LOCAL" "$REMOTE"

depending if there is a base or not.  Another example is gvimdiff:

		gvimdiff -f -- "$LOCAL" "$path" "$REMOTE"

which seems not to care if there is a base.

Now, would it not be much better if we had a way to specify the tool and 
the convention indepentently?  Like

merge.tkdiff.path = C:\bla\blub\wish.exe C:\blub\bleh\tkdiff.tcl
merge.tkdiff.options = -o %p -- %l %r
merge.tkdiff.optionsWithBase = -a %b -o %p -- %l %r

and have defaults for the tools we have in git-mergetool.sh _already_?

Ciao,
Dscho

Re: [PATCH v2] mergetool: support setting path to tool as config var mergetool.<tool>.path

[Steffen Prohaska]

On Oct 10, 2007, at 4:33 PM, Johannes Schindelin wrote:

> Hi,
>
> On Tue, 9 Oct 2007, Steffen Prohaska wrote:
>
>> This commit adds a mechanism to provide absolute paths to the  
>> external
>> programs called by 'git mergetool'. A path can be specified in the
>> configuation variable mergetool.<tool>.path. The configuration  
>> variable
>> is similar to how we name branches and remotes. It is extensible  
>> if we
>> need to specify more details about a tool.
>
> Okay, let's step back a bit.
>
> What does mergetool do?  It calls different merge helpers, each  
> with its
> own convention how to call it.  For example, tkdiff is called  
> either as
>
> 		tkdiff -a "$BASE" -o "$path" -- "$LOCAL" "$REMOTE"
>
> or as
>
> 		tkdiff -o "$path" -- "$LOCAL" "$REMOTE"
>
> depending if there is a base or not.  Another example is gvimdiff:
>
> 		gvimdiff -f -- "$LOCAL" "$path" "$REMOTE"
>
> which seems not to care if there is a base.
>
> Now, would it not be much better if we had a way to specify the  
> tool and
> the convention indepentently?  Like
>
> merge.tkdiff.path = C:\bla\blub\wish.exe C:\blub\bleh\tkdiff.tcl
> merge.tkdiff.options = -o %p -- %l %r
> merge.tkdiff.optionsWithBase = -a %b -o %p -- %l %r
>
> and have defaults for the tools we have in git-mergetool.sh _already_?

If you provide a generic mechanism to call an external tool, there's no
need to name the tool. A single mechanism (let's call it external)  
would be
sufficient, like

mergetool.external.path = c:\any\path\merge.exe
mergetool.external.arg2way = %l %r --merge2 --to=%p
mergetool.external.arg3way = %b %l %r --merge3 --to=%p

But this places the burdon on the user to figure out the command line  
syntax
and specify it correctly using git-config. Things like proper  
escaping may
be a hassel.

The solution I'm proposing is more user-friendly. Only the information
that is hard to figure out automatically and is easy to provide by the
user is asked for. The user only needs to tell the path to the  
executable.

git-mergetool 'knows' about the correct command line syntax. There's  
no need
to ask the user. The command line syntax is fixed and know. No option  
here.
"git mergetool" could, for example, know that a certain tool just  
doesn't
support 3-way.

And the code of git-mergetool is also quite easy. The only input  
validation
that is needed is to check if mergetool.<tool>.path points to a valid
executable. If we provide a complex syntax for specifying command line
options we may have to do a lot more of input validation and processing.

I strongly favor my solution of including the command line syntax in
git-mergetool, and only ask the user for the path. I'm not against a  
generic
mechanism to configure any tool. However, I do not plan to implement it.

If someone plans to develop a generic mechanism I see two options:
1) What's explained above.
2) _Define_ the parameters passed to the external command and ask the  
user
    to write a wrapper script to call his tool of choice. Similar to  
what
    GIT_EXTERNAL_DIFF does. Wrapper scripts would go to contrib/.

Both solutions would require more work from the user than needed.

	Steffen

Re: [PATCH v2] mergetool: support setting path to tool as config var mergetool.<tool>.path

[Johannes Schindelin]

Hi,

On Wed, 10 Oct 2007, Steffen Prohaska wrote:

> On Oct 10, 2007, at 4:33 PM, Johannes Schindelin wrote:
> 
> > On Tue, 9 Oct 2007, Steffen Prohaska wrote:
> > 
> > > This commit adds a mechanism to provide absolute paths to the 
> > > external programs called by 'git mergetool'. A path can be specified 
> > > in the configuation variable mergetool.<tool>.path. The 
> > > configuration variable is similar to how we name branches and 
> > > remotes. It is extensible if we need to specify more details about a 
> > > tool.
> > 
> > Okay, let's step back a bit.
> > 
> > What does mergetool do?  It calls different merge helpers, each with its
> > own convention how to call it.  For example, tkdiff is called either as
> > 
> > 		tkdiff -a "$BASE" -o "$path" -- "$LOCAL" "$REMOTE"
> > 
> > or as
> > 
> > 		tkdiff -o "$path" -- "$LOCAL" "$REMOTE"
> > 
> > depending if there is a base or not.  Another example is gvimdiff:
> > 
> > 		gvimdiff -f -- "$LOCAL" "$path" "$REMOTE"
> > 
> > which seems not to care if there is a base.
> > 
> > Now, would it not be much better if we had a way to specify the tool 
> > and the convention indepentently?  Like
> > 
> > merge.tkdiff.path = C:\bla\blub\wish.exe C:\blub\bleh\tkdiff.tcl
> > merge.tkdiff.options = -o %p -- %l %r
> > merge.tkdiff.optionsWithBase = -a %b -o %p -- %l %r
> > 
> > and have defaults for the tools we have in git-mergetool.sh _already_?
> 
> If you provide a generic mechanism to call an external tool, there's no 
> need to name the tool. A single mechanism (let's call it external) would 
> be sufficient, like
> 
> mergetool.external.path = c:\any\path\merge.exe
> mergetool.external.arg2way = %l %r --merge2 --to=%p
> mergetool.external.arg3way = %b %l %r --merge3 --to=%p
> 
> But this places the burdon on the user to figure out the command line syntax
> and specify it correctly using git-config.

Guess why I did not want to  have a single mechanism.  I did _not_ propose 
to place the burden on the user to figure out the command line.

Besides, I do not want to do away with the automatic detection of the 
tool, which _implies_ having a list of defaults for the command line 
syntax, which in turn _commands_ having the name instead of "external".

While your solution is an obvious short term fix, I maintain that my 
proposal is the Right Thing in the long run.

Ciao,
Dscho

Re: [PATCH v2] mergetool: support setting path to tool as config var mergetool.<tool>.path

[Steffen Prohaska]

On Oct 10, 2007, at 7:03 PM, Johannes Schindelin wrote:

> Hi,
>
> On Wed, 10 Oct 2007, Steffen Prohaska wrote:
>
>> On Oct 10, 2007, at 4:33 PM, Johannes Schindelin wrote:
>>
>>> On Tue, 9 Oct 2007, Steffen Prohaska wrote:
>>>
>>>> This commit adds a mechanism to provide absolute paths to the
>>>> external programs called by 'git mergetool'. A path can be  
>>>> specified
>>>> in the configuation variable mergetool.<tool>.path. The
>>>> configuration variable is similar to how we name branches and
>>>> remotes. It is extensible if we need to specify more details  
>>>> about a
>>>> tool.
>>>
>>> Okay, let's step back a bit.
>>>
>>> What does mergetool do?  It calls different merge helpers, each  
>>> with its
>>> own convention how to call it.  For example, tkdiff is called  
>>> either as
>>>
>>> 		tkdiff -a "$BASE" -o "$path" -- "$LOCAL" "$REMOTE"
>>>
>>> or as
>>>
>>> 		tkdiff -o "$path" -- "$LOCAL" "$REMOTE"
>>>
>>> depending if there is a base or not.  Another example is gvimdiff:
>>>
>>> 		gvimdiff -f -- "$LOCAL" "$path" "$REMOTE"
>>>
>>> which seems not to care if there is a base.
>>>
>>> Now, would it not be much better if we had a way to specify the tool
>>> and the convention indepentently?  Like
>>>
>>> merge.tkdiff.path = C:\bla\blub\wish.exe C:\blub\bleh\tkdiff.tcl
>>> merge.tkdiff.options = -o %p -- %l %r
>>> merge.tkdiff.optionsWithBase = -a %b -o %p -- %l %r
>>>
>>> and have defaults for the tools we have in git-mergetool.sh  
>>> _already_?
>>
>> If you provide a generic mechanism to call an external tool,  
>> there's no
>> need to name the tool. A single mechanism (let's call it external)  
>> would
>> be sufficient, like
>>
>> mergetool.external.path = c:\any\path\merge.exe
>> mergetool.external.arg2way = %l %r --merge2 --to=%p
>> mergetool.external.arg3way = %b %l %r --merge3 --to=%p
>>
>> But this places the burdon on the user to figure out the command  
>> line syntax
>> and specify it correctly using git-config.
>
> Guess why I did not want to  have a single mechanism.  I did _not_  
> propose
> to place the burden on the user to figure out the command line.
>
> Besides, I do not want to do away with the automatic detection of the
> tool, which _implies_ having a list of defaults for the command line
> syntax, which in turn _commands_ having the name instead of  
> "external".
>
> While your solution is an obvious short term fix, I maintain that my
> proposal is the Right Thing in the long run.

I did not intend to replace everything with an external mechanism.  
What I
wanted to say is the following: If you add a generic mechanism in  
addition
to the known tools it is sufficient to add a single generic mechanism
called 'external'. You could then choose from the list of known tools or
you could choose 'external' and provide everything needed as described
above.

	Steffen

Re: [PATCH v2] mergetool: support setting path to tool as config var mergetool.<tool>.path

[Steffen Prohaska]

On Oct 9, 2007, at 10:54 PM, Steffen Prohaska wrote:

> This commit adds a mechanism to provide absolute paths to the
> external programs called by 'git mergetool'. A path can be
> specified in the configuation variable mergetool.<tool>.path.

Any news on this patch?
Will you apply it (or recommend that Junio does)?

	Steffen

Re: [PATCH v2] mergetool: support setting path to tool as config var mergetool.<tool>.path

[Theodore Tso]

On Sun, Oct 14, 2007 at 02:52:25PM +0200, Steffen Prohaska wrote:
>
>> This commit adds a mechanism to provide absolute paths to the
>> external programs called by 'git mergetool'. A path can be
>> specified in the configuation variable mergetool.<tool>.path.
>
> Any news on this patch?
> Will you apply it (or recommend that Junio does)?

Hmm, for some reason I never received the v2 version of the patch.  I
see Johannes' comments to it, but not the original e-mail for some
reason.  It doesn't seem to be in the spam filter, so I'm not sure
what happened.

Can you resend, please?   Thanks!!

							- Ted