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

Hi,

Here a patch I made after Junio's remarks in this thread :
http://thread.gmane.org/gmane.comp.version-control.git/148267

I think git-mergetool--lib.sh needs some refactoring. I'm no sh expert,
far from it, but I know a bit about scripting (I have been developing in PHP
for more than 6 years now :P) and I had quite some difficulties to understand
the behavior of some functions. For example, get_merge_tool_path returns the
name of the tool if no mergetool.<tool>.path have been set, which, from my
point of view, makes no sense.

I know there is maybe no time or no need to rewrite something which works
rather well but the +5 hours I spent to write this poor patch make me 
wonder if I am overrating myself. So if someone could just agree with me   
that would be a huge step to help me to regain some self respect :)

Cheers.

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
+		elif test -d "$merge_tool_path"; then
+			# 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
+		# 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
+
 	case "$1" in
 	kdiff3)
 		if merge_mode; then

-- 
1.7.1

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

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

Files and Directories are both paths...

+		elif test -d "$merge_tool_path"; then

But...

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

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

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

Cheers,

-- 
		David

David Aguilar [off-list ref] writes:

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

Thanks for a review.

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

I concur; it is not just about simplicity, but setting the value to the
parent directory of the tool feels downright confusing.

quoted

+		# mergetool.<tool>.path is the same as mergetool.<tool>.cmd
...
+	fi

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

Sounds like a sane thing to do.

On 05/06/2010 03:31, Sylvain Rabot wrote:

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(-)

First, my apologies for only having had the time to skim this so far.

Can I just ask some basic questions about the purpose of this patch. Is 
it the intention that if mergetool.<tool>.cmd is set then you want to 
mergetool to behave 'as if' the merge tool wasn't a 'known' tool and 
just performed the "*)" case ?

If so, it seems like a lot of extra boiler-plate and error handling that 
doesn't exist in the normal "*)" case. Should we have have this in the 
"*)" case as well? If so, we should look to rework it so that we can 
re-use the code rather than duplicating it.

 From a user perspective, if they want to run a "known" tool but in a 
way that is different from the default behaviour can't they just give it 
a different name, e.g. merge.tool=my_kdiff3 , mergetool.my_kdiff3.cmd=... ?

Thanks,

Charles.

-- 
Almost dormant mergetool maintainer.

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

On Wed, Jun 9, 2010 at 21:24, Charles Bailey [off-list ref] wrote:

On 05/06/2010 03:31, 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(-)

First, my apologies for only having had the time to skim this so far.

No worries ;)

Can I just ask some basic questions about the purpose of this patch. Is it
the intention that if mergetool.<tool>.cmd is set then you want to mergetool
to behave 'as if' the merge tool wasn't a 'known' tool and just performed
the "*)" case ?

yes

If so, it seems like a lot of extra boiler-plate and error handling that
doesn't exist in the normal "*)" case. Should we have have this in the "*)"
case as well? If so, we should look to rework it so that we can re-use the
code rather than duplicating it.

I did not modify the "*)" being afraid to break it all, but that would
be the right thing to do.

From a user perspective, if they want to run a "known" tool but in a way
that is different from the default behaviour can't they just give it a
different name, e.g. merge.tool=my_kdiff3 , mergetool.my_kdiff3.cmd=... ?

That's the workaround, yes, but, if you are the user and you are not
aware of this bahavior, you will do exactly like a did, i.e., lose
your time to torture your git configuration because it is not working
the way you was expecting it would.

Thanks,

Thanks for your time.

Charles.

--
Almost dormant mergetool maintainer.

-- 
Sylvain