Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 29, 2013, 20:22 UTC
- Message-ID
- <7vr4l3oi1z.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20130129194846.GD1342@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
Show 23 quoted lines
> 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.
Show 28 quoted lines
>> + 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.
Show 11 quoted lines
> 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.