| Message ID | 20230326173147.39626-2-edwinfernando734@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Avoid suppression of git's exit code in tests | expand |
Hello, Edwin. In subject line "t3701: Avoid suppression of exit status of git" -- the first word after a colon shouldn't be capitalized. By the way, similar existing commits call it: don't lose "git" exit codes and preserve "git" exit codes On 26/03/2023 19:31, Edwin Fernando wrote: > Pipes and command substitution in bash suppress the exit codes of the Bash is not the only Shell used for running Git's test suite, so referring to it like this in the commit message might not be a good idea. See also similar commit 9fdc79ecba ("tests: don't lose misc "git" exit codes", 2023-02-06) for inspiration in writing the commit message. > first executed command. In the test scripts errors when running git > should be exposed. Remove all such suppressions of git by writing > output to files, chaining commands with &&, or other means. > > Signed-off-by: Edwin Fernando <edwinfernando734@gmail.com> > --- > t/t3701-add-interactive.sh | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index 3a99837d9b..80446b311d 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -292,8 +292,10 @@ test_expect_success FILEMODE 'patch does not affect mode' ' > echo content >>file && > chmod +x file && > printf "n\\ny\\n" | git add -p && > - git show :file | grep content && > - git diff file | grep "new mode" > + git show :file > show_file && Code style here and in all output redirections below: lose the space between redirection and the path, like so: git show :file >show_file Also, the suffix "_file" is unnecessary. Just git show :file >show or git show :file >out or git show :file >actual might be better. > + grep content show_file && > + git diff file > diff_file && > + grep "new mode" diff_file > ' > > test_expect_success FILEMODE 'stage mode but not hunk' ' > @@ -301,8 +303,10 @@ test_expect_success FILEMODE 'stage mode but not hunk' ' > echo content >>file && > chmod +x file && > printf "y\\nn\\n" | git add -p && > - git diff --cached file | grep "new mode" && > - git diff file | grep "+content" > + git diff --cached file > diff_file && > + grep "new mode" diff_file && > + git diff file > diff_file && > + grep "+content" diff_file > ' > > > @@ -311,9 +315,12 @@ test_expect_success FILEMODE 'stage mode and hunk' ' > echo content >>file && > chmod +x file && > printf "y\\ny\\n" | git add -p && > - git diff --cached file | grep "new mode" && > - git diff --cached file | grep "+content" && > - test -z "$(git diff file)" > + git diff --cached file > diff_file && > + grep "new mode" diff_file && > + git diff --cached file diff_file && > + grep "+content" diff_file && > + git diff file > diff_file && > + test -z $(cat diff_file) Function test_stdout_line_count can be used here instead of "test -z". test_stdout_line_count would produce a more helpful error message in case of test failure. > ' > > # end of tests disabled when filemode is not usable > @@ -970,8 +977,8 @@ test_expect_success 'handle submodules' ' > > force_color git -C for-submodules add -p dirty-head >output 2>&1 <y && > git -C for-submodules ls-files --stage dirty-head >actual && > - rev="$(git -C for-submodules/dirty-head rev-parse HEAD)" && > - grep "$rev" actual > + git -C for-submodules/dirty-head rev-parse HEAD > rev && > + grep -f rev actual > ' > > test_expect_success 'set up pathological context' '
On Sun, Mar 26, 2023 at 4:21 PM Andrei Rybak <rybak.a.v@gmail.com> wrote: > On 26/03/2023 19:31, Edwin Fernando wrote: > In subject line "t3701: Avoid suppression of exit status of git" -- the first > word after a colon shouldn't be capitalized. By the way, similar existing > commits call it: > don't lose "git" exit codes > and > preserve "git" exit codes Thanks, your review comments covered almost everything I was going to say, and I agree with all you said, so I'll just add a couple comments not covered by your review. > > + git show :file > show_file && > > Code style here and in all output redirections below: lose the space between > redirection and the path, like so: > git show :file >show_file > > Also, the suffix "_file" is unnecessary. Just > git show :file >show > or > git show :file >out > or > git show :file >actual > might be better. Fully agree with either of the latter two suggestions. > > @@ -311,9 +315,12 @@ test_expect_success FILEMODE 'stage mode and hunk' ' > > - git diff --cached file | grep "new mode" && > > - git diff --cached file | grep "+content" && > > - test -z "$(git diff file)" > > + git diff --cached file > diff_file && > > + grep "new mode" diff_file && > > + git diff --cached file diff_file && > > + grep "+content" diff_file && > > + git diff file > diff_file && > > + test -z $(cat diff_file) > > Function test_stdout_line_count can be used here instead of "test -z". > test_stdout_line_count would produce a more helpful error message in > case of test failure. The other idiomatic possibility would be to use test_must_be_empty() to verify that the file is empty. > > - rev="$(git -C for-submodules/dirty-head rev-parse HEAD)" && > > - grep "$rev" actual > > + git -C for-submodules/dirty-head rev-parse HEAD > rev && > > + grep -f rev actual This change is unnecessary since this is a cases in which the exit code does not get lost; the exit code of the command substitution becomes the exit code of the assignment: % x="$(true)" % echo $? 0 % x="$(false)" % echo $? 1 So let's drop this change from the patch.
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 3a99837d9b..80446b311d 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -292,8 +292,10 @@ test_expect_success FILEMODE 'patch does not affect mode' ' echo content >>file && chmod +x file && printf "n\\ny\\n" | git add -p && - git show :file | grep content && - git diff file | grep "new mode" + git show :file > show_file && + grep content show_file && + git diff file > diff_file && + grep "new mode" diff_file ' test_expect_success FILEMODE 'stage mode but not hunk' ' @@ -301,8 +303,10 @@ test_expect_success FILEMODE 'stage mode but not hunk' ' echo content >>file && chmod +x file && printf "y\\nn\\n" | git add -p && - git diff --cached file | grep "new mode" && - git diff file | grep "+content" + git diff --cached file > diff_file && + grep "new mode" diff_file && + git diff file > diff_file && + grep "+content" diff_file ' @@ -311,9 +315,12 @@ test_expect_success FILEMODE 'stage mode and hunk' ' echo content >>file && chmod +x file && printf "y\\ny\\n" | git add -p && - git diff --cached file | grep "new mode" && - git diff --cached file | grep "+content" && - test -z "$(git diff file)" + git diff --cached file > diff_file && + grep "new mode" diff_file && + git diff --cached file diff_file && + grep "+content" diff_file && + git diff file > diff_file && + test -z $(cat diff_file) ' # end of tests disabled when filemode is not usable @@ -970,8 +977,8 @@ test_expect_success 'handle submodules' ' force_color git -C for-submodules add -p dirty-head >output 2>&1 <y && git -C for-submodules ls-files --stage dirty-head >actual && - rev="$(git -C for-submodules/dirty-head rev-parse HEAD)" && - grep "$rev" actual + git -C for-submodules/dirty-head rev-parse HEAD > rev && + grep -f rev actual ' test_expect_success 'set up pathological context' '
Pipes and command substitution in bash suppress the exit codes of the first executed command. In the test scripts errors when running git should be exposed. Remove all such suppressions of git by writing output to files, chaining commands with &&, or other means. Signed-off-by: Edwin Fernando <edwinfernando734@gmail.com> --- t/t3701-add-interactive.sh | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)