Re: [PATCH v2 3/4] mergetool--lib: Add functions for finding available tools
From: John Keeping <hidden>
Date: 2016-06-15 22:55:56
On Sun, Jan 27, 2013 at 04:52:25PM -0800, David Aguilar wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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
}