[FFmpeg-devel,v1,1/3] doc/developer.texi: Improvements in Submitting patches section.

The section has been expanded to outline how to manage patch revisions.
---
 doc/developer.texi | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

On 2020-07-05 14:45, Manolis Stamatogiannakis wrote:

> The section has been expanded to outline how to manage patch revisions.
> ---
>   doc/developer.texi | 37 ++++++++++++++++++++++++++-----------
>   1 file changed, 26 insertions(+), 11 deletions(-)


Thank you for taking the time to improve the documentation about how to 
submit patches. Once these changes are accepted, you will have smoothed 
the way for many other developers.

       —Jim DeLaHunt, software engineer, Vancouver, Canada

Manolis Stamatogiannakis (12020-07-05):
> The section has been expanded to outline how to manage patch revisions.
> ---
>  doc/developer.texi | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/doc/developer.texi b/doc/developer.texi
> index b33cab0fc7..dec27cb509 100644
> --- a/doc/developer.texi
> +++ b/doc/developer.texi
> @@ -491,18 +491,25 @@ as base64-encoded attachments, so your patch is not trashed during
>  transmission. Also ensure the correct mime type is used
>  (text/x-diff or text/x-patch or at least text/plain) and that only one
>  patch is inline or attached per mail.
> -You can check @url{https://patchwork.ffmpeg.org}, if your patch does not show up, its mime type
> -likely was wrong.
> +You can check the most recently received patches on
> +@url{https://patchwork.ffmpeg.org, patchwork}.
> +If your patch does not show up, its mime type likely was wrong.
>  
> -Your patch will be reviewed on the mailing list. You will likely be asked
> +Your patch will be reviewed on the mailing list. Give us a few days to react.
> +But if some time passes without reaction, send a reminder by email.
> +Your patch should eventually be dealt with. However, you will likely be asked
>  to make some changes and are expected to send in an improved version that
>  incorporates the requests from the review. This process may go through
>  several iterations. Once your patch is deemed good enough, some developer
>  will pick it up and commit it to the official FFmpeg tree.
>  
> -Give us a few days to react. But if some time passes without reaction,
> -send a reminder by email. Your patch should eventually be dealt with.
> -

> +When preparing an updated version of you patch, make sure you increment
> +its @emph{roll-counter}. This is achieved by adding a @code{-v <n>} argument
> +to @code{git format-patch}/@code{git send-email} commands.

I know many core developers do not bother with that, and I never found
these "v3" very useful: if I am not involved in the discussion, I will
not remember what number is the latest. And if I become involved in the
discussions, my mail client can sort the patch by date and I take the
latest.

> +Additionally, it is recommended to register for a
> +@uref{https://patchwork.ffmpeg.org, patchwork account}.
> +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.

Is interacting with Patchwork to become mandatory when submitting
patches?

There is this classic scenario:

1. People work on something.

2. Somebody sets up a tool to make the work easier.

3. People start relying on the tool.

4. The tool proves imperfect.

5. People are required extra work to go around the flaws of the tool.

6. Overall, the tool has made the work not easier but harder.

Are we going that way with Patchwork?

If I am required to log into a website and do maintenance each time I
send an updated patch, I will just send less updated patches. I am not
alone in this.

>  
>  @chapter New codecs or formats checklist
>  
> @@ -563,7 +570,19 @@ Did you make sure it compiles standalone, i.e. with
>  Does @code{make fate} pass with the patch applied?
>  
>  @item
> -Was the patch generated with git format-patch or send-email?
> +Was the patch generated with @code{git format-patch} or @code{git send-email}?
> +
> +@item
> +If you are submitting a revised patch, did you increment the roll-counter
> +with @code{-v <n>}?
> +
> +@item
> +If you are submitting a revised patch, did you marked previous versions of
> +the patch as "Superseded" on @uref{https://patchwork.ffmpeg.org/, patchwork}?
> +
> +@item
> +Are you subscribed to ffmpeg-devel?
> +(the list is subscribers only due to spam)
>  
>  @item
>  Did you sign-off your patch? (@code{git commit -s})
> @@ -576,10 +595,6 @@ Did you provide a clear git commit log message?
>  @item
>  Is the patch against latest FFmpeg git master branch?
>  
> -@item
> -Are you subscribed to ffmpeg-devel?
> -(the list is subscribers only due to spam)
> -
>  @item
>  Have you checked that the changes are minimal, so that the same cannot be
>  achieved with a smaller patch and/or simpler final code?

Regards,

On 7/7/20, Nicolas George <george@nsup.org> wrote:
> Manolis Stamatogiannakis (12020-07-05):
>> The section has been expanded to outline how to manage patch revisions.
>> ---
>>  doc/developer.texi | 37 ++++++++++++++++++++++++++-----------
>>  1 file changed, 26 insertions(+), 11 deletions(-)
>>
>> diff --git a/doc/developer.texi b/doc/developer.texi
>> index b33cab0fc7..dec27cb509 100644
>> --- a/doc/developer.texi
>> +++ b/doc/developer.texi
>> @@ -491,18 +491,25 @@ as base64-encoded attachments, so your patch is not
>> trashed during
>>  transmission. Also ensure the correct mime type is used
>>  (text/x-diff or text/x-patch or at least text/plain) and that only one
>>  patch is inline or attached per mail.
>> -You can check @url{https://patchwork.ffmpeg.org}, if your patch does not
>> show up, its mime type
>> -likely was wrong.
>> +You can check the most recently received patches on
>> +@url{https://patchwork.ffmpeg.org, patchwork}.
>> +If your patch does not show up, its mime type likely was wrong.
>>
>> -Your patch will be reviewed on the mailing list. You will likely be
>> asked
>> +Your patch will be reviewed on the mailing list. Give us a few days to
>> react.
>> +But if some time passes without reaction, send a reminder by email.
>> +Your patch should eventually be dealt with. However, you will likely be
>> asked
>>  to make some changes and are expected to send in an improved version
>> that
>>  incorporates the requests from the review. This process may go through
>>  several iterations. Once your patch is deemed good enough, some
>> developer
>>  will pick it up and commit it to the official FFmpeg tree.
>>
>> -Give us a few days to react. But if some time passes without reaction,
>> -send a reminder by email. Your patch should eventually be dealt with.
>> -
>
>> +When preparing an updated version of you patch, make sure you increment
>> +its @emph{roll-counter}. This is achieved by adding a @code{-v <n>}
>> argument
>> +to @code{git format-patch}/@code{git send-email} commands.
>
> I know many core developers do not bother with that, and I never found
> these "v3" very useful: if I am not involved in the discussion, I will
> not remember what number is the latest. And if I become involved in the
> discussions, my mail client can sort the patch by date and I take the
> latest.
>
>> +Additionally, it is recommended to register for a
>> +@uref{https://patchwork.ffmpeg.org, patchwork account}.
>> +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.
>
> Is interacting with Patchwork to become mandatory when submitting
> patches?
>
> There is this classic scenario:
>
> 1. People work on something.
>
> 2. Somebody sets up a tool to make the work easier.
>
> 3. People start relying on the tool.
>
> 4. The tool proves imperfect.
>
> 5. People are required extra work to go around the flaws of the tool.
>
> 6. Overall, the tool has made the work not easier but harder.
>
> Are we going that way with Patchwork?
>
> If I am required to log into a website and do maintenance each time I
> send an updated patch, I will just send less updated patches. I am not
> alone in this.
>
>>
>>  @chapter New codecs or formats checklist
>>
>> @@ -563,7 +570,19 @@ Did you make sure it compiles standalone, i.e. with
>>  Does @code{make fate} pass with the patch applied?
>>
>>  @item
>> -Was the patch generated with git format-patch or send-email?
>> +Was the patch generated with @code{git format-patch} or @code{git
>> send-email}?
>> +
>> +@item
>> +If you are submitting a revised patch, did you increment the
>> roll-counter
>> +with @code{-v <n>}?
>> +
>> +@item
>> +If you are submitting a revised patch, did you marked previous versions
>> of
>> +the patch as "Superseded" on @uref{https://patchwork.ffmpeg.org/,
>> patchwork}?
>> +
>> +@item
>> +Are you subscribed to ffmpeg-devel?
>> +(the list is subscribers only due to spam)
>>
>>  @item
>>  Did you sign-off your patch? (@code{git commit -s})
>> @@ -576,10 +595,6 @@ Did you provide a clear git commit log message?
>>  @item
>>  Is the patch against latest FFmpeg git master branch?
>>
>> -@item
>> -Are you subscribed to ffmpeg-devel?
>> -(the list is subscribers only due to spam)
>> -
>>  @item
>>  Have you checked that the changes are minimal, so that the same cannot
>> be
>>  achieved with a smaller patch and/or simpler final code?
>

I remember days when I used patchwork and actually cared to keep it
sync with reality (at least with my patches).
But then it just stopped working - IIRC I was not able to log in and
there my motivation for it
was lost.

Now that it reappeared again I'm not convinced it will stay for long
so I just do not bother with it any more.

> Regards,
>
> --
>   Nicolas George
>

On Tue, Jul 07, 2020 at 04:00:10PM +0200, Nicolas George wrote:
> Manolis Stamatogiannakis (12020-07-05):
> > The section has been expanded to outline how to manage patch revisions.
> > ---
> >  doc/developer.texi | 37 ++++++++++++++++++++++++++-----------
> >  1 file changed, 26 insertions(+), 11 deletions(-)
> > 
> > diff --git a/doc/developer.texi b/doc/developer.texi
> > index b33cab0fc7..dec27cb509 100644
> > --- a/doc/developer.texi
> > +++ b/doc/developer.texi
> > @@ -491,18 +491,25 @@ as base64-encoded attachments, so your patch is not trashed during
> >  transmission. Also ensure the correct mime type is used
> >  (text/x-diff or text/x-patch or at least text/plain) and that only one
> >  patch is inline or attached per mail.
> > -You can check @url{https://patchwork.ffmpeg.org}, if your patch does not show up, its mime type
> > -likely was wrong.
> > +You can check the most recently received patches on
> > +@url{https://patchwork.ffmpeg.org, patchwork}.
> > +If your patch does not show up, its mime type likely was wrong.
> >  
> > -Your patch will be reviewed on the mailing list. You will likely be asked
> > +Your patch will be reviewed on the mailing list. Give us a few days to react.
> > +But if some time passes without reaction, send a reminder by email.
> > +Your patch should eventually be dealt with. However, you will likely be asked
> >  to make some changes and are expected to send in an improved version that
> >  incorporates the requests from the review. This process may go through
> >  several iterations. Once your patch is deemed good enough, some developer
> >  will pick it up and commit it to the official FFmpeg tree.
> >  
> > -Give us a few days to react. But if some time passes without reaction,
> > -send a reminder by email. Your patch should eventually be dealt with.
> > -
> 
> > +When preparing an updated version of you patch, make sure you increment
> > +its @emph{roll-counter}. This is achieved by adding a @code{-v <n>} argument
> > +to @code{git format-patch}/@code{git send-email} commands.
> 
> I know many core developers do not bother with that, and I never found
> these "v3" very useful: if I am not involved in the discussion, I will
> not remember what number is the latest. And if I become involved in the
> discussions, my mail client can sort the patch by date and I take the
> latest.
> 
> > +Additionally, it is recommended to register for a
> > +@uref{https://patchwork.ffmpeg.org, patchwork account}.
> > +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.
> 
> Is interacting with Patchwork to become mandatory when submitting
> patches?
> 
> There is this classic scenario:
> 
> 1. People work on something.
> 
> 2. Somebody sets up a tool to make the work easier.
> 
> 3. People start relying on the tool.
> 
> 4. The tool proves imperfect.
> 
> 5. People are required extra work to go around the flaws of the tool.
> 
> 6. Overall, the tool has made the work not easier but harder.
> 
> Are we going that way with Patchwork?
> 
> If I am required to log into a website and do maintenance each time I
> send an updated patch, I will just send less updated patches. I am not
> alone in this.

a lot of patchwork patch maintaince can be automated, ive written a script
to do that.
What this does is basically look at local git, fetch all patches from patchwork
(and cache them locally)
and then find stuff which is either invalid, superseeded, applied, ... but not
marked correctly
last it dumps the command line commands to the screen for a human to decide
if they are correct and should be run as is or not.
See:
https://github.com/michaelni/patchwork-update-bot

It worked fine until the server was updated or maybe the number of patches
became too big.
now it times out  while fetching patches, the command line tool (pwclient) 
times out too (xmlrpclib.ProtocolError: <ProtocolError for patchwork.ffmpeg.org/xmlrpc/: 504 Gateway Time-out>)
so the bug is not in patchwork-update-bot
Someone has to investigate why this timeout happens and make it not timeout
then run this automatically in regular intervalls

thx

[...]

On Tue, 7 Jul 2020 at 16:00, Nicolas George <george@nsup.org> wrote:

> Manolis Stamatogiannakis (12020-07-05):
> > The section has been expanded to outline how to manage patch revisions.
> > ---
> >  doc/developer.texi | 37 ++++++++++++++++++++++++++-----------
> >  1 file changed, 26 insertions(+), 11 deletions(-)
> >
> > diff --git a/doc/developer.texi b/doc/developer.texi
> > index b33cab0fc7..dec27cb509 100644
> > --- a/doc/developer.texi
> > +++ b/doc/developer.texi
> > @@ -491,18 +491,25 @@ as base64-encoded attachments, so your patch is
> not trashed during
> >  transmission. Also ensure the correct mime type is used
> >  (text/x-diff or text/x-patch or at least text/plain) and that only one
> >  patch is inline or attached per mail.
> > -You can check @url{https://patchwork.ffmpeg.org}, if your patch does
> not show up, its mime type
> > -likely was wrong.
> > +You can check the most recently received patches on
> > +@url{https://patchwork.ffmpeg.org, patchwork}.
> > +If your patch does not show up, its mime type likely was wrong.
> >
> > -Your patch will be reviewed on the mailing list. You will likely be
> asked
> > +Your patch will be reviewed on the mailing list. Give us a few days to
> react.
> > +But if some time passes without reaction, send a reminder by email.
> > +Your patch should eventually be dealt with. However, you will likely be
> asked
> >  to make some changes and are expected to send in an improved version
> that
> >  incorporates the requests from the review. This process may go through
> >  several iterations. Once your patch is deemed good enough, some
> developer
> >  will pick it up and commit it to the official FFmpeg tree.
> >
> > -Give us a few days to react. But if some time passes without reaction,
> > -send a reminder by email. Your patch should eventually be dealt with.
> > -
>
> > +When preparing an updated version of you patch, make sure you increment
> > +its @emph{roll-counter}. This is achieved by adding a @code{-v <n>}
> argument
> > +to @code{git format-patch}/@code{git send-email} commands.
>
> I know many core developers do not bother with that, and I never found
> these "v3" very useful: if I am not involved in the discussion, I will
> not remember what number is the latest. And if I become involved in the
> discussions, my mail client can sort the patch by date and I take the
> latest.
>

The documentation aims to establish some good practices for people who want
to start contributing to the project.
By the patches arriving to the list, adding a roll counter seems to be an
established good practice. So it should probably be included in the docs.

But keep in mind that this is only a recommendation anyway. Maybe changing
"make sure" to "should" would make that more clear.
Core developers can stray off recommendations, since they are in position
to make an informed decision when they are not relevant.

In this case, you have a very well setup email-based workflow. Other people
may not have that, and the roll counter may come handy.

Would normalizing the verbs in developer.texi help to avoid confusion?
Maybe it's just me, but when I'm reading any technical documents, I
interpret the verbs according to RFC2119 to understand what is mandatory,
what is recommended and what optional.



>
> > +Additionally, it is recommended to register for a
> > +@uref{https://patchwork.ffmpeg.org, patchwork account}.
> > +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.
>
> Is interacting with Patchwork to become mandatory when submitting
> patches?
>

Nicolas, why would you ever interpret "recommended" as mandatory? I think
it's as clear as it gets that it is not mandatory.

Again, this is a good practice which was not mentioned in the
documentation. I have no attachment whatsoever to the tool, or intention to
impose it to anyone.

@Michael Niedermayer, since you seem to be the most involved with patchwork
in the thread, what would be better for this? Keep the wording as a
recommendation, or to move it outside the list as purely informational text?


> There is this classic scenario:
>
> 1. People work on something.
>
> 2. Somebody sets up a tool to make the work easier.
>
> 3. People start relying on the tool.
>
> 4. The tool proves imperfect.
>
> 5. People are required extra work to go around the flaws of the tool.
>
> 6. Overall, the tool has made the work not easier but harder.
>
> Are we going that way with Patchwork?
>

So your recommendation is to not setup any tool ever, because people may
start relying on it?
Tools come and go. As long as use is optional and there's no data lock-in,
I don't see why we should be wary of any tool that makes running the
project smoother.



>
> If I am required to log into a website and do maintenance each time I
> send an updated patch, I will just send less updated patches.


You certainly don't need to do that each time. A weekly cleanup would
probably be more than enough, and would let someone visiting patchwork to
have a good picture on what people are working on.


> I am not
> alone in this.
>
>
You probably aren't. But let other people speak for themselves. I'm happy
to listen to their comments too.
"I am not alone in this" sounds pretty divisive/passive aggressive.

Best regards,
Manolis

On Tue, 7 Jul 2020 at 19:14, Michael Niedermayer <michael@niedermayer.cc>
wrote:

>
> a lot of patchwork patch maintaince can be automated, ive written a script
> to do that.
> What this does is basically look at local git, fetch all patches from
> patchwork
> (and cache them locally)
> and then find stuff which is either invalid, superseeded, applied, ... but
> not
> marked correctly
> last it dumps the command line commands to the screen for a human to decide
> if they are correct and should be run as is or not.
> See:
> https://github.com/michaelni/patchwork-update-bot
>
> It worked fine until the server was updated or maybe the number of patches
> became too big.
> now it times out  while fetching patches, the command line tool (pwclient)
> times out too (xmlrpclib.ProtocolError: <ProtocolError for
> patchwork.ffmpeg.org/xmlrpc/: 504 Gateway Time-out>)
> so the bug is not in patchwork-update-bot
> Someone has to investigate why this timeout happens and make it not timeout
> then run this automatically in regular intervalls
>
> thx
>
>
Since it's python, I can take a look, provided it doesn't need any special
permissions.

M.

On Thu, Jul 09, 2020 at 02:01:45AM +0200, Manolis Stamatogiannakis wrote:
> On Tue, 7 Jul 2020 at 16:00, Nicolas George <george@nsup.org> wrote:
> 
> > Manolis Stamatogiannakis (12020-07-05):
[...]
> > > +Additionally, it is recommended to register for a
> > > +@uref{https://patchwork.ffmpeg.org, patchwork account}.
> > > +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.
> >
> > Is interacting with Patchwork to become mandatory when submitting
> > patches?
> >
> 
> Nicolas, why would you ever interpret "recommended" as mandatory? I think
> it's as clear as it gets that it is not mandatory.
> 
> Again, this is a good practice which was not mentioned in the
> documentation. I have no attachment whatsoever to the tool, or intention to
> impose it to anyone.
> 

> @Michael Niedermayer, since you seem to be the most involved with patchwork
> in the thread, what would be better for this? Keep the wording as a
> recommendation, or to move it outside the list as purely informational text?

I think there are 2 aspects here
First is a "mostly automatic patchwork with only people who want to play with
it obtaining accounts and doing something with them" vs one where its
recommanded to get accounts
These are 2 different philosophies ;)

Second is, time vs time
Is the gain from manually working with patchwork saving us more time elsewhere while
we achieve the same quality of code?

and Third
Can we not automate whatever we would do manually ?

And describing the actual cases with examples what is wrong and needs manual
update would be interresting.

Are these caseses that are clear to human but unclear to a computer ?
or we are just missing a line or 2 in the script ?
or are they maybe unclear to humans too ? which could then lead 
a discussion on how to make that clear to humans first ...

I think its needed to understand where automation fails to really discuss
this all

thx

[...]

On Thu, 9 Jul 2020 at 22:39, Michael Niedermayer <michael@niedermayer.cc>
wrote:

>
> > @Michael Niedermayer, since you seem to be the most involved with
> patchwork
> > in the thread, what would be better for this? Keep the wording as a
> > recommendation, or to move it outside the list as purely informational
> text?
>
> I think there are 2 aspects here
> First is a "mostly automatic patchwork with only people who want to play
> with
> it obtaining accounts and doing something with them" vs one where its
> recommanded to get accounts
> These are 2 different philosophies ;)
>
> Second is, time vs time
> Is the gain from manually working with patchwork saving us more time
> elsewhere while
> we achieve the same quality of code?
>
> and Third
> Can we not automate whatever we would do manually ?
>
> And describing the actual cases with examples what is wrong and needs
> manual
> update would be interresting.
>
> Are these caseses that are clear to human but unclear to a computer ?
> or we are just missing a line or 2 in the script ?
> or are they maybe unclear to humans too ? which could then lead
> a discussion on how to make that clear to humans first ...
>
> I think its needed to understand where automation fails to really discuss
> this all
>
> thx
>
>
Thanks for the input Michael.

If you find those benefits are not clear-cut, I agree we should refrain
from recommending to everyone to create a patchwork account.

I'm not intimate enough with patchwork automation, so my intuition on the
benefits of recommending manual patch management may be off.

I've updated the patch to v2, presenting the registration of a patchwork
account as an optional step that may come handy in some cases, but not
generally required.

If anyone can make a more informed proposal on that matter, I can
incorporate it in the patch, while we're looking at it.

Best regards,
Manolis