| Message ID | xmqqmszg987u.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 010447cf098f7407008f2940b4fefa5351477044 |
| Headers | show |
| Series | [v4] MyFirstContribution: refrain from self-iterating too much | expand |
On 23/07/27 05:43PM, Junio C Hamano wrote: > Finding mistakes in and improving your own patches is a good idea, > but doing so too quickly is being inconsiderate to reviewers who > have just seen the initial iteration and taking their time to review > it. Encourage new developers to perform such a self review before > they send out their patches, not after. After sending a patch that > they immediately found mistakes in, they are welcome to comment on > them, mentioning what and how they plan to improve them in an > updated version, before sending out their updates. > > [...] > > +Please be considerate of the time needed by reviewers to examine each > +new version of your patch. Rather than seeing the initial version right > +now (followed by several "oops, I like this version better than the > +previous one" patches over 2 days), reviewers would strongly prefer if a > +single polished version came 2 days later instead, and that version with > +fewer mistakes were the only one they would need to review. > + > + > [...] Speaking as a fairly green contributor to the project, it may be helpful to include some guidance on what is "too long" vs "too short" when waiting to send out the next revision. Likewise it may be worthwhile to mention how the expected "minimum time between revisions" will generally shrink as you get to higher revision counts and fewer changes between revisions.
Junio C Hamano <gitster@pobox.com> writes: > ... > > The inter/range-diff with my v3 was totally useless, but here is to > show three minor edits I made to Linus's version I am responding to. > > * Simplify parenthesized "because it may be the case that". > > * As if you were "a" reviewer, as we do not designate "the > reviewer(s)" to a patch. Anybody can (volunteer to) be a > reviewer for a patch, and you can be, too. > > * Stress that a single polished patch that comes later (because it > took time to polish) is vastly preferred than flurry of "oops > this is better" updates. All very reasonable and sensible. LGTM, thanks!
Jacob Abel <jacobabel@nullpo.dev> writes: > On 23/07/27 05:43PM, Junio C Hamano wrote: >> Finding mistakes in and improving your own patches is a good idea, >> but doing so too quickly is being inconsiderate to reviewers who >> have just seen the initial iteration and taking their time to review >> it. Encourage new developers to perform such a self review before >> they send out their patches, not after. After sending a patch that >> they immediately found mistakes in, they are welcome to comment on >> them, mentioning what and how they plan to improve them in an >> updated version, before sending out their updates. >> >> [...] >> >> +Please be considerate of the time needed by reviewers to examine each >> +new version of your patch. Rather than seeing the initial version right >> +now (followed by several "oops, I like this version better than the >> +previous one" patches over 2 days), reviewers would strongly prefer if a >> +single polished version came 2 days later instead, and that version with >> +fewer mistakes were the only one they would need to review. >> + >> + >> [...] > > Speaking as a fairly green contributor to the project, it may be helpful > to include some guidance on what is "too long" vs "too short" when > waiting to send out the next revision. We generally do not want to be too prescriptive, as the right interval depends on many factors like the complexity of the topic, how busy the reviewers are otherwise, etc. And that is why I did not go any more specific than "several rounds within 2 days is way too frequent". But as a general common-sense guideline, I would encourage people to wait for at least one earth rotation, given that there are list participants across many timezones. I do not know offhand how to fit that well in the narrative being proposed, though. > Likewise it may be worthwhile to mention how the expected "minimum time > between revisions" will generally shrink as you get to higher revision > counts and fewer changes between revisions. I am not sure if I follow. As a topic gets iterated and getting closer to completion, maximum time allowed between revisions to keep the minds of those involved in the topic fresh may shrink, but I do not think it would affect the minimum interval too much. Thanks.
Linus Arver <linusa@google.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> ... >> >> The inter/range-diff with my v3 was totally useless, but here is to >> show three minor edits I made to Linus's version I am responding to. >> >> * Simplify parenthesized "because it may be the case that". >> >> * As if you were "a" reviewer, as we do not designate "the >> reviewer(s)" to a patch. Anybody can (volunteer to) be a >> reviewer for a patch, and you can be, too. >> >> * Stress that a single polished patch that comes later (because it >> took time to polish) is vastly preferred than flurry of "oops >> this is better" updates. > > All very reasonable and sensible. LGTM, thanks! Thanks.
On Thu, Jul 27, 2023 at 05:43:17PM -0700, Junio C Hamano wrote: > Finding mistakes in and improving your own patches is a good idea, > but doing so too quickly is being inconsiderate to reviewers who > have just seen the initial iteration and taking their time to review > it. Encourage new developers to perform such a self review before > they send out their patches, not after. After sending a patch that > they immediately found mistakes in, they are welcome to comment on > them, mentioning what and how they plan to improve them in an > updated version, before sending out their updates. That's all good, no possible improvements from my side. However, a possible question below. [] > +Please give reviewers enough time to process your initial patch before > +sending an updated version. That is, resist the temptation to send a new > +version immediately, because others may have already started reviewing > +your initial version. > + > +While waiting for review comments, you may find mistakes in your initial > +patch, or perhaps realize a different and better way to achieve the goal > +of the patch. In this case you may communicate your findings to other > +reviewers as follows: > + > + - If the mistakes you found are minor, send a reply to your patch as if > + you were a reviewer and mention that you will fix them in an > + updated version. > + > + - On the other hand, if you think you want to change the course so > + drastically that reviews on the initial patch would be a waste of > + time (for everyone involved), retract the patch immediately with > + a reply like "I am working on a much better approach, so please > + ignore this patch and wait for the updated version." > + (That's all good) > +Now, the above is a good practice if you sent your initial patch > +prematurely without polish. But a better approach of course is to avoid > +sending your patch prematurely in the first place. That is of course a good suggestion. I wonder, how much a first time contributor knows about "polishing", in the Git sense ? From my experience, the polishing is or could be a learning process, which needs interaction with the reviewers. Would it make sense to remove the sentences above and ask people to mark their patch with RFC ? Or is this all too much bikeshedding, IOW I am happy with V4 as is.
Torsten Bögershausen <tboegi@web.de> writes: >> +While waiting for review comments, you may find mistakes in your initial >> +patch, or perhaps realize a different and better way to achieve the goal >> +of the patch. In this case you may communicate your findings to other >> +reviewers as follows: >> + >> + - If the mistakes you found are minor, send a reply to your patch as if >> + you were a reviewer and mention that you will fix them in an >> + updated version. >> + >> + - On the other hand, if you think you want to change the course so >> + drastically that reviews on the initial patch would be a waste of >> + time (for everyone involved), retract the patch immediately with >> + a reply like "I am working on a much better approach, so please >> + ignore this patch and wait for the updated version." >> + > (That's all good) > > >> +Now, the above is a good practice if you sent your initial patch >> +prematurely without polish. But a better approach of course is to avoid >> +sending your patch prematurely in the first place. > > That is of course a good suggestion. > I wonder, how much a first time contributor knows about "polishing", > in the Git sense ? I do not know if "without polish" has any strong "Git sense" to begin with. The actions the contributor would have done, referred to as "the above", are the result of re-reading their own patches and re-thinking their own approaches, which led them to discover fixes and alternative solutions, and I was hoping that "without polish" would be understood by anybody to mean "a version that did not go through such proofreading" without knowing much about how we develop our patches. I am OK to just say "sent your initial patch prematurely" and without "without polish". I do think it would make the resulting text encourage less strongly to proofread their own patches before sending them, but if you think "polish" may not be understood, I am fine with such a copyediting. Or using some alternative phrases is also fine, if it improves our chances to be understood better. > From my experience, the polishing is or could be a learning process, > which needs interaction with the reviewers. Yes, once they see what valuable insight reviewers offer, in their next topic, they will learn to stop and think before sending the fresh-off-the-press version without sleeping on it a bit. > Would it make sense to remove the sentences above and ask people > to mark their patch with RFC ? I doubt it. Nobody will stay newbie forever. If they do not know what kind of flaws to look for and how to find them themselves in their work, that is perfectly OK and they can just send a regular PATCH. A reviewer hopefully will notice and point out that it is not yet beyond RFC quality if that is the case, but this document should not be suggesting that before seeing their work ;-) > Or is this all too much bikeshedding, IOW I am happy with V4 as is. Thanks.
diff --git a/Documentation/MyFirstContribution.txt b/Documentation/MyFirstContribution.txt index ccfd0cb5f3..dd46f751b7 100644 --- a/Documentation/MyFirstContribution.txt +++ b/Documentation/MyFirstContribution.txt @@ -1256,6 +1256,38 @@ index 88f126184c..38da593a60 100644 [[now-what]] == My Patch Got Emailed - Now What? +Please give reviewers enough time to process your initial patch before +sending an updated version. That is, resist the temptation to send a new +version immediately, because others may have already started reviewing +your initial version. + +While waiting for review comments, you may find mistakes in your initial +patch, or perhaps realize a different and better way to achieve the goal +of the patch. In this case you may communicate your findings to other +reviewers as follows: + + - If the mistakes you found are minor, send a reply to your patch as if + you were a reviewer and mention that you will fix them in an + updated version. + + - On the other hand, if you think you want to change the course so + drastically that reviews on the initial patch would be a waste of + time (for everyone involved), retract the patch immediately with + a reply like "I am working on a much better approach, so please + ignore this patch and wait for the updated version." + +Now, the above is a good practice if you sent your initial patch +prematurely without polish. But a better approach of course is to avoid +sending your patch prematurely in the first place. + +Please be considerate of the time needed by reviewers to examine each +new version of your patch. Rather than seeing the initial version right +now (followed by several "oops, I like this version better than the +previous one" patches over 2 days), reviewers would strongly prefer if a +single polished version came 2 days later instead, and that version with +fewer mistakes were the only one they would need to review. + + [[reviewing]] === Responding to Reviews
Finding mistakes in and improving your own patches is a good idea, but doing so too quickly is being inconsiderate to reviewers who have just seen the initial iteration and taking their time to review it. Encourage new developers to perform such a self review before they send out their patches, not after. After sending a patch that they immediately found mistakes in, they are welcome to comment on them, mentioning what and how they plan to improve them in an updated version, before sending out their updates. Helped-by: Torsten Bögershausen <tboegi@web.de> Helped-by: Linus Arver <linusa@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- The inter/range-diff with my v3 was totally useless, but here is to show three minor edits I made to Linus's version I am responding to. * Simplify parenthesized "because it may be the case that". * As if you were "a" reviewer, as we do not designate "the reviewer(s)" to a patch. Anybody can (volunteer to) be a reviewer for a patch, and you can be, too. * Stress that a single polished patch that comes later (because it took time to polish) is vastly preferred than flurry of "oops this is better" updates. --- /var/tmp/b 2023-07-27 17:38:56.928040307 -0700 +++ /var/tmp/a 2023-07-27 17:38:36.100067020 -0700 @@ -1,7 +1,7 @@ Please give reviewers enough time to process your initial patch before sending an updated version. That is, resist the temptation to send a new -version immediately (because it may be the case that others may have -already started reviewing your initial version). +version immediately, because others may have already started reviewing +your initial version. While waiting for review comments, you may find mistakes in your initial patch, or perhaps realize a different and better way to achieve the goal @@ -9,7 +9,7 @@ reviewers as follows: - If the mistakes you found are minor, send a reply to your patch as if - you were the reviewer and mention that you will fix them in an + you were a reviewer and mention that you will fix them in an updated version. - On the other hand, if you think you want to change the course so @@ -26,5 +26,5 @@ new version of your patch. Rather than seeing the initial version right now (followed by several "oops, I like this version better than the previous one" patches over 2 days), reviewers would strongly prefer if a -single polished version came instead, and that version with fewer -mistakes were the only one they would need to review. +single polished version came 2 days later instead, and that version with +fewer mistakes were the only one they would need to review. Thanks. Documentation/MyFirstContribution.txt | 32 +++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)