diff mbox series

[FFmpeg-devel,v1,2/3] doc/developer.texi: Restructured Submitting patches section.

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.
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Manolis Stamatogiannakis July 5, 2020, 9:45 p.m. UTC
- 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(-)

Comments

Nicolas George July 7, 2020, 2:07 p.m. UTC | #1
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,
Manolis Stamatogiannakis July 7, 2020, 4:05 p.m. UTC | #2
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 mbox series

Patch

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?