Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools

> John Keeping <john@keeping.me.uk> writes:
>
>> On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:
>>> --- a/git-mergetool--lib.sh
>>> +++ b/git-mergetool--lib.sh
>>> @@ -2,6 +2,35 @@
>>>  # git-mergetool--lib is a library for common merge tool functions
>>>  MERGE_TOOLS_DIR=$(git --exec-path)/mergetools
>>>
>>> +mode_ok () {
>>> +    diff_mode && can_diff ||
>>> +    merge_mode && can_merge
>>> +}
>>> +
>>> +is_available () {
>>> +    merge_tool_path=$(translate_merge_tool_path "$1") &&
>>> +    type "$merge_tool_path" >/dev/null 2>&1
>>> +}
>>> +
>>> +show_tool_names () {
>>> +    condition=${1:-true} per_line_prefix=${2:-} preamble=${3:-}
>>> +
>>> +    ( cd "$MERGE_TOOLS_DIR" && ls -1 * ) |
>>
>> Is the '*' necessary here?
>
> No, it was just from a bad habit (I have ls aliased to ls -A or ls
> -a in my interactive environment, which trained my fingers to this).
>
> I also think you can lose -1, although it does not hurt.
>>> +    tool_opt="'git ${TOOL_MODE}tool --tool-<tool>'"
>>> +    available=$(show_tool_names 'mode_ok && is_available' '\t\t' \
>>> +            "$tool_opt may be set to one of the following:")
>>> +    unavailable=$(show_tool_names 'mode_ok && ! is_available' '\t\t' \
>>> +            "The following tools are valid, but not currently available:")
>>>      if test -n "$available"
>>>      then
>>> -            echo "'git $cmd_name --tool=<tool>' may be set to one of the following:"
>>> -            echo "$available" | sort | sed -e 's/^/ /'
>>> +            echo "$available"
>>>      else
>>>              echo "No suitable tool for 'git $cmd_name --tool=<tool>' found."
>>>      fi
>>>      if test -n "$unavailable"
>>>      then
>>>              echo
>>> -            echo 'The following tools are valid, but not currently available:'
>>> -            echo "$unavailable" | sort | sed -e 's/^/       /'
>>> +            echo "$unavailable"
>>>      fi
>>>      if test -n "$unavailable$available"
>>>      then
>>
>> You haven't taken full advantage of the simplification Junio suggested
>> in response to v1 here.  We can change the "unavailable" block to be:
>>
>>     show_tool_names 'mode_ok && ! is_available' "$TAB$TAB" \
>>         "${LF}The following tools are valid, but not currently available:"
>
> Actually I was hoping that we can enhance show_tool_names so that we
> can do without the $available and $unavailable variables at all.
>
>> If you also add a "not_found_msg" parameter to show_tool_names then the
>> "available" case is also simplified:
>>
>>     show_tool_names 'mode_ok && is_available' "$TAB$TAB" \
>>         "$tool_opt may be set to one of the following:" \
>>         "No suitable tool for 'git $cmd_name --tool=<tool>' found."
>>
>> with this at the end of show_tool_names:
>>
>>     test -n "$preamble" && test -n "$not_found_msg" && \
>>         echo "$not_found_msg"
>
> Yes, something along that line.