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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 27, 2013, 22:31 UTC
Message-ID: <7vvcaitfzq.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1359321886-80523-4-git-send-email-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 20 quoted lines

> Refactor show_tool_help() so that the tool-finding logic is broken out
> into separate functions.
>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
>  git-mergetool--lib.sh | 60 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 34 insertions(+), 26 deletions(-)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index cf52423..894b849 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -2,6 +2,33 @@
>  # 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
> +}

I think you got the operator precedence mixed up.

What happens when diff_mode=true, can_diff=true, merge_mode=true and can_merge=false?

	if true && true || true && false
	then
		echo OK
	else
		echo NO
	fi
	if (true && true) || (true && false)
	then
		echo OK
	else
		echo NO
	fi

Show 16 quoted lines

> +is_available () {
> +	merge_tool_path=$(translate_merge_tool_path "$1") &&
> +	type "$merge_tool_path" >/dev/null 2>&1
> +}
> +
> +filter_tools () {
> +	filter="$1"
> +	prefix="$2"
> +	(
> +		cd "$MERGE_TOOLS_DIR" &&
> +		for i in *
> +		do
> +			echo "$i"
> +		done
> +	) | sort | while read tool
> +	do

Please start a new line before keywords that define the syntactic structure, like "while", i.e.

	... piped | commands |
	while read tool
	do
		...