| Message ID | 20240509211318.641896-3-gitster@pobox.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Describe patch-flow better in SubmittingPatches | expand |
Junio C Hamano <gitster@pobox.com> writes: > Explain a full lifecycle of a patch series upfront, so that it is > clear when key decisions to "accept" a series is made and how a new > patch series becomes a part of a new release. > > Earlier, we described an idealized patch flow that nobody followed > in practice. Instead, describe what flow was used in practice for > the past decade that worked well for us. > > Fold the "you need to monitor the progress of your topic" section > into the primary "patch lifecycle" section, as that is one of the > things the patch submitter is responsible for. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > Documentation/SubmittingPatches | 112 +++++++++++++++++++------------- > 1 file changed, 68 insertions(+), 44 deletions(-) > > diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches > index 142b82a71b..8922aae4a5 100644 > --- a/Documentation/SubmittingPatches > +++ b/Documentation/SubmittingPatches > @@ -8,53 +8,76 @@ project. There is also a link:MyFirstContribution.html[step-by-step tutorial] > available which covers many of these same guidelines. > > [[patch-flow]] > -=== An ideal patch flow > - > -Here is an ideal patch flow for this project the current maintainer > -suggests to the contributors: > - > -. You come up with an itch. You code it up. > - > -. Send it to the list and cc people who may need to know about > - the change. > +=== A not-so ideal patch flow > + > +To help us understand the reason behind various guidelines given later > +in the document, first lets understand how the lifecycle of a typical > +patch series for this project goes. > + > +. You come up with an itch. You code it up. You do not need any > + pre-authorization from the project to do so. Your patches will be Wouldn't it be better to have the following sentences after the next para? So the flow would be - Have an itch. Code it up. - Send patches to list. - Get reviews. > + reviewed by other contributors on the mailing list, and the reviews > + will be done to assess the merit of various things, like the general > + idea behind your patch (including "is it solving a problem worth > + solving in the first place?"), the reason behind the design of the > + solution, and the actual implementation. > + > +. You send the patches to the list and cc people who may need to know > + about the change. Your goal is *not* necessarily to convince others > + that what you are building is a good idea. Your goal is to get help > + in coming up with a solution for the "itch" that is better than what > + you can build alone. > + > -The people who may need to know are the ones whose code you > -are butchering. These people happen to be the ones who are > +The people who may need to know are the ones who worked on the code > +you are touching. These people happen to be the ones who are This reads much _nicer_. > most likely to be knowledgeable enough to help you, but > -they have no obligation to help you (i.e. you ask for help, > -don't demand). +git log -p {litdd} _$area_you_are_modifying_+ would > +they have no obligation to help you (i.e. you ask them for help, > +you don't demand). +git log -p {litdd} _$area_you_are_modifying_+ would > help you find out who they are. > > -. You get comments and suggestions for improvements. You may > - even get them in an "on top of your change" patch form. > - > -. Polish, refine, and re-send to the list and the people who > - spend their time to improve your patch. Go back to step (2). > - > -. The list forms consensus that the last round of your patch is > - good. Send it to the maintainer and cc the list. > - > -. A topic branch is created with the patch and is merged to `next`, > - and cooked further and eventually graduates to `master`. > - > -In any time between the (2)-(3) cycle, the maintainer may pick it up > -from the list and queue it to `seen`, in order to make it easier for > -people to play with it without having to pick up and apply the patch to > -their trees themselves. > - > -[[patch-status]] > -=== Know the status of your patch after submission > - > -* You can use Git itself to find out when your patch is merged in > - master. `git pull --rebase` will automatically skip already-applied > - patches, and will let you know. This works only if you rebase on top > - of the branch in which your patch has been merged (i.e. it will not > - tell you if your patch is merged in `seen` if you rebase on top of > - master). > +. You get comments and suggestions for improvements. You may even get > + them in an "on top of your change" patch form. You are expected to > + respond to them with "Reply-All" on the mailing list, while taking > + them into account while preparing an updated set of patches. > + > +. Polish, refine, and re-send your patches to the list and the people who > + spent their time to improve your patch. Go back to step (2). > + > +. While the above iterations improve your patches, the maintainer may > + pick the patches up from the list and queue them to the `seen` > + branch, in order to make it easier for people to play with it > + without having to pick up and apply the patches to their trees > + themselves. Being in `seen` has no other meaning. Specifically, it > + does not mean the patch was "accepted" in any way. > + > +. When the discussion reaches a consensus that the latest iteration of > + the patches are in good enough shape, the maintainer includes the > + topic in the "What's cooking" report that are sent out a few times a > + week to the mailing list, marked as "Will merge to 'next'." This > + decision is primarily made by the maintainer with the help from > + reviewers. > + > +. Once the patches hit 'next', the discussion can still continue to > + further improve them by adding more patches on top, but by the time > + a topic gets merged to 'next', it is expected that everybody agreed > + that the scope and the basic direction of the topic are appropriate, > + so such an incremental updates are expected to be limited to small > + corrections and polishing. After a topic cooks for some time (like > + 7 calendar days) in 'next' without further tweaks on top, it gets > + merged to the 'master' branch and wait to become part of the next > + major release. > + > +Earlier versions of this document outlined a slightly different patch > +flow in an idealized world, where the original submitter gathered > +agreements from the participants of the discussion and sent the final > +"we all agreed that this is the good version--please apply" patches > +to the maintainer. In practice, this almost never happened. The flow > +described above reflects the reality much better and can be considered > +the "canonical" procedure to get the patch accepted to the project. > + > +In the following sections, many techniques and conventions are listed > +to help your patches get reviewed effectively. > > -* Read the Git mailing list, the maintainer regularly posts messages > - entitled "What's cooking in git.git" giving > - the status of various proposed changes. > > [[choose-starting-point]] > === Choose a starting point. > @@ -241,8 +264,9 @@ reasons: > which case, they can explain why they extend your code to cover > files, too). > > -The goal of your log message is to convey the _why_ behind your > -change to help future developers. > +The goal of your log message is to convey the _why_ behind your change > +to help future developers. The reviewers will also make sure that > +your proposed log message will serve this purpose well. > > The first line of the commit message should be a short description (50 > characters is the soft limit, see DISCUSSION in linkgit:git-commit[1]), > -- > 2.45.0-119-g0f3415f1f8 Thanks, this is great improvement.
Karthik Nayak <karthik.188@gmail.com> writes: >> +=== A not-so ideal patch flow >> + >> +To help us understand the reason behind various guidelines given later >> +in the document, first lets understand how the lifecycle of a typical >> +patch series for this project goes. >> + >> +. You come up with an itch. You code it up. You do not need any >> + pre-authorization from the project to do so. Your patches will be > > Wouldn't it be better to have the following sentences after the next > para? > > So the flow would be > - Have an itch. Code it up. > - Send patches to list. > - Get reviews. I am not sure what exactly you are suggesting. "The next para" meaning? The sentence far below that begins with "In the following sections, many techniques and ..."? Also, "Get reviews" is not a single step that is an end of story, so what you wrote is a bit misleading as a short summary. The goal of this update is to reduce duplicates by describing a typical life-cycle of a patch series from the inception of an idea to the decision to include it in the next release here, so the proposed "decision making" document can focus on issues at a level larger than a topic of a patch series, and a contributor, especially a new one who wants to give us their first patch series, can learn by only reading these paragraphs how the world works around here with their patch series from the beginning to the end. So what happens after "Get reviews." is a part of the same "flow". Namely these three paragraphs---the original submitter cannot just leave with "now it is their problem" after they get reviews. They are now integral part of the discussion and we expect to see them see the process through. >> +. While the above iterations improve your patches, the maintainer may >> + pick the patches up from the list and queue them to the `seen` >> + branch, in order to make it easier for people to play with it >> + without having to pick up and apply the patches to their trees >> + themselves. Being in `seen` has no other meaning. Specifically, it >> + does not mean the patch was "accepted" in any way. >> + >> +. When the discussion reaches a consensus that the latest iteration of >> + the patches are in good enough shape, the maintainer includes the >> + topic in the "What's cooking" report that are sent out a few times a >> + week to the mailing list, marked as "Will merge to 'next'." This >> + decision is primarily made by the maintainer with the help from >> + reviewers. >> + >> +. Once the patches hit 'next', the discussion can still continue to >> + further improve them by adding more patches on top, but by the time >> + a topic gets merged to 'next', it is expected that everybody agreed >> + that the scope and the basic direction of the topic are appropriate, >> + so such an incremental updates are expected to be limited to small >> + corrections and polishing. After a topic cooks for some time (like >> + 7 calendar days) in 'next' without further tweaks on top, it gets >> + merged to the 'master' branch and wait to become part of the next >> + major release. >> +Earlier versions of this document outlined a slightly different patch >> +flow in an idealized world, where the original submitter gathered >> +agreements from the participants of the discussion and sent the final >> +"we all agreed that this is the good version--please apply" patches >> +to the maintainer. In practice, this almost never happened. The flow >> +described above reflects the reality much better and can be considered >> +the "canonical" procedure to get the patch accepted to the project. I actually was expecting to hear more comments about this paragraph, which makes a lame excuse for naming the section "A not-so ideal". After sleeping on it, I think it belongs to the log message of this change, not here. Future wanna-be developers do not have to know what process we wanted to have---they benefit from reading what the process _is_ in practice in a more direct way. >> +In the following sections, many techniques and conventions are listed >> +to help your patches get reviewed effectively. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >>> +=== A not-so ideal patch flow >>> + >>> +To help us understand the reason behind various guidelines given later >>> +in the document, first lets understand how the lifecycle of a typical >>> +patch series for this project goes. >>> + >>> +. You come up with an itch. You code it up. You do not need any >>> + pre-authorization from the project to do so. Your patches will be >> >> Wouldn't it be better to have the following sentences after the next >> para? >> >> So the flow would be >> - Have an itch. Code it up. >> - Send patches to list. >> - Get reviews. > > I am not sure what exactly you are suggesting. "The next para" > meaning? The sentence far below that begins with "In the following > sections, many techniques and ..."? > Let me be more verbose, I was suggesting to change it to: +. You come up with an itch. You code it up. You do not need any + pre-authorization from the project to do so. + +. You send the patches to the list and cc people who may need to know + about the change. Your goal is *not* necessarily to convince others + that what you are building is a good idea. Your goal is to get help + in coming up with a solution for the "itch" that is better than what + you can build alone. + +. Your patches will be reviewed by other contributors on the mailing + list, and the reviews will be done to assess the merit of various + things, like the general idea behind your patch (including "is it + solving a problem worth solving in the first place?"), the reason + behind the design of the solution, and the actual implementation. > Also, "Get reviews" is not a single step that is an end of story, so > what you wrote is a bit misleading as a short summary. > I was trying to be brief, my intention was to capture the whole block which, I agree wasn't the best shortening. > The goal of this update is to reduce duplicates by describing a > typical life-cycle of a patch series from the inception of an idea > to the decision to include it in the next release here, so the > proposed "decision making" document can focus on issues at a level > larger than a topic of a patch series, and a contributor, especially > a new one who wants to give us their first patch series, can learn > by only reading these paragraphs how the world works around here > with their patch series from the beginning to the end. So what > happens after "Get reviews." is a part of the same "flow". Namely > these three paragraphs---the original submitter cannot just leave > with "now it is their problem" after they get reviews. They are now > integral part of the discussion and we expect to see them see the > process through. I see what you're trying to say, my comment was on the fact that it seemed like there was jumps between the timeline of events and seemed confusing, with my suggestion we still hold the same points and just reshuffle the order a little to ensure that the flow of events is a little easier to understand. >>> +. While the above iterations improve your patches, the maintainer may >>> + pick the patches up from the list and queue them to the `seen` >>> + branch, in order to make it easier for people to play with it >>> + without having to pick up and apply the patches to their trees >>> + themselves. Being in `seen` has no other meaning. Specifically, it >>> + does not mean the patch was "accepted" in any way. >>> + >>> +. When the discussion reaches a consensus that the latest iteration of >>> + the patches are in good enough shape, the maintainer includes the >>> + topic in the "What's cooking" report that are sent out a few times a >>> + week to the mailing list, marked as "Will merge to 'next'." This >>> + decision is primarily made by the maintainer with the help from >>> + reviewers. >>> + >>> +. Once the patches hit 'next', the discussion can still continue to >>> + further improve them by adding more patches on top, but by the time >>> + a topic gets merged to 'next', it is expected that everybody agreed >>> + that the scope and the basic direction of the topic are appropriate, >>> + so such an incremental updates are expected to be limited to small >>> + corrections and polishing. After a topic cooks for some time (like >>> + 7 calendar days) in 'next' without further tweaks on top, it gets >>> + merged to the 'master' branch and wait to become part of the next >>> + major release. > I wasn't referring to the above three paragraphs and I agree with the points laid down here, I also happened to have missed reading and responding to your mail on my series [1], around how to iterate your patches and check for and resolve conflicts with other ongoing topics. I think that would be a great value add to these points. I will smoothen it and send it to the list soon. >>> +Earlier versions of this document outlined a slightly different patch >>> +flow in an idealized world, where the original submitter gathered >>> +agreements from the participants of the discussion and sent the final >>> +"we all agreed that this is the good version--please apply" patches >>> +to the maintainer. In practice, this almost never happened. The flow >>> +described above reflects the reality much better and can be considered >>> +the "canonical" procedure to get the patch accepted to the project. > > I actually was expecting to hear more comments about this paragraph, > which makes a lame excuse for naming the section "A not-so ideal". > After sleeping on it, I think it belongs to the log message of this > change, not here. Future wanna-be developers do not have to know > what process we wanted to have---they benefit from reading what the > process _is_ in practice in a more direct way. > >>> +In the following sections, many techniques and conventions are listed >>> +to help your patches get reviewed effectively. > > Thanks. Thanks [1]: https://lore.kernel.org/git/xmqqy18lpoqg.fsf@gitster.g/
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index 142b82a71b..8922aae4a5 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -8,53 +8,76 @@ project. There is also a link:MyFirstContribution.html[step-by-step tutorial] available which covers many of these same guidelines. [[patch-flow]] -=== An ideal patch flow - -Here is an ideal patch flow for this project the current maintainer -suggests to the contributors: - -. You come up with an itch. You code it up. - -. Send it to the list and cc people who may need to know about - the change. +=== A not-so ideal patch flow + +To help us understand the reason behind various guidelines given later +in the document, first lets understand how the lifecycle of a typical +patch series for this project goes. + +. You come up with an itch. You code it up. You do not need any + pre-authorization from the project to do so. Your patches will be + reviewed by other contributors on the mailing list, and the reviews + will be done to assess the merit of various things, like the general + idea behind your patch (including "is it solving a problem worth + solving in the first place?"), the reason behind the design of the + solution, and the actual implementation. + +. You send the patches to the list and cc people who may need to know + about the change. Your goal is *not* necessarily to convince others + that what you are building is a good idea. Your goal is to get help + in coming up with a solution for the "itch" that is better than what + you can build alone. + -The people who may need to know are the ones whose code you -are butchering. These people happen to be the ones who are +The people who may need to know are the ones who worked on the code +you are touching. These people happen to be the ones who are most likely to be knowledgeable enough to help you, but -they have no obligation to help you (i.e. you ask for help, -don't demand). +git log -p {litdd} _$area_you_are_modifying_+ would +they have no obligation to help you (i.e. you ask them for help, +you don't demand). +git log -p {litdd} _$area_you_are_modifying_+ would help you find out who they are. -. You get comments and suggestions for improvements. You may - even get them in an "on top of your change" patch form. - -. Polish, refine, and re-send to the list and the people who - spend their time to improve your patch. Go back to step (2). - -. The list forms consensus that the last round of your patch is - good. Send it to the maintainer and cc the list. - -. A topic branch is created with the patch and is merged to `next`, - and cooked further and eventually graduates to `master`. - -In any time between the (2)-(3) cycle, the maintainer may pick it up -from the list and queue it to `seen`, in order to make it easier for -people to play with it without having to pick up and apply the patch to -their trees themselves. - -[[patch-status]] -=== Know the status of your patch after submission - -* You can use Git itself to find out when your patch is merged in - master. `git pull --rebase` will automatically skip already-applied - patches, and will let you know. This works only if you rebase on top - of the branch in which your patch has been merged (i.e. it will not - tell you if your patch is merged in `seen` if you rebase on top of - master). +. You get comments and suggestions for improvements. You may even get + them in an "on top of your change" patch form. You are expected to + respond to them with "Reply-All" on the mailing list, while taking + them into account while preparing an updated set of patches. + +. Polish, refine, and re-send your patches to the list and the people who + spent their time to improve your patch. Go back to step (2). + +. While the above iterations improve your patches, the maintainer may + pick the patches up from the list and queue them to the `seen` + branch, in order to make it easier for people to play with it + without having to pick up and apply the patches to their trees + themselves. Being in `seen` has no other meaning. Specifically, it + does not mean the patch was "accepted" in any way. + +. When the discussion reaches a consensus that the latest iteration of + the patches are in good enough shape, the maintainer includes the + topic in the "What's cooking" report that are sent out a few times a + week to the mailing list, marked as "Will merge to 'next'." This + decision is primarily made by the maintainer with the help from + reviewers. + +. Once the patches hit 'next', the discussion can still continue to + further improve them by adding more patches on top, but by the time + a topic gets merged to 'next', it is expected that everybody agreed + that the scope and the basic direction of the topic are appropriate, + so such an incremental updates are expected to be limited to small + corrections and polishing. After a topic cooks for some time (like + 7 calendar days) in 'next' without further tweaks on top, it gets + merged to the 'master' branch and wait to become part of the next + major release. + +Earlier versions of this document outlined a slightly different patch +flow in an idealized world, where the original submitter gathered +agreements from the participants of the discussion and sent the final +"we all agreed that this is the good version--please apply" patches +to the maintainer. In practice, this almost never happened. The flow +described above reflects the reality much better and can be considered +the "canonical" procedure to get the patch accepted to the project. + +In the following sections, many techniques and conventions are listed +to help your patches get reviewed effectively. -* Read the Git mailing list, the maintainer regularly posts messages - entitled "What's cooking in git.git" giving - the status of various proposed changes. [[choose-starting-point]] === Choose a starting point. @@ -241,8 +264,9 @@ reasons: which case, they can explain why they extend your code to cover files, too). -The goal of your log message is to convey the _why_ behind your -change to help future developers. +The goal of your log message is to convey the _why_ behind your change +to help future developers. The reviewers will also make sure that +your proposed log message will serve this purpose well. The first line of the commit message should be a short description (50 characters is the soft limit, see DISCUSSION in linkgit:git-commit[1]),
Explain a full lifecycle of a patch series upfront, so that it is clear when key decisions to "accept" a series is made and how a new patch series becomes a part of a new release. Earlier, we described an idealized patch flow that nobody followed in practice. Instead, describe what flow was used in practice for the past decade that worked well for us. Fold the "you need to monitor the progress of your topic" section into the primary "patch lifecycle" section, as that is one of the things the patch submitter is responsible for. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/SubmittingPatches | 112 +++++++++++++++++++------------- 1 file changed, 68 insertions(+), 44 deletions(-)