* [PATCH 0/4] Replace use of `test <expr> -o/a <expr>`
@ 2023-11-09 10:53 Patrick Steinhardt
2023-11-09 10:53 ` [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>` Patrick Steinhardt
` (4 more replies)
0 siblings, 5 replies; 26+ messages in thread
From: Patrick Steinhardt @ 2023-11-09 10:53 UTC (permalink / raw
To: git; +Cc: Jeff King, Junio C Hamano
[-- 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 --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
2023-11-09 10:53 [PATCH 0/4] Replace use of `test <expr> -o/a <expr>` Patrick Steinhardt
@ 2023-11-09 10:53 ` Patrick Steinhardt
2023-11-09 11:41 ` Junio C Hamano
2023-11-09 10:53 ` [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args Patrick Steinhardt
` (3 subsequent siblings)
4 siblings, 1 reply; 26+ messages in thread
From: Patrick Steinhardt @ 2023-11-09 10:53 UTC (permalink / raw
To: git; +Cc: Jeff King, Junio C Hamano
[-- 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 --]
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
2023-11-09 10:53 [PATCH 0/4] Replace use of `test <expr> -o/a <expr>` Patrick Steinhardt
2023-11-09 10:53 ` [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>` Patrick Steinhardt
@ 2023-11-09 10:53 ` Patrick Steinhardt
2023-11-09 18:55 ` Jeff King
2023-11-09 10:53 ` [PATCH 3/4] contrib/subtree: convert subtree type check to use case statement Patrick Steinhardt
` (2 subsequent siblings)
4 siblings, 1 reply; 26+ messages in thread
From: Patrick Steinhardt @ 2023-11-09 10:53 UTC (permalink / raw
To: git; +Cc: Jeff King, Junio C Hamano
[-- 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 --]
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/4] contrib/subtree: convert subtree type check to use case statement
2023-11-09 10:53 [PATCH 0/4] Replace use of `test <expr> -o/a <expr>` Patrick Steinhardt
2023-11-09 10:53 ` [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>` Patrick Steinhardt
2023-11-09 10:53 ` [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args Patrick Steinhardt
@ 2023-11-09 10:53 ` Patrick Steinhardt
2023-11-09 18:56 ` Jeff King
2023-11-09 10:53 ` [PATCH 4/4] Makefile: stop using `test -o` when unlinking duplicate executables Patrick Steinhardt
2023-11-10 10:01 ` [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>` Patrick Steinhardt
4 siblings, 1 reply; 26+ messages in thread
From: Patrick Steinhardt @ 2023-11-09 10:53 UTC (permalink / raw
To: git; +Cc: Jeff King, Junio C Hamano
[-- 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 --]
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 4/4] Makefile: stop using `test -o` when unlinking duplicate executables
2023-11-09 10:53 [PATCH 0/4] Replace use of `test <expr> -o/a <expr>` Patrick Steinhardt
` (2 preceding siblings ...)
2023-11-09 10:53 ` [PATCH 3/4] contrib/subtree: convert subtree type check to use case statement Patrick Steinhardt
@ 2023-11-09 10:53 ` Patrick Steinhardt
2023-11-10 10:01 ` [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>` Patrick Steinhardt
4 siblings, 0 replies; 26+ messages in thread
From: Patrick Steinhardt @ 2023-11-09 10:53 UTC (permalink / raw
To: git; +Cc: Jeff King, Junio C Hamano
[-- 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 --]
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
2023-11-09 10:53 ` [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>` Patrick Steinhardt
@ 2023-11-09 11:41 ` Junio C Hamano
2023-11-09 18:48 ` Jeff King
0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2023-11-09 11:41 UTC (permalink / raw
To: Patrick Steinhardt; +Cc: git, Jeff King
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"
> ;;
> *)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
2023-11-09 11:41 ` Junio C Hamano
@ 2023-11-09 18:48 ` Jeff King
2023-11-09 22:56 ` Junio C Hamano
2023-11-10 10:18 ` Patrick Steinhardt
0 siblings, 2 replies; 26+ messages in thread
From: Jeff King @ 2023-11-09 18:48 UTC (permalink / raw
To: Junio C Hamano; +Cc: Patrick Steinhardt, git
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
2023-11-09 10:53 ` [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args Patrick Steinhardt
@ 2023-11-09 18:55 ` Jeff King
2023-11-09 23:02 ` Junio C Hamano
2023-11-10 10:18 ` Patrick Steinhardt
0 siblings, 2 replies; 26+ messages in thread
From: Jeff King @ 2023-11-09 18:55 UTC (permalink / raw
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/4] contrib/subtree: convert subtree type check to use case statement
2023-11-09 10:53 ` [PATCH 3/4] contrib/subtree: convert subtree type check to use case statement Patrick Steinhardt
@ 2023-11-09 18:56 ` Jeff King
0 siblings, 0 replies; 26+ messages in thread
From: Jeff King @ 2023-11-09 18:56 UTC (permalink / raw
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
2023-11-09 18:48 ` Jeff King
@ 2023-11-09 22:56 ` Junio C Hamano
2023-11-10 10:18 ` Patrick Steinhardt
1 sibling, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2023-11-09 22:56 UTC (permalink / raw
To: Jeff King; +Cc: Patrick Steinhardt, git
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. :)
;-).
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
2023-11-09 18:55 ` Jeff King
@ 2023-11-09 23:02 ` Junio C Hamano
2023-11-10 10:18 ` Patrick Steinhardt
2023-11-10 10:18 ` Patrick Steinhardt
1 sibling, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2023-11-09 23:02 UTC (permalink / raw
To: Jeff King; +Cc: Patrick Steinhardt, git
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>`
2023-11-09 10:53 [PATCH 0/4] Replace use of `test <expr> -o/a <expr>` Patrick Steinhardt
` (3 preceding siblings ...)
2023-11-09 10:53 ` [PATCH 4/4] Makefile: stop using `test -o` when unlinking duplicate executables Patrick Steinhardt
@ 2023-11-10 10:01 ` Patrick Steinhardt
2023-11-10 10:01 ` [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>` Patrick Steinhardt
` (4 more replies)
4 siblings, 5 replies; 26+ messages in thread
From: Patrick Steinhardt @ 2023-11-10 10:01 UTC (permalink / raw
To: git; +Cc: Jeff King, Junio C Hamano
[-- 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 --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
2023-11-10 10:01 ` [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>` Patrick Steinhardt
@ 2023-11-10 10:01 ` Patrick Steinhardt
2023-11-10 21:44 ` Jeff King
2023-11-11 0:12 ` Junio C Hamano
2023-11-10 10:01 ` [PATCH v2 2/4] contrib/subtree: stop using `-o` to test for number of args Patrick Steinhardt
` (3 subsequent siblings)
4 siblings, 2 replies; 26+ messages in thread
From: Patrick Steinhardt @ 2023-11-10 10:01 UTC (permalink / raw
To: git; +Cc: Jeff King, Junio C Hamano
[-- 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 --]
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 2/4] contrib/subtree: stop using `-o` to test for number of args
2023-11-10 10:01 ` [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>` Patrick Steinhardt
2023-11-10 10:01 ` [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>` Patrick Steinhardt
@ 2023-11-10 10:01 ` Patrick Steinhardt
2023-11-10 10:01 ` [PATCH v2 3/4] contrib/subtree: convert subtree type check to use case statement Patrick Steinhardt
` (2 subsequent siblings)
4 siblings, 0 replies; 26+ messages in thread
From: Patrick Steinhardt @ 2023-11-10 10:01 UTC (permalink / raw
To: git; +Cc: Jeff King, Junio C Hamano
[-- 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 --]
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 3/4] contrib/subtree: convert subtree type check to use case statement
2023-11-10 10:01 ` [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>` Patrick Steinhardt
2023-11-10 10:01 ` [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>` Patrick Steinhardt
2023-11-10 10:01 ` [PATCH v2 2/4] contrib/subtree: stop using `-o` to test for number of args Patrick Steinhardt
@ 2023-11-10 10:01 ` Patrick Steinhardt
2023-11-10 10:01 ` [PATCH v2 4/4] Makefile: stop using `test -o` when unlinking duplicate executables Patrick Steinhardt
2023-11-10 21:46 ` [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>` Jeff King
4 siblings, 0 replies; 26+ messages in thread
From: Patrick Steinhardt @ 2023-11-10 10:01 UTC (permalink / raw
To: git; +Cc: Jeff King, Junio C Hamano
[-- 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 --]
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 4/4] Makefile: stop using `test -o` when unlinking duplicate executables
2023-11-10 10:01 ` [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>` Patrick Steinhardt
` (2 preceding siblings ...)
2023-11-10 10:01 ` [PATCH v2 3/4] contrib/subtree: convert subtree type check to use case statement Patrick Steinhardt
@ 2023-11-10 10:01 ` Patrick Steinhardt
2023-11-10 21:46 ` [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>` Jeff King
4 siblings, 0 replies; 26+ messages in thread
From: Patrick Steinhardt @ 2023-11-10 10:01 UTC (permalink / raw
To: git; +Cc: Jeff King, Junio C Hamano
[-- 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 --]
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
2023-11-09 18:48 ` Jeff King
2023-11-09 22:56 ` Junio C Hamano
@ 2023-11-10 10:18 ` Patrick Steinhardt
1 sibling, 0 replies; 26+ messages in thread
From: Patrick Steinhardt @ 2023-11-10 10:18 UTC (permalink / raw
To: Jeff King; +Cc: Junio C Hamano, git
[-- 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 --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
2023-11-09 18:55 ` Jeff King
2023-11-09 23:02 ` Junio C Hamano
@ 2023-11-10 10:18 ` Patrick Steinhardt
1 sibling, 0 replies; 26+ messages in thread
From: Patrick Steinhardt @ 2023-11-10 10:18 UTC (permalink / raw
To: Jeff King; +Cc: git, Junio C Hamano
[-- 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 --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
2023-11-09 23:02 ` Junio C Hamano
@ 2023-11-10 10:18 ` Patrick Steinhardt
2023-11-10 23:27 ` Junio C Hamano
0 siblings, 1 reply; 26+ messages in thread
From: Patrick Steinhardt @ 2023-11-10 10:18 UTC (permalink / raw
To: Junio C Hamano; +Cc: Jeff King, git
[-- 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 --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
2023-11-10 10:01 ` [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>` Patrick Steinhardt
@ 2023-11-10 21:44 ` Jeff King
2023-11-11 0:14 ` Junio C Hamano
2023-11-11 0:12 ` Junio C Hamano
1 sibling, 1 reply; 26+ messages in thread
From: Jeff King @ 2023-11-10 21:44 UTC (permalink / raw
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>`
2023-11-10 10:01 ` [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>` Patrick Steinhardt
` (3 preceding siblings ...)
2023-11-10 10:01 ` [PATCH v2 4/4] Makefile: stop using `test -o` when unlinking duplicate executables Patrick Steinhardt
@ 2023-11-10 21:46 ` Jeff King
4 siblings, 0 replies; 26+ messages in thread
From: Jeff King @ 2023-11-10 21:46 UTC (permalink / raw
To: Patrick Steinhardt; +Cc: git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
2023-11-10 10:18 ` Patrick Steinhardt
@ 2023-11-10 23:27 ` Junio C Hamano
0 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2023-11-10 23:27 UTC (permalink / raw
To: Patrick Steinhardt; +Cc: Jeff King, git
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
2023-11-10 10:01 ` [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>` Patrick Steinhardt
2023-11-10 21:44 ` Jeff King
@ 2023-11-11 0:12 ` Junio C Hamano
1 sibling, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2023-11-11 0:12 UTC (permalink / raw
To: Patrick Steinhardt; +Cc: git, Jeff King
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
2023-11-10 21:44 ` Jeff King
@ 2023-11-11 0:14 ` Junio C Hamano
2023-11-11 0:20 ` Junio C Hamano
0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2023-11-11 0:14 UTC (permalink / raw
To: Jeff King; +Cc: Patrick Steinhardt, git
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
2023-11-11 0:14 ` Junio C Hamano
@ 2023-11-11 0:20 ` Junio C Hamano
2023-11-13 7:12 ` Patrick Steinhardt
0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2023-11-11 0:20 UTC (permalink / raw
To: Jeff King; +Cc: Patrick Steinhardt, git
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"
;;
*)
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
2023-11-11 0:20 ` Junio C Hamano
@ 2023-11-13 7:12 ` Patrick Steinhardt
0 siblings, 0 replies; 26+ messages in thread
From: Patrick Steinhardt @ 2023-11-13 7:12 UTC (permalink / raw
To: Junio C Hamano; +Cc: Jeff King, git
[-- 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 --]
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2023-11-13 7:13 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-09 10:53 [PATCH 0/4] Replace use of `test <expr> -o/a <expr>` Patrick Steinhardt
2023-11-09 10:53 ` [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>` Patrick Steinhardt
2023-11-09 11:41 ` Junio C Hamano
2023-11-09 18:48 ` Jeff King
2023-11-09 22:56 ` Junio C Hamano
2023-11-10 10:18 ` Patrick Steinhardt
2023-11-09 10:53 ` [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args Patrick Steinhardt
2023-11-09 18:55 ` Jeff King
2023-11-09 23:02 ` Junio C Hamano
2023-11-10 10:18 ` Patrick Steinhardt
2023-11-10 23:27 ` Junio C Hamano
2023-11-10 10:18 ` Patrick Steinhardt
2023-11-09 10:53 ` [PATCH 3/4] contrib/subtree: convert subtree type check to use case statement Patrick Steinhardt
2023-11-09 18:56 ` Jeff King
2023-11-09 10:53 ` [PATCH 4/4] Makefile: stop using `test -o` when unlinking duplicate executables Patrick Steinhardt
2023-11-10 10:01 ` [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>` Patrick Steinhardt
2023-11-10 10:01 ` [PATCH v2 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>` Patrick Steinhardt
2023-11-10 21:44 ` Jeff King
2023-11-11 0:14 ` Junio C Hamano
2023-11-11 0:20 ` Junio C Hamano
2023-11-13 7:12 ` Patrick Steinhardt
2023-11-11 0:12 ` Junio C Hamano
2023-11-10 10:01 ` [PATCH v2 2/4] contrib/subtree: stop using `-o` to test for number of args Patrick Steinhardt
2023-11-10 10:01 ` [PATCH v2 3/4] contrib/subtree: convert subtree type check to use case statement Patrick Steinhardt
2023-11-10 10:01 ` [PATCH v2 4/4] Makefile: stop using `test -o` when unlinking duplicate executables Patrick Steinhardt
2023-11-10 21:46 ` [PATCH v2 0/4] Replace use of `test <expr> -o/a <expr>` Jeff King
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).