Re: [PATCH 08/10] git-mergetool.sh: don't use the -a or -b option with the test command

[David Aguilar <davvid@gmail.com>]

On Thu, May 15, 2014 at 07:17:35AM -0700, Elia Pinto wrote:
> Even though POSIX.1 lists -a/-o as options to "test", they are
> marked "Obsolescent XSI". Scripts using these expressions
> should be converted  as follow:
> 
> test "$1" -a "$2"
> 
> should be written as:
> 
> test "$1" && test "$2"
> 
> Likewise
> 
> test "$1" -o "$2"
> 
> should be written as:
> 
> test "$1"  test "$2"
> 
> But note that, in test, -a has higher precedence than -o while
> "&&" and "||" have equal precedence in the shell.
> 
> The reason for this is that the precedence rules were never well
> specified, and this made many sane-looking uses of "test -a/-o" problematic.
> 
> For example, if $x is "=", these work according to POSIX (it's not
> portable, but in practice it's okay):
> 
>    $ test -z "$x"
>    $ test -z "$x" && test a = b
> 
> but this doesn't
> 
>    $ test -z "$x" -a a = b
>    bash: test: too many arguments
> 
> because it groups "test -n = -a" and is left with "a = b".
> 
> Similarly, if $x is "-f", these
> 
>    $ test "$x"
>    $ test "$x" || test c = d
> 
> correctly adds an implicit "-n", but this fails:
> 
>    $ test "$x" -o c = d
>    bash: test: too many arguments
> 
> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> ---
> Inspired from this discussion http://permalink.gmane.org/gmane.comp.version-control.git/137056

Looks good, thanks.

Acked-by: David Aguilar <davvid@gmail.com>


>  git-mergetool.sh |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 332528f..88e853f 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -205,7 +205,7 @@ checkout_staged_file () {
>  		"$(git checkout-index --temp --stage="$1" "$2" 2>/dev/null)" \
>  		: '\([^	]*\)	')
>  
> -	if test $? -eq 0 -a -n "$tmpfile"
> +	if test $? -eq 0 && test -n "$tmpfile"
>  	then
>  		mv -- "$(git rev-parse --show-cdup)$tmpfile" "$3"
>  	else
> @@ -256,7 +256,7 @@ merge_file () {
>  	checkout_staged_file 2 "$MERGED" "$LOCAL"
>  	checkout_staged_file 3 "$MERGED" "$REMOTE"
>  
> -	if test -z "$local_mode" -o -z "$remote_mode"
> +	if test -z "$local_mode" || test -z "$remote_mode"
>  	then
>  		echo "Deleted merge conflict for '$MERGED':"
>  		describe_file "$local_mode" "local" "$LOCAL"
> -- 
> 1.7.10.4
-- 
David