On Tue, Jan 29, 2013 at 12:22 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> John Keeping <john@xxxxxxxxxxxxx> 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.
I don't want to stomp on your feet and poke at this file too much if
you're planning on building on top of it (I already did a few times
;-). My git time is a bit limited for the next few days so I don't
want to hold you up. I can help shepherd through small fixups that
come up until the weekend rolls around and I have more time, but I
also don't want to hold you back until then.
I will have some time tonight. If you guys would prefer an
incremental patch I can send one that changes the "ls" expression and
the way the unavailable block is structured. Otherwise, I can send
replacements to handle the "test -z" thing, $TAB$TAB, and the
simplification of the unavailable block.
Later patches (that are working towards the new feature) can
generalize show_tool_names() further and eliminate the need for the
available/unavailable variables altogether. John, I would imagine
that you'd want to pick that up since you're driving towards having
--tool-help honor custom tools.
What do you think?
--
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html