{"thread":{"id":"60497","subject":"[PATCH 0/4] Replace use of `test -o/a `","startedAt":"2023-11-09T10:53:34Z","lastAt":"2023-11-13T07:12:52Z","messageCount":26,"participants":["Patrick Steinhardt","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"484636","messageId":"cover.1699526999.git.ps@pks.im","threadId":"60497","inReplyTo":null,"subject":"[PATCH 0/4] Replace use of `test -o/a `","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-09T10:53:26Z","receivedAt":"2023-11-09T10:53:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series replaces all uses of `test -o/a ` that I\nwas able to find in our codebase. This is in accordance to our coding\nguidelines:\n\n - We do not write our \"test\" command with \"-a\" and \"-o\" and use \"&&\"\n or \"||\" to concatenate multiple \"test\" commands instead, because\n the use of \"-a/-o\" is often error-prone. E.g.\n\n test -n \"$x\" -a \"$a\" = \"$b\"\n\n is buggy and breaks when $x is \"=\", but\n\n test -n \"$x\" && test \"$a\" = \"$b\"\n\n does not have such a problem.\n\nThis patch series is a result of the discussion at [1].\n\nPatrick\n\n[1]: <20231109073250.GA2698227@coredump.intra.peff.net>\n\nPatrick Steinhardt (4):\n global: convert trivial usages of `test -a/-o `\n contrib/subtree: stop using `-o` to test for number of args\n contrib/subtree: convert subtree type check to use case statement\n Makefile: stop using `test -o` when unlinking duplicate executables\n\n GIT-VERSION-GEN | 2 +-\n Makefile | 2 +-\n configure.ac | 2 +-\n contrib/subtree/git-subtree.sh | 34 +++++++++++++++++++++++-----------\n t/perf/perf-lib.sh | 2 +-\n t/perf/run | 9 +++++----\n t/valgrind/valgrind.sh | 2 +-\n 7 files changed, 33 insertions(+), 20 deletions(-)\n\n-- \n2.42.0\n\n"},{"id":"484637","messageId":"c5e627eb3fef0df162da56723093a03bfd2321fb.1699526999.git.ps@pks.im","threadId":"60497","inReplyTo":"cover.1699526999.git.ps@pks.im","subject":"[PATCH 1/4] global: convert trivial usages of `test -a/-o `","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-09T10:53:31Z","receivedAt":"2023-11-09T10:53:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Our coding guidelines say to not use `test` with `-a` and `-o` because\nit can easily lead to bugs. Convert trivial cases where we still use\nthese to instead instead concatenate multiple invocations of `test` via\n`&&` and `||`, respectively.\n\nSigned-off-by: Patrick Steinhardt \n---\n GIT-VERSION-GEN | 2 +-\n configure.ac | 2 +-\n contrib/subtree/git-subtree.sh | 4 ++--\n t/perf/perf-lib.sh | 2 +-\n t/perf/run | 9 +++++----\n t/valgrind/valgrind.sh | 2 +-\n 6 files changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN\nindex e54492f8271..e4d31cbbd6a 100755\n--- a/GIT-VERSION-GEN\n+++ b/GIT-VERSION-GEN\n@@ -11,7 +11,7 @@ LF='\n if test -f version\n then\n \tVN=$(cat version) || VN=\"$DEF_VER\"\n-elif test -d ${GIT_DIR:-.git} -o -f .git &&\n+elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&\n \tVN=$(git describe --match \"v[0-9]*\" HEAD 2>/dev/null) &&\n \tcase \"$VN\" in\n \t*$LF*) (exit 1) ;;\ndiff --git a/configure.ac b/configure.ac\nindex 276593cd9dd..d1a96da14eb 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -94,7 +94,7 @@ AC_DEFUN([GIT_PARSE_WITH_SET_MAKE_VAR],\n [AC_ARG_WITH([$1],\n [AS_HELP_STRING([--with-$1=VALUE], $3)],\n if test -n \"$withval\"; then\n- if test \"$withval\" = \"yes\" -o \"$withval\" = \"no\"; then\n+ if test \"$withval\" = \"yes\" || test \"$withval\" = \"no\"; then\n AC_MSG_WARN([You likely do not want either 'yes' or 'no' as]\n \t\t [a value for $1 ($2). Maybe you do...?])\n fi\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex e0c5d3b0de6..43b5fec7320 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -489,13 +489,13 @@ find_existing_splits () {\n \t\t\t;;\n \t\tEND)\n \t\t\tdebug \"Main is: '$main'\"\n-\t\t\tif test -z \"$main\" -a -n \"$sub\"\n+\t\t\tif test -z \"$main\" && test -n \"$sub\"\n \t\t\tthen\n \t\t\t\t# squash commits refer to a subtree\n \t\t\t\tdebug \" Squash: $sq from $sub\"\n \t\t\t\tcache_set \"$sq\" \"$sub\"\n \t\t\tfi\n-\t\t\tif test -n \"$main\" -a -n \"$sub\"\n+\t\t\tif test -n \"$main\" && test -n \"$sub\"\n \t\t\tthen\n \t\t\t\tdebug \" Prior: $main -> $sub\"\n \t\t\t\tcache_set $main $sub\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex e7786775a90..b952e5024b4 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -31,7 +31,7 @@ unset GIT_CONFIG_NOSYSTEM\n GIT_CONFIG_SYSTEM=\"$TEST_DIRECTORY/perf/config\"\n export GIT_CONFIG_SYSTEM\n \n-if test -n \"$GIT_TEST_INSTALLED\" -a -z \"$PERF_SET_GIT_TEST_INSTALLED\"\n+if test -n \"$GIT_TEST_INSTALLED\" && test -z \"$PERF_SET_GIT_TEST_INSTALLED\"\n then\n \terror \"Do not use GIT_TEST_INSTALLED with the perf tests.\n \ndiff --git a/t/perf/run b/t/perf/run\nindex 34115edec35..486ead21980 100755\n--- a/t/perf/run\n+++ b/t/perf/run\n@@ -91,10 +91,10 @@ set_git_test_installed () {\n run_dirs_helper () {\n \tmydir=${1%/}\n \tshift\n-\twhile test $# -gt 0 -a \"$1\" != -- -a ! -f \"$1\"; do\n+\twhile test $# -gt 0 && test \"$1\" != -- && test ! -f \"$1\"; do\n \t\tshift\n \tdone\n-\tif test $# -gt 0 -a \"$1\" = --; then\n+\tif test $# -gt 0 && test \"$1\" = --; then\n \t\tshift\n \tfi\n \n@@ -124,7 +124,7 @@ run_dirs_helper () {\n }\n \n run_dirs () {\n-\twhile test $# -gt 0 -a \"$1\" != -- -a ! -f \"$1\"; do\n+\twhile test $# -gt 0 && test \"$1\" != -- && test ! -f \"$1\"; do\n \t\trun_dirs_helper \"$@\"\n \t\tshift\n \tdone\n@@ -180,7 +180,8 @@ run_subsection () {\n \tGIT_PERF_AGGREGATING_LATER=t\n \texport GIT_PERF_AGGREGATING_LATER\n \n-\tif test $# = 0 -o \"$1\" = -- -o -f \"$1\"; then\n+\tif test $# = 0 || test \"$1\" = -- || test -f \"$1\"\n+\tthen\n \t\tset -- . \"$@\"\n \tfi\n \ndiff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh\nindex 669ebaf68be..9fbf90cee7c 100755\n--- a/t/valgrind/valgrind.sh\n+++ b/t/valgrind/valgrind.sh\n@@ -23,7 +23,7 @@ memcheck)\n \tVALGRIND_MAJOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*\\([0-9]*\\)')\n \tVALGRIND_MINOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*[0-9]*\\.\\([0-9]*\\)')\n \ttest 3 -gt \"$VALGRIND_MAJOR\" ||\n-\ttest 3 -eq \"$VALGRIND_MAJOR\" -a 4 -gt \"$VALGRIND_MINOR\" ||\n+\t( test 3 -eq \"$VALGRIND_MAJOR\" && test 4 -gt \"$VALGRIND_MINOR\" ) ||\n \tTOOL_OPTIONS=\"$TOOL_OPTIONS --track-origins=yes\"\n \t;;\n *)\n-- \n2.42.0\n\n"},{"id":"484638","messageId":"b1ea45b8a8884d09ab070bb0f099834447d28938.1699526999.git.ps@pks.im","threadId":"60497","inReplyTo":"cover.1699526999.git.ps@pks.im","subject":"[PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-09T10:53:35Z","receivedAt":"2023-11-09T10:53:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Functions in git-subtree.sh all assert that they are being passed the\ncorrect number of arguments. In cases where we accept a variable number\nof arguments we assert this via a single call to `test` with `-o`, which\nis discouraged by our coding guidelines.\n\nConvert these cases to stop doing so.\n\nSigned-off-by: Patrick Steinhardt \n---\n contrib/subtree/git-subtree.sh | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 43b5fec7320..8af0a81ba3f 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -373,7 +373,8 @@ try_remove_previous () {\n \n # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]\n process_subtree_split_trailer () {\n-\tassert test $# = 2 -o $# = 3\n+\tassert test $# -ge 2\n+\tassert test $# -le 3\n \tb=\"$1\"\n \tsq=\"$2\"\n \trepository=\"\"\n@@ -402,7 +403,8 @@ process_subtree_split_trailer () {\n \n # Usage: find_latest_squash DIR [REPOSITORY]\n find_latest_squash () {\n-\tassert test $# = 1 -o $# = 2\n+\tassert test $# -ge 1\n+\tassert test $# -le 2\n \tdir=\"$1\"\n \trepository=\"\"\n \tif test \"$#\" = 2\n@@ -455,7 +457,8 @@ find_latest_squash () {\n \n # Usage: find_existing_splits DIR REV [REPOSITORY]\n find_existing_splits () {\n-\tassert test $# = 2 -o $# = 3\n+\tassert test $# -ge 2\n+\tassert test $# -le 3\n \tdebug \"Looking for prior splits...\"\n \tlocal indent=$(($indent + 1))\n \n@@ -916,7 +919,7 @@ cmd_split () {\n \tif test $# -eq 0\n \tthen\n \t\trev=$(git rev-parse HEAD)\n-\telif test $# -eq 1 -o $# -eq 2\n+\telif test $# -eq 1 || test $# -eq 2\n \tthen\n \t\trev=$(git rev-parse -q --verify \"$1^{commit}\") ||\n \t\t\tdie \"fatal: '$1' does not refer to a commit\"\n@@ -1006,8 +1009,11 @@ cmd_split () {\n \n # Usage: cmd_merge REV [REPOSITORY]\n cmd_merge () {\n-\ttest $# -eq 1 -o $# -eq 2 ||\n+\tif test $# -lt 1 || test $# -gt 2\n+\tthen\n \t\tdie \"fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'\"\n+\tfi\n+\n \trev=$(git rev-parse -q --verify \"$1^{commit}\") ||\n \t\tdie \"fatal: '$1' does not refer to a commit\"\n \trepository=\"\"\n-- \n2.42.0\n\n"},{"id":"484639","messageId":"7c54d9070fac15b8f0504251d920d0e1fc1fb1f4.1699526999.git.ps@pks.im","threadId":"60497","inReplyTo":"cover.1699526999.git.ps@pks.im","subject":"[PATCH 3/4] contrib/subtree: convert subtree type check to use case statement","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-09T10:53:39Z","receivedAt":"2023-11-09T10:53:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `subtree_for_commit ()` helper function asserts that the subtree\nidentified by its parameters are either a commit or tree. This is done\nvia the `-o` parameter of test, which is discouraged.\n\nRefactor the code to instead use a switch statement over the type.\nDespite being aligned with our coding guidelines, the resulting code is\narguably also easier to read.\n\nSigned-off-by: Patrick Steinhardt \n---\n contrib/subtree/git-subtree.sh | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 8af0a81ba3f..3028029ac2d 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -641,10 +641,16 @@ subtree_for_commit () {\n \twhile read mode type tree name\n \tdo\n \t\tassert test \"$name\" = \"$dir\"\n-\t\tassert test \"$type\" = \"tree\" -o \"$type\" = \"commit\"\n-\t\ttest \"$type\" = \"commit\" && continue # ignore submodules\n-\t\techo $tree\n-\t\tbreak\n+\n+\t\tcase \"$type\" in\n+\t\tcommit)\n+\t\t\tcontinue;; # ignore submodules\n+\t\ttree)\n+\t\t\techo $tree\n+\t\t\tbreak;;\n+\t\t*)\n+\t\t\tdie \"fatal: tree entry is of type ${type}, expected tree or commit\";;\n+\t\tesac\n \tdone || exit $?\n }\n \n-- \n2.42.0\n\n"},{"id":"484640","messageId":"bc9489ca5b815b1a83e24771f1d7449b865a7e02.1699526999.git.ps@pks.im","threadId":"60497","inReplyTo":"cover.1699526999.git.ps@pks.im","subject":"[PATCH 4/4] Makefile: stop using `test -o` when unlinking duplicate executables","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-09T10:53:45Z","receivedAt":"2023-11-09T10:53:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When building executables we may end up with both `foo` and `foo.exe` in\nthe project's root directory. This can cause issues on Cygwin, which is\nwhy we unlink the `foo` binary (see 6fc301bbf68 (Makefile: remove $foo\nwhen $foo.exe is built/installed., 2007-01-10)). This step is skipped if\neither:\n\n - `foo` is a directory, which can happen when building Git on\n Windows via MSVC (see ade2ca0ca9f (Do not try to remove\n directories when removing old links, 2009-10-27)).\n\n - `foo` is a hardlink to `foo.exe`, which can happen on Cygwin (see\n 0d768f7c8f1 (Makefile: building git in cygwin 1.7.0, 2008-08-15)).\n\nThese two conditions are currently chained together via `test -o`, which\nis discouraged by our code style guide. Convert the recipe to instead\nuse an `if` statement with `&&`'d conditions, which both matches our\nstyle guide and is easier to ready.\n\nSigned-off-by: Patrick Steinhardt \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 03adcb5a480..1094a557711 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2342,7 +2342,7 @@ profile-fast: profile-clean\n \n all:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS\n ifneq (,$X)\n-\t$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)\n+\t$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), if test ! -d '$p' && test ! '$p' -ef '$p$X'; then $(RM) '$p'; fi;)\n endif\n \n all::\n-- \n2.42.0\n\n"},{"id":"484642","messageId":"xmqqpm0jyx02.fsf@gitster.g","threadId":"60497","inReplyTo":"c5e627eb3fef0df162da56723093a03bfd2321fb.1699526999.git.ps@pks.im","subject":"Re: [PATCH 1/4] global: convert trivial usages of `test -a/-o `","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-09T11:41:33Z","receivedAt":"2023-11-09T11:41:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN\n> index e54492f8271..e4d31cbbd6a 100755\n> --- a/GIT-VERSION-GEN\n> +++ b/GIT-VERSION-GEN\n> @@ -11,7 +11,7 @@ LF='\n> if test -f version\n> then\n> \tVN=$(cat version) || VN=\"$DEF_VER\"\n> -elif test -d ${GIT_DIR:-.git} -o -f .git &&\n> +elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&\n\nI do not think this is strictly necessary.\n\nBecause the command line parser of \"test\" comes from left, notices\n\"-d\" and takes the next one to check if it is a directory. There is\nno value in ${GIT_DIR} can make \"test -d ${GIT_DIR} -o ...\" fail the\nsame way as the problem Peff pointed out during the discussion.\n\nI do not need a subshell for grouping, either. Plain {} should do\n(but you may need a LF or semicolon after the statement)..\n\n> diff --git a/configure.ac b/configure.ac\n> index 276593cd9dd..d1a96da14eb 100644\n> --- a/configure.ac\n> +++ b/configure.ac\n> @@ -94,7 +94,7 @@ AC_DEFUN([GIT_PARSE_WITH_SET_MAKE_VAR],\n> [AC_ARG_WITH([$1],\n> [AS_HELP_STRING([--with-$1=VALUE], $3)],\n> if test -n \"$withval\"; then\n> - if test \"$withval\" = \"yes\" -o \"$withval\" = \"no\"; then\n> + if test \"$withval\" = \"yes\" || test \"$withval\" = \"no\"; then\n\nThis is correct and is in line with the way generated ./configure\nprotects \"if $withval is yes or no, then do this\" against a funny\nvalue like \"-f\" in \"$withval\" breaking the parsing.\n\n> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\n> index e0c5d3b0de6..43b5fec7320 100755\n> --- a/contrib/subtree/git-subtree.sh\n> +++ b/contrib/subtree/git-subtree.sh\n> @@ -489,13 +489,13 @@ find_existing_splits () {\n> \t\t\t;;\n> \t\tEND)\n> \t\t\tdebug \"Main is: '$main'\"\n> -\t\t\tif test -z \"$main\" -a -n \"$sub\"\n> +\t\t\tif test -z \"$main\" && test -n \"$sub\"\n\nThis should be OK as-is, for the same reason \"-d\" operator would not\nbe broken by arbitrary operand..\n\n> \t\t\tthen\n> \t\t\t\t# squash commits refer to a subtree\n> \t\t\t\tdebug \" Squash: $sq from $sub\"\n> \t\t\t\tcache_set \"$sq\" \"$sub\"\n> \t\t\tfi\n> -\t\t\tif test -n \"$main\" -a -n \"$sub\"\n> +\t\t\tif test -n \"$main\" && test -n \"$sub\"\n\nDitto.\n\n> \t\t\tthen\n> \t\t\t\tdebug \" Prior: $main -> $sub\"\n> \t\t\t\tcache_set $main $sub\n> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> index e7786775a90..b952e5024b4 100644\n> --- a/t/perf/perf-lib.sh\n> +++ b/t/perf/perf-lib.sh\n> @@ -31,7 +31,7 @@ unset GIT_CONFIG_NOSYSTEM\n> GIT_CONFIG_SYSTEM=\"$TEST_DIRECTORY/perf/config\"\n> export GIT_CONFIG_SYSTEM\n> \n> -if test -n \"$GIT_TEST_INSTALLED\" -a -z \"$PERF_SET_GIT_TEST_INSTALLED\"\n> +if test -n \"$GIT_TEST_INSTALLED\" && test -z \"$PERF_SET_GIT_TEST_INSTALLED\"\n\nDitto.\n\n> then\n> \terror \"Do not use GIT_TEST_INSTALLED with the perf tests.\n> \n> diff --git a/t/perf/run b/t/perf/run\n> index 34115edec35..486ead21980 100755\n> --- a/t/perf/run\n> +++ b/t/perf/run\n> @@ -91,10 +91,10 @@ set_git_test_installed () {\n> run_dirs_helper () {\n> \tmydir=${1%/}\n> \tshift\n> -\twhile test $# -gt 0 -a \"$1\" != -- -a ! -f \"$1\"; do\n> +\twhile test $# -gt 0 && test \"$1\" != -- && test ! -f \"$1\"; do\n\nAs \"$1\" could be anything (including an insanity like \"-n\"), this\nchange is prudent.\n\n> \t\tshift\n> \tdone\n> -\tif test $# -gt 0 -a \"$1\" = --; then\n> +\tif test $# -gt 0 && test \"$1\" = --; then\n\nDitto.\n\n> \t\tshift\n> \tfi\n> \n> @@ -124,7 +124,7 @@ run_dirs_helper () {\n> }\n> \n> run_dirs () {\n> -\twhile test $# -gt 0 -a \"$1\" != -- -a ! -f \"$1\"; do\n> +\twhile test $# -gt 0 && test \"$1\" != -- && test ! -f \"$1\"; do\n> \t\trun_dirs_helper \"$@\"\n> \t\tshift\n> \tdone\n> @@ -180,7 +180,8 @@ run_subsection () {\n> \tGIT_PERF_AGGREGATING_LATER=t\n> \texport GIT_PERF_AGGREGATING_LATER\n> \n> -\tif test $# = 0 -o \"$1\" = -- -o -f \"$1\"; then\n> +\tif test $# = 0 || test \"$1\" = -- || test -f \"$1\"\n> +\tthen\n\nDitto.\n\n> \t\tset -- . \"$@\"\n> \tfi\n> \n> diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh\n> index 669ebaf68be..9fbf90cee7c 100755\n> --- a/t/valgrind/valgrind.sh\n> +++ b/t/valgrind/valgrind.sh\n> @@ -23,7 +23,7 @@ memcheck)\n> \tVALGRIND_MAJOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*\\([0-9]*\\)')\n> \tVALGRIND_MINOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*[0-9]*\\.\\([0-9]*\\)')\n> \ttest 3 -gt \"$VALGRIND_MAJOR\" ||\n> -\ttest 3 -eq \"$VALGRIND_MAJOR\" -a 4 -gt \"$VALGRIND_MINOR\" ||\n> +\t( test 3 -eq \"$VALGRIND_MAJOR\" && test 4 -gt \"$VALGRIND_MINOR\" ) ||\n\nThis should be OK as-is; once \"3 --eq\" is parsed the parameter\nreference would not be taken as anything but just a value.\n\n> \tTOOL_OPTIONS=\"$TOOL_OPTIONS --track-origins=yes\"\n> \t;;\n> *)\n"},{"id":"484654","messageId":"20231109184843.GC2711684@coredump.intra.peff.net","threadId":"60497","inReplyTo":"xmqqpm0jyx02.fsf@gitster.g","subject":"Re: [PATCH 1/4] global: convert trivial usages of `test -a/-o `","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T18:48:43Z","receivedAt":"2023-11-09T18:48:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 09, 2023 at 08:41:33PM +0900, Junio C Hamano wrote:\n\n> > -elif test -d ${GIT_DIR:-.git} -o -f .git &&\n> > +elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&\n> \n> I do not think this is strictly necessary.\n> \n> Because the command line parser of \"test\" comes from left, notices\n> \"-d\" and takes the next one to check if it is a directory. There is\n> no value in ${GIT_DIR} can make \"test -d ${GIT_DIR} -o ...\" fail the\n> same way as the problem Peff pointed out during the discussion.\n\nI think this is one of the ambiguous cases. If $GIT_DIR is \"=\", then\n\"test\" cannot tell if you meant:\n\n var1=-d\n var2=-o\n test \"$var1\" = \"$var2\" ...\n\nor:\n\n var1=\"=\"\n test -d \"$var1\" -o ...\n\nWith bash, for example:\n\n $ test -d /tmp -o -f .git; echo $?\n 0\n $ test -d = -o -f .git; echo $?\n bash: test: syntax error: `-f' unexpected\n 2\n\nWithout \"-o\", it uses the number of arguments to disambiguate (though of\ncourse the lack of quotes around $GIT_DIR is another potential problem\nhere).\n\nAnd I think the same is true of the other cases below using \"-z\", \"-n\",\nand so on.\n\nBut IMHO it is worth getting rid of all -o/-a regardless. Even\nnon-ambiguous cases make reasoning about the code harder, and we don't\nwant to encourage people to think they're OK to use.\n\n> I do not need a subshell for grouping, either. Plain {} should do\n> (but you may need a LF or semicolon after the statement)..\n\nThis I definitely agree with. :)\n\n-Peff\n"},{"id":"484659","messageId":"20231109185515.GD2711684@coredump.intra.peff.net","threadId":"60497","inReplyTo":"b1ea45b8a8884d09ab070bb0f099834447d28938.1699526999.git.ps@pks.im","subject":"Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T18:55:15Z","receivedAt":"2023-11-09T18:55:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 09, 2023 at 11:53:35AM +0100, Patrick Steinhardt wrote:\n\n> Functions in git-subtree.sh all assert that they are being passed the\n> correct number of arguments. In cases where we accept a variable number\n> of arguments we assert this via a single call to `test` with `-o`, which\n> is discouraged by our coding guidelines.\n> \n> Convert these cases to stop doing so.\n\nOK. I think these ones really are safe, because they're only expanding\n$#, but I agree with the principle to follow the guidelines.\n\n> # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]\n> process_subtree_split_trailer () {\n> -\tassert test $# = 2 -o $# = 3\n> +\tassert test $# -ge 2\n> +\tassert test $# -le 3\n\nIt took me a minute to figure out why we were swapping \"=\" for \"-ge\". It\nis because we want to logical-OR the two conditions, but \"assert\"\nrequires that we test one at a time. I think that is probably worth\nexplaining in the commit message.\n\n> @@ -916,7 +919,7 @@ cmd_split () {\n> \tif test $# -eq 0\n> \tthen\n> \t\trev=$(git rev-parse HEAD)\n> -\telif test $# -eq 1 -o $# -eq 2\n> +\telif test $# -eq 1 || test $# -eq 2\n\nOK, this one is a straight-forward use of \"||\".\n\n> cmd_merge () {\n> -\ttest $# -eq 1 -o $# -eq 2 ||\n> +\tif test $# -lt 1 || test $# -gt 2\n> +\tthen\n> \t\tdie \"fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'\"\n> +\tfi\n> +\n\nBut here we swap \"-eq\" for other operators. We have to because we went\nfrom \"||\" to an \"if\". I think what you have here is correct, but you\ncould also write:\n\n if ! { test $# -eq 1 || test $# -eq 2; }\n\n(I am OK with either, it just took me a minute to verify that your\nconversion was correct. But that is a one-time issue now while\nreviewing, and I think the code is readable going forward).\n\n-Peff\n"},{"id":"484660","messageId":"20231109185642.GE2711684@coredump.intra.peff.net","threadId":"60497","inReplyTo":"7c54d9070fac15b8f0504251d920d0e1fc1fb1f4.1699526999.git.ps@pks.im","subject":"Re: [PATCH 3/4] contrib/subtree: convert subtree type check to use case statement","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-09T18:56:42Z","receivedAt":"2023-11-09T18:56:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 09, 2023 at 11:53:39AM +0100, Patrick Steinhardt wrote:\n\n> The `subtree_for_commit ()` helper function asserts that the subtree\n> identified by its parameters are either a commit or tree. This is done\n> via the `-o` parameter of test, which is discouraged.\n> \n> Refactor the code to instead use a switch statement over the type.\n> Despite being aligned with our coding guidelines, the resulting code is\n> arguably also easier to read.\n\nYes, I'd agree that the result is much easier to follow.\n\n-Peff\n"},{"id":"484677","messageId":"xmqqh6luzgc1.fsf@gitster.g","threadId":"60497","inReplyTo":"20231109184843.GC2711684@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] global: convert trivial usages of `test -a/-o `","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-09T22:56:14Z","receivedAt":"2023-11-09T22:56:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But IMHO it is worth getting rid of all -o/-a regardless. Even\n> non-ambiguous cases make reasoning about the code harder, and we don't\n> want to encourage people to think they're OK to use.\n\nYou're 100% correct. \"We do not have to worry about it\" is a very\nstrong incentive to go after.\n>\n>> I do not need a subshell for grouping, either. Plain {} should do\n>> (but you may need a LF or semicolon after the statement)..\n>\n> This I definitely agree with. :)\n\n;-).\n"},{"id":"484679","messageId":"xmqq8r76zg1j.fsf@gitster.g","threadId":"60497","inReplyTo":"20231109185515.GD2711684@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-09T23:02:32Z","receivedAt":"2023-11-09T23:02:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]\n>> process_subtree_split_trailer () {\n>> -\tassert test $# = 2 -o $# = 3\n>> +\tassert test $# -ge 2\n>> +\tassert test $# -le 3\n>\n> It took me a minute to figure out why we were swapping \"=\" for \"-ge\". It\n> is because we want to logical-OR the two conditions, but \"assert\"\n> requires that we test one at a time. I think that is probably worth\n> explaining in the commit message.\n\nI wish we could write something like\n\n\tassert test $# -ge 2 && test $# -le 3\n\n(and I'd allow double quoting the whole thing after assert if\nneeded) but we cannot do so without tweaking the implementation of\nassert.\n\n>\n>> @@ -916,7 +919,7 @@ cmd_split () {\n>> \tif test $# -eq 0\n>> \tthen\n>> \t\trev=$(git rev-parse HEAD)\n>> -\telif test $# -eq 1 -o $# -eq 2\n>> +\telif test $# -eq 1 || test $# -eq 2\n>\n> OK, this one is a straight-forward use of \"||\".\n\nYes, but why not consistently use the range notation like the\nearlier one here, or below?\n\n\telif test $# -ge 1 && test $# -le 2\n\n>> cmd_merge () {\n>> -\ttest $# -eq 1 -o $# -eq 2 ||\n>> +\tif test $# -lt 1 || test $# -gt 2\n>> ...\n> (I am OK with either, it just took me a minute to verify that your\n> conversion was correct. But that is a one-time issue now while\n> reviewing, and I think the code is readable going forward).\n\nYeah, the end result looks good.\n\nThanks, both.\n"},{"id":"484699","messageId":"cover.1699609940.git.ps@pks.im","threadId":"60497","inReplyTo":"cover.1699526999.git.ps@pks.im","subject":"[PATCH v2 0/4] Replace use of `test -o/a `","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-10T10:01:11Z","receivedAt":"2023-11-10T10:01:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that replaces all uses of\n`test -o/a -a/-o `\n contrib/subtree: stop using `-o` to test for number of args\n contrib/subtree: convert subtree type check to use case statement\n Makefile: stop using `test -o` when unlinking duplicate executables\n\n GIT-VERSION-GEN | 2 +-\n Makefile | 2 +-\n configure.ac | 2 +-\n contrib/subtree/git-subtree.sh | 34 +++++++++++++++++++++++-----------\n t/perf/perf-lib.sh | 2 +-\n t/perf/run | 9 +++++----\n t/valgrind/valgrind.sh | 2 +-\n 7 files changed, 33 insertions(+), 20 deletions(-)\n\nRange-diff against v1:\n1: c5e627eb3fe ! 1: 2967c8ebb46 global: convert trivial usages of `test -a/-o `\n @@ Commit message\n these to instead instead concatenate multiple invocations of `test` via\n `&&` and `||`, respectively.\n \n + While not all of the converted instances can cause ambiguity, it is\n + worth getting rid of all of them regardless:\n +\n + - It becomes easier to reason about the code as we do not have to\n + argue why one use of `-a`/`-o` is okay while another one isn't.\n +\n + - We don't encourage people to use these expressions.\n +\n Signed-off-by: Patrick Steinhardt \n \n ## GIT-VERSION-GEN ##\n @@ GIT-VERSION-GEN: LF='\n then\n \tVN=$(cat version) || VN=\"$DEF_VER\"\n -elif test -d ${GIT_DIR:-.git} -o -f .git &&\n -+elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&\n ++elif { test -d \"${GIT_DIR:-.git}\" || test -f .git; } &&\n \tVN=$(git describe --match \"v[0-9]*\" HEAD 2>/dev/null) &&\n \tcase \"$VN\" in\n \t*$LF*) (exit 1) ;;\n2: b1ea45b8a88 ! 2: 977132d2236 contrib/subtree: stop using `-o` to test for number of args\n @@ Commit message\n of arguments we assert this via a single call to `test` with `-o`, which\n is discouraged by our coding guidelines.\n \n - Convert these cases to stop doing so.\n + Convert these cases to stop doing so. This requires us to decompose\n + assertions of the style `assert test $# = 2 -o $# = 3` into two calls\n + because we have no easy way to logically chain statements passed to the\n + assert function.\n \n Signed-off-by: Patrick Steinhardt \n \n3: 7c54d9070fa = 3: 761cde1b341 contrib/subtree: convert subtree type check to use case statement\n4: bc9489ca5b8 = 4: 5326d86888a Makefile: stop using `test -o` when unlinking duplicate executables\n\nbase-commit: dadef801b365989099a9929e995589e455c51fed\n-- \n2.42.0\n\n"},{"id":"484700","messageId":"2967c8ebb460934eb4aaaaebe5941bff643d4a94.1699609940.git.ps@pks.im","threadId":"60497","inReplyTo":"cover.1699609940.git.ps@pks.im","subject":"[PATCH v2 1/4] global: convert trivial usages of `test -a/-o `","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-10T10:01:15Z","receivedAt":"2023-11-10T10:01:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Our coding guidelines say to not use `test` with `-a` and `-o` because\nit can easily lead to bugs. Convert trivial cases where we still use\nthese to instead instead concatenate multiple invocations of `test` via\n`&&` and `||`, respectively.\n\nWhile not all of the converted instances can cause ambiguity, it is\nworth getting rid of all of them regardless:\n\n - It becomes easier to reason about the code as we do not have to\n argue why one use of `-a`/`-o` is okay while another one isn't.\n\n - We don't encourage people to use these expressions.\n\nSigned-off-by: Patrick Steinhardt \n---\n GIT-VERSION-GEN | 2 +-\n configure.ac | 2 +-\n contrib/subtree/git-subtree.sh | 4 ++--\n t/perf/perf-lib.sh | 2 +-\n t/perf/run | 9 +++++----\n t/valgrind/valgrind.sh | 2 +-\n 6 files changed, 11 insertions(+), 10 deletions(-)\n\ndiff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN\nindex e54492f8271..7246ab7c78c 100755\n--- a/GIT-VERSION-GEN\n+++ b/GIT-VERSION-GEN\n@@ -11,7 +11,7 @@ LF='\n if test -f version\n then\n \tVN=$(cat version) || VN=\"$DEF_VER\"\n-elif test -d ${GIT_DIR:-.git} -o -f .git &&\n+elif { test -d \"${GIT_DIR:-.git}\" || test -f .git; } &&\n \tVN=$(git describe --match \"v[0-9]*\" HEAD 2>/dev/null) &&\n \tcase \"$VN\" in\n \t*$LF*) (exit 1) ;;\ndiff --git a/configure.ac b/configure.ac\nindex 276593cd9dd..d1a96da14eb 100644\n--- a/configure.ac\n+++ b/configure.ac\n@@ -94,7 +94,7 @@ AC_DEFUN([GIT_PARSE_WITH_SET_MAKE_VAR],\n [AC_ARG_WITH([$1],\n [AS_HELP_STRING([--with-$1=VALUE], $3)],\n if test -n \"$withval\"; then\n- if test \"$withval\" = \"yes\" -o \"$withval\" = \"no\"; then\n+ if test \"$withval\" = \"yes\" || test \"$withval\" = \"no\"; then\n AC_MSG_WARN([You likely do not want either 'yes' or 'no' as]\n \t\t [a value for $1 ($2). Maybe you do...?])\n fi\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex e0c5d3b0de6..43b5fec7320 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -489,13 +489,13 @@ find_existing_splits () {\n \t\t\t;;\n \t\tEND)\n \t\t\tdebug \"Main is: '$main'\"\n-\t\t\tif test -z \"$main\" -a -n \"$sub\"\n+\t\t\tif test -z \"$main\" && test -n \"$sub\"\n \t\t\tthen\n \t\t\t\t# squash commits refer to a subtree\n \t\t\t\tdebug \" Squash: $sq from $sub\"\n \t\t\t\tcache_set \"$sq\" \"$sub\"\n \t\t\tfi\n-\t\t\tif test -n \"$main\" -a -n \"$sub\"\n+\t\t\tif test -n \"$main\" && test -n \"$sub\"\n \t\t\tthen\n \t\t\t\tdebug \" Prior: $main -> $sub\"\n \t\t\t\tcache_set $main $sub\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex e7786775a90..b952e5024b4 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -31,7 +31,7 @@ unset GIT_CONFIG_NOSYSTEM\n GIT_CONFIG_SYSTEM=\"$TEST_DIRECTORY/perf/config\"\n export GIT_CONFIG_SYSTEM\n \n-if test -n \"$GIT_TEST_INSTALLED\" -a -z \"$PERF_SET_GIT_TEST_INSTALLED\"\n+if test -n \"$GIT_TEST_INSTALLED\" && test -z \"$PERF_SET_GIT_TEST_INSTALLED\"\n then\n \terror \"Do not use GIT_TEST_INSTALLED with the perf tests.\n \ndiff --git a/t/perf/run b/t/perf/run\nindex 34115edec35..486ead21980 100755\n--- a/t/perf/run\n+++ b/t/perf/run\n@@ -91,10 +91,10 @@ set_git_test_installed () {\n run_dirs_helper () {\n \tmydir=${1%/}\n \tshift\n-\twhile test $# -gt 0 -a \"$1\" != -- -a ! -f \"$1\"; do\n+\twhile test $# -gt 0 && test \"$1\" != -- && test ! -f \"$1\"; do\n \t\tshift\n \tdone\n-\tif test $# -gt 0 -a \"$1\" = --; then\n+\tif test $# -gt 0 && test \"$1\" = --; then\n \t\tshift\n \tfi\n \n@@ -124,7 +124,7 @@ run_dirs_helper () {\n }\n \n run_dirs () {\n-\twhile test $# -gt 0 -a \"$1\" != -- -a ! -f \"$1\"; do\n+\twhile test $# -gt 0 && test \"$1\" != -- && test ! -f \"$1\"; do\n \t\trun_dirs_helper \"$@\"\n \t\tshift\n \tdone\n@@ -180,7 +180,8 @@ run_subsection () {\n \tGIT_PERF_AGGREGATING_LATER=t\n \texport GIT_PERF_AGGREGATING_LATER\n \n-\tif test $# = 0 -o \"$1\" = -- -o -f \"$1\"; then\n+\tif test $# = 0 || test \"$1\" = -- || test -f \"$1\"\n+\tthen\n \t\tset -- . \"$@\"\n \tfi\n \ndiff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh\nindex 669ebaf68be..9fbf90cee7c 100755\n--- a/t/valgrind/valgrind.sh\n+++ b/t/valgrind/valgrind.sh\n@@ -23,7 +23,7 @@ memcheck)\n \tVALGRIND_MAJOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*\\([0-9]*\\)')\n \tVALGRIND_MINOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*[0-9]*\\.\\([0-9]*\\)')\n \ttest 3 -gt \"$VALGRIND_MAJOR\" ||\n-\ttest 3 -eq \"$VALGRIND_MAJOR\" -a 4 -gt \"$VALGRIND_MINOR\" ||\n+\t( test 3 -eq \"$VALGRIND_MAJOR\" && test 4 -gt \"$VALGRIND_MINOR\" ) ||\n \tTOOL_OPTIONS=\"$TOOL_OPTIONS --track-origins=yes\"\n \t;;\n *)\n-- \n2.42.0\n\n"},{"id":"484701","messageId":"977132d22366423fc0bc32af38ed027be3b1ba2c.1699609940.git.ps@pks.im","threadId":"60497","inReplyTo":"cover.1699609940.git.ps@pks.im","subject":"[PATCH v2 2/4] contrib/subtree: stop using `-o` to test for number of args","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-10T10:01:19Z","receivedAt":"2023-11-10T10:01:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Functions in git-subtree.sh all assert that they are being passed the\ncorrect number of arguments. In cases where we accept a variable number\nof arguments we assert this via a single call to `test` with `-o`, which\nis discouraged by our coding guidelines.\n\nConvert these cases to stop doing so. This requires us to decompose\nassertions of the style `assert test $# = 2 -o $# = 3` into two calls\nbecause we have no easy way to logically chain statements passed to the\nassert function.\n\nSigned-off-by: Patrick Steinhardt \n---\n contrib/subtree/git-subtree.sh | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 43b5fec7320..8af0a81ba3f 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -373,7 +373,8 @@ try_remove_previous () {\n \n # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]\n process_subtree_split_trailer () {\n-\tassert test $# = 2 -o $# = 3\n+\tassert test $# -ge 2\n+\tassert test $# -le 3\n \tb=\"$1\"\n \tsq=\"$2\"\n \trepository=\"\"\n@@ -402,7 +403,8 @@ process_subtree_split_trailer () {\n \n # Usage: find_latest_squash DIR [REPOSITORY]\n find_latest_squash () {\n-\tassert test $# = 1 -o $# = 2\n+\tassert test $# -ge 1\n+\tassert test $# -le 2\n \tdir=\"$1\"\n \trepository=\"\"\n \tif test \"$#\" = 2\n@@ -455,7 +457,8 @@ find_latest_squash () {\n \n # Usage: find_existing_splits DIR REV [REPOSITORY]\n find_existing_splits () {\n-\tassert test $# = 2 -o $# = 3\n+\tassert test $# -ge 2\n+\tassert test $# -le 3\n \tdebug \"Looking for prior splits...\"\n \tlocal indent=$(($indent + 1))\n \n@@ -916,7 +919,7 @@ cmd_split () {\n \tif test $# -eq 0\n \tthen\n \t\trev=$(git rev-parse HEAD)\n-\telif test $# -eq 1 -o $# -eq 2\n+\telif test $# -eq 1 || test $# -eq 2\n \tthen\n \t\trev=$(git rev-parse -q --verify \"$1^{commit}\") ||\n \t\t\tdie \"fatal: '$1' does not refer to a commit\"\n@@ -1006,8 +1009,11 @@ cmd_split () {\n \n # Usage: cmd_merge REV [REPOSITORY]\n cmd_merge () {\n-\ttest $# -eq 1 -o $# -eq 2 ||\n+\tif test $# -lt 1 || test $# -gt 2\n+\tthen\n \t\tdie \"fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'\"\n+\tfi\n+\n \trev=$(git rev-parse -q --verify \"$1^{commit}\") ||\n \t\tdie \"fatal: '$1' does not refer to a commit\"\n \trepository=\"\"\n-- \n2.42.0\n\n"},{"id":"484702","messageId":"761cde1b341e22b20063ce3baa6b70d80aa05168.1699609940.git.ps@pks.im","threadId":"60497","inReplyTo":"cover.1699609940.git.ps@pks.im","subject":"[PATCH v2 3/4] contrib/subtree: convert subtree type check to use case statement","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-10T10:01:24Z","receivedAt":"2023-11-10T10:01:30Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `subtree_for_commit ()` helper function asserts that the subtree\nidentified by its parameters are either a commit or tree. This is done\nvia the `-o` parameter of test, which is discouraged.\n\nRefactor the code to instead use a switch statement over the type.\nDespite being aligned with our coding guidelines, the resulting code is\narguably also easier to read.\n\nSigned-off-by: Patrick Steinhardt \n---\n contrib/subtree/git-subtree.sh | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh\nindex 8af0a81ba3f..3028029ac2d 100755\n--- a/contrib/subtree/git-subtree.sh\n+++ b/contrib/subtree/git-subtree.sh\n@@ -641,10 +641,16 @@ subtree_for_commit () {\n \twhile read mode type tree name\n \tdo\n \t\tassert test \"$name\" = \"$dir\"\n-\t\tassert test \"$type\" = \"tree\" -o \"$type\" = \"commit\"\n-\t\ttest \"$type\" = \"commit\" && continue # ignore submodules\n-\t\techo $tree\n-\t\tbreak\n+\n+\t\tcase \"$type\" in\n+\t\tcommit)\n+\t\t\tcontinue;; # ignore submodules\n+\t\ttree)\n+\t\t\techo $tree\n+\t\t\tbreak;;\n+\t\t*)\n+\t\t\tdie \"fatal: tree entry is of type ${type}, expected tree or commit\";;\n+\t\tesac\n \tdone || exit $?\n }\n \n-- \n2.42.0\n\n"},{"id":"484703","messageId":"5326d86888a46931e8893657bda9a2063a5cee3a.1699609940.git.ps@pks.im","threadId":"60497","inReplyTo":"cover.1699609940.git.ps@pks.im","subject":"[PATCH v2 4/4] Makefile: stop using `test -o` when unlinking duplicate executables","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-10T10:01:28Z","receivedAt":"2023-11-10T10:01:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When building executables we may end up with both `foo` and `foo.exe` in\nthe project's root directory. This can cause issues on Cygwin, which is\nwhy we unlink the `foo` binary (see 6fc301bbf68 (Makefile: remove $foo\nwhen $foo.exe is built/installed., 2007-01-10)). This step is skipped if\neither:\n\n - `foo` is a directory, which can happen when building Git on\n Windows via MSVC (see ade2ca0ca9f (Do not try to remove\n directories when removing old links, 2009-10-27)).\n\n - `foo` is a hardlink to `foo.exe`, which can happen on Cygwin (see\n 0d768f7c8f1 (Makefile: building git in cygwin 1.7.0, 2008-08-15)).\n\nThese two conditions are currently chained together via `test -o`, which\nis discouraged by our code style guide. Convert the recipe to instead\nuse an `if` statement with `&&`'d conditions, which both matches our\nstyle guide and is easier to ready.\n\nSigned-off-by: Patrick Steinhardt \n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 03adcb5a480..1094a557711 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2342,7 +2342,7 @@ profile-fast: profile-clean\n \n all:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS\n ifneq (,$X)\n-\t$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)\n+\t$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), if test ! -d '$p' && test ! '$p' -ef '$p$X'; then $(RM) '$p'; fi;)\n endif\n \n all::\n-- \n2.42.0\n\n"},{"id":"484704","messageId":"ZU4DeaoGMLGJXF0o@tanuki","threadId":"60497","inReplyTo":"20231109184843.GC2711684@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] global: convert trivial usages of `test -a/-o `","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-10T10:18:33Z","receivedAt":"2023-11-10T10:18:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Nov 09, 2023 at 01:48:43PM -0500, Jeff King wrote:\n> On Thu, Nov 09, 2023 at 08:41:33PM +0900, Junio C Hamano wrote:\n> \n> > > -elif test -d ${GIT_DIR:-.git} -o -f .git &&\n> > > +elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&\n> > \n> > I do not think this is strictly necessary.\n> > \n> > Because the command line parser of \"test\" comes from left, notices\n> > \"-d\" and takes the next one to check if it is a directory. There is\n> > no value in ${GIT_DIR} can make \"test -d ${GIT_DIR} -o ...\" fail the\n> > same way as the problem Peff pointed out during the discussion.\n> \n> I think this is one of the ambiguous cases. If $GIT_DIR is \"=\", then\n> \"test\" cannot tell if you meant:\n> \n> var1=-d\n> var2=-o\n> test \"$var1\" = \"$var2\" ...\n> \n> or:\n> \n> var1=\"=\"\n> test -d \"$var1\" -o ...\n> \n> With bash, for example:\n> \n> $ test -d /tmp -o -f .git; echo $?\n> 0\n> $ test -d = -o -f .git; echo $?\n> bash: test: syntax error: `-f' unexpected\n> 2\n> \n> Without \"-o\", it uses the number of arguments to disambiguate (though of\n> course the lack of quotes around $GIT_DIR is another potential problem\n> here).\n\nRight, let me fix the missing quoting while at it.\n\n> And I think the same is true of the other cases below using \"-z\", \"-n\",\n> and so on.\n> \n> But IMHO it is worth getting rid of all -o/-a regardless. Even\n> non-ambiguous cases make reasoning about the code harder, and we don't\n> want to encourage people to think they're OK to use.\n\nAgreed. I'll amend the commit message to say so.\n\n> > I do not need a subshell for grouping, either. Plain {} should do\n> > (but you may need a LF or semicolon after the statement)..\n> \n> This I definitely agree with. :)\n\nWill fix.\n\nPatrick\n"},{"id":"484705","messageId":"ZU4DgaVfLhSags-r@tanuki","threadId":"60497","inReplyTo":"20231109185515.GD2711684@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-10T10:18:41Z","receivedAt":"2023-11-10T10:18:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Nov 09, 2023 at 01:55:15PM -0500, Jeff King wrote:\n> On Thu, Nov 09, 2023 at 11:53:35AM +0100, Patrick Steinhardt wrote:\n> \n> > Functions in git-subtree.sh all assert that they are being passed the\n> > correct number of arguments. In cases where we accept a variable number\n> > of arguments we assert this via a single call to `test` with `-o`, which\n> > is discouraged by our coding guidelines.\n> > \n> > Convert these cases to stop doing so.\n> \n> OK. I think these ones really are safe, because they're only expanding\n> $#, but I agree with the principle to follow the guidelines.\n> \n> > # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]\n> > process_subtree_split_trailer () {\n> > -\tassert test $# = 2 -o $# = 3\n> > +\tassert test $# -ge 2\n> > +\tassert test $# -le 3\n> \n> It took me a minute to figure out why we were swapping \"=\" for \"-ge\". It\n> is because we want to logical-OR the two conditions, but \"assert\"\n> requires that we test one at a time. I think that is probably worth\n> explaining in the commit message.\n\nI really hate to admit how long I've pondered over this patch series in\ntotal, up to the point where I did a `git rebase --reset-author-date` at\nthe end just so that it's not obvious. So I totally get everyone who\nneeds to stop and think for a bit here.\n\nWill adapt the commit message.\n\nPatrick\n\n> > @@ -916,7 +919,7 @@ cmd_split () {\n> > \tif test $# -eq 0\n> > \tthen\n> > \t\trev=$(git rev-parse HEAD)\n> > -\telif test $# -eq 1 -o $# -eq 2\n> > +\telif test $# -eq 1 || test $# -eq 2\n> \n> OK, this one is a straight-forward use of \"||\".\n> \n> > cmd_merge () {\n> > -\ttest $# -eq 1 -o $# -eq 2 ||\n> > +\tif test $# -lt 1 || test $# -gt 2\n> > +\tthen\n> > \t\tdie \"fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'\"\n> > +\tfi\n> > +\n> \n> But here we swap \"-eq\" for other operators. We have to because we went\n> from \"||\" to an \"if\". I think what you have here is correct, but you\n> could also write:\n> \n> if ! { test $# -eq 1 || test $# -eq 2; }\n> \n> (I am OK with either, it just took me a minute to verify that your\n> conversion was correct. But that is a one-time issue now while\n> reviewing, and I think the code is readable going forward).\n"},{"id":"484706","messageId":"ZU4DiTQbKyuuT55k@tanuki","threadId":"60497","inReplyTo":"xmqq8r76zg1j.fsf@gitster.g","subject":"Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-10T10:18:49Z","receivedAt":"2023-11-10T10:18:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 10, 2023 at 08:02:32AM +0900, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> >> # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]\n> >> process_subtree_split_trailer () {\n> >> -\tassert test $# = 2 -o $# = 3\n> >> +\tassert test $# -ge 2\n> >> +\tassert test $# -le 3\n> >\n> > It took me a minute to figure out why we were swapping \"=\" for \"-ge\". It\n> > is because we want to logical-OR the two conditions, but \"assert\"\n> > requires that we test one at a time. I think that is probably worth\n> > explaining in the commit message.\n> \n> I wish we could write something like\n> \n> \tassert test $# -ge 2 && test $# -le 3\n> \n> (and I'd allow double quoting the whole thing after assert if\n> needed) but we cannot do so without tweaking the implementation of\n> assert.\n> \n> >\n> >> @@ -916,7 +919,7 @@ cmd_split () {\n> >> \tif test $# -eq 0\n> >> \tthen\n> >> \t\trev=$(git rev-parse HEAD)\n> >> -\telif test $# -eq 1 -o $# -eq 2\n> >> +\telif test $# -eq 1 || test $# -eq 2\n> >\n> > OK, this one is a straight-forward use of \"||\".\n> \n> Yes, but why not consistently use the range notation like the\n> earlier one here, or below?\n\nI opted to go for the \"obvious\" conversion, if there was one easily\navailable, to make the diff easier to read. We could of course use a\nrange notation like this:\n\n \t\trev=$(git rev-parse HEAD)\n-\telif test $# -eq 1 || test $# -eq 2\n+\telif test $# -ge 1 && test $# -le 2\n \tthen\n \t\trev=$(git rev-parse -q --verify \"$1^{commit}\") ||\n \t\t\tdie \"fatal: '$1' does not refer to a commit\"\n\nOr :\n\n \t\trev=$(git rev-parse HEAD)\n-\telif test $# -eq 1 || test $# -eq 2\n+\telif ! { test $# -lt 1 || test $# -gt 2; }\n \tthen\n \t\trev=$(git rev-parse -q --verify \"$1^{commit}\") ||\n \t\t\tdie \"fatal: '$1' does not refer to a commit\"\n\nBut both of these are not consistent with the other cases due to the\nnegation here, and both of them are harder to read in my opinion. So\nI'm not sure whether we gain anything by trying to make this a bit more\nconsistent with the other conversions.\n\nPatrick\n\n> \telif test $# -ge 1 && test $# -le 2\n> \n> >> cmd_merge () {\n> >> -\ttest $# -eq 1 -o $# -eq 2 ||\n> >> +\tif test $# -lt 1 || test $# -gt 2\n> >> ...\n> > (I am OK with either, it just took me a minute to verify that your\n> > conversion was correct. But that is a one-time issue now while\n> > reviewing, and I think the code is readable going forward).\n> \n> Yeah, the end result looks good.\n> \n> Thanks, both.\n"},{"id":"484716","messageId":"20231110214423.GC2758295@coredump.intra.peff.net","threadId":"60497","inReplyTo":"2967c8ebb460934eb4aaaaebe5941bff643d4a94.1699609940.git.ps@pks.im","subject":"Re: [PATCH v2 1/4] global: convert trivial usages of `test -a/-o `","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-10T21:44:23Z","receivedAt":"2023-11-10T21:44:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 10, 2023 at 11:01:15AM +0100, Patrick Steinhardt wrote:\n\n> diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh\n> index 669ebaf68be..9fbf90cee7c 100755\n> --- a/t/valgrind/valgrind.sh\n> +++ b/t/valgrind/valgrind.sh\n> @@ -23,7 +23,7 @@ memcheck)\n> \tVALGRIND_MAJOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*\\([0-9]*\\)')\n> \tVALGRIND_MINOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*[0-9]*\\.\\([0-9]*\\)')\n> \ttest 3 -gt \"$VALGRIND_MAJOR\" ||\n> -\ttest 3 -eq \"$VALGRIND_MAJOR\" -a 4 -gt \"$VALGRIND_MINOR\" ||\n> +\t( test 3 -eq \"$VALGRIND_MAJOR\" && test 4 -gt \"$VALGRIND_MINOR\" ) ||\n> \tTOOL_OPTIONS=\"$TOOL_OPTIONS --track-origins=yes\"\n\nI was surprised to see this one as a subshell after you adjusted the\nother. It probably isn't that big a deal either way, though (as a style\nthing I generally try to use braces unless I am relying on the separate\nenvironment provided by the subshell, but it's certainly not wrong in\nthis case).\n\n-Peff\n"},{"id":"484717","messageId":"20231110214640.GD2758295@coredump.intra.peff.net","threadId":"60497","inReplyTo":"cover.1699609940.git.ps@pks.im","subject":"Re: [PATCH v2 0/4] Replace use of `test -o/a `","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-11-10T21:46:40Z","receivedAt":"2023-11-10T21:46:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 10, 2023 at 11:01:11AM +0100, Patrick Steinhardt wrote:\n\n> this is the second version of my patch series that replaces all uses of\n> `test -o/a \n> Changes compared to v1:\n> \n> - I've expanded a bit on why we want to do these conversions in the\n> first place in the first commit message.\n> \n> - Dropped a needless subshell and added missing quoting while at it.\n> \n> - Explained why we need to decompose the asserts in the second patch\n> into two asserts.\n\nThese look OK to me. I mentioned a small nit on the first patch, but I\nam OK with ignoring it (and we are reaching diminishing returns\npolishing an otherwise trivial series).\n\n-Peff\n"},{"id":"484726","messageId":"xmqqpm0hur31.fsf@gitster.g","threadId":"60497","inReplyTo":"ZU4DiTQbKyuuT55k@tanuki","subject":"Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-10T23:27:30Z","receivedAt":"2023-11-10T23:27:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Fri, Nov 10, 2023 at 08:02:32AM +0900, Junio C Hamano wrote:\n>> Jeff King writes:\n>> \n>> >> # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]\n>> >> process_subtree_split_trailer () {\n>> >> -\tassert test $# = 2 -o $# = 3\n>> >> +\tassert test $# -ge 2\n>> >> +\tassert test $# -le 3\n>> > ...\n>> >> -\telif test $# -eq 1 -o $# -eq 2\n>> >> +\telif test $# -eq 1 || test $# -eq 2\n>> >\n>> > OK, this one is a straight-forward use of \"||\".\n>> \n>> Yes, but why not consistently use the range notation like the\n>> earlier one here, or below?\n>\n> I opted to go for the \"obvious\" conversion, if there was one easily\n> available, to make the diff easier to read.\n\n... and due to the limitation of \"assert\" we cannot do the obvious\n\n\ttest $# = 2 || test $# = 3\n\nand feed it to \"assert\" (and for equality with $#, = and -eq would\nwork equally fine, and = is much more readable, by the way).\n\nOK, then.\n"},{"id":"484734","messageId":"xmqqcywhtaff.fsf@gitster.g","threadId":"60497","inReplyTo":"2967c8ebb460934eb4aaaaebe5941bff643d4a94.1699609940.git.ps@pks.im","subject":"Re: [PATCH v2 1/4] global: convert trivial usages of `test -a/-o `","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-11T00:12:36Z","receivedAt":"2023-11-11T00:12:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Our coding guidelines say to not use `test` with `-a` and `-o` because\n> it can easily lead to bugs. Convert trivial cases where we still use\n> these to instead instead concatenate multiple invocations of `test` via\n> `&&` and `||`, respectively.\n>\n> While not all of the converted instances can cause ambiguity, it is\n> worth getting rid of all of them regardless:\n>\n> - It becomes easier to reason about the code as we do not have to\n> argue why one use of `-a`/`-o` is okay while another one isn't.\n>\n> - We don't encourage people to use these expressions.\n\nThanks for these additional notes. Nicely done.\n"},{"id":"484735","messageId":"xmqq34xdtac3.fsf@gitster.g","threadId":"60497","inReplyTo":"20231110214423.GC2758295@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/4] global: convert trivial usages of `test -a/-o `","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-11T00:14:36Z","receivedAt":"2023-11-11T00:14:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Nov 10, 2023 at 11:01:15AM +0100, Patrick Steinhardt wrote:\n>\n>> diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh\n>> index 669ebaf68be..9fbf90cee7c 100755\n>> --- a/t/valgrind/valgrind.sh\n>> +++ b/t/valgrind/valgrind.sh\n>> @@ -23,7 +23,7 @@ memcheck)\n>> \tVALGRIND_MAJOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*\\([0-9]*\\)')\n>> \tVALGRIND_MINOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*[0-9]*\\.\\([0-9]*\\)')\n>> \ttest 3 -gt \"$VALGRIND_MAJOR\" ||\n>> -\ttest 3 -eq \"$VALGRIND_MAJOR\" -a 4 -gt \"$VALGRIND_MINOR\" ||\n>> +\t( test 3 -eq \"$VALGRIND_MAJOR\" && test 4 -gt \"$VALGRIND_MINOR\" ) ||\n>> \tTOOL_OPTIONS=\"$TOOL_OPTIONS --track-origins=yes\"\n>\n> I was surprised to see this one as a subshell after you adjusted the\n> other.\n\n;-)\n\nI am not so surprised that this one was missed, though. I didn't\npoint this one out during my review of the previous round, either,\nand not everybody is as careful as you are.\n\nThanks, both.\n"},{"id":"484737","messageId":"xmqqr0kxrvij.fsf@gitster.g","threadId":"60497","inReplyTo":"xmqq34xdtac3.fsf@gitster.g","subject":"Re: [PATCH v2 1/4] global: convert trivial usages of `test -a/-o `","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-11T00:20:04Z","receivedAt":"2023-11-11T00:20:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I am not so surprised that this one was missed, though. I didn't\n> point this one out during my review of the previous round, either,\n> and not everybody is as careful as you are.\n\nAh, sorry, thist came out in a way I did not mean to.\n\nI didn't mean \"I did not point it out explicitly. It is not\nsurprising if a contributor who was not careful did not find it on\ntheir own and took initiative to fix it themselves\".\n\nI meant \"I failed to spot it myself hence I didn't point it out in\nmy review---I was not being so careful to aim for thoroughly cover\nand find all the similar issues\".\n\nIn any case, I'll tweak it while queueing. Thanks for noticing.\n\ndiff --git i/t/valgrind/valgrind.sh w/t/valgrind/valgrind.sh\nindex 9fbf90cee7..3c8ee19975 100755\n--- i/t/valgrind/valgrind.sh\n+++ w/t/valgrind/valgrind.sh\n@@ -23,7 +23,7 @@ memcheck)\n \tVALGRIND_MAJOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*\\([0-9]*\\)')\n \tVALGRIND_MINOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*[0-9]*\\.\\([0-9]*\\)')\n \ttest 3 -gt \"$VALGRIND_MAJOR\" ||\n-\t( test 3 -eq \"$VALGRIND_MAJOR\" && test 4 -gt \"$VALGRIND_MINOR\" ) ||\n+\t{ test 3 -eq \"$VALGRIND_MAJOR\" && test 4 -gt \"$VALGRIND_MINOR\"; } ||\n \tTOOL_OPTIONS=\"$TOOL_OPTIONS --track-origins=yes\"\n \t;;\n *)\n"},{"id":"484783","messageId":"ZVHMaoHW_nxzWnRb@tanuki","threadId":"60497","inReplyTo":"xmqqr0kxrvij.fsf@gitster.g","subject":"Re: [PATCH v2 1/4] global: convert trivial usages of `test -a/-o `","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-13T07:12:42Z","receivedAt":"2023-11-13T07:12:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Nov 11, 2023 at 09:20:04AM +0900, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > I am not so surprised that this one was missed, though. I didn't\n> > point this one out during my review of the previous round, either,\n> > and not everybody is as careful as you are.\n> \n> Ah, sorry, thist came out in a way I did not mean to.\n> \n> I didn't mean \"I did not point it out explicitly. It is not\n> surprising if a contributor who was not careful did not find it on\n> their own and took initiative to fix it themselves\".\n> \n> I meant \"I failed to spot it myself hence I didn't point it out in\n> my review---I was not being so careful to aim for thoroughly cover\n> and find all the similar issues\".\n> \n> In any case, I'll tweak it while queueing. Thanks for noticing.\n\nThanks indeed, I missed this instance as well when I scanned for any\nadditional subshells.\n\nPatrick\n\n> diff --git i/t/valgrind/valgrind.sh w/t/valgrind/valgrind.sh\n> index 9fbf90cee7..3c8ee19975 100755\n> --- i/t/valgrind/valgrind.sh\n> +++ w/t/valgrind/valgrind.sh\n> @@ -23,7 +23,7 @@ memcheck)\n> \tVALGRIND_MAJOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*\\([0-9]*\\)')\n> \tVALGRIND_MINOR=$(expr \"$VALGRIND_VERSION\" : '[^0-9]*[0-9]*\\.\\([0-9]*\\)')\n> \ttest 3 -gt \"$VALGRIND_MAJOR\" ||\n> -\t( test 3 -eq \"$VALGRIND_MAJOR\" && test 4 -gt \"$VALGRIND_MINOR\" ) ||\n> +\t{ test 3 -eq \"$VALGRIND_MAJOR\" && test 4 -gt \"$VALGRIND_MINOR\"; } ||\n> \tTOOL_OPTIONS=\"$TOOL_OPTIONS --track-origins=yes\"\n> \t;;\n> *)\n"}]}