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

From: Sylvain Rabot <sylvain@abstraction.fr>
Date: Jun 11, 2010, 09:54 UTC
Message-ID: <AANLkTikK0t_-H5zgzUToiTlyWCFGCJ63wok-t7wX09OW@mail.gmail.com>
In-Reply-To: <20100608083445.GC14366@gmail.com>

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