Re: [PATCH v4 2/8] [Newcomer] t7004: Do not lose exit status to pipe

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 6, 2024, 18:05 UTC
Message-ID: <CAPig+cRQSVZrOrHUaQCG91PbWpgUEbpFVAM5sriRgTSrkCnMvg@mail.gmail.com>
In-Reply-To: <3cba795c-631b-4e86-a1f2-ddefe76ef0f4@gmail.com>

On Tue, Aug 6, 2024 at 4:38 AM AbdAlRahman Gad <abdobngad@gmail.com> wrote:

Show 19 quoted lines

> On 8/6/24 06:13, Eric Sunshine wrote:
> > On Mon, Aug 5, 2024 at 8:00 PM AbdAlRahman Gad <abdobngad@gmail.com> wrote:
> >> -       test-tool ref-store main for-each-reflog-ent refs/tags/tag_with_reflog1 | sed -e "s/^.* //" >actual &&
> >> +       test-tool ref-store main for-each-reflog-ent refs/tags/tag_with_reflog1 >actual.body &&
> >> +       sed -e "s/^.*   //" actual.body >actual &&
> >
> > It's not just `test_tool` we care about; we also (importantly) don't
> > want to see `git` itself upstream of a pipe, and there are many such
> > instances remaining in this script. Here are some common examples:
> >
> >      test $(git tag -l | wc -l) -eq 0 &&
> >      git cat-file tag "$1" | sed -e "/BEGIN PGP/q"
> >      git tag -l | grep "^tag-one-line" >actual &&
> >      forged=$(git cat-file tag ... | sed -e ... | git mktag) &&
> >      git tag -l --no-sort "foo*" | sort >actual &&
>
> Thanks for the review. Could this be done in a separate patch series? As
> modifying a patch in the beginning of a patch series requires lots of
> time to adapt the rest of the series to the change.

As the person doing the work and submitting the patches, you choose how much effort to put in. If you feel that fixing all the "`git` upstream of a pipe" problems is better left for a future "modernization" series, then that's almost certainly fine. Since reviewers are trying to help you polish your patches, they usually prefer to review shorter series than longer ones anyhow. If you decide not to make these changes, though, be sure to explain your decision in the cover letter so that reviewers understand why such a cleanup is missing from the current series.

Having said that, if you're not going to fix all the "`git` upstream of a pipe" cases, then you might consider dropping this patch from the current series since it is merely touching the "tip of the iceberg" by fixing only these two minor cases while leaving all the more significant cases unfixed. But other reviewers may feel differently.

If you're worried about the ripple effect of making significant changes to this patch which sits near the beginning of the series, then one way to sidestep the issue is to relocate this patch to the end of the series. This is possible because, unlike many other patch series in which there is inherent order in the patches where each change must follow the preceding change, that is not the case with this patch series. The specific order of patches in this series is pretty much immaterial. So, moving this patch to the end of the series would cause only a tiny ripple since it touches only two small chunks of code, and once the patch is last in the series, then it becomes easy to apply all the other "`git` upstream of a pipe" fixes.

But it's up to you if you want to do that extra work. As noted above, reviewers usually prefer a shorter simpler series over a longer more complex one, so relegating such fixes to a followup series is almost certainly acceptable. (But be sure to explain your decision in the cover letter.)