t: avoid incorrect negated test expressions

In commit a5d20de of PR git-lfs#5282 we
revised a number of our test scripts so as to avoid the use of the
"!" shell word in cases where we want to confirm that a command fails
and returns a non-zero exit code.  We made this change because we had
a variety of instances where we had checks of this form which would
in practice always pass, regardless of the exit code of their commands.

(This behaviour is a consequence of our use of "set -e" in all our
test scripts.  The Git project's shell test scripts do not use this
option, but ours do; it means that the shell will exit immediately
if a normal command returns a non-zero exit code.  However, according
to the POSIX shell specification, when the "set -e" option is enabled
and a command's exit code is inverted with "!", the shell will not exit
immediately even if the inverted value is non-zero.)

Commit a5d20de addressed all of the
instances in our test scripts where we relied on an incorrect usage
of the "!" shell word.  However, we have another similar form of
incorrect usage in a number of our test scripts where we rely on the
"!" test operator to confirm that a command's exit code is non-zero
(i.e., in conditions where we expect the command to fail).

In particular, we often use tests of the following form, with the
expectation that "foo" should not appear in the log file, and if it
does the test should fail:

  [ ! $(grep "foo" bar.log) ]

These checks always succeed, but for the wrong reason.  When the
grep(1) command does not find the given string in its input stream
or the specified file, it prints nothing to its standard output and
returns a non-zero exit code.  Because we are evaluating a command
substitution inside the test expression, and the output of that
substitution is the standard output of the command, which is empty,
the test devolves to an expression of the form:

  [ ! ]

The Bash shell treats the absence of an expression as a false value,
which is then inverted by the "!" operator, so the test passes, but
not because has confirmed that the command's exit code was non-zero.

Some of these improper test expressions could still catch regressions
in the Git LFS client's behaviour, though.  For instance, in the
example above, if "foo" was found in the log file, the test expression
that would be evaluated would have the following form, where all the
log lines which matched the string "foo" would appear after the "!"
operator:

  [ ! ... foo ... ]

This might form an invalid shell expression, or one with too many
arguments (thus causing a "[: too many arguments" error), in which
case the test would fail.

We can replace all the instances where we rely on these improper
test expressions using the same approach we used in commit
a5d20de.  We simply perform the
commands from what was enclosed in a command substitution as the first
command pipeline in an AND list, where "exit 1" is the second and
final command in the list.

If the first command pipeline returns a non-zero exit code, as we
expect it to, the "exit 1" is not executed.  However, if the pipeline
should catch a regression and unexpectedly succeed, "exit 1" will be
executed, causing our test to fail.  Note that we do not rely on the
"set -e" option here, because it ignores the return values of command
lists entirely.

In cases where our revised check is the last line of one of our tests,
we add a "true" statement, because otherwise the test will fail since
the last evaluated command would be the first one in the list, i.e.,
the grep(1) command that we expect will find no matches in its input.

In our t/t-migrate-import.sh test script we also have some improper
test expresssions of the following form, where we expect that the
output of the "git cat-file" command should be empty because the
".gitattributes" file at the given Git ref is blank:

  [ ! $(git cat-file -p "ref:.gitattributes") ]

As with the improper tests of the output of grep(1) commands, these
tests succeed but for the wrong reason, because they also devolve
to expressions of the form:

  [ ! ]

To repair these checks, we simply replace the "!" test operator
with the "-z" one, as we just want to confirm that the output
of the command substitution is a zero-length string.

Author: chrisd8088

t/t-askpass.sh

@@ -112,7 +112,7 @@ begin_test "askpass: defaults to provided credentials"
 
   GIT_ASKPASS="lfs-askpass" GIT_TRACE=1 GIT_CURL_VERBOSE=1 git push origin main 2>&1 | tee push.log
 
-  [ ! $(grep "filling with GIT_ASKPASS" push.log) ]
+  grep "filling with GIT_ASKPASS" push.log && exit 1
   grep "main -> main" push.log
 )
 end_test

t/t-clone.sh

@@ -57,8 +57,8 @@ begin_test "clone"
   grep "Cloning into" lfsclone.log
   grep "Downloading LFS objects:" lfsclone.log
   # should be no filter errors
-  [ ! $(grep "filter" lfsclone.log) ]
-  [ ! $(grep "error" lfsclone.log) ]
+  grep "filter" lfsclone.log && exit 1
+  grep "error" lfsclone.log && exit 1
   # should be cloned into location as per arg
   [ -d "$newclonedir" ]
 
@@ -78,8 +78,8 @@ begin_test "clone"
   grep "Cloning into" lfsclone.log
   grep "Downloading LFS objects:" lfsclone.log
   # should be no filter errors
-  [ ! $(grep "filter" lfsclone.log) ]
-  [ ! $(grep "error" lfsclone.log) ]
+  grep "filter" lfsclone.log && exit 1
+  grep "error" lfsclone.log && exit 1
   # clone location should be implied
   [ -d "$reponame" ]
 
@@ -142,8 +142,8 @@ begin_test "cloneSSL"
   grep "Cloning into" lfsclone.log
   grep "Downloading LFS objects:" lfsclone.log
   # should be no filter errors
-  [ ! $(grep "filter" lfsclone.log) ]
-  [ ! $(grep "error" lfsclone.log) ]
+  grep "filter" lfsclone.log && exit 1
+  grep "error" lfsclone.log && exit 1
   # should be cloned into location as per arg
   [ -d "$newclonedir" ]
 
@@ -216,8 +216,8 @@ begin_test "clone ClientCert"
     grep "Cloning into" lfsclone.log
     grep "Downloading LFS objects:" lfsclone.log
     # should be no filter errors
-    [ ! $(grep "filter" lfsclone.log) ]
-    [ ! $(grep "error" lfsclone.log) ]
+    grep "filter" lfsclone.log && exit 1
+    grep "error" lfsclone.log && exit 1
     # should be cloned into location as per arg
     [ -d "$newclonedir" ]
 
@@ -804,8 +804,8 @@ begin_test "clone (HTTP server/proxy require cookies)"
   grep "Cloning into" lfsclone.log
   grep "Downloading LFS objects:" lfsclone.log
   # should be no filter errors
-  [ ! $(grep "filter" lfsclone.log) ]
-  [ ! $(grep "error" lfsclone.log) ]
+  grep "filter" lfsclone.log && exit 1
+  grep "error" lfsclone.log && exit 1
   # should be cloned into location as per arg
   [ -d "$newclonedir" ]
 
@@ -825,8 +825,8 @@ begin_test "clone (HTTP server/proxy require cookies)"
   grep "Cloning into" lfsclone.log
   grep "Downloading LFS objects:" lfsclone.log
   # should be no filter errors
-  [ ! $(grep "filter" lfsclone.log) ]
-  [ ! $(grep "error" lfsclone.log) ]
+  grep "filter" lfsclone.log && exit 1
+  grep "error" lfsclone.log && exit 1
   # clone location should be implied
   [ -d "$reponame" ]
 

t/t-credentials.sh

@@ -637,7 +637,7 @@ begin_test "credentials from lfs.url"
   grep "HTTP: 401" push.log
   # Ensure we didn't make a second batch request, which means the request
   # was successfully retried internally
-  [ ! "$(grep "tq: retrying object" push.log)" ]
+  grep "tq: retrying object" push.log && exit 1
   grep "Uploading LFS objects:   0% (0/1), 0 B" push.log
 
   echo "bad fetch"
@@ -654,7 +654,7 @@ begin_test "credentials from lfs.url"
   GIT_TRACE=1 git lfs fetch --all 2>&1 | tee fetch.log
   # No 401 should occur as we've already set an access mode for the
   # storage endpoint during the push
-  [ ! "$(grep "HTTP: 401" fetch.log)" ]
+  grep "HTTP: 401" fetch.log && exit 1
   git lfs fsck
 
   echo "good fetch, setting access mode"
@@ -668,7 +668,7 @@ begin_test "credentials from lfs.url"
   grep "HTTP: 401" fetch.log
   # Ensure we didn't make a second batch request, which means the request
   # was successfully retried internally
-  [ ! "$(grep "tq: retrying object" fetch.log)" ]
+  grep "tq: retrying object" fetch.log && exit 1
 
   git lfs fsck
 )
@@ -702,7 +702,7 @@ begin_test "credentials from remote.origin.url"
   grep "HTTP: 401" push.log
   # Ensure we didn't make a second batch request, which means the request
   # was successfully retried internally
-  [ ! "$(grep "tq: retrying object" push.log)" ]
+  grep "tq: retrying object" push.log && exit 1
   grep "Uploading LFS objects: 100% (1/1), 7 B" push.log
 
   echo "bad fetch"
@@ -720,7 +720,7 @@ begin_test "credentials from remote.origin.url"
   GIT_TRACE=1 git lfs fetch --all 2>&1 | tee fetch.log
   # No 401 should occur as we've already set an access mode for the
   # storage endpoint during the push
-  [ ! "$(grep "HTTP: 401" fetch.log)" ]
+  grep "HTTP: 401" fetch.log && exit 1
   git lfs fsck
 
   echo "good fetch, setting access mode"
@@ -734,7 +734,7 @@ begin_test "credentials from remote.origin.url"
   grep "HTTP: 401" fetch.log
   # Ensure we didn't make a second batch request, which means the request
   # was successfully retried internally
-  [ ! "$(grep "tq: retrying object" fetch.log)" ]
+  grep "tq: retrying object" fetch.log && exit 1
 
   git lfs fsck
 )

t/t-migrate-export.sh

@@ -52,8 +52,9 @@ begin_test "migrate export (default branch)"
   echo "$main_attrs" | grep -q "*.md !text !filter !merge !diff"
   echo "$main_attrs" | grep -q "*.txt !text !filter !merge !diff"
 
-  [ ! $(echo "$feature_attrs" | grep -q "*.md !text !filter !merge !diff") ]
-  [ ! $(echo "$feature_attrs" | grep -q "*.txt !text !filter !merge !diff") ]
+  echo "$feature_attrs" | grep -q "*.md !text !filter !merge !diff" && exit 1
+  echo "$feature_attrs" | grep -q "*.txt !text !filter !merge !diff" && exit 1
+  true
 )
 end_test
 
@@ -253,8 +254,9 @@ begin_test "migrate export (exclude remote refs)"
   echo "$main_attrs" | grep -q "*.md !text !filter !merge !diff"
   echo "$main_attrs" | grep -q "*.txt !text !filter !merge !diff"
 
-  [ ! $(echo "$remote_attrs" | grep -q "*.md !text !filter !merge !diff") ]
-  [ ! $(echo "$remote_attrs" | grep -q "*.txt !text !filter !merge !diff") ]
+  echo "$remote_attrs" | grep -q "*.md !text !filter !merge !diff" && exit 1
+  echo "$remote_attrs" | grep -q "*.txt !text !filter !merge !diff" && exit 1
+  true
 )
 end_test
 
@@ -361,8 +363,8 @@ begin_test "migrate export (include/exclude ref)"
   remote_attrs="$(git cat-file -p "$remote:.gitattributes")"
   feature_attrs="$(git cat-file -p "$feature:.gitattributes")"
 
-  [ ! $(echo "$main_attrs" | grep -q "*.txt !text !filter !merge !diff") ]
-  [ ! $(echo "$remote_attrs" | grep -q "*.txt !text !filter !merge !diff") ]
+  echo "$main_attrs" | grep -q "*.txt !text !filter !merge !diff" && exit 1
+  echo "$remote_attrs" | grep -q "*.txt !text !filter !merge !diff" && exit 1
   echo "$feature_attrs" | grep -q "*.txt !text !filter !merge !diff"
 )
 end_test

t/t-migrate-import-no-rewrite.sh

@@ -132,7 +132,7 @@ begin_test "migrate import --no-rewrite (nested .gitattributes)"
 
   # Ensure a .md filter does not exist in the top-level .gitattributes
   main_attrs="$(git cat-file -p "$main:.gitattributes")"
-  [ !"$(echo "$main_attrs" | grep -q ".md")" ]
+  echo "$main_attrs" | grep -q ".md" && exit 1
 
   # Ensure a .md filter exists in the nested .gitattributes
   nested_attrs="$(git cat-file -p "$main:b/.gitattributes")"

t/t-migrate-import.sh

@@ -26,7 +26,7 @@ begin_test "migrate import (default branch)"
   feature="$(git rev-parse refs/heads/my-feature)"
 
   main_attrs="$(git cat-file -p "$main:.gitattributes")"
-  [ ! $(git cat-file -p "$feature:.gitattributes") ]
+  [ -z "$(git cat-file -p "$feature:.gitattributes")" ]
 
   echo "$main_attrs" | grep -q "*.md filter=lfs diff=lfs merge=lfs"
   echo "$main_attrs" | grep -q "*.txt filter=lfs diff=lfs merge=lfs"
@@ -103,7 +103,7 @@ begin_test "migrate import (default branch with filter)"
   feature="$(git rev-parse refs/heads/my-feature)"
 
   main_attrs="$(git cat-file -p "$main:.gitattributes")"
-  [ ! $(git cat-file -p "$feature:.gitattributes") ]
+  [ -z "$(git cat-file -p "$feature:.gitattributes")" ]
 
   echo "$main_attrs" | grep -q "*.md filter=lfs diff=lfs merge=lfs"
   echo "$main_attrs" | grep -vq "*.txt filter=lfs diff=lfs merge=lfs"
@@ -167,7 +167,7 @@ begin_test "migrate import (default branch, exclude remote refs)"
   remote="$(git rev-parse refs/remotes/origin/main)"
 
   main_attrs="$(git cat-file -p "$main:.gitattributes")"
-  [ ! $(git cat-file -p "$remote:.gitattributes") ]
+  [ -z "$(git cat-file -p "$remote:.gitattributes")" ]
 
   echo "$main_attrs" | grep -q "*.md filter=lfs diff=lfs merge=lfs"
   echo "$main_attrs" | grep -vq "*.txt filter=lfs diff=lfs merge=lfs"
@@ -206,7 +206,7 @@ begin_test "migrate import (given branch, exclude remote refs)"
   remote="$(git rev-parse refs/remotes/origin/main)"
 
   main_attrs="$(git cat-file -p "$main:.gitattributes")"
-  [ ! $(git cat-file -p "$remote:.gitattributes") ]
+  [ -z "$(git cat-file -p "$remote:.gitattributes")" ]
   feature_attrs="$(git cat-file -p "$feature:.gitattributes")"
 
   echo "$main_attrs" | grep -q "*.md filter=lfs diff=lfs merge=lfs"
@@ -322,8 +322,8 @@ begin_test "migrate import (include/exclude ref)"
   feature="$(git rev-parse refs/heads/my-feature)"
   remote="$(git rev-parse refs/remotes/origin/main)"
 
-  [ ! $(git cat-file -p "$main:.gitattributes") ]
-  [ ! $(git cat-file -p "$remote:.gitattributes") ]
+  [ -z "$(git cat-file -p "$main:.gitattributes")" ]
+  [ -z "$(git cat-file -p "$remote:.gitattributes")" ]
   feature_attrs="$(git cat-file -p "$feature:.gitattributes")"
 
   echo "$feature_attrs" | grep -q "*.md filter=lfs diff=lfs merge=lfs"
@@ -360,8 +360,8 @@ begin_test "migrate import (include/exclude ref args)"
   feature="$(git rev-parse refs/heads/my-feature)"
   remote="$(git rev-parse refs/remotes/origin/main)"
 
-  [ ! $(git cat-file -p "$main:.gitattributes") ]
-  [ ! $(git cat-file -p "$remote:.gitattributes") ]
+  [ -z "$(git cat-file -p "$main:.gitattributes")" ]
+  [ -z "$(git cat-file -p "$remote:.gitattributes")" ]
   feature_attrs="$(git cat-file -p "$feature:.gitattributes")"
 
   echo "$feature_attrs" | grep -q "*.md filter=lfs diff=lfs merge=lfs"
@@ -400,8 +400,8 @@ begin_test "migrate import (include/exclude ref with filter)"
   feature="$(git rev-parse refs/heads/my-feature)"
   remote="$(git rev-parse refs/remotes/origin/main)"
 
-  [ ! $(git cat-file -p "$main:.gitattributes") ]
-  [ ! $(git cat-file -p "$remote:.gitattributes") ]
+  [ -z "$(git cat-file -p "$main:.gitattributes")" ]
+  [ -z "$(git cat-file -p "$remote:.gitattributes")" ]
   feature_attrs="$(git cat-file -p "$feature:.gitattributes")"
 
   echo "$feature_attrs" | grep -vq "*.md filter=lfs diff=lfs merge=lfs"

t/t-track-wildcards.sh

@@ -30,7 +30,7 @@ begin_test "track files using wildcard pattern with leading slash"
   git lfs ls-files | tee files.log
 
   grep "a.dat" files.log
-  [ ! $(grep "dir/b.dat" files.log) ] # Subdirectories ignored
+  grep "dir/b.dat" files.log && exit 1 # Subdirectories ignored
 
   # Add files after being tracked by LFS
   printf "contents" > c.dat
@@ -43,9 +43,10 @@ begin_test "track files using wildcard pattern with leading slash"
   git lfs ls-files | tee new_files.log
 
   grep "a.dat" new_files.log
-  [ ! $(grep "dir/b.dat" new_files.log) ]
+  grep "dir/b.dat" new_files.log && exit 1
   grep "c.dat" new_files.log
-  [ ! $(grep "dir/d.dat" new_files.log) ]
+  grep "dir/d.dat" new_files.log && exit 1
+  true
 )
 end_test