Git development
 help / color / mirror / Atom feed
From: Weijie Yuan <wy@wyuan.org>
To: Patrick Steinhardt <ps@pks.im>
Cc: git@vger.kernel.org, gitster@pobox.com
Subject: Re: [RFC PATCH 1/2] doc: encourage review replies before rerolling
Date: Mon, 15 Jun 2026 22:35:28 +0800	[thread overview]
Message-ID: <ajANsC5pfuk0PAn1@wyuan.org> (raw)
In-Reply-To: <ai_7Wh7hrD8PZozg@pks.im>

On Mon, Jun 15, 2026 at 03:17:14PM +0200, Patrick Steinhardt wrote:
> On Sat, Jun 13, 2026 at 10:08:30PM +0800, Weijie Yuan wrote:
> > Review feedback should not be answered only by sending a new patch
> > version. Encourage contributors to discuss their planned response in the
> > mailing-list thread before rerolling.
> > 
> > This makes the author's reasoning explicit before the next version is
> > prepared, instead of forcing reviewers to infer it from the rerolled
> > patches.
> 
> Not only that, but it also encourages more social interactions between
> contributors.

Thank you, yes, let me add "social interactions" in v2.

> > diff --git a/Documentation/MyFirstContribution.adoc b/Documentation/MyFirstContribution.adoc
> > index 0e2a9313ce..59891e3c14 100644
> > --- a/Documentation/MyFirstContribution.adoc
> > +++ b/Documentation/MyFirstContribution.adoc
> > @@ -1423,11 +1423,13 @@ fewer mistakes were the only one they would need to review.
> >  After a few days, you will hopefully receive a reply to your patchset with some
> >  comments. Woohoo! Now you can get back to work.
> >  
> > -It's good manners to reply to each comment, notifying the reviewer that you have
> > -made the change suggested, feel the original is better, or that the comment
> > -inspired you to do something a new way which is superior to both the original
> > -and the suggested change. This way reviewers don't need to inspect your v2 to
> > -figure out whether you implemented their comment or not.
> > +It's good manners to reply to each comment in the mailing list discussion
> > +instead of letting the next version of your patch be your only response. Tell
> > +the reviewer whether you plan to make the suggested change, keep the original,
> > +or pursue a different approach. This way reviewers can respond to your reasoning
> > +before you spend time preparing a version they may not agree with, and later do
> > +not need to inspect your v2 to figure out whether you implemented their comment
> > +or not.
> >  
> >  Reviewers may ask you about what you wrote in the patchset, either in
> >  the proposed commit log message or in the changes themselves.  You
> 
> I feel like the new version doesn't really add anything significant to
> this paragraph that it didn't already say before your patch, but it does
> so with more words.
> 
> I'm of course biased though, so maybe more words help newcomers?

Yes, this diff only and merely emphasizes "use the normal response
first, rather than re-rerolling directly." a litte bit, as I described
in commit message. But indend, the existing sentence:

"This way reviewers don't need to inspect your v2 to
-figure out whether you implemented their comment or not."

basically already has the same meaning.

So it does seem a bit wordy, but in other words, explicitly emphasizing
what not to do and what to do instead is more straightforward and clear
for new contributors.

Then the newly added sentence can correspond with the existing content
at the end of this paragraph (of course, this doesn't really make much
sense.)

> Overall it's a bit on the annoying side that we have to always make sure
> to update both SubmittingPatches and MyFirstContribution in tandem.
> Makes me wonder whether they are mostly redundant and whether it would
> make sense to eventually merge them. But that's a tangent and not
> anything that needs to be addressed in this (or any other) patch series.

To be honest, when I first started reading and writing, I had this
feeling and confusion. Since I haven't gone through the history of these
two documents yet, I just completed the preset task first.

Overall, I feel that the two documents have overlapping parts as well as
their own distinct focuses, and like being intertwined with each other.

For new contributors, it seems more convenient to just look at one
document, rather than trying to understand how the two files are
cross-referenced with each other. (although I found some newcomers in
GitHub PR pages don't even read one!) But this is obviously not an easy
task for the writer.

Perhaps start a discussion in a separate thread? with all relevant
personnel? But I understand that writing documentation is time-consuming
and not easy.

Thank you very much.

  reply	other threads:[~2026-06-15 14:36 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-13 14:08 [RFC PATCH 0/2] doc: clarify review replies and reroll timing Weijie Yuan
2026-06-13 14:08 ` [RFC PATCH 1/2] doc: encourage review replies before rerolling Weijie Yuan
2026-06-15 13:17   ` Patrick Steinhardt
2026-06-15 14:35     ` Weijie Yuan [this message]
2026-06-13 14:09 ` [RFC PATCH 2/2] doc: advise batching patch rerolls Weijie Yuan
2026-06-13 16:02   ` Junio C Hamano
2026-06-13 17:05     ` Weijie Yuan
2026-06-15 13:17       ` Patrick Steinhardt
2026-06-15 14:49         ` Weijie Yuan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ajANsC5pfuk0PAn1@wyuan.org \
    --to=wy@wyuan.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=ps@pks.im \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox