t: avoid incorrect negated commands #5282
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bk2204
force-pushed
the
negated-grep
branch
4 times, most recently
from
ddbab88 to
e959493
Compare
When `set -e` is enabled, not all commands trigger an error exit if they
return false. For example, it's clear that commands in an `if` or
`while` statement don't cause an error if they are false.
What is less obvious, however, is that negated commands and negated
pipelines also have no effect on `set -e`. From POSIX 1003.1-2017 (on
`sh -e`):
When this option is on, if a simple command fails for any of the
reasons listed in Consequences of Shell Errors or returns an exit
status value >0, and is not part of the compound list following a
while, until, or if keyword, and is not a part of an AND or OR list,
and is not a pipeline preceded by the ! reserved word, then the
shell shall immediately exit.
As such, writing something like `! grep` will never fail. Fortunately,
we can append `&& exit 1` instead of the `!` and that will work
correctly.
To make this work, run the following command to make the code properly
check the exit status of our commands:
git grep -l '! [a-z]' t | \
xargs ruby -pi -e '$_.gsub!(/^(\s+)! ([a-z].*)$/, "\\1\\2 && exit 1")'
Because such a command will still have a non-zero exit status, even if
it doesn't trigger `set -e`, add a `true` if this is the last statement
in a block, so that the test exits successfully and therefore passes.
chrisd8088
approved these changes
Feb 23, 2023
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Nov 18, 2024
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When
set -eis enabled, not all commands trigger an error exit if they return false. For example, it's clear that commands in aniforwhilestatement don't cause an error if they are false.What is less obvious, however, is that negated commands and negated pipelines also have no effect on
set -e. From POSIX 1003.1-2017 (onsh -e):As such, writing something like
! grepwill never fail. Fortunately, we can append&& exit 1instead of the!and that will work correctly.To make this work, run the following command to make the code properly check the exit status of our commands:
Fixes #5183