On Sun, Oct 6, 2024 at 9:19 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > 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). 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.
