threads / patch / 24015

patchgit-mergetool--lib.sh: fix mergetool.<tool>.* configurations ignored for known tools

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

## tl;dr

9 messages between Jun 5, 2010 and Jun 11, 2010. Diffs are folded; open one to read it.

replies: 8people: 5as markdown or json

Sylvain Rabot· Jun 5, 2010, 02:31 UTC · lore
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.
Sylvain Rabot· Jun 5, 2010, 02:31 UTC · re: Sylvain Rabot · lore

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(-)
Show changes to git-mergetool--lib.sh +60 −0
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
Andreas Schwab· Jun 5, 2010, 09:11 UTC · re: Sylvain Rabot · lore

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

Sylvain Rabot <sylvain@abstraction.fr> writes:
> +			if !test -f "$merge_tool_path/$merge_tool_cmd_base"; then
Missing space after '!'.
> +			elif !test -x "$merge_tool_path/$merge_tool_cmd_base"; then
Likewise.
Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Sylvain Rabot· Jun 5, 2010, 22:42 UTC · re: Andreas Schwab · lore

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

On Sat, 2010-06-05 at 11:11 +0200, Andreas Schwab wrote:
Show 12 quoted lines
> Sylvain Rabot <sylvain@abstraction.fr> writes:
> 
> > +			if !test -f "$merge_tool_path/$merge_tool_cmd_base"; then
> 
> Missing space after '!'.
> 
> > +			elif !test -x "$merge_tool_path/$merge_tool_cmd_base"; then
> 
> Likewise.
> 
> Andreas.
> 
Thanks,

I have updated the patch, you can find it here git://git.abstraction.fr/~sylvain/git.git in the mergetool-lib branch.

http://git.abstraction.fr/~sylvain/?p=git.git;a=commitdiff;h=905bfb5cea0750a67bf9bcc2baf22079054742fa
-- 
Sylvain Rabot <sylvain@abstraction.fr>
David Aguilar· Jun 8, 2010, 08:34 UTC · re: Sylvain Rabot · lore

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

Hi, sorry for the delay in responding to this email.
On Sat, Jun 05, 2010 at 04:31:52AM +0200, Sylvain Rabot wrote:
Show 40 quoted lines
> 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...
Show 12 quoted lines
> +			# 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.

Show 32 quoted lines
> +		# 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?

Show 6 quoted lines
> +
>  	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
Junio C Hamano· Jun 8, 2010, 21:05 UTC · re: David Aguilar · lore

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

David Aguilar <davvid@gmail.com> writes:
> Hi, sorry for the delay in responding to this email.
Thanks for a review.
Show 7 quoted lines
> 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.

Show 7 quoted lines
>> +		# 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.
Sylvain Rabot· Jun 11, 2010, 09:54 UTC · re: David Aguilar · lore

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

On Tue, Jun 8, 2010 at 10:34, David Aguilar <davvid@gmail.com> wrote:
Show 67 quoted lines
>
> 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 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.

Show 42 quoted lines
>
> 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?

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

Show 10 quoted lines
>
>> +
>>       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
Charles Bailey· Jun 9, 2010, 19:24 UTC · re: Sylvain Rabot · lore

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

On 05/06/2010 03:31, Sylvain Rabot wrote:
Show 15 quoted lines
> 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(-)
>
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.
Sylvain Rabot· Jun 11, 2010, 10:06 UTC · re: Charles Bailey · lore

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

On Wed, Jun 9, 2010 at 21:24, Charles Bailey <charles@hashpling.org> wrote:
Show 20 quoted lines
> 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<sylvain@abstraction.fr>
>> ---
>>  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 ;)
Show 5 quoted lines
>
> 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
Show 5 quoted lines
>
> 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.
Show 6 quoted lines
>
> Charles.
>
> --
> Almost dormant mergetool maintainer.
>
-- 
Sylvain

← back to recent threads