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

[David Aguilar <davvid@gmail.com>]

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

On Sat, Jun 05, 2010 at 04:31:52AM +0200, 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 <sylvain@abstraction.fr>
> ---
>  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...

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