* [PATCH] git-mergetool--lib.sh: fix mergetool.<tool>.* configurations ignored for known tools
@ 2010-06-05 2:31 Sylvain Rabot
2010-06-05 2:31 ` Sylvain Rabot
0 siblings, 1 reply; 9+ messages in thread
From: Sylvain Rabot @ 2010-06-05 2:31 UTC (permalink / raw
To: gitster; +Cc: git
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] git-mergetool--lib.sh: fix mergetool.<tool>.* configurations ignored for known tools
2010-06-05 2:31 [PATCH] git-mergetool--lib.sh: fix mergetool.<tool>.* configurations ignored for known tools Sylvain Rabot
@ 2010-06-05 2:31 ` Sylvain Rabot
2010-06-05 9:11 ` Andreas Schwab
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Sylvain Rabot @ 2010-06-05 2:31 UTC (permalink / raw
To: gitster; +Cc: git, Sylvain Rabot
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
+ 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] git-mergetool--lib.sh: fix mergetool.<tool>.* configurations ignored for known tools
2010-06-05 2:31 ` Sylvain Rabot
@ 2010-06-05 9:11 ` Andreas Schwab
2010-06-05 22:42 ` Sylvain Rabot
2010-06-08 8:34 ` David Aguilar
2010-06-09 19:24 ` Charles Bailey
2 siblings, 1 reply; 9+ messages in thread
From: Andreas Schwab @ 2010-06-05 9:11 UTC (permalink / raw
To: Sylvain Rabot; +Cc: gitster, git
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."
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] git-mergetool--lib.sh: fix mergetool.<tool>.* configurations ignored for known tools
2010-06-05 9:11 ` Andreas Schwab
@ 2010-06-05 22:42 ` Sylvain Rabot
0 siblings, 0 replies; 9+ messages in thread
From: Sylvain Rabot @ 2010-06-05 22:42 UTC (permalink / raw
To: Andreas Schwab; +Cc: gitster, git
[-- Attachment #1: Type: text/plain, Size: 607 bytes --]
On Sat, 2010-06-05 at 11:11 +0200, Andreas Schwab wrote:
> 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>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] git-mergetool--lib.sh: fix mergetool.<tool>.* configurations ignored for known tools
2010-06-05 2:31 ` Sylvain Rabot
2010-06-05 9:11 ` Andreas Schwab
@ 2010-06-08 8:34 ` David Aguilar
2010-06-08 21:05 ` Junio C Hamano
2010-06-11 9:54 ` Sylvain Rabot
2010-06-09 19:24 ` Charles Bailey
2 siblings, 2 replies; 9+ messages in thread
From: David Aguilar @ 2010-06-08 8:34 UTC (permalink / raw
To: Sylvain Rabot; +Cc: gitster, git
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] git-mergetool--lib.sh: fix mergetool.<tool>.* configurations ignored for known tools
2010-06-08 8:34 ` David Aguilar
@ 2010-06-08 21:05 ` Junio C Hamano
2010-06-11 9:54 ` Sylvain Rabot
1 sibling, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2010-06-08 21:05 UTC (permalink / raw
To: David Aguilar; +Cc: Sylvain Rabot, git
David Aguilar <davvid@gmail.com> 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.
>> + # 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] git-mergetool--lib.sh: fix mergetool.<tool>.* configurations ignored for known tools
2010-06-05 2:31 ` Sylvain Rabot
2010-06-05 9:11 ` Andreas Schwab
2010-06-08 8:34 ` David Aguilar
@ 2010-06-09 19:24 ` Charles Bailey
2010-06-11 10:06 ` Sylvain Rabot
2 siblings, 1 reply; 9+ messages in thread
From: Charles Bailey @ 2010-06-09 19:24 UTC (permalink / raw
To: Sylvain Rabot; +Cc: gitster, git, davvid
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.
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] git-mergetool--lib.sh: fix mergetool.<tool>.* configurations ignored for known tools
2010-06-08 8:34 ` David Aguilar
2010-06-08 21:05 ` Junio C Hamano
@ 2010-06-11 9:54 ` Sylvain Rabot
1 sibling, 0 replies; 9+ messages in thread
From: Sylvain Rabot @ 2010-06-11 9:54 UTC (permalink / raw
To: David Aguilar; +Cc: gitster, git
On Tue, Jun 8, 2010 at 10:34, David Aguilar <davvid@gmail.com> wrote:
>
> 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.
>
> 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.
>
>> +
>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] git-mergetool--lib.sh: fix mergetool.<tool>.* configurations ignored for known tools
2010-06-09 19:24 ` Charles Bailey
@ 2010-06-11 10:06 ` Sylvain Rabot
0 siblings, 0 replies; 9+ messages in thread
From: Sylvain Rabot @ 2010-06-11 10:06 UTC (permalink / raw
To: Charles Bailey; +Cc: gitster, git, davvid
On Wed, Jun 9, 2010 at 21:24, Charles Bailey <charles@hashpling.org> wrote:
> 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 ;)
>
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-06-11 10:07 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-06-05 2:31 [PATCH] git-mergetool--lib.sh: fix mergetool.<tool>.* configurations ignored for known tools Sylvain Rabot
2010-06-05 2:31 ` Sylvain Rabot
2010-06-05 9:11 ` Andreas Schwab
2010-06-05 22:42 ` Sylvain Rabot
2010-06-08 8:34 ` David Aguilar
2010-06-08 21:05 ` Junio C Hamano
2010-06-11 9:54 ` Sylvain Rabot
2010-06-09 19:24 ` Charles Bailey
2010-06-11 10:06 ` Sylvain Rabot
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).