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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 27, 2023, 10:11 UTC
Message-ID: <CAPig+cRS5M65MskbWKBuqtzuGUMjibbBHpTBDxLYEJF33aJX4Q@mail.gmail.com>
In-Reply-To: <be2f23ec-4b3c-eb9f-fc2d-fecdf76604e1@gmail.com>

On Sun, Mar 26, 2023 at 4:21 PM Andrei Rybak <rybak.a.v@gmail.com> wrote:

Show 7 quoted lines

> 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.

Show 13 quoted lines

> > +     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.

Show 14 quoted lines

> > @@ -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.