Re: [PATCH v2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes
- From
Usman Akinyemi <usmanakinyemi202@gmail.com>
- Date
- Oct 6, 2024, 12:28 UTC
- Message-ID
- <CAPSxiM-bx7KdhP0OyajfiTczg-WqiJDPso1EAxLzntLqcuOUkA@mail.gmail.com>
- In-Reply-To
- <CAPig+cTVf=W5KXkE2iTxCSPmZjOSbmf90J_JugDuTH_xudcW2g@mail.gmail.com>
Hello,
I really appreciate all the maintainers for their help and time.
I am a bit confused now, I am already working on converting the test_line_count right now. Can I update the patch or do something regarding the first patch ?
Thank you.
On Sun, Oct 6, 2024 at 12:06 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 25 quoted lines
> > On Sun, Oct 6, 2024 at 7:12 AM shejialuo <shejialuo@gmail.com> wrote: > > On Sun, Oct 06, 2024 at 05:19:13AM -0400, Eric Sunshine wrote: > > > 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. > > > > Except that, the commit title should not either include > > "[Outreachy][Patch v1]" here. From these two reasons, I think we should > > reroll the patch. > > Your observation about outdated/confusing "[foo]" annotations is > certainly something the submitter should take into consideration for > future submissions, but does not seem worthy of a reroll, IMHO. First, > `git am` will strip those off automatically, so they won't become part > of the permanent project history anyhow when/if Junio picks up the > patch. Second, asking for a reroll for something which does not impact > the correctness of either the patch or the commit message just makes > busy-work for the submitter and wastes reviewer time (which is a > limited resource on this project). Third, the point of a microproject > is to expose the submitter to the workflow of the Git project and to > the review process, and for reviewers to see how the submitter > responds. That goal has already been achieved in this case, and > rerolling for something so minor provides no additional benefit in > that regard.