[FFmpeg-devel] doc: clarify development rules

Submitted by wm4 on Feb. 25, 2017, 2:59 p.m.

Details

Message ID 20170225145949.11516-1-nfxjfg@googlemail.com
State New
Headers show

Commit Message

wm4 Feb. 25, 2017, 2:59 p.m.
I'm documenting existing practice.

I'm pulling the "6 months" timeout out of my ass, but I think it's
pretty suitable.
---
 doc/developer.texi | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Ivan Kalvachev Feb. 25, 2017, 3:03 p.m.
On 2/25/17, wm4 <nfxjfg@googlemail.com> wrote:
> I'm documenting existing practice.
>
> I'm pulling the "6 months" timeout out of my ass, but I think it's
> pretty suitable.
> ---
>  doc/developer.texi | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/doc/developer.texi b/doc/developer.texi
> index dbe1f5421f..41551498ad 100644
> --- a/doc/developer.texi
> +++ b/doc/developer.texi
> @@ -338,13 +338,21 @@ you applied the patch.
>  When applying patches that have been discussed (at length) on the mailing
>  list, reference the thread in the log message.
>
> -@subheading Always wait long enough before pushing changes
> +@subheading Always wait long enough before pushing changes to code actively
> maintained by others
>  Do NOT commit to code actively maintained by others without permission.
>  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!
>
> +@subheading Pushing patches without sending them to the mailing list
> +If you're the maintainer of a file, or the file is not actively maintained
> by
> +anyone, or the file is not covered by the MAINTAINERS file, you can just
> push
> +them without asking for permission, and without sending them to
> ffmpeg-devel.
> +This right only applies to developers with git push access, of course.
> +A maintainer is considered not active if he hasn't posted a mail to
> ffmpeg-devel
> +for longer than 6 months, and hasn't pushed a patch in that time period
> himself.
> +
>  @subsection Code
>  @subheading API/ABI changes should be discussed before they are made.
>  Do not change behavior of the programs (renaming options etc) or public

I object on this.

I do prefer all the code to go though the maillist.
Even trivial changes.
wm4 Feb. 25, 2017, 3:13 p.m.
On Sat, 25 Feb 2017 17:03:58 +0200
Ivan Kalvachev <ikalvachev@gmail.com> wrote:

> On 2/25/17, wm4 <nfxjfg@googlemail.com> wrote:
> > I'm documenting existing practice.
> >
> > I'm pulling the "6 months" timeout out of my ass, but I think it's
> > pretty suitable.
> > ---
> >  doc/developer.texi | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/doc/developer.texi b/doc/developer.texi
> > index dbe1f5421f..41551498ad 100644
> > --- a/doc/developer.texi
> > +++ b/doc/developer.texi
> > @@ -338,13 +338,21 @@ you applied the patch.
> >  When applying patches that have been discussed (at length) on the mailing
> >  list, reference the thread in the log message.
> >
> > -@subheading Always wait long enough before pushing changes
> > +@subheading Always wait long enough before pushing changes to code actively
> > maintained by others
> >  Do NOT commit to code actively maintained by others without permission.
> >  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!
> >
> > +@subheading Pushing patches without sending them to the mailing list
> > +If you're the maintainer of a file, or the file is not actively maintained
> > by
> > +anyone, or the file is not covered by the MAINTAINERS file, you can just
> > push
> > +them without asking for permission, and without sending them to
> > ffmpeg-devel.
> > +This right only applies to developers with git push access, of course.
> > +A maintainer is considered not active if he hasn't posted a mail to
> > ffmpeg-devel
> > +for longer than 6 months, and hasn't pushed a patch in that time period
> > himself.
> > +
> >  @subsection Code
> >  @subheading API/ABI changes should be discussed before they are made.
> >  Do not change behavior of the programs (renaming options etc) or public  
> 
> I object on this.
> 
> I do prefer all the code to go though the maillist.
> Even trivial changes.

That doesn't reflect existing practice.

I'm only documenting existing practice.

If you want to change the rules, post a separate patch.
Rostislav Pehlivanov Feb. 25, 2017, 5:59 p.m.
On 25 February 2017 at 14:59, wm4 <nfxjfg@googlemail.com> wrote:

> I'm documenting existing practice.
>
> I'm pulling the "6 months" timeout out of my ass, but I think it's
> pretty suitable.
> ---
>  doc/developer.texi | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/doc/developer.texi b/doc/developer.texi
> index dbe1f5421f..41551498ad 100644
> --- a/doc/developer.texi
> +++ b/doc/developer.texi
> @@ -338,13 +338,21 @@ you applied the patch.
>  When applying patches that have been discussed (at length) on the mailing
>  list, reference the thread in the log message.
>
> -@subheading Always wait long enough before pushing changes
> +@subheading Always wait long enough before pushing changes to code
> actively maintained by others
>  Do NOT commit to code actively maintained by others without permission.
>  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!
>
> +@subheading Pushing patches without sending them to the mailing list
> +If you're the maintainer of a file, or the file is not actively
> maintained by
> +anyone, or the file is not covered by the MAINTAINERS file, you can just
> push
> +them without asking for permission, and without sending them to
> ffmpeg-devel.
> +This right only applies to developers with git push access, of course.
> +A maintainer is considered not active if he hasn't posted a mail to
> ffmpeg-devel
> +for longer than 6 months, and hasn't pushed a patch in that time period
> himself.
> +
>  @subsection Code
>  @subheading API/ABI changes should be discussed before they are made.
>  Do not change behavior of the programs (renaming options etc) or public
> --
> 2.11.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Patch LGTM
Marton Balint Feb. 25, 2017, 6:35 p.m.
On Sat, 25 Feb 2017, wm4 wrote:

> I'm documenting existing practice.
>
> I'm pulling the "6 months" timeout out of my ass, but I think it's
> pretty suitable.
> ---
> doc/developer.texi | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/doc/developer.texi b/doc/developer.texi
> index dbe1f5421f..41551498ad 100644
> --- a/doc/developer.texi
> +++ b/doc/developer.texi
> @@ -338,13 +338,21 @@ you applied the patch.
> When applying patches that have been discussed (at length) on the mailing
> list, reference the thread in the log message.
> 
> -@subheading Always wait long enough before pushing changes
> +@subheading Always wait long enough before pushing changes to code actively maintained by others
> Do NOT commit to code actively maintained by others without permission.
> 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!
> 
> +@subheading Pushing patches without sending them to the mailing list
> +If you're the maintainer of a file, or the file is not actively maintained by
> +anyone, or the file is not covered by the MAINTAINERS file, you can just push
> +them without asking for permission, and without sending them to ffmpeg-devel.
> +This right only applies to developers with git push access, of course.
> +A maintainer is considered not active if he hasn't posted a mail to ffmpeg-devel
> +for longer than 6 months, and hasn't pushed a patch in that time period himself.
> +

Instead of this, I'd prefer all patches on the ML. Exceptions can be OKish 
(e.g. libav merges, which are hard as they are, or very trivial fixes), 
but I definitely would not make unreviewed pushes an encouraged and 
written policy.

If this gets pushed, I am inclined to clean up the MAINTAINERS file and 
remove everybody who is no longer "active", and assume maintainership of 
parts I actively use and care about, but which has no maintainership 
anymore.

Regards,
Marton
Rostislav Pehlivanov Feb. 25, 2017, 7:26 p.m.
On 25 February 2017 at 18:35, Marton Balint <cus@passwd.hu> wrote:

>
> On Sat, 25 Feb 2017, wm4 wrote:
>
> I'm documenting existing practice.
>>
>> I'm pulling the "6 months" timeout out of my ass, but I think it's
>> pretty suitable.
>> ---
>> doc/developer.texi | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/doc/developer.texi b/doc/developer.texi
>> index dbe1f5421f..41551498ad 100644
>> --- a/doc/developer.texi
>> +++ b/doc/developer.texi
>> @@ -338,13 +338,21 @@ you applied the patch.
>> When applying patches that have been discussed (at length) on the mailing
>> list, reference the thread in the log message.
>>
>> -@subheading Always wait long enough before pushing changes
>> +@subheading Always wait long enough before pushing changes to code
>> actively maintained by others
>> Do NOT commit to code actively maintained by others without permission.
>> 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!
>>
>> +@subheading Pushing patches without sending them to the mailing list
>> +If you're the maintainer of a file, or the file is not actively
>> maintained by
>> +anyone, or the file is not covered by the MAINTAINERS file, you can just
>> push
>> +them without asking for permission, and without sending them to
>> ffmpeg-devel.
>> +This right only applies to developers with git push access, of course.
>> +A maintainer is considered not active if he hasn't posted a mail to
>> ffmpeg-devel
>> +for longer than 6 months, and hasn't pushed a patch in that time period
>> himself.
>> +
>>
>
> Instead of this, I'd prefer all patches on the ML. Exceptions can be OKish
> (e.g. libav merges, which are hard as they are, or very trivial fixes), but
> I definitely would not make unreviewed pushes an encouraged and written
> policy.
>
>
This is the way it has been done for years and its the way the project has
been able to move as rapidly as it has. That would slow down anything large
from being developed in the codebase, like encoders or decoders for a new
format, which are usually being developed by a single person who
understands the code better than anybody. I am okay with it being an
unwritten rule, anyone who needs to know about it knows about it and
everyone that knows about it knows that moderation is the key. But
forbidding it will kill the project.


> If this gets pushed, I am inclined to clean up the MAINTAINERS file and
> remove everybody who is no longer "active", and assume maintainership of
> parts I actively use and care about, but which has no maintainership
> anymore.
>

So you're okay as long as maintainers gets sorted out? You might as well do
it, its what's the file is supposed to reflect.
Marton Balint Feb. 25, 2017, 8:23 p.m.
On Sat, 25 Feb 2017, Rostislav Pehlivanov wrote:

> On 25 February 2017 at 18:35, Marton Balint <cus@passwd.hu> wrote:
>
>>
>> On Sat, 25 Feb 2017, wm4 wrote:
>>
>> I'm documenting existing practice.
>>>
>>> I'm pulling the "6 months" timeout out of my ass, but I think it's
>>> pretty suitable.
>>> ---

[...]

> This is the way it has been done for years and its the way the project has
> been able to move as rapidly as it has. That would slow down anything large
> from being developed in the codebase, like encoders or decoders for a new
> format, which are usually being developed by a single person who
> understands the code better than anybody. I am okay with it being an
> unwritten rule, anyone who needs to know about it knows about it and
> everyone that knows about it knows that moderation is the key. But
> forbidding it will kill the project.

I only want to have a chance to review before patches got pushed. I am not 
saying we should explicitly demand a review for each patch. So this 
normally should only cause any developer an additional sent email to the 
ML and 1-2 days of delay. With git, I don't think this is that much 
additional work.

Of course, I could just subscribe to csvlog as well, and give post-commit 
reviews if I want, it is just better to do it earlier, and less chance of 
revert wars, because with the 'written' rule above I just as easily revert 
anything in unmaintained code without a discussion, and remain within the 
'rules'.


>> If this gets pushed, I am inclined to clean up the MAINTAINERS file and
>> remove everybody who is no longer "active", and assume maintainership of
>> parts I actively use and care about, but which has no maintainership
>> anymore.
>>
>
> So you're okay as long as maintainers gets sorted out? You might as well do
> it, its what's the file is supposed to reflect.

I still prefer if this is not applied. MAINTAINERS file might be cleaned 
up regardless of this patch, however I think we should at least ping 
everybody we are trying to remove from it.

Regards,
Marton
Lou Logan Feb. 25, 2017, 8:40 p.m.
On Sat, Feb 25, 2017, at 11:23 AM, Marton Balint wrote:
>
> I only want to have a chance to review before patches got pushed. I am
> not 
> saying we should explicitly demand a review for each patch. So this 
> normally should only cause any developer an additional sent email to the 
> ML and 1-2 days of delay. With git, I don't think this is that much 
> additional work.

One downside to a forced delay is that patches will occasionally be
completely forgotten instead of being applied. Patchwork helps, but not
everyone uses it or maintains the status of their patches.
Rostislav Pehlivanov Feb. 25, 2017, 10:47 p.m.
On 25 February 2017 at 20:23, Marton Balint <cus@passwd.hu> wrote:

>
> On Sat, 25 Feb 2017, Rostislav Pehlivanov wrote:
>
> On 25 February 2017 at 18:35, Marton Balint <cus@passwd.hu> wrote:
>>
>>
>>> On Sat, 25 Feb 2017, wm4 wrote:
>>>
>>> I'm documenting existing practice.
>>>
>>>>
>>>> I'm pulling the "6 months" timeout out of my ass, but I think it's
>>>> pretty suitable.
>>>> ---
>>>>
>>>
> [...]
>
> This is the way it has been done for years and its the way the project has
>> been able to move as rapidly as it has. That would slow down anything
>> large
>> from being developed in the codebase, like encoders or decoders for a new
>> format, which are usually being developed by a single person who
>> understands the code better than anybody. I am okay with it being an
>> unwritten rule, anyone who needs to know about it knows about it and
>> everyone that knows about it knows that moderation is the key. But
>> forbidding it will kill the project.
>>
>
> I only want to have a chance to review before patches got pushed. I am not
> saying we should explicitly demand a review for each patch. So this
> normally should only cause any developer an additional sent email to the ML
> and 1-2 days of delay. With git, I don't think this is that much additional
> work.
>
>
Review or not, too slow still. If I have a fix for something I wrote and
understand I'm not willing to wait and I'll push it, if I have some doubts
I'll post it to the ML. You seem to give people way less trust than they
deserve, especially if they have commit rights. I can't accept that.
Hendrik Leppkes Feb. 25, 2017, 10:55 p.m.
On Sat, Feb 25, 2017 at 11:47 PM, Rostislav Pehlivanov
<atomnuker@gmail.com> wrote:
>>
>> This is the way it has been done for years and its the way the project has
>>> been able to move as rapidly as it has. That would slow down anything
>>> large
>>> from being developed in the codebase, like encoders or decoders for a new
>>> format, which are usually being developed by a single person who
>>> understands the code better than anybody. I am okay with it being an
>>> unwritten rule, anyone who needs to know about it knows about it and
>>> everyone that knows about it knows that moderation is the key. But
>>> forbidding it will kill the project.
>>>
>>
>> I only want to have a chance to review before patches got pushed. I am not
>> saying we should explicitly demand a review for each patch. So this
>> normally should only cause any developer an additional sent email to the ML
>> and 1-2 days of delay. With git, I don't think this is that much additional
>> work.
>>
>>
> Review or not, too slow still. If I have a fix for something I wrote and
> understand I'm not willing to wait and I'll push it, if I have some doubts
> I'll post it to the ML. You seem to give people way less trust than they
> deserve, especially if they have commit rights. I can't accept that.

I would say that if you are the maintainer of something, you should be
able to push to it, but still be encouraged to post to the ML with
bigger changes (not simple fixes), but things that are unmaintained or
maintained by someone else should generally go through the ML.
Thats how most had interpreted the policy in the past, I would think,
even if it didn't agree with the written policy entirely.

- Hendrik
wm4 March 1, 2017, 11:05 a.m.
On Sat, 25 Feb 2017 21:23:27 +0100 (CET)
Marton Balint <cus@passwd.hu> wrote:

> On Sat, 25 Feb 2017, Rostislav Pehlivanov wrote:
> 
> > On 25 February 2017 at 18:35, Marton Balint <cus@passwd.hu> wrote:
> >  
> >>
> >> On Sat, 25 Feb 2017, wm4 wrote:
> >>
> >> I'm documenting existing practice.  
> >>>
> >>> I'm pulling the "6 months" timeout out of my ass, but I think it's
> >>> pretty suitable.
> >>> ---  
> 
> [...]
> 
> > This is the way it has been done for years and its the way the project has
> > been able to move as rapidly as it has. That would slow down anything large
> > from being developed in the codebase, like encoders or decoders for a new
> > format, which are usually being developed by a single person who
> > understands the code better than anybody. I am okay with it being an
> > unwritten rule, anyone who needs to know about it knows about it and
> > everyone that knows about it knows that moderation is the key. But
> > forbidding it will kill the project.  
> 
> I only want to have a chance to review before patches got pushed. I am not 
> saying we should explicitly demand a review for each patch. So this 
> normally should only cause any developer an additional sent email to the 
> ML and 1-2 days of delay. With git, I don't think this is that much 
> additional work.
> 
> Of course, I could just subscribe to csvlog as well, and give post-commit 
> reviews if I want, it is just better to do it earlier, and less chance of 
> revert wars, because with the 'written' rule above I just as easily revert 
> anything in unmaintained code without a discussion, and remain within the 
> 'rules'.
> 
> 
> >> If this gets pushed, I am inclined to clean up the MAINTAINERS file and
> >> remove everybody who is no longer "active", and assume maintainership of
> >> parts I actively use and care about, but which has no maintainership
> >> anymore.
> >>  
> >
> > So you're okay as long as maintainers gets sorted out? You might as well do
> > it, its what's the file is supposed to reflect.  
> 
> I still prefer if this is not applied. MAINTAINERS file might be cleaned 
> up regardless of this patch, however I think we should at least ping 
> everybody we are trying to remove from it.

As I said in another reply, I'm only documenting existing practice. We
can still change the rules in a separate patch (which of course will be
major drama etc. in which I don't necessarily want to be involved).

Or are there any doubts that this patch does what I claim it does
(documenting existing practice)?

Apart from that, I don't even know whether I can push this patch, or
what I have to do to get clear rejection/approval. The MAINTAINERS
file has ironically no maintainer.

So far, the score is 1 approval, and 2 invalid rejects. I guess I'll
push it on Friday morning if nobody rejects it by then.
Carl Eugen Hoyos March 1, 2017, 11:20 a.m.
2017-02-25 15:59 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> I'm documenting existing practice.

> -@subheading Always wait long enough before pushing changes
> +@subheading Always wait long enough before pushing changes to code actively maintained by others

I suspect this is missing an exception for security issues if you want to
document the actual practice.

> +@subheading Pushing patches without sending them to the mailing list
> +If you're the maintainer of a file, or the file is not actively maintained by
> +anyone, or the file is not covered by the MAINTAINERS file, you can just push
> +them without asking for permission, and without sending them to ffmpeg-devel.
> +This right only applies to developers with git push access, of course.

> +A maintainer is considered not active if he hasn't posted a mail to ffmpeg-devel
> +for longer than 6 months, and hasn't pushed a patch in that time period himself.

Unfortunately, there are maintainers who are happy to review patches
sent to improve their code but the files were not touched for more than
six months so they did not seem active for more than six months.

Carl Eugen
Carl Eugen Hoyos March 1, 2017, 11:21 a.m.
2017-02-25 16:03 GMT+01:00 Ivan Kalvachev <ikalvachev@gmail.com>:

> I do prefer all the code to go though the maillist.
> Even trivial changes.

This is not how we have done it for more than a decade
and I am against changing our (extremely successful) practice.

Carl Eugen
Carl Eugen Hoyos March 1, 2017, 11:24 a.m.
2017-02-25 19:35 GMT+01:00 Marton Balint <cus@passwd.hu>:

> Instead of this, I'd prefer all patches on the ML.

This would mean we changed from a policy that we successful
applied for over a decade to a policy of which we - to the best
of my knowledge - know that it does not work well.

> Exceptions can be OKish
> (e.g. libav merges, which are hard as they are, or very trivial fixes), but
> I definitely would not make unreviewed pushes an encouraged and written
> policy.

I was under the impression that this - luckily - already was
written policy.

> If this gets pushed, I am inclined to clean up the MAINTAINERS file and
> remove everybody who is no longer "active"

While a cleanup is of course ok, please note that there MAINTAINERS
who are willing to maintain but had no reason to do so for longer than
six months.

Carl Eugen
Carl Eugen Hoyos March 1, 2017, 11:25 a.m.
2017-02-25 21:23 GMT+01:00 Marton Balint <cus@passwd.hu>:

> Of course, I could just subscribe to csvlog as well

I was under the impression that if you want commit rights,
you are required to be subscribed to -cvslog.

Carl Eugen
wm4 March 1, 2017, 11:36 a.m.
On Wed, 1 Mar 2017 12:20:10 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-02-25 15:59 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> > I'm documenting existing practice.  
> 
> > -@subheading Always wait long enough before pushing changes
> > +@subheading Always wait long enough before pushing changes to code actively maintained by others  
> 
> I suspect this is missing an exception for security issues if you want to
> document the actual practice.

I can add to the end of the subheading:

  Critical security issues are an exception. These can be pushed after
  the patch has been for 24 hours on the mailing list and the maintainer
  didn't respond yet, and nobody has rejected the patch. In addition,
  if another committer has approved the patch and acknowledged the
  urgency of the fix, the patch can be pushed immediately.

Maybe a bit long, but should cover all bases.

> > +@subheading Pushing patches without sending them to the mailing list
> > +If you're the maintainer of a file, or the file is not actively maintained by
> > +anyone, or the file is not covered by the MAINTAINERS file, you can just push
> > +them without asking for permission, and without sending them to ffmpeg-devel.
> > +This right only applies to developers with git push access, of course.  
> 
> > +A maintainer is considered not active if he hasn't posted a mail to ffmpeg-devel
> > +for longer than 6 months, and hasn't pushed a patch in that time period himself.  
> 
> Unfortunately, there are maintainers who are happy to review patches
> sent to improve their code but the files were not touched for more than
> six months so they did not seem active for more than six months.

So what is a reasonable method of determining whether a maintainer is
reachable?

The worst part is that not even "active" maintainers always respond,
even if you go a timeout.
Carl Eugen Hoyos March 1, 2017, 12:06 p.m.
2017-03-01 12:36 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> On Wed, 1 Mar 2017 12:20:10 +0100
> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> 2017-02-25 15:59 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> > I'm documenting existing practice.
>>
>> > -@subheading Always wait long enough before pushing changes
>> > +@subheading Always wait long enough before pushing changes
>> > to code actively maintained by others
>>
>> I suspect this is missing an exception for security issues if you want to
>> document the actual practice.
>
> I can add to the end of the subheading:
>
>   Critical security issues are an exception. These can be pushed after
>   the patch has been for 24 hours on the mailing list and the maintainer
>   didn't respond yet, and nobody has rejected the patch. In addition,
>   if another committer has approved the patch and acknowledged the
>   urgency of the fix, the patch can be pushed immediately.

I will most likely not fix a (real) security issue, but above seems quite
unpractical to me (and unreasonable for real security issues).

> Maybe a bit long, but should cover all bases.
>
>> > +@subheading Pushing patches without sending them to the mailing list
>> > +If you're the maintainer of a file, or the file is not actively maintained by
>> > +anyone, or the file is not covered by the MAINTAINERS file, you can just push
>> > +them without asking for permission, and without sending them to ffmpeg-devel.
>> > +This right only applies to developers with git push access, of course.
>>
>> > +A maintainer is considered not active if he hasn't posted a mail to ffmpeg-devel
>> > +for longer than 6 months, and hasn't pushed a patch in that time period himself.
>>
>> Unfortunately, there are maintainers who are happy to review patches
>> sent to improve their code but the files were not touched for more than
>> six months so they did not seem active for more than six months.
>
> So what is a reasonable method of determining whether a maintainer
> is reachable?

I fear there is no strict definition, patches can always be reverted though
if a maintainer requests that.

I am just (slightly) against writing "after six months, you are no more a
maintainer".

> The worst part is that not even "active" maintainers always respond,
> even if you go a timeout.

Then you push after the timeout (if no delay was requested).

Carl Eugen
wm4 March 1, 2017, 12:31 p.m.
On Wed, 1 Mar 2017 13:06:29 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-03-01 12:36 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> > On Wed, 1 Mar 2017 12:20:10 +0100
> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >  
> >> 2017-02-25 15:59 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> >> > I'm documenting existing practice.  
> >>  
> >> > -@subheading Always wait long enough before pushing changes
> >> > +@subheading Always wait long enough before pushing changes
> >> > to code actively maintained by others  
> >>
> >> I suspect this is missing an exception for security issues if you want to
> >> document the actual practice.  
> >
> > I can add to the end of the subheading:
> >
> >   Critical security issues are an exception. These can be pushed after
> >   the patch has been for 24 hours on the mailing list and the maintainer
> >   didn't respond yet, and nobody has rejected the patch. In addition,
> >   if another committer has approved the patch and acknowledged the
> >   urgency of the fix, the patch can be pushed immediately.  
> 
> I will most likely not fix a (real) security issue, but above seems quite
> unpractical to me (and unreasonable for real security issues).

How is that impractical? What do you suggest?

I certainly hope you're not suggesting that security-sensitive changes
to code the patch author does not maintain can be pushed without reviews
at all.

> > Maybe a bit long, but should cover all bases.
> >  
> >> > +@subheading Pushing patches without sending them to the mailing list
> >> > +If you're the maintainer of a file, or the file is not actively maintained by
> >> > +anyone, or the file is not covered by the MAINTAINERS file, you can just push
> >> > +them without asking for permission, and without sending them to ffmpeg-devel.
> >> > +This right only applies to developers with git push access, of course.  
> >>  
> >> > +A maintainer is considered not active if he hasn't posted a mail to ffmpeg-devel
> >> > +for longer than 6 months, and hasn't pushed a patch in that time period himself.  
> >>
> >> Unfortunately, there are maintainers who are happy to review patches
> >> sent to improve their code but the files were not touched for more than
> >> six months so they did not seem active for more than six months.  
> >
> > So what is a reasonable method of determining whether a maintainer
> > is reachable?  
> 
> I fear there is no strict definition, patches can always be reverted though
> if a maintainer requests that.
> 
> I am just (slightly) against writing "after six months, you are no more a
> maintainer".

That's not what I wrote.

It merely means that if you have showed no sign of activity after 6
months, the timeout can be skipped.

> > The worst part is that not even "active" maintainers always respond,
> > even if you go a timeout.  
> 
> Then you push after the timeout (if no delay was requested).

michaelni didn't wait when pushing his vp56.c patches (didn't even send
them to the mailing list), even though it was a mid-sized (i.e. not
small) change. The maintainer of vp56.c is still reachable by mail, but
hasn't posted or contributed anything to ffmpeg in 3.5 years. michaelni
didn't wait for the timeout, which does not match with what you seem to
demand.

Please explain what the rules of this project are.
Michael Niedermayer March 2, 2017, 11:44 a.m.
On Wed, Mar 01, 2017 at 01:31:02PM +0100, wm4 wrote:
> On Wed, 1 Mar 2017 13:06:29 +0100
> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 
> > 2017-03-01 12:36 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> > > On Wed, 1 Mar 2017 12:20:10 +0100
> > > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> > >  
> > >> 2017-02-25 15:59 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> > >> > I'm documenting existing practice.  
> > >>  
> > >> > -@subheading Always wait long enough before pushing changes
> > >> > +@subheading Always wait long enough before pushing changes
> > >> > to code actively maintained by others  
> > >>
> > >> I suspect this is missing an exception for security issues if you want to
> > >> document the actual practice.  
> > >
> > > I can add to the end of the subheading:
> > >
> > >   Critical security issues are an exception. These can be pushed after
> > >   the patch has been for 24 hours on the mailing list and the maintainer
> > >   didn't respond yet, and nobody has rejected the patch. In addition,
> > >   if another committer has approved the patch and acknowledged the
> > >   urgency of the fix, the patch can be pushed immediately.  
> > 
> > I will most likely not fix a (real) security issue, but above seems quite
> > unpractical to me (and unreasonable for real security issues).
> 
> How is that impractical? What do you suggest?
> 
> I certainly hope you're not suggesting that security-sensitive changes
> to code the patch author does not maintain can be pushed without reviews
> at all.

I dont know what carl meant exactly but as one doing many of the fixes
there are a few words i should say

Posting patches is only usefull if more issues are found in the
posted patch as a result of it being posted than if it wasnt posted.
It seems few issues are found in patches in this category when posted.
Posting patches interrupts the development cycle, normally i would
debug an issue, write a fix, test and review the fix and if iam
unsure and theres someone who knows the code better post a patch
(either privatly or publically as the case demands)
otherwise push and backport and then the next issue.
The cases where no patch is posted, the pushing and local backport
happens immediately, but if i post a patch and wait 24h by the time i
backport it to the releases its been a longer time and there have been
many other issues in the meantime, that makes backports harder,
less common and lower quality. Thats what iam seeing in the last
days not a imagined issue.
Also i think iam more sloppy with self reviewing my code when i post
a patch

Also adding a unconditional 24h delay for changes makes the whole
process more work. The time i can spend on FFmpeg is limited, if i
send a patch in every case and not just in cases were theres an
active maitainer who wants and does review things before being
pushed. Then the result is some other things arent done anymore simply
due to lack of time, If you want a concrete example you can look at
my gnu/kfreebsd fate client, it stoped working days ago and i noticed
but simply didnt had the time to look into it, previously i always
looked into such things quickly when i noticed.


And posting about (serious) security issues in public before fixing
them adds time for anyone wanting to exploit it

posting patches and waiting has a cost and we should only pay that
when we get more value back.

On a slightly differnt topic, accusing other fellow developers also
has a cost, not just everyones time but also at least for me, the
will and interrest to work in this environment

and now ill try to stay out of this (rather timeconsuming disscussion)
like i did before, just wanted to send one mail as the discussion was
about security and related patches which i work on alot ...

thx

[...]
wm4 March 2, 2017, 11:50 a.m.
On Thu, 2 Mar 2017 12:44:57 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Wed, Mar 01, 2017 at 01:31:02PM +0100, wm4 wrote:
> > On Wed, 1 Mar 2017 13:06:29 +0100
> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >   
> > > 2017-03-01 12:36 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> > > > On Wed, 1 Mar 2017 12:20:10 +0100
> > > > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> > > >    
> > > >> 2017-02-25 15:59 GMT+01:00 wm4 <nfxjfg@googlemail.com>:    
> > > >> > I'm documenting existing practice.    
> > > >>    
> > > >> > -@subheading Always wait long enough before pushing changes
> > > >> > +@subheading Always wait long enough before pushing changes
> > > >> > to code actively maintained by others    
> > > >>
> > > >> I suspect this is missing an exception for security issues if you want to
> > > >> document the actual practice.    
> > > >
> > > > I can add to the end of the subheading:
> > > >
> > > >   Critical security issues are an exception. These can be pushed after
> > > >   the patch has been for 24 hours on the mailing list and the maintainer
> > > >   didn't respond yet, and nobody has rejected the patch. In addition,
> > > >   if another committer has approved the patch and acknowledged the
> > > >   urgency of the fix, the patch can be pushed immediately.    
> > > 
> > > I will most likely not fix a (real) security issue, but above seems quite
> > > unpractical to me (and unreasonable for real security issues).  
> > 
> > How is that impractical? What do you suggest?
> > 
> > I certainly hope you're not suggesting that security-sensitive changes
> > to code the patch author does not maintain can be pushed without reviews
> > at all.  
> 
> I dont know what carl meant exactly but as one doing many of the fixes
> there are a few words i should say
> 
> Posting patches is only usefull if more issues are found in the
> posted patch as a result of it being posted than if it wasnt posted.
> It seems few issues are found in patches in this category when posted.
> Posting patches interrupts the development cycle, normally i would
> debug an issue, write a fix, test and review the fix and if iam
> unsure and theres someone who knows the code better post a patch
> (either privatly or publically as the case demands)
> otherwise push and backport and then the next issue.
> The cases where no patch is posted, the pushing and local backport
> happens immediately, but if i post a patch and wait 24h by the time i
> backport it to the releases its been a longer time and there have been
> many other issues in the meantime, that makes backports harder,
> less common and lower quality. Thats what iam seeing in the last
> days not a imagined issue.
> Also i think iam more sloppy with self reviewing my code when i post
> a patch
> 
> Also adding a unconditional 24h delay for changes makes the whole
> process more work. The time i can spend on FFmpeg is limited, if i
> send a patch in every case and not just in cases were theres an
> active maitainer who wants and does review things before being
> pushed. Then the result is some other things arent done anymore simply
> due to lack of time, If you want a concrete example you can look at
> my gnu/kfreebsd fate client, it stoped working days ago and i noticed
> but simply didnt had the time to look into it, previously i always
> looked into such things quickly when i noticed.
> 
> 
> And posting about (serious) security issues in public before fixing
> them adds time for anyone wanting to exploit it
> 
> posting patches and waiting has a cost and we should only pay that
> when we get more value back.
> 
> On a slightly differnt topic, accusing other fellow developers also
> has a cost, not just everyones time but also at least for me, the
> will and interrest to work in this environment
> 
> and now ill try to stay out of this (rather timeconsuming disscussion)
> like i did before, just wanted to send one mail as the discussion was
> about security and related patches which i work on alot ...
> 
> thx
> 
> [...]

It seems you didn't read what I wrote. If another committer says ok to
the patch (even on IRC), it would be ok.
Carl Eugen Hoyos March 2, 2017, 4:34 p.m.
2017-03-01 13:31 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> On Wed, 1 Mar 2017 13:06:29 +0100
> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> 2017-03-01 12:36 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> > On Wed, 1 Mar 2017 12:20:10 +0100
>> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> >
>> >> 2017-02-25 15:59 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> >> > I'm documenting existing practice.
>> >>
>> >> > -@subheading Always wait long enough before pushing changes
>> >> > +@subheading Always wait long enough before pushing changes
>> >> > to code actively maintained by others
>> >>
>> >> I suspect this is missing an exception for security issues if you want to
>> >> document the actual practice.
>> >
>> > I can add to the end of the subheading:
>> >
>> >   Critical security issues are an exception. These can be pushed after
>> >   the patch has been for 24 hours on the mailing list and the maintainer
>> >   didn't respond yet, and nobody has rejected the patch. In addition,
>> >   if another committer has approved the patch and acknowledged the
>> >   urgency of the fix, the patch can be pushed immediately.
>>
>> I will most likely not fix a (real) security issue, but above seems quite
>> unpractical to me (and unreasonable for real security issues).
>
> How is that impractical? What do you suggest?

Posting security issues and wait for comments does not seem
like a useful approach.

> I certainly hope you're not suggesting that security-sensitive changes
> to code the patch author does not maintain can be pushed without reviews
> at all.

I believe this is the established practice that you want to
document iiuc.

>> > Maybe a bit long, but should cover all bases.
>> >
>> >> > +@subheading Pushing patches without sending them to the mailing list
>> >> > +If you're the maintainer of a file, or the file is not actively maintained by
>> >> > +anyone, or the file is not covered by the MAINTAINERS file, you can just push
>> >> > +them without asking for permission, and without sending them to ffmpeg-devel.
>> >> > +This right only applies to developers with git push access, of course.
>> >>
>> >> > +A maintainer is considered not active if he hasn't posted a mail to ffmpeg-devel
>> >> > +for longer than 6 months, and hasn't pushed a patch in that time period himself.
>> >>
>> >> Unfortunately, there are maintainers who are happy to review patches
>> >> sent to improve their code but the files were not touched for more than
>> >> six months so they did not seem active for more than six months.
>> >
>> > So what is a reasonable method of determining whether a maintainer
>> > is reachable?
>>
>> I fear there is no strict definition, patches can always be reverted though
>> if a maintainer requests that.
>>
>> I am just (slightly) against writing "after six months, you are no more a
>> maintainer".
>
> That's not what I wrote.

But this is how the change of text can be interpreted.

> It merely means that if you have showed no sign of activity after 6
> months, the timeout can be skipped.

As said, this is not a sufficient sign for non-maintenance.

>> > The worst part is that not even "active" maintainers always respond,
>> > even if you go a timeout.
>>
>> Then you push after the timeout (if no delay was requested).
>
> michaelni didn't wait when pushing his vp56.c patches (didn't even send
> them to the mailing list), even though it was a mid-sized (i.e. not
> small) change.

The patch did not change API (which would require the patch
sent to the mailing list) and vp56.c is not actively maintained, so
I don't understand what you are trying to say.

> The maintainer of vp56.c is still reachable by mail, but
> hasn't posted or contributed anything to ffmpeg in 3.5 years. michaelni
> didn't wait for the timeout, which does not match with what you seem to
> demand.

No, you misunderstand:
I am just pointing out that a pure time-out is not sufficient to
decide on maintenance.

> Please explain what the rules of this project are.

As you know, I disagree with several of the rules, and I have said so.
You have - afaict - agreed to them and I still feel that you prefer not
to obey. I have no idea what I can explain to you.

Generally, I honestly don't understand what you are trying to
achieve with your emails.

Carl Eugen
wm4 March 2, 2017, 5:30 p.m.
On Thu, 2 Mar 2017 17:34:11 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-03-01 13:31 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> > On Wed, 1 Mar 2017 13:06:29 +0100
> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >  
> >> 2017-03-01 12:36 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> >> > On Wed, 1 Mar 2017 12:20:10 +0100
> >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >> >  
> >> >> 2017-02-25 15:59 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> >> >> > I'm documenting existing practice.  
> >> >>  
> >> >> > -@subheading Always wait long enough before pushing changes
> >> >> > +@subheading Always wait long enough before pushing changes
> >> >> > to code actively maintained by others  
> >> >>
> >> >> I suspect this is missing an exception for security issues if you want to
> >> >> document the actual practice.  
> >> >
> >> > I can add to the end of the subheading:
> >> >
> >> >   Critical security issues are an exception. These can be pushed after
> >> >   the patch has been for 24 hours on the mailing list and the maintainer
> >> >   didn't respond yet, and nobody has rejected the patch. In addition,
> >> >   if another committer has approved the patch and acknowledged the
> >> >   urgency of the fix, the patch can be pushed immediately.  
> >>
> >> I will most likely not fix a (real) security issue, but above seems quite
> >> unpractical to me (and unreasonable for real security issues).  
> >
> > How is that impractical? What do you suggest?  
> 
> Posting security issues and wait for comments does not seem
> like a useful approach.
> 
> > I certainly hope you're not suggesting that security-sensitive changes
> > to code the patch author does not maintain can be pushed without reviews
> > at all.  
> 
> I believe this is the established practice that you want to
> document iiuc.
> 
> >> > Maybe a bit long, but should cover all bases.
> >> >  
> >> >> > +@subheading Pushing patches without sending them to the mailing list
> >> >> > +If you're the maintainer of a file, or the file is not actively maintained by
> >> >> > +anyone, or the file is not covered by the MAINTAINERS file, you can just push
> >> >> > +them without asking for permission, and without sending them to ffmpeg-devel.
> >> >> > +This right only applies to developers with git push access, of course.  
> >> >>  
> >> >> > +A maintainer is considered not active if he hasn't posted a mail to ffmpeg-devel
> >> >> > +for longer than 6 months, and hasn't pushed a patch in that time period himself.  
> >> >>
> >> >> Unfortunately, there are maintainers who are happy to review patches
> >> >> sent to improve their code but the files were not touched for more than
> >> >> six months so they did not seem active for more than six months.  
> >> >
> >> > So what is a reasonable method of determining whether a maintainer
> >> > is reachable?  
> >>
> >> I fear there is no strict definition, patches can always be reverted though
> >> if a maintainer requests that.
> >>
> >> I am just (slightly) against writing "after six months, you are no more a
> >> maintainer".  
> >
> > That's not what I wrote.  
> 
> But this is how the change of text can be interpreted.
> 
> > It merely means that if you have showed no sign of activity after 6
> > months, the timeout can be skipped.  
> 
> As said, this is not a sufficient sign for non-maintenance.
> 
> >> > The worst part is that not even "active" maintainers always respond,
> >> > even if you go a timeout.  
> >>
> >> Then you push after the timeout (if no delay was requested).  
> >
> > michaelni didn't wait when pushing his vp56.c patches (didn't even send
> > them to the mailing list), even though it was a mid-sized (i.e. not
> > small) change.  
> 
> The patch did not change API (which would require the patch
> sent to the mailing list) and vp56.c is not actively maintained, so
> I don't understand what you are trying to say.
> 
> > The maintainer of vp56.c is still reachable by mail, but
> > hasn't posted or contributed anything to ffmpeg in 3.5 years. michaelni
> > didn't wait for the timeout, which does not match with what you seem to
> > demand.  
> 
> No, you misunderstand:
> I am just pointing out that a pure time-out is not sufficient to
> decide on maintenance.
> 
> > Please explain what the rules of this project are.  
> 
> As you know, I disagree with several of the rules, and I have said so.
> You have - afaict - agreed to them and I still feel that you prefer not
> to obey. I have no idea what I can explain to you.
> 
> Generally, I honestly don't understand what you are trying to
> achieve with your emails.
> 
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

I give up.

Please write a patch to clarify the rules yourself.

Otherwise I'm going to assume that the rules are as fuzzy as they seem
to be, and I'll adjust my behavior accordingly.

Patch hide | download patch | download mbox

diff --git a/doc/developer.texi b/doc/developer.texi
index dbe1f5421f..41551498ad 100644
--- a/doc/developer.texi
+++ b/doc/developer.texi
@@ -338,13 +338,21 @@  you applied the patch.
 When applying patches that have been discussed (at length) on the mailing
 list, reference the thread in the log message.
 
-@subheading Always wait long enough before pushing changes
+@subheading Always wait long enough before pushing changes to code actively maintained by others
 Do NOT commit to code actively maintained by others without permission.
 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!
 
+@subheading Pushing patches without sending them to the mailing list
+If you're the maintainer of a file, or the file is not actively maintained by
+anyone, or the file is not covered by the MAINTAINERS file, you can just push
+them without asking for permission, and without sending them to ffmpeg-devel.
+This right only applies to developers with git push access, of course.
+A maintainer is considered not active if he hasn't posted a mail to ffmpeg-devel
+for longer than 6 months, and hasn't pushed a patch in that time period himself.
+
 @subsection Code
 @subheading API/ABI changes should be discussed before they are made.
 Do not change behavior of the programs (renaming options etc) or public