| Message ID | 20200705214547.16485-2-mstamat@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,v1,1/3] doc/developer.texi: Improvements in "Submitting patches" section. | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Manolis Stamatogiannakis (12020-07-05): > - Main text split to two sections. > - Detailed checklist for new codecs or formats demoted to section. > - Detailed checklist for patch submission demoted to section. > > Signed-off-by: Manolis Stamatogiannakis <mstamat@gmail.com> > --- > doc/developer.texi | 64 +++++++++++++++++++++++++++++++--------------- > 1 file changed, 44 insertions(+), 20 deletions(-) > > diff --git a/doc/developer.texi b/doc/developer.texi > index dec27cb509..6d4f6afcf9 100644 > --- a/doc/developer.texi > +++ b/doc/developer.texi > @@ -457,31 +457,49 @@ Finally, keep in mind the immortal words of Bill and Ted, > @anchor{Submitting patches} > @chapter Submitting patches > > -First, read the @ref{Coding Rules} above if you did not yet, in particular > +@anchor{patch guidelines} > +@section Guidelines for preparing a patch > + > +The @strong{absolute minimum} you have to do before submitting a patch are the > +following: > + > +@enumerate > +@item Carefully read the @ref{Coding Rules} above if you did not yet, in particular > the rules regarding patch submission. > > -When you submit your patch, please use @code{git format-patch} or > -@code{git send-email}. We cannot read other diffs :-). > +@item Make sure your commit messages accurately describe the changes made > +(e.g. 'replaces lrint by lrintf') and why these changes are made (e.g. > +'*BSD isn't C99 compliant and has no lrint()'). > > -Also please do not submit a patch which contains several unrelated changes. > -Split it into separate, self-contained pieces. This does not mean splitting > -file by file. Instead, make the patch as small as possible while still > -keeping it as a logical unit that contains an individual change, even > -if it spans multiple files. This makes reviewing your patches much easier > -for us and greatly increases your chances of getting your patch applied. > +@item Make sure you use @code{git format-patch} or @code{git send-email} to prepare > +your patch. We cannot read other diffs :-). > + > +@item Run the @ref{Regression tests, regression tests} before submitting a patch > +in order to verify it does not cause unexpected problems. > > -Use the patcheck tool of FFmpeg to check your patch. > -The tool is located in the tools directory. > +@item If you send your patches with an external email client > +(i.e. not @code{git send-email}), make sure to send each patch as a separate > +email. Do not attach several patches to the same email! This is a new rule, it did not exist before, and I see little value in it except making Patchwork happy. > > -Run the @ref{Regression tests} before submitting a patch in order to verify > -it does not cause unexpected problems. > +@item Do not submit a patch which contains several unrelated changes. > +@end enumerate > + > +Additionally, it is also important that the commits comprising a patch > +are logically self-contained. I.e. each commit should be as small as Uh? Are you making a distinction between commits and patches? So, can we have a single patch with several commits in one mail? Or maybe the accurate wording is just not consistent. > +possible while still containing a meaningful individual change. > +Commits spanning multiple files are perfectly fine, as long as the > +commit can be seen as a single logical unit. > > -It also helps quite a bit if you tell us what the patch does (for example > -'replaces lrint by lrintf'), and why (for example '*BSD isn't C99 compliant > -and has no lrint()') > +Following these guidelines makes reviewing your patches much easier > +for us and greatly increases your chances of getting your patch applied. > +To further reduce the chance that you will need to revise your patch, > +it is also recommended to go through the detailed > +@ref{patch submission checklist, patch} and > +@ref{new codec format checklist, new codec or format} > +checklists. > > -Also please if you send several patches, send each patch as a separate mail, > -do not attach several unrelated patches to the same mail. > +@anchor{patch submission process} > +@section Patch submission and revision process > > Patches should be posted to the > @uref{https://lists.ffmpeg.org/mailman/listinfo/ffmpeg-devel, ffmpeg-devel} > @@ -511,7 +529,8 @@ Additionally, it is recommended to register for a > This will allow you to mark previous version of your patches as "Superseded", > and reduce the chance of someone spending time to review a stale patch. > > -@chapter New codecs or formats checklist > +@anchor{new codec format checklist} > +@section New codecs or formats checklist > > @enumerate > @item > @@ -563,7 +582,8 @@ Did you make sure it compiles standalone, i.e. with > @end enumerate > > > -@chapter Patch submission checklist > +@anchor{patch submission checklist} > +@section Patch submission checklist > > @enumerate > @item > @@ -592,6 +612,10 @@ of @dfn{sign-off}. > @item > Did you provide a clear git commit log message? > > +@item > +Did you use the @code{patcheck} tool of FFmpeg to check your patch > +for common issues? E.g. @code{tools/patcheck *.patch}. > + > @item > Is the patch against latest FFmpeg git master branch? > Regards,
On Tue, 7 Jul 2020 at 16:07, Nicolas George <george@nsup.org> wrote: > Manolis Stamatogiannakis (12020-07-05): > > > +@item If you send your patches with an external email client > > +(i.e. not @code{git send-email}), make sure to send each patch as a > separate > > +email. Do not attach several patches to the same email! > > This is a new rule, it did not exist before, and I see little value in > it except making Patchwork happy. > The current documentation mentions: L1: Also please do not submit a patch which contains several unrelated changes. ... L2: Also please if you send several patches, send each patch as a separate mail, do not attach several unrelated patches to the same mail. ... L3: Use git send-email when possible since it will properly send patches without requiring extra care. The rule in the list is a summary of these three lines. I may have interpreted them wrong, as there's a slight overlap (L1-L2: they look the same) and a slight conflict (L2-L3: git send-email sends one email per commit, not per patch). Since you send your patches with an external email client, can you come up with a better/more accurate phrasing based on your workflow? > > > > -Run the @ref{Regression tests} before submitting a patch in order to > verify > > -it does not cause unexpected problems. > > +@item Do not submit a patch which contains several unrelated changes. > > +@end enumerate > > + > > > +Additionally, it is also important that the commits comprising a patch > > +are logically self-contained. I.e. each commit should be as small as > > Uh? Are you making a distinction between commits and patches? So, can we > have a single patch with several commits in one mail? > > Or maybe the accurate wording is just not consistent. > I believe that much of the wording in developer.html stems from the svn days, so it reads a bit funny if you've been using git exclusively for 5+ years. Here are the number of lines committed per year in developer.texi: 2014: 3 2018: 8 2012: 21 2015: 29 2017: 50 2016: 86 2011: 105 2020: 124 2009: 127 2013: 351 So let's stick with "patches" and forget about commits for now. Does this sound better? "Additionally, it is also important that each patch is logically self-contained. I.e. each patch should be as small as possible while still containing a meaningful individual change. Patches spanning multiple files are perfectly fine, as long as they can be seen as a single logical unit." Thanks for the review! Best regards, Manolis
diff --git a/doc/developer.texi b/doc/developer.texi index dec27cb509..6d4f6afcf9 100644 --- a/doc/developer.texi +++ b/doc/developer.texi @@ -457,31 +457,49 @@ Finally, keep in mind the immortal words of Bill and Ted, @anchor{Submitting patches} @chapter Submitting patches -First, read the @ref{Coding Rules} above if you did not yet, in particular +@anchor{patch guidelines} +@section Guidelines for preparing a patch + +The @strong{absolute minimum} you have to do before submitting a patch are the +following: + +@enumerate +@item Carefully read the @ref{Coding Rules} above if you did not yet, in particular the rules regarding patch submission. -When you submit your patch, please use @code{git format-patch} or -@code{git send-email}. We cannot read other diffs :-). +@item Make sure your commit messages accurately describe the changes made +(e.g. 'replaces lrint by lrintf') and why these changes are made (e.g. +'*BSD isn't C99 compliant and has no lrint()'). -Also please do not submit a patch which contains several unrelated changes. -Split it into separate, self-contained pieces. This does not mean splitting -file by file. Instead, make the patch as small as possible while still -keeping it as a logical unit that contains an individual change, even -if it spans multiple files. This makes reviewing your patches much easier -for us and greatly increases your chances of getting your patch applied. +@item Make sure you use @code{git format-patch} or @code{git send-email} to prepare +your patch. We cannot read other diffs :-). + +@item Run the @ref{Regression tests, regression tests} before submitting a patch +in order to verify it does not cause unexpected problems. -Use the patcheck tool of FFmpeg to check your patch. -The tool is located in the tools directory. +@item If you send your patches with an external email client +(i.e. not @code{git send-email}), make sure to send each patch as a separate +email. Do not attach several patches to the same email! -Run the @ref{Regression tests} before submitting a patch in order to verify -it does not cause unexpected problems. +@item Do not submit a patch which contains several unrelated changes. +@end enumerate + +Additionally, it is also important that the commits comprising a patch +are logically self-contained. I.e. each commit should be as small as +possible while still containing a meaningful individual change. +Commits spanning multiple files are perfectly fine, as long as the +commit can be seen as a single logical unit. -It also helps quite a bit if you tell us what the patch does (for example -'replaces lrint by lrintf'), and why (for example '*BSD isn't C99 compliant -and has no lrint()') +Following these guidelines makes reviewing your patches much easier +for us and greatly increases your chances of getting your patch applied. +To further reduce the chance that you will need to revise your patch, +it is also recommended to go through the detailed +@ref{patch submission checklist, patch} and +@ref{new codec format checklist, new codec or format} +checklists. -Also please if you send several patches, send each patch as a separate mail, -do not attach several unrelated patches to the same mail. +@anchor{patch submission process} +@section Patch submission and revision process Patches should be posted to the @uref{https://lists.ffmpeg.org/mailman/listinfo/ffmpeg-devel, ffmpeg-devel} @@ -511,7 +529,8 @@ Additionally, it is recommended to register for a This will allow you to mark previous version of your patches as "Superseded", and reduce the chance of someone spending time to review a stale patch. -@chapter New codecs or formats checklist +@anchor{new codec format checklist} +@section New codecs or formats checklist @enumerate @item @@ -563,7 +582,8 @@ Did you make sure it compiles standalone, i.e. with @end enumerate -@chapter Patch submission checklist +@anchor{patch submission checklist} +@section Patch submission checklist @enumerate @item @@ -592,6 +612,10 @@ of @dfn{sign-off}. @item Did you provide a clear git commit log message? +@item +Did you use the @code{patcheck} tool of FFmpeg to check your patch +for common issues? E.g. @code{tools/patcheck *.patch}. + @item Is the patch against latest FFmpeg git master branch?
- Main text split to two sections. - Detailed checklist for new codecs or formats demoted to section. - Detailed checklist for patch submission demoted to section. Signed-off-by: Manolis Stamatogiannakis <mstamat@gmail.com> --- doc/developer.texi | 64 +++++++++++++++++++++++++++++++--------------- 1 file changed, 44 insertions(+), 20 deletions(-)