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

[Josh de Kock]

On 02/10/2016 22:47, Michael Niedermayer wrote:
> On Sun, Oct 02, 2016 at 01:51:41AM +0100, 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.
>
> "You can" and "may be permitted under-circumstances" has rather
> different meaning. Also the later is bad in a text like this
> as its ambigous ...
>
>
>>
>> - 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.
>
> I dont think this should list "breakages"
> "breakages" are a subset of "changes"
> and except in exteemly rare cases "breakages" should not happen
> intentionally
>

That makes sense, I'll push a couple days with `s/breakages and //` if 
there are no further comments.

Thanks for the review.

-- 
Josh