Re: [GSOC][PATCH 1/1] t3701: Avoid suppression of exit status of git

[Eric Sunshine <sunshine@sunshineco.com>]

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.