Re: [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 9, 2023, 11:41 UTC
Message-ID: <xmqqpm0jyx02.fsf@gitster.g>
In-Reply-To: <c5e627eb3fef0df162da56723093a03bfd2321fb.1699526999.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 10 quoted lines

> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> index e54492f8271..e4d31cbbd6a 100755
> --- a/GIT-VERSION-GEN
> +++ b/GIT-VERSION-GEN
> @@ -11,7 +11,7 @@ LF='
>  if test -f version
>  then
>  	VN=$(cat version) || VN="$DEF_VER"
> -elif test -d ${GIT_DIR:-.git} -o -f .git &&
> +elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&

I do not think this is strictly necessary.

Because the command line parser of "test" comes from left, notices "-d" and takes the next one to check if it is a directory. There is no value in ${GIT_DIR} can make "test -d ${GIT_DIR} -o ..." fail the same way as the problem Peff pointed out during the discussion.

I do not need a subshell for grouping, either. Plain {} should do (but you may need a LF or semicolon after the statement)..

Show 10 quoted lines

> diff --git a/configure.ac b/configure.ac
> index 276593cd9dd..d1a96da14eb 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -94,7 +94,7 @@ AC_DEFUN([GIT_PARSE_WITH_SET_MAKE_VAR],
>  [AC_ARG_WITH([$1],
>   [AS_HELP_STRING([--with-$1=VALUE], $3)],
>   if test -n "$withval"; then
> -  if test "$withval" = "yes" -o "$withval" = "no"; then
> +  if test "$withval" = "yes" || test "$withval" = "no"; then

This is correct and is in line with the way generated ./configure protects "if $withval is yes or no, then do this" against a funny value like "-f" in "$withval" breaking the parsing.

Show 10 quoted lines

> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index e0c5d3b0de6..43b5fec7320 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -489,13 +489,13 @@ find_existing_splits () {
>  			;;
>  		END)
>  			debug "Main is: '$main'"
> -			if test -z "$main" -a -n "$sub"
> +			if test -z "$main" && test -n "$sub"

This should be OK as-is, for the same reason "-d" operator would not be broken by arbitrary operand..

Show 7 quoted lines

>  			then
>  				# squash commits refer to a subtree
>  				debug "  Squash: $sq from $sub"
>  				cache_set "$sq" "$sub"
>  			fi
> -			if test -n "$main" -a -n "$sub"
> +			if test -n "$main" && test -n "$sub"

Ditto.

Show 13 quoted lines

>  			then
>  				debug "  Prior: $main -> $sub"
>  				cache_set $main $sub
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index e7786775a90..b952e5024b4 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -31,7 +31,7 @@ unset GIT_CONFIG_NOSYSTEM
>  GIT_CONFIG_SYSTEM="$TEST_DIRECTORY/perf/config"
>  export GIT_CONFIG_SYSTEM
>  
> -if test -n "$GIT_TEST_INSTALLED" -a -z "$PERF_SET_GIT_TEST_INSTALLED"
> +if test -n "$GIT_TEST_INSTALLED" && test -z "$PERF_SET_GIT_TEST_INSTALLED"

Ditto.

Show 13 quoted lines

>  then
>  	error "Do not use GIT_TEST_INSTALLED with the perf tests.
>  
> diff --git a/t/perf/run b/t/perf/run
> index 34115edec35..486ead21980 100755
> --- a/t/perf/run
> +++ b/t/perf/run
> @@ -91,10 +91,10 @@ set_git_test_installed () {
>  run_dirs_helper () {
>  	mydir=${1%/}
>  	shift
> -	while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
> +	while test $# -gt 0 && test "$1" != -- && test ! -f "$1"; do

As "$1" could be anything (including an insanity like "-n"), this change is prudent.

>  		shift
>  	done
> -	if test $# -gt 0 -a "$1" = --; then
> +	if test $# -gt 0 && test "$1" = --; then

Ditto.

Show 19 quoted lines

>  		shift
>  	fi
>  
> @@ -124,7 +124,7 @@ run_dirs_helper () {
>  }
>  
>  run_dirs () {
> -	while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
> +	while test $# -gt 0 && test "$1" != -- && test ! -f "$1"; do
>  		run_dirs_helper "$@"
>  		shift
>  	done
> @@ -180,7 +180,8 @@ run_subsection () {
>  	GIT_PERF_AGGREGATING_LATER=t
>  	export GIT_PERF_AGGREGATING_LATER
>  
> -	if test $# = 0 -o "$1" = -- -o -f "$1"; then
> +	if test $# = 0 || test "$1" = -- || test -f "$1"
> +	then

Ditto.

Show 13 quoted lines

>  		set -- . "$@"
>  	fi
>  
> diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
> index 669ebaf68be..9fbf90cee7c 100755
> --- a/t/valgrind/valgrind.sh
> +++ b/t/valgrind/valgrind.sh
> @@ -23,7 +23,7 @@ memcheck)
>  	VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
>  	VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
>  	test 3 -gt "$VALGRIND_MAJOR" ||
> -	test 3 -eq "$VALGRIND_MAJOR" -a 4 -gt "$VALGRIND_MINOR" ||
> +	( test 3 -eq "$VALGRIND_MAJOR" && test 4 -gt "$VALGRIND_MINOR" ) ||

This should be OK as-is; once "3 --eq" is parsed the parameter reference would not be taken as anything but just a value.

>  	TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
>  	;;
>  *)