From: John Keeping <john@keeping.me.uk>
Date: Tue, 29 Jan 2013 19:22:04 GMT
Subject: Re: [PATCH v2 1/4] mergetool--lib: Simplify command expressions
Message-ID: <20130129192204.GC1342@serenity.lan>
In-Reply-To: <1359334346-5879-2-git-send-email-davvid@gmail.com>

On Sun, Jan 27, 2013 at 04:52:23PM -0800, David Aguilar wrote:
> Update variable assignments to always use $(command "$arg")
> in their RHS instead of "$(command "$arg")" as the latter
> is harder to read.  Make get_merge_tool_cmd() simpler by
> avoiding "echo" and $(command) substitutions completely.
> 
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> @@ -300,9 +292,9 @@ get_merge_tool_path () {
>  	fi
>  	if test -z "$merge_tool_path"
>  	then
> -		merge_tool_path="$(translate_merge_tool_path "$merge_tool")"
> +		merge_tool_path=$(translate_merge_tool_path "$merge_tool")
>  	fi
> -	if test -z "$(get_merge_tool_cmd "$merge_tool")" &&
> +	if test -z $(get_merge_tool_cmd "$merge_tool") &&

This change should be reverted to avoid calling "test -z" without any
other arguments, as Johannes pointed out in v1.

The rest of this patch looks good to me.

>  		! type "$merge_tool_path" >/dev/null 2>&1
>  	then
>  		echo >&2 "The $TOOL_MODE tool $merge_tool is not available as"\