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

[Manolis Stamatogiannakis]

On Tue, 7 Jul 2020 at 16:00, Nicolas George <george at 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
> > + at 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
> > + at 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