Re: [PATCH] git-mergetool--lib.sh: fix mergetool.<tool>.* configurations ignored for known tools

On Tue, Jun 8, 2010 at 10:34, David Aguilar [off-list ref] wrote:

Hi, sorry for the delay in responding to this email.

On Sat, Jun 05, 2010 at 04:31:52AM +0200, Sylvain Rabot wrote:

quoted

At this time when you define merge.tool with a known tool,
such as meld, p4merge, diffuse ... etc, mergetool.<tool>.*
configurations are ignored and git mergetool will use its
own templates.

This patch adds a detection for mergetool.<tool>.cmd configuration
in the run_merge_tool function. If the configuration is set, it will
try to run the tool with mergetool.<tool>.path if its set. It also
consider the mergetool.<tool>.trustExitCode configuration.

Signed-off-by: Sylvain Rabot <redacted>
---
 git-mergetool--lib.sh |   60 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 51dd0d6..2a58d88 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh

@@ -84,9 +84,69 @@ get_merge_tool_cmd () {

 run_merge_tool () {
      merge_tool_path="$(get_merge_tool_path "$1")" || exit
+     merge_tool_cmd="$(get_merge_tool_cmd "$1")"
+     merge_tool_cmd_base="$(echo $merge_tool_cmd | cut -f1 -d " ")"
      base_present="$2"
      status=0

+     # if mergetool.<tool>.cmd is set we execute it, not a template
+     if test -n "$merge_tool_cmd"; then
+             # mergetool.<tool>.path is empty
+             if test -z "$merge_tool_path"; then
+                     # mergetool.<tool>.cmd not found
+                     if ! $(which "$merge_tool_cmd_base" > /dev/null 2>&1); then
+                             echo >&2 "Configuration mergetool.$1.cmd \"$merge_tool_cmd_base\" not found"
+                             exit 1
+                     else
+                             merge_cmd="$merge_tool_path/$merge_tool_cmd"
+                     fi
+             # mergetool.<tool>.path is a path

Files and Directories are both paths...

quoted

+             elif test -d "$merge_tool_path"; then

But...

quoted

+                     # mergetool.<tool>.cmd not found
+                     if !test -f "$merge_tool_path/$merge_tool_cmd_base"; then
+                             echo >&2 "Configuration mergetool.$1.cmd \"$(echo "$merge_tool_path/$merge_tool_cmd_base" | sed 's#//\+#/#')\" not found"
+                             exit 1
+                     # mergetool.<tool>.cmd not executable
+                     elif !test -x "$merge_tool_path/$merge_tool_cmd_base"; then
+                             echo >&2 "Configuration mergetool.$1.cmd \"$(echo "$merge_tool_path/$merge_tool_cmd_base" | sed 's#//\+#/#')\" is not executable"
+                             exit 1
+                     # tool ok
+                     else
+                             merge_cmd="$merge_tool_path/$merge_tool_cmd"
+                     fi

I don't think we ever signed up to support this configuration.
mergetool.<tool>.path has always (from my naive reading of the
documentation) been the absolute path to <tool>.

I did not see it that way but it seems cleaner.
So mergetool.<tool>.path would be the absolute path to the tool and
mergetool.<tool>.cmd would be only the args to call the tool with.

I don't think it should have a dual-role where it can be either
the tool's parent directory or the path to the tool itself.
I would prefer to keep it as simple as possible, if we can.

quoted

+             # mergetool.<tool>.path is the same as mergetool.<tool>.cmd
+             elif test "$merge_tool_path" = "$merge_tool_cmd_base"; then
+                     # mergetool.<tool>.cmd not found
+                     if ! $(which "$merge_tool_cmd_base" > /dev/null 2>&1); then
+                             echo >&2 "Configuration mergetool.$1.cmd \"$merge_tool_cmd_base\" not found"
+                             exit 1
+                     else
+                             merge_cmd="$merge_tool_cmd"
+                     fi
+             # mergetool.<tool>.path is the tool itself
+             elif $(which "$merge_tool_path" > /dev/null 2>&1); then
+                     merge_cmd="$merge_tool_path $merge_tool_cmd"
+             # mergetool.<tool>.path invalid
+             else
+                     echo >&2 "Configuration mergetool.$1.path \"$merge_tool_path\" is not valid path"
+                     exit 1
+             fi
+
+             # trust exit code
+             trust_exit_code="$(git config --bool mergetool."$1".trustExitCode || echo false)"
+
+             if test "$trust_exit_code" = "false"; then
+                     touch "$BACKUP"
+                     (eval "$merge_cmd")
+                     check_unchanged
+                     return $status
+             else
+                     (eval "$merge_cmd")
+                     status=$?
+                     return $status
+             fi
+     fi

This section is getting pretty nested.
Should we break the handling for configs-that-override-builtins
into a separate function?

I think the whole patch can be simplified if we assume path can only
be the absolute path to the tool.

quoted

+
      case "$1" in
      kdiff3)
              if merge_mode; then
--
1.7.1

One last thing -- I tried to fetch from the repo you
mentioned elsewhere in this thread but it was offline.

My bad, new box, new setup, forgot to authorize git daemon port to iptables.

Cheers,

Thanks for your time.

--
               David

-- 
Sylvain