[FFmpeg-devel] [PATCH v2 1/2] doc/developer: reword some of the policies

[James Almer]

On 10/1/2016 9:51 PM, Josh de Kock wrote:
> Explicitly state that FATE should pass, and code should work
> for all reviewers who tested.
> 
> Signed-off-by: Josh de Kock <josh at itanimul.li>
> ---
>  doc/developer.texi | 91 ++++++++++++++++++++++++++----------------------------
>  1 file changed, 44 insertions(+), 47 deletions(-)
> 
> diff --git a/doc/developer.texi b/doc/developer.texi
> index 4d3a7ae..0075a27 100644
> --- a/doc/developer.texi
> +++ b/doc/developer.texi
> @@ -247,7 +247,7 @@ For Emacs, add these roughly equivalent lines to your @file{.emacs.d/init.el}:
>  @section Development Policy
>  
>  @enumerate
> - at item
> + at item Licenses for patches must be compatible with FFmpeg.
>  Contributions should be licensed under the
>  @uref{http://www.gnu.org/licenses/lgpl-2.1.html, LGPL 2.1},
>  including an "or any later version" clause, or, if you prefer
> @@ -260,15 +260,15 @@ preferred.
>  If you add a new file, give it a proper license header. Do not copy and
>  paste it from a random place, use an existing file as template.
>  
> - at item
> -You must not commit code which breaks FFmpeg! (Meaning unfinished but
> -enabled code which breaks compilation or compiles but does not work or
> -breaks the regression tests)
> -You can commit unfinished stuff (for testing etc), but it must be disabled
> -(#ifdef etc) by default so it does not interfere with other developers'
> -work.
> + at item You must not commit code which breaks FFmpeg!
> +This means unfinished code which is enabled and breaks compilation,
> +or compiles but does not work/breaks the regression tests. Code which
> +is unfinished but disabled may be permitted under-circumstances, like
> +missing samples or an implementation with a small subset of features.
> +Always check the mailing list for any reviewers with issues and test
> +FATE before you push.
>  
> - at item
> + at item Keep the main commit message short with an extended description below.
>  The commit message should have a short first line in the form of
>  a @samp{topic: short description} as a header, separated by a newline
>  from the body consisting of an explanation of why the change is necessary.
> @@ -276,30 +276,29 @@ If the commit fixes a known bug on the bug tracker, the commit message
>  should include its bug ID. Referring to the issue on the bug tracker does
>  not exempt you from writing an excerpt of the bug in the commit message.
>  
> - at item
> -You do not have to over-test things. If it works for you, and you think it
> -should work for others, then commit. If your code has problems
> -(portability, triggers compiler bugs, unusual environment etc) they will be
> -reported and eventually fixed.
> -
> - at item
> -Do not commit unrelated changes together, split them into self-contained
> -pieces. Also do not forget that if part B depends on part A, but A does not
> -depend on B, then A can and should be committed first and separate from B.
> -Keeping changes well split into self-contained parts makes reviewing and
> -understanding them on the commit log mailing list easier. This also helps
> -in case of debugging later on.
> + at item Testing must be adequate but not excessive.
> +If it works for you, others, and passes FATE then it should be OK to commit
> +it, provided it fits the other committing criteria. You should not worry about
> +over-testing things. If your code has problems (portability, triggers
> +compiler bugs, unusual environment etc) they will be reported and eventually
> +fixed.
> +
> + at item Do not commit unrelated changes together.
> +They should be split them into self-contained pieces. Also do not forget
> +that if part B depends on part A, but A does not depend on B, then A can
> +and should be committed first and separate from B. Keeping changes well
> +split into self-contained parts makes reviewing and understanding them on
> +the commit log mailing list easier. This also helps in case of debugging
> +later on.
>  Also if you have doubts about splitting or not splitting, do not hesitate to
>  ask/discuss it on the developer mailing list.
>  
> - at item
> + at item API/ABI breakages and changes should be discussed before they are made.
>  Do not change behavior of the programs (renaming options etc) or public
>  API or ABI without first discussing it on the ffmpeg-devel mailing list.
> -Do not remove functionality from the code. Just improve!
> -
> -Note: Redundant code can be removed.
> +Do not remove widely used functionality or features (redundant code can be removed).
>  
> - at item
> + at item Ask before you change the build system (configure, etc).
>  Do not commit changes to the build system (Makefiles, configure script)
>  which change behavior, defaults etc, without asking first. The same
>  applies to compiler warning fixes, trivial looking fixes and to code
> @@ -308,7 +307,7 @@ the way we do. Send your changes as patches to the ffmpeg-devel mailing
>  list, and if the code maintainers say OK, you may commit. This does not
>  apply to files you wrote and/or maintain.
>  
> - at item
> + at item Cosmetic changes should be kept in separate patches.
>  We refuse source indentation and other cosmetic changes if they are mixed
>  with functional changes, such commits will be rejected and removed. Every
>  developer has his own indentation style, you should not change it. Of course
> @@ -322,7 +321,7 @@ NOTE: If you had to put if()@{ .. @} over a large (> 5 lines) chunk of code,
>  then either do NOT change the indentation of the inner part within (do not
>  move it to the right)! or do so in a separate commit
>  
> - at item
> + at item Commit messages should always be filled out properly.
>  Always fill out the commit log message. Describe in a few lines what you
>  changed and why. You can refer to mailing list postings if you fix a
>  particular bug. Comments such as "fixed!" or "Changed it." are unacceptable.
> @@ -334,35 +333,35 @@ area changed: Short 1 line description
>  details describing what and why and giving references.
>  @end example
>  
> - at item
> + at item Credit the author of the patch.
>  Make sure the author of the commit is set correctly. (see git commit --author)
>  If you apply a patch, send an
>  answer to ffmpeg-devel (or wherever you got the patch from) saying that
>  you applied the patch.
>  
> - at item
> + at item Complex patches should refer to discussion surrounding them.
>  When applying patches that have been discussed (at length) on the mailing
>  list, reference the thread in the log message.
>  
> - at item
> + at item Always wait long enough before pushing changes
>  Do NOT commit to code actively maintained by others without permission.
> -Send a patch to ffmpeg-devel instead. If no one answers within a reasonable
> -timeframe (12h for build failures and security fixes, 3 days small changes,
> +Send a patch to ffmpeg-devel. If no one answers within a reasonable
> +time-frame (12h for build failures and security fixes, 3 days small changes,
>  1 week for big patches) then commit your patch if you think it is OK.
>  Also note, the maintainer can simply ask for more time to review!
>  
> - at item
> -Subscribe to the ffmpeg-cvslog mailing list. The diffs of all commits
> -are sent there and reviewed by all the other developers. Bugs and possible
> -improvements or general questions regarding commits are discussed there. We
> -expect you to react if problems with your code are uncovered.
> + at item Subscribe to the ffmpeg-cvslog mailing list.
> +It is important to do this as the diffs of all commits are sent there and
> +reviewed by all the other developers. Bugs and possible improvements or
> +general questions regarding commits are discussed there. We expect you to
> +react if problems with your code are uncovered.
>  
> - at item
> + at item Keep the documentation up to date.
>  Update the documentation if you change behavior or add features. If you are
>  unsure how best to do this, send a patch to ffmpeg-devel, the documentation
>  maintainer(s) will review and commit your stuff.
>  
> - at item
> + at item Important discussions should be accessible to all.
>  Try to keep important discussions and requests (also) on the public
>  developer mailing list, so that all developers can benefit from them.
>  
> @@ -371,10 +370,8 @@ Never write to unallocated memory, never write over the end of arrays,
>  always check values read from some untrusted source before using them
>  as array index or other risky things.
>  
> - at item
> -Remember to check if you need to bump versions for the specific libav*
> -parts (libavutil, libavcodec, libavformat) you are changing. You need
> -to change the version integer.
> + at item Remember to check if you need to bump versions for libav*.
> +Depending on the change, you may need to change the version integer.
>  Incrementing the first component means no backward compatibility to
>  previous versions (e.g. removal of a function from the public API).
>  Incrementing the second component means backward compatible change
> @@ -384,7 +381,7 @@ Incrementing the third component means a noteworthy binary compatible
>  change (e.g. encoder bug fix that matters for the decoder). The third
>  component always starts at 100 to distinguish FFmpeg from Libav.
>  
> - at item
> + at item Warnings for correct code may be disabled if there is no other option.
>  Compiler warnings indicate potential bugs or code with bad style. If a type of
>  warning always points to correct and clean code, that warning should
>  be disabled, not the code changed.
> @@ -393,7 +390,7 @@ If it is a bug, the bug has to be fixed. If it is not, the code should
>  be changed to not generate a warning unless that causes a slowdown
>  or obfuscates the code.
>  
> - at item
> + at item Check your entries in MAINTAINERS.
>  Make sure that no parts of the codebase that you maintain are missing from the
>  @file{MAINTAINERS} file. If something that you want to maintain is missing add it with
>  your name after it.
> 

Can't find anything wrong. The additions seem fine, so LGTM.
In any case, wait a bit for someone else to comment as well.