On Sun, Oct 6, 2024 at 4:31 AM Usman Akinyemi via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote: > 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 <usmanakinyemi202@xxxxxxxxx> > --- > 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@xxxxxxxxxxxxxx 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).
