On Sun, Oct 6, 2024 at 9:19 AM Eric Sunshine [off-list ref] wrote:

On Sun, Oct 6, 2024 at 4:31 AM Usman Akinyemi via GitGitGadget
[off-list ref] wrote:

quoted

The exit code of the preceding command in a pipe is disregarded. So
if that preceding command is a Git command that fails, the test would
not fail. Instead, by saving the output of that Git command to a file,
and removing the pipe, we make sure the test will fail if that Git
command fails.

Signed-off-by: Usman Akinyemi <redacted>
---
    After submitting the first patch, I noticed that the output of wc -l was
    failing due to trailing whitespace. I attempted to fix this by using tr
    -d to remove the whitespace. However, instead of squashing the two
    patches into one, I inadvertently created another commit.

    Eric Sunshine sunshine@sunshineco.com provided valuable feedback during
    the review process. He explained the details of the patches to me and
    pointed out that using tr -d was unnecessary to resolve the whitespace
    issue.

Thanks. This version of the patch looks fine.

I notice that there are still quite a few instances of `git` upstream
of a pipe remaining in t3404 even after this patch. But that's okay;
fixing all of them would have made the patch far longer and more
tiresome to review, so it is not a problem that you selectively
converted only `git show` and `git cat-file` cases. It probably would
have been helpful to reviewers if the patch's commit message mentioned
that it only converts some of the instances, but it's not worth
rerolling the patch just for that.

So, I think it makes sense to stop here and consider this microproject
successful (unless some other reviewer notices some problem or
requests some other change).

Thanks Eric Sunshine, I appreciate your time and review. I am more
eager to continue working on it after review from the others. And also
would like to work on the test_line_count also if permitted. Thank you
very much.