On Sun, Mar 26, 2023 at 4:21 PM Andrei Rybak <rybak.a.v@xxxxxxxxx> 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.
