Re: [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
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"
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"
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"; doAs "$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
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"
> + thenShow 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"
> ;;
> *)
Patrick Steinhardt
