On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:
> Refactor show_tool_help() so that the tool-finding logic is broken out
> into a separate show_tool_names() function.
>
> Signed-off-by: David Aguilar <dav...@gmail.com>
> ---
> filter_tools renamed to show_tool_names() and simplfied
> to use ls -1. show_tool_names() now has a preamble as discussed.
>
> git-mergetool--lib.sh | 68
> +++++++++++++++++++++++++++++----------------------
> 1 file changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index db3eb58..fe068f6 100644
> --- 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
> +}
> +
Can we move show_tool_names() to be above show_tool_help()? It's a
very minor nit but I prefer having related functionality grouped
together.
> +show_tool_names () {
> + condition=${1:-true} per_line_prefix=${2:-} preamble=${3:-}
Would this be better with one value on each line? Also perhaps
per_line_prefix -> line_prefix.
> +
> + ( cd "$MERGE_TOOLS_DIR" && ls -1 * ) |
> + while read toolname
> + do
> + if setup_tool "$toolname" 2>/dev/null &&
> + (eval "$condition" "$toolname")
> + then
> + if test -n "$preamble"
> + then
> + echo "$preamble"
> + preamble=
> + fi
> + printf "%s%s\n" "$per_line_prefix" "$tool"
This needs to be:
printf "$per_line_prefix%s\n" "$tool"
since $per_line_prefix is usually '\t\t' which isn't expanded if we
format it with %s - an alternative would be to change the value passed
in to '$TAB$TAB' with literal tabs.
> + fi
> + done
> +}
> +
> diff_mode() {
> test "$TOOL_MODE" = diff
> }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
