[PATCH 0/4] Replace use of `test <expr> -o/a <expr>`

[PATCH 0/4] Replace use of `test <expr> -o/a <expr>`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]

Hi,

this patch series replaces all uses of `test <expr> -o/a <expr>` that I
was able to find in our codebase. This is in accordance to our coding
guidelines:

 - We do not write our "test" command with "-a" and "-o" and use "&&"
   or "||" to concatenate multiple "test" commands instead, because
   the use of "-a/-o" is often error-prone.  E.g.

     test -n "$x" -a "$a" = "$b"

   is buggy and breaks when $x is "=", but

     test -n "$x" && test "$a" = "$b"

   does not have such a problem.

This patch series is a result of the discussion at [1].

Patrick

[1]: <20231109073250.GA2698227@coredump.intra.peff.net>

Patrick Steinhardt (4):
  global: convert trivial usages of `test <expr> -a/-o <expr>`
  contrib/subtree: stop using `-o` to test for number of args
  contrib/subtree: convert subtree type check to use case statement
  Makefile: stop using `test -o` when unlinking duplicate executables

 GIT-VERSION-GEN                |  2 +-
 Makefile                       |  2 +-
 configure.ac                   |  2 +-
 contrib/subtree/git-subtree.sh | 34 +++++++++++++++++++++++-----------
 t/perf/perf-lib.sh             |  2 +-
 t/perf/run                     |  9 +++++----
 t/valgrind/valgrind.sh         |  2 +-
 7 files changed, 33 insertions(+), 20 deletions(-)

-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4188 bytes --]

Our coding guidelines say to not use `test` with `-a` and `-o` because
it can easily lead to bugs. Convert trivial cases where we still use
these to instead instead concatenate multiple invocations of `test` via
`&&` and `||`, respectively.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 GIT-VERSION-GEN                | 2 +-
 configure.ac                   | 2 +-
 contrib/subtree/git-subtree.sh | 4 ++--
 t/perf/perf-lib.sh             | 2 +-
 t/perf/run                     | 9 +++++----
 t/valgrind/valgrind.sh         | 2 +-
 6 files changed, 11 insertions(+), 10 deletions(-)

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 ) &&
 	VN=$(git describe --match "v[0-9]*" HEAD 2>/dev/null) &&
 	case "$VN" in
 	*$LF*) (exit 1) ;;
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
     AC_MSG_WARN([You likely do not want either 'yes' or 'no' as]
 		     [a value for $1 ($2).  Maybe you do...?])
   fi
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"
 			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"
 			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"
 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
 		shift
 	done
-	if test $# -gt 0 -a "$1" = --; then
+	if test $# -gt 0 && test "$1" = --; then
 		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
 		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" ) ||
 	TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
 	;;
 *)
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> 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)..

> 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.

> 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..

>  			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.

>  			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.

>  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.

>  		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.

>  		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"
>  	;;
>  *)

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

[Jeff King]

On Thu, Nov 09, 2023 at 08:41:33PM +0900, Junio C Hamano wrote:

> > -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 think this is one of the ambiguous cases. If $GIT_DIR is "=", then
"test" cannot tell if you meant:

  var1=-d
  var2=-o
  test "$var1" = "$var2" ...

or:

  var1="="
  test -d "$var1" -o ...

With bash, for example:

  $ test -d /tmp -o -f .git; echo $?
  0
  $ test -d = -o -f .git; echo $?
  bash: test: syntax error: `-f' unexpected
  2

Without "-o", it uses the number of arguments to disambiguate (though of
course the lack of quotes around $GIT_DIR is another potential problem
here).

And I think the same is true of the other cases below using "-z", "-n",
and so on.

But IMHO it is worth getting rid of all -o/-a regardless. Even
non-ambiguous cases make reasoning about the code harder, and we don't
want to encourage people to think they're OK to use.

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

This I definitely agree with. :)

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But IMHO it is worth getting rid of all -o/-a regardless. Even
> non-ambiguous cases make reasoning about the code harder, and we don't
> want to encourage people to think they're OK to use.

You're 100% correct.  "We do not have to worry about it" is a very
strong incentive to go after.
>
>> I do not need a subshell for grouping, either.  Plain {} should do
>> (but you may need a LF or semicolon after the statement)..
>
> This I definitely agree with. :)

;-).

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]

On Thu, Nov 09, 2023 at 01:48:43PM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 08:41:33PM +0900, Junio C Hamano wrote:
> 
> > > -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 think this is one of the ambiguous cases. If $GIT_DIR is "=", then
> "test" cannot tell if you meant:
> 
>   var1=-d
>   var2=-o
>   test "$var1" = "$var2" ...
> 
> or:
> 
>   var1="="
>   test -d "$var1" -o ...
> 
> With bash, for example:
> 
>   $ test -d /tmp -o -f .git; echo $?
>   0
>   $ test -d = -o -f .git; echo $?
>   bash: test: syntax error: `-f' unexpected
>   2
> 
> Without "-o", it uses the number of arguments to disambiguate (though of
> course the lack of quotes around $GIT_DIR is another potential problem
> here).

Right, let me fix the missing quoting while at it.

> And I think the same is true of the other cases below using "-z", "-n",
> and so on.
> 
> But IMHO it is worth getting rid of all -o/-a regardless. Even
> non-ambiguous cases make reasoning about the code harder, and we don't
> want to encourage people to think they're OK to use.

Agreed. I'll amend the commit message to say so.

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

Will fix.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]

Functions in git-subtree.sh all assert that they are being passed the
correct number of arguments. In cases where we accept a variable number
of arguments we assert this via a single call to `test` with `-o`, which
is discouraged by our coding guidelines.

Convert these cases to stop doing so.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 contrib/subtree/git-subtree.sh | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 43b5fec7320..8af0a81ba3f 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -373,7 +373,8 @@ try_remove_previous () {
 
 # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
 process_subtree_split_trailer () {
-	assert test $# = 2 -o $# = 3
+	assert test $# -ge 2
+	assert test $# -le 3
 	b="$1"
 	sq="$2"
 	repository=""
@@ -402,7 +403,8 @@ process_subtree_split_trailer () {
 
 # Usage: find_latest_squash DIR [REPOSITORY]
 find_latest_squash () {
-	assert test $# = 1 -o $# = 2
+	assert test $# -ge 1
+	assert test $# -le 2
 	dir="$1"
 	repository=""
 	if test "$#" = 2
@@ -455,7 +457,8 @@ find_latest_squash () {
 
 # Usage: find_existing_splits DIR REV [REPOSITORY]
 find_existing_splits () {
-	assert test $# = 2 -o $# = 3
+	assert test $# -ge 2
+	assert test $# -le 3
 	debug "Looking for prior splits..."
 	local indent=$(($indent + 1))
 
@@ -916,7 +919,7 @@ cmd_split () {
 	if test $# -eq 0
 	then
 		rev=$(git rev-parse HEAD)
-	elif test $# -eq 1 -o $# -eq 2
+	elif test $# -eq 1 || test $# -eq 2
 	then
 		rev=$(git rev-parse -q --verify "$1^{commit}") ||
 			die "fatal: '$1' does not refer to a commit"
@@ -1006,8 +1009,11 @@ cmd_split () {
 
 # Usage: cmd_merge REV [REPOSITORY]
 cmd_merge () {
-	test $# -eq 1 -o $# -eq 2 ||
+	if test $# -lt 1 || test $# -gt 2
+	then
 		die "fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'"
+	fi
+
 	rev=$(git rev-parse -q --verify "$1^{commit}") ||
 		die "fatal: '$1' does not refer to a commit"
 	repository=""
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args

[Jeff King]

On Thu, Nov 09, 2023 at 11:53:35AM +0100, Patrick Steinhardt wrote:

> Functions in git-subtree.sh all assert that they are being passed the
> correct number of arguments. In cases where we accept a variable number
> of arguments we assert this via a single call to `test` with `-o`, which
> is discouraged by our coding guidelines.
> 
> Convert these cases to stop doing so.

OK. I think these ones really are safe, because they're only expanding
$#, but I agree with the principle to follow the guidelines.

>  # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
>  process_subtree_split_trailer () {
> -	assert test $# = 2 -o $# = 3
> +	assert test $# -ge 2
> +	assert test $# -le 3

It took me a minute to figure out why we were swapping "=" for "-ge". It
is because we want to logical-OR the two conditions, but "assert"
requires that we test one at a time. I think that is probably worth
explaining in the commit message.

> @@ -916,7 +919,7 @@ cmd_split () {
>  	if test $# -eq 0
>  	then
>  		rev=$(git rev-parse HEAD)
> -	elif test $# -eq 1 -o $# -eq 2
> +	elif test $# -eq 1 || test $# -eq 2

OK, this one is a straight-forward use of "||".

>  cmd_merge () {
> -	test $# -eq 1 -o $# -eq 2 ||
> +	if test $# -lt 1 || test $# -gt 2
> +	then
>  		die "fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'"
> +	fi
> +

But here we swap "-eq" for other operators. We have to because we went
from "||" to an "if". I think what you have here is correct, but you
could also write:

  if ! { test $# -eq 1 || test $# -eq 2; }

(I am OK with either, it just took me a minute to verify that your
conversion was correct. But that is a one-time issue now while
reviewing, and I think the code is readable going forward).

-Peff

Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>>  # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
>>  process_subtree_split_trailer () {
>> -	assert test $# = 2 -o $# = 3
>> +	assert test $# -ge 2
>> +	assert test $# -le 3
>
> It took me a minute to figure out why we were swapping "=" for "-ge". It
> is because we want to logical-OR the two conditions, but "assert"
> requires that we test one at a time. I think that is probably worth
> explaining in the commit message.

I wish we could write something like

	assert test $# -ge 2 && test $# -le 3

(and I'd allow double quoting the whole thing after assert if
needed) but we cannot do so without tweaking the implementation of
assert.

>
>> @@ -916,7 +919,7 @@ cmd_split () {
>>  	if test $# -eq 0
>>  	then
>>  		rev=$(git rev-parse HEAD)
>> -	elif test $# -eq 1 -o $# -eq 2
>> +	elif test $# -eq 1 || test $# -eq 2
>
> OK, this one is a straight-forward use of "||".

Yes, but why not consistently use the range notation like the
earlier one here, or below?

	elif test $# -ge 1 && test $# -le 2

>>  cmd_merge () {
>> -	test $# -eq 1 -o $# -eq 2 ||
>> +	if test $# -lt 1 || test $# -gt 2
>> ...
> (I am OK with either, it just took me a minute to verify that your
> conversion was correct. But that is a one-time issue now while
> reviewing, and I think the code is readable going forward).

Yeah, the end result looks good.

Thanks, both.

Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2509 bytes --]

On Fri, Nov 10, 2023 at 08:02:32AM +0900, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> >>  # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
> >>  process_subtree_split_trailer () {
> >> -	assert test $# = 2 -o $# = 3
> >> +	assert test $# -ge 2
> >> +	assert test $# -le 3
> >
> > It took me a minute to figure out why we were swapping "=" for "-ge". It
> > is because we want to logical-OR the two conditions, but "assert"
> > requires that we test one at a time. I think that is probably worth
> > explaining in the commit message.
> 
> I wish we could write something like
> 
> 	assert test $# -ge 2 && test $# -le 3
> 
> (and I'd allow double quoting the whole thing after assert if
> needed) but we cannot do so without tweaking the implementation of
> assert.
> 
> >
> >> @@ -916,7 +919,7 @@ cmd_split () {
> >>  	if test $# -eq 0
> >>  	then
> >>  		rev=$(git rev-parse HEAD)
> >> -	elif test $# -eq 1 -o $# -eq 2
> >> +	elif test $# -eq 1 || test $# -eq 2
> >
> > OK, this one is a straight-forward use of "||".
> 
> Yes, but why not consistently use the range notation like the
> earlier one here, or below?

I opted to go for the "obvious" conversion, if there was one easily
available, to make the diff easier to read. We could of course use a
range notation like this:

 		rev=$(git rev-parse HEAD)
-	elif test $# -eq 1 || test $# -eq 2
+	elif test $# -ge 1 && test $# -le 2
 	then
 		rev=$(git rev-parse -q --verify "$1^{commit}") ||
 			die "fatal: '$1' does not refer to a commit"

Or :

 		rev=$(git rev-parse HEAD)
-	elif test $# -eq 1 || test $# -eq 2
+	elif ! { test $# -lt 1 || test $# -gt 2; }
 	then
 		rev=$(git rev-parse -q --verify "$1^{commit}") ||
 			die "fatal: '$1' does not refer to a commit"

But both of these are not consistent with the other cases due to the
negation here, and both of them are harder to read in my opinion. So
I'm not sure whether we gain anything by trying to make this a bit more
consistent with the other conversions.

Patrick

> 	elif test $# -ge 1 && test $# -le 2
> 
> >>  cmd_merge () {
> >> -	test $# -eq 1 -o $# -eq 2 ||
> >> +	if test $# -lt 1 || test $# -gt 2
> >> ...
> > (I am OK with either, it just took me a minute to verify that your
> > conversion was correct. But that is a one-time issue now while
> > reviewing, and I think the code is readable going forward).
> 
> Yeah, the end result looks good.
> 
> Thanks, both.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Nov 10, 2023 at 08:02:32AM +0900, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>> 
>> >>  # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
>> >>  process_subtree_split_trailer () {
>> >> -	assert test $# = 2 -o $# = 3
>> >> +	assert test $# -ge 2
>> >> +	assert test $# -le 3
>> > ...
>> >> -	elif test $# -eq 1 -o $# -eq 2
>> >> +	elif test $# -eq 1 || test $# -eq 2
>> >
>> > OK, this one is a straight-forward use of "||".
>> 
>> Yes, but why not consistently use the range notation like the
>> earlier one here, or below?
>
> I opted to go for the "obvious" conversion, if there was one easily
> available, to make the diff easier to read.

... and due to the limitation of "assert" we cannot do the obvious

	test $# = 2 || test $# = 3

and feed it to "assert" (and for equality with $#, = and -eq would
work equally fine, and = is much more readable, by the way).

OK, then.

Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2282 bytes --]

On Thu, Nov 09, 2023 at 01:55:15PM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 11:53:35AM +0100, Patrick Steinhardt wrote:
> 
> > Functions in git-subtree.sh all assert that they are being passed the
> > correct number of arguments. In cases where we accept a variable number
> > of arguments we assert this via a single call to `test` with `-o`, which
> > is discouraged by our coding guidelines.
> > 
> > Convert these cases to stop doing so.
> 
> OK. I think these ones really are safe, because they're only expanding
> $#, but I agree with the principle to follow the guidelines.
> 
> >  # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
> >  process_subtree_split_trailer () {
> > -	assert test $# = 2 -o $# = 3
> > +	assert test $# -ge 2
> > +	assert test $# -le 3
> 
> It took me a minute to figure out why we were swapping "=" for "-ge". It
> is because we want to logical-OR the two conditions, but "assert"
> requires that we test one at a time. I think that is probably worth
> explaining in the commit message.

I really hate to admit how long I've pondered over this patch series in
total, up to the point where I did a `git rebase --reset-author-date` at
the end just so that it's not obvious. So I totally get everyone who
needs to stop and think for a bit here.

Will adapt the commit message.

Patrick

> > @@ -916,7 +919,7 @@ cmd_split () {
> >  	if test $# -eq 0
> >  	then
> >  		rev=$(git rev-parse HEAD)
> > -	elif test $# -eq 1 -o $# -eq 2
> > +	elif test $# -eq 1 || test $# -eq 2
> 
> OK, this one is a straight-forward use of "||".
> 
> >  cmd_merge () {
> > -	test $# -eq 1 -o $# -eq 2 ||
> > +	if test $# -lt 1 || test $# -gt 2
> > +	then
> >  		die "fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'"
> > +	fi
> > +
> 
> But here we swap "-eq" for other operators. We have to because we went
> from "||" to an "if". I think what you have here is correct, but you
> could also write:
> 
>   if ! { test $# -eq 1 || test $# -eq 2; }
> 
> (I am OK with either, it just took me a minute to verify that your
> conversion was correct. But that is a one-time issue now while
> reviewing, and I think the code is readable going forward).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/4] contrib/subtree: convert subtree type check to use case statement

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]

The `subtree_for_commit ()` helper function asserts that the subtree
identified by its parameters are either a commit or tree. This is done
via the `-o` parameter of test, which is discouraged.

Refactor the code to instead use a switch statement over the type.
Despite being aligned with our coding guidelines, the resulting code is
arguably also easier to read.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 contrib/subtree/git-subtree.sh | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 8af0a81ba3f..3028029ac2d 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -641,10 +641,16 @@ subtree_for_commit () {
 	while read mode type tree name
 	do
 		assert test "$name" = "$dir"
-		assert test "$type" = "tree" -o "$type" = "commit"
-		test "$type" = "commit" && continue  # ignore submodules
-		echo $tree
-		break
+
+		case "$type" in
+		commit)
+			continue;; # ignore submodules
+		tree)
+			echo $tree
+			break;;
+		*)
+			die "fatal: tree entry is of type ${type}, expected tree or commit";;
+		esac
 	done || exit $?
 }
 
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] contrib/subtree: convert subtree type check to use case statement

[Jeff King]

On Thu, Nov 09, 2023 at 11:53:39AM +0100, Patrick Steinhardt wrote:

> The `subtree_for_commit ()` helper function asserts that the subtree
> identified by its parameters are either a commit or tree. This is done
> via the `-o` parameter of test, which is discouraged.
> 
> Refactor the code to instead use a switch statement over the type.
> Despite being aligned with our coding guidelines, the resulting code is
> arguably also easier to read.

Yes, I'd agree that the result is much easier to follow.

-Peff

[PATCH 4/4] Makefile: stop using `test -o` when unlinking duplicate executables

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

When building executables we may end up with both `foo` and `foo.exe` in
the project's root directory. This can cause issues on Cygwin, which is
why we unlink the `foo` binary (see 6fc301bbf68 (Makefile: remove $foo
when $foo.exe is built/installed., 2007-01-10)). This step is skipped if
either:

    - `foo` is a directory, which can happen when building Git on
      Windows via MSVC (see ade2ca0ca9f (Do not try to remove
      directories when removing old links, 2009-10-27)).

    - `foo` is a hardlink to `foo.exe`, which can happen on Cygwin (see
      0d768f7c8f1 (Makefile: building git in cygwin 1.7.0, 2008-08-15)).

These two conditions are currently chained together via `test -o`, which
is discouraged by our code style guide. Convert the recipe to instead
use an `if` statement with `&&`'d conditions, which both matches our
style guide and is easier to ready.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 03adcb5a480..1094a557711 100644
--- a/Makefile
+++ b/Makefile
@@ -2342,7 +2342,7 @@ profile-fast: profile-clean
 
 all:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
 ifneq (,$X)
-	$(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';)
+	$(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;)
 endif
 
 all::
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3171 bytes --]

Hi,

this is the second version of my patch series that replaces all uses of
`test <expr> -o/a <expr`.

Changes compared to v1:

    - I've expanded a bit on why we want to do these conversions in the
      first place in the first commit message.

    - Dropped a needless subshell and added missing quoting while at it.

    - Explained why we need to decompose the asserts in the second patch
      into two asserts.

Thanks!

Patrick

Patrick Steinhardt (4):
  global: convert trivial usages of `test <expr> -a/-o <expr>`
  contrib/subtree: stop using `-o` to test for number of args
  contrib/subtree: convert subtree type check to use case statement
  Makefile: stop using `test -o` when unlinking duplicate executables

 GIT-VERSION-GEN                |  2 +-
 Makefile                       |  2 +-
 configure.ac                   |  2 +-
 contrib/subtree/git-subtree.sh | 34 +++++++++++++++++++++++-----------
 t/perf/perf-lib.sh             |  2 +-
 t/perf/run                     |  9 +++++----
 t/valgrind/valgrind.sh         |  2 +-
 7 files changed, 33 insertions(+), 20 deletions(-)

Range-diff against v1:
1:  c5e627eb3fe ! 1:  2967c8ebb46 global: convert trivial usages of `test <expr> -a/-o <expr>`
    @@ Commit message
         these to instead instead concatenate multiple invocations of `test` via
         `&&` and `||`, respectively.
     
    +    While not all of the converted instances can cause ambiguity, it is
    +    worth getting rid of all of them regardless:
    +
    +        - It becomes easier to reason about the code as we do not have to
    +          argue why one use of `-a`/`-o` is okay while another one isn't.
    +
    +        - We don't encourage people to use these expressions.
    +
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## GIT-VERSION-GEN ##
    @@ GIT-VERSION-GEN: LF='
      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 ) &&
    ++elif { test -d "${GIT_DIR:-.git}" || test -f .git; } &&
      	VN=$(git describe --match "v[0-9]*" HEAD 2>/dev/null) &&
      	case "$VN" in
      	*$LF*) (exit 1) ;;
2:  b1ea45b8a88 ! 2:  977132d2236 contrib/subtree: stop using `-o` to test for number of args
    @@ Commit message
         of arguments we assert this via a single call to `test` with `-o`, which
         is discouraged by our coding guidelines.
     
    -    Convert these cases to stop doing so.
    +    Convert these cases to stop doing so. This requires us to decompose
    +    assertions of the style `assert test $# = 2 -o $# = 3` into two calls
    +    because we have no easy way to logically chain statements passed to the
    +    assert function.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
3:  7c54d9070fa = 3:  761cde1b341 contrib/subtree: convert subtree type check to use case statement
4:  bc9489ca5b8 = 4:  5326d86888a Makefile: stop using `test -o` when unlinking duplicate executables

base-commit: dadef801b365989099a9929e995589e455c51fed
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4513 bytes --]

Our coding guidelines say to not use `test` with `-a` and `-o` because
it can easily lead to bugs. Convert trivial cases where we still use
these to instead instead concatenate multiple invocations of `test` via
`&&` and `||`, respectively.

While not all of the converted instances can cause ambiguity, it is
worth getting rid of all of them regardless:

    - It becomes easier to reason about the code as we do not have to
      argue why one use of `-a`/`-o` is okay while another one isn't.

    - We don't encourage people to use these expressions.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 GIT-VERSION-GEN                | 2 +-
 configure.ac                   | 2 +-
 contrib/subtree/git-subtree.sh | 4 ++--
 t/perf/perf-lib.sh             | 2 +-
 t/perf/run                     | 9 +++++----
 t/valgrind/valgrind.sh         | 2 +-
 6 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index e54492f8271..7246ab7c78c 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; } &&
 	VN=$(git describe --match "v[0-9]*" HEAD 2>/dev/null) &&
 	case "$VN" in
 	*$LF*) (exit 1) ;;
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
     AC_MSG_WARN([You likely do not want either 'yes' or 'no' as]
 		     [a value for $1 ($2).  Maybe you do...?])
   fi
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"
 			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"
 			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"
 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
 		shift
 	done
-	if test $# -gt 0 -a "$1" = --; then
+	if test $# -gt 0 && test "$1" = --; then
 		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
 		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" ) ||
 	TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
 	;;
 *)
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Jeff King]

On Fri, Nov 10, 2023 at 11:01:15AM +0100, Patrick Steinhardt wrote:

> 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" ) ||
>  	TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"

I was surprised to see this one as a subshell after you adjusted the
other. It probably isn't that big a deal either way, though (as a style
thing I generally try to use braces unless I am relying on the separate
environment provided by the subshell, but it's certainly not wrong in
this case).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Nov 10, 2023 at 11:01:15AM +0100, Patrick Steinhardt wrote:
>
>> 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" ) ||
>>  	TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
>
> I was surprised to see this one as a subshell after you adjusted the
> other.

;-)

I am not so surprised that this one was missed, though.  I didn't
point this one out during my review of the previous round, either,
and not everybody is as careful as you are.

Thanks, both.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I am not so surprised that this one was missed, though.  I didn't
> point this one out during my review of the previous round, either,
> and not everybody is as careful as you are.

Ah, sorry, thist came out in a way I did not mean to.

I didn't mean "I did not point it out explicitly.  It is not
surprising if a contributor who was not careful did not find it on
their own and took initiative to fix it themselves".

I meant "I failed to spot it myself hence I didn't point it out in
my review---I was not being so careful to aim for thoroughly cover
and find all the similar issues".

In any case, I'll tweak it while queueing.  Thanks for noticing.

diff --git i/t/valgrind/valgrind.sh w/t/valgrind/valgrind.sh
index 9fbf90cee7..3c8ee19975 100755
--- i/t/valgrind/valgrind.sh
+++ w/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" && test 4 -gt "$VALGRIND_MINOR" ) ||
+	{ test 3 -eq "$VALGRIND_MAJOR" && test 4 -gt "$VALGRIND_MINOR"; } ||
 	TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
 	;;
 *)

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

On Sat, Nov 11, 2023 at 09:20:04AM +0900, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I am not so surprised that this one was missed, though.  I didn't
> > point this one out during my review of the previous round, either,
> > and not everybody is as careful as you are.
> 
> Ah, sorry, thist came out in a way I did not mean to.
> 
> I didn't mean "I did not point it out explicitly.  It is not
> surprising if a contributor who was not careful did not find it on
> their own and took initiative to fix it themselves".
> 
> I meant "I failed to spot it myself hence I didn't point it out in
> my review---I was not being so careful to aim for thoroughly cover
> and find all the similar issues".
> 
> In any case, I'll tweak it while queueing.  Thanks for noticing.

Thanks indeed, I missed this instance as well when I scanned for any
additional subshells.

Patrick

> diff --git i/t/valgrind/valgrind.sh w/t/valgrind/valgrind.sh
> index 9fbf90cee7..3c8ee19975 100755
> --- i/t/valgrind/valgrind.sh
> +++ w/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" && test 4 -gt "$VALGRIND_MINOR" ) ||
> +	{ test 3 -eq "$VALGRIND_MAJOR" && test 4 -gt "$VALGRIND_MINOR"; } ||
>  	TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
>  	;;
>  *)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Our coding guidelines say to not use `test` with `-a` and `-o` because
> it can easily lead to bugs. Convert trivial cases where we still use
> these to instead instead concatenate multiple invocations of `test` via
> `&&` and `||`, respectively.
>
> While not all of the converted instances can cause ambiguity, it is
> worth getting rid of all of them regardless:
>
>     - It becomes easier to reason about the code as we do not have to
>       argue why one use of `-a`/`-o` is okay while another one isn't.
>
>     - We don't encourage people to use these expressions.

Thanks for these additional notes.  Nicely done.

[PATCH v2 2/4] contrib/subtree: stop using `-o` to test for number of args

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]

Functions in git-subtree.sh all assert that they are being passed the
correct number of arguments. In cases where we accept a variable number
of arguments we assert this via a single call to `test` with `-o`, which
is discouraged by our coding guidelines.

Convert these cases to stop doing so. This requires us to decompose
assertions of the style `assert test $# = 2 -o $# = 3` into two calls
because we have no easy way to logically chain statements passed to the
assert function.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 contrib/subtree/git-subtree.sh | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 43b5fec7320..8af0a81ba3f 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -373,7 +373,8 @@ try_remove_previous () {
 
 # Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
 process_subtree_split_trailer () {
-	assert test $# = 2 -o $# = 3
+	assert test $# -ge 2
+	assert test $# -le 3
 	b="$1"
 	sq="$2"
 	repository=""
@@ -402,7 +403,8 @@ process_subtree_split_trailer () {
 
 # Usage: find_latest_squash DIR [REPOSITORY]
 find_latest_squash () {
-	assert test $# = 1 -o $# = 2
+	assert test $# -ge 1
+	assert test $# -le 2
 	dir="$1"
 	repository=""
 	if test "$#" = 2
@@ -455,7 +457,8 @@ find_latest_squash () {
 
 # Usage: find_existing_splits DIR REV [REPOSITORY]
 find_existing_splits () {
-	assert test $# = 2 -o $# = 3
+	assert test $# -ge 2
+	assert test $# -le 3
 	debug "Looking for prior splits..."
 	local indent=$(($indent + 1))
 
@@ -916,7 +919,7 @@ cmd_split () {
 	if test $# -eq 0
 	then
 		rev=$(git rev-parse HEAD)
-	elif test $# -eq 1 -o $# -eq 2
+	elif test $# -eq 1 || test $# -eq 2
 	then
 		rev=$(git rev-parse -q --verify "$1^{commit}") ||
 			die "fatal: '$1' does not refer to a commit"
@@ -1006,8 +1009,11 @@ cmd_split () {
 
 # Usage: cmd_merge REV [REPOSITORY]
 cmd_merge () {
-	test $# -eq 1 -o $# -eq 2 ||
+	if test $# -lt 1 || test $# -gt 2
+	then
 		die "fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'"
+	fi
+
 	rev=$(git rev-parse -q --verify "$1^{commit}") ||
 		die "fatal: '$1' does not refer to a commit"
 	repository=""
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 3/4] contrib/subtree: convert subtree type check to use case statement

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]

The `subtree_for_commit ()` helper function asserts that the subtree
identified by its parameters are either a commit or tree. This is done
via the `-o` parameter of test, which is discouraged.

Refactor the code to instead use a switch statement over the type.
Despite being aligned with our coding guidelines, the resulting code is
arguably also easier to read.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 contrib/subtree/git-subtree.sh | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 8af0a81ba3f..3028029ac2d 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -641,10 +641,16 @@ subtree_for_commit () {
 	while read mode type tree name
 	do
 		assert test "$name" = "$dir"
-		assert test "$type" = "tree" -o "$type" = "commit"
-		test "$type" = "commit" && continue  # ignore submodules
-		echo $tree
-		break
+
+		case "$type" in
+		commit)
+			continue;; # ignore submodules
+		tree)
+			echo $tree
+			break;;
+		*)
+			die "fatal: tree entry is of type ${type}, expected tree or commit";;
+		esac
 	done || exit $?
 }
 
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 4/4] Makefile: stop using `test -o` when unlinking duplicate executables

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

When building executables we may end up with both `foo` and `foo.exe` in
the project's root directory. This can cause issues on Cygwin, which is
why we unlink the `foo` binary (see 6fc301bbf68 (Makefile: remove $foo
when $foo.exe is built/installed., 2007-01-10)). This step is skipped if
either:

    - `foo` is a directory, which can happen when building Git on
      Windows via MSVC (see ade2ca0ca9f (Do not try to remove
      directories when removing old links, 2009-10-27)).

    - `foo` is a hardlink to `foo.exe`, which can happen on Cygwin (see
      0d768f7c8f1 (Makefile: building git in cygwin 1.7.0, 2008-08-15)).

These two conditions are currently chained together via `test -o`, which
is discouraged by our code style guide. Convert the recipe to instead
use an `if` statement with `&&`'d conditions, which both matches our
style guide and is easier to ready.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 03adcb5a480..1094a557711 100644
--- a/Makefile
+++ b/Makefile
@@ -2342,7 +2342,7 @@ profile-fast: profile-clean
 
 all:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
 ifneq (,$X)
-	$(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';)
+	$(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;)
 endif
 
 all::
-- 
2.42.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>`

[Jeff King]

On Fri, Nov 10, 2023 at 11:01:11AM +0100, Patrick Steinhardt wrote:

> this is the second version of my patch series that replaces all uses of
> `test <expr> -o/a <expr`.
> 
> Changes compared to v1:
> 
>     - I've expanded a bit on why we want to do these conversions in the
>       first place in the first commit message.
> 
>     - Dropped a needless subshell and added missing quoting while at it.
> 
>     - Explained why we need to decompose the asserts in the second patch
>       into two asserts.

These look OK to me. I mentioned a small nit on the first patch, but I
am OK with ignoring it (and we are reaching diminishing returns
polishing an otherwise trivial series).

-Peff