| Message ID | 20160907115125.32001-1-michael@niedermayer.cc |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi, On Wed, Sep 7, 2016 at 7:51 AM, Michael Niedermayer <michael@niedermayer.cc> wrote: > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > doc/developer.texi | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/doc/developer.texi b/doc/developer.texi > index 4d3a7ae..51e3da7 100644 > --- a/doc/developer.texi > +++ b/doc/developer.texi > @@ -641,6 +641,12 @@ are notoriously left unchecked, which is a serious > problem. > @item > Test your code with valgrind and or Address Sanitizer to ensure it's free > of leaks, out of array accesses, etc. > + > +@item > +Check that your submitted patch shows up on @url{ > https://patchwork.ffmpeg.org}. > +Also make sure its status is updated, you can create an account and > update it. > +If your patch is incorrectly or not listed in patchwork then it might be > +missed by developers using patchwork to find patches needing review or > pushing. > @end enumerate I don't think we should require developers to use (or check, or update, or create-an-account-on) patchwork. Wasn't the whole point of patchwork that you can use it if you care, and you can ignore it if you don't care? Ronald
On Wed, Sep 07, 2016 at 08:03:20AM -0400, Ronald S. Bultje wrote: > Hi, > > On Wed, Sep 7, 2016 at 7:51 AM, Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > doc/developer.texi | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/doc/developer.texi b/doc/developer.texi > > index 4d3a7ae..51e3da7 100644 > > --- a/doc/developer.texi > > +++ b/doc/developer.texi > > @@ -641,6 +641,12 @@ are notoriously left unchecked, which is a serious > > problem. > > @item > > Test your code with valgrind and or Address Sanitizer to ensure it's free > > of leaks, out of array accesses, etc. > > + > > +@item > > +Check that your submitted patch shows up on @url{ > > https://patchwork.ffmpeg.org}. > > +Also make sure its status is updated, you can create an account and > > update it. > > +If your patch is incorrectly or not listed in patchwork then it might be > > +missed by developers using patchwork to find patches needing review or > > pushing. > > @end enumerate > > > I don't think we should require developers to use (or check, or update, or > create-an-account-on) patchwork. Wasn't the whole point of patchwork that > you can use it if you care, and you can ignore it if you don't care? yes one can ignore it but its alot more usefull if people keep in mind that theres patchwork. For example, if you reply with LGTM or Acked-by: or Reviewed-by: or Tested-by: patchwork will pick that up, OTOH if you write "that works well, i tested it" patchwork will not pick that up, nor will it "dude that looks fine, please push it" nor does it "I dont like that change" the result of that is that if everyone ignores patchwork it will contain most submited patches (some odd non standard attachmets are missed) it would pick up most but not all applied patches (change the patch before push and it will quite potentially not realize its the same) it wont on its own detect superseded patches nor would it know if a patch is forgotten or dropped it also picks the fate output up as patch as it is techinically a patch ive writte a script to automate this a bit further so fate output gets marked as not applicable, so that more applied patches are detected and so that most superseeded patches are detected as such but people keeping an eye on patchwork and updating status where it just cannot figure it out or improving its automation or making sure they reply with keywords it understands or update it so it picks up the used keywords would all be a good idea. And it would improve the usefulness of patchwork, finding out which patches need a review needs all applied, superseeded an droped patches to be marked accordingly. Also patchwork supports keeping track of delegation so if someone is working on a patch review or whatever patchwork can be told about that and people would then know not to push the patch before the reviewer is done. Thats not automatic though either, it requires developers to have an account and set the status of the patch by hand AFAIK [...]
On 09/07/2016 04:51 AM, Michael Niedermayer wrote: > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > doc/developer.texi | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/doc/developer.texi b/doc/developer.texi > index 4d3a7ae..51e3da7 100644 > --- a/doc/developer.texi > +++ b/doc/developer.texi > @@ -641,6 +641,12 @@ are notoriously left unchecked, which is a serious problem. > @item > Test your code with valgrind and or Address Sanitizer to ensure it's free > of leaks, out of array accesses, etc. > + > +@item > +Check that your submitted patch shows up on @url{https://patchwork.ffmpeg.org}. > +Also make sure its status is updated, you can create an account and update it. > +If your patch is incorrectly or not listed in patchwork then it might be > +missed by developers using patchwork to find patches needing review or pushing. > @end enumerate > > @section Patch review process > I only see one of the 4 patches for AC-3 consistent noise generation (the libavutil version bump) on the patchwork site. Can I assume the other three will make their way into the patchwork list when the time is right, or did I submit the patch wrong? Jonathan Campbell
On Wed, Sep 07, 2016 at 07:19:00AM -0700, Jonathan Campbell wrote: > On 09/07/2016 04:51 AM, Michael Niedermayer wrote: > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > doc/developer.texi | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/doc/developer.texi b/doc/developer.texi > > index 4d3a7ae..51e3da7 100644 > > --- a/doc/developer.texi > > +++ b/doc/developer.texi > > @@ -641,6 +641,12 @@ are notoriously left unchecked, which is a serious problem. > > @item > > Test your code with valgrind and or Address Sanitizer to ensure it's free > > of leaks, out of array accesses, etc. > > + > > +@item > > +Check that your submitted patch shows up on @url{https://patchwork.ffmpeg.org}. > > +Also make sure its status is updated, you can create an account and update it. > > +If your patch is incorrectly or not listed in patchwork then it might be > > +missed by developers using patchwork to find patches needing review or pushing. > > @end enumerate > > > > @section Patch review process > > > > I only see one of the 4 patches for AC-3 consistent noise generation (the libavutil version bump) on the patchwork site. Can I assume the other three will make their way into the patchwork list when the time is right, or did I submit the patch wrong? all 4 patches are there: https://patchwork.ffmpeg.org/project/ffmpeg/list/?submitter=82&state=%2A&archive=both patches which arent there will not appear later unless the mails from the mailing list are stuck [...]
On Wed, Sep 07, 2016 at 04:39:54PM +0200, Michael Niedermayer wrote: > On Wed, Sep 07, 2016 at 07:19:00AM -0700, Jonathan Campbell wrote: > > On 09/07/2016 04:51 AM, Michael Niedermayer wrote: > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > doc/developer.texi | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/doc/developer.texi b/doc/developer.texi > > > index 4d3a7ae..51e3da7 100644 > > > --- a/doc/developer.texi > > > +++ b/doc/developer.texi > > > @@ -641,6 +641,12 @@ are notoriously left unchecked, which is a serious problem. > > > @item > > > Test your code with valgrind and or Address Sanitizer to ensure it's free > > > of leaks, out of array accesses, etc. > > > + > > > +@item > > > +Check that your submitted patch shows up on @url{https://patchwork.ffmpeg.org}. > > > +Also make sure its status is updated, you can create an account and update it. > > > +If your patch is incorrectly or not listed in patchwork then it might be > > > +missed by developers using patchwork to find patches needing review or pushing. > > > @end enumerate > > > > > > @section Patch review process > > > > > > > I only see one of the 4 patches for AC-3 consistent noise generation (the libavutil version bump) on the patchwork site. Can I assume the other three will make their way into the patchwork list when the time is right, or did I submit the patch wrong? > > all 4 patches are there: > https://patchwork.ffmpeg.org/project/ffmpeg/list/?submitter=82&state=%2A&archive=both in fact thats the 4 mails not the 4 patches from the last mail patchwork does not support multiple patches per mail AFAIK [...]
On Wed, Sep 07, 2016 at 02:27:07PM +0200, Michael Niedermayer wrote: > On Wed, Sep 07, 2016 at 08:03:20AM -0400, Ronald S. Bultje wrote: > > Hi, > > > > On Wed, Sep 7, 2016 at 7:51 AM, Michael Niedermayer <michael@niedermayer.cc> > > wrote: > > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > doc/developer.texi | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/doc/developer.texi b/doc/developer.texi > > > index 4d3a7ae..51e3da7 100644 > > > --- a/doc/developer.texi > > > +++ b/doc/developer.texi > > > @@ -641,6 +641,12 @@ are notoriously left unchecked, which is a serious > > > problem. > > > @item > > > Test your code with valgrind and or Address Sanitizer to ensure it's free > > > of leaks, out of array accesses, etc. > > > + > > > +@item > > > +Check that your submitted patch shows up on @url{ > > > https://patchwork.ffmpeg.org}. > > > +Also make sure its status is updated, you can create an account and > > > update it. > > > +If your patch is incorrectly or not listed in patchwork then it might be > > > +missed by developers using patchwork to find patches needing review or > > > pushing. > > > @end enumerate > > > > > > I don't think we should require developers to use (or check, or update, or > > create-an-account-on) patchwork. Wasn't the whole point of patchwork that > > you can use it if you care, and you can ignore it if you don't care? > > yes one can ignore it but its alot more usefull if people keep in > mind that theres patchwork. > > For example, if you reply with LGTM or Acked-by: or Reviewed-by: > or Tested-by: > patchwork will pick that up, OTOH if you write "that works well, > i tested it" patchwork will not pick that up, nor will it "dude that > looks fine, please push it" nor does it "I dont like that change" > > the result of that is that if everyone ignores patchwork it will > contain most submited patches (some odd non standard attachmets are > missed) also due to this, people using patchwork to find patches that need a review will miss patches that patchwork didnt pick up. So its useful if such not picked up patches are 0 Iam one of the people using patchwork to keep track of patches still needing review or application if i dont review them immedeatly. Using my MUA for this has become too painfull and inefficient and noone else knows whats marked as unread (needing review) in my MUA, anyone knows what still needs a review in patchwork as its vissible to the world [...]
On Wed, Sep 07, 2016 at 04:44:54PM +0200, Michael Niedermayer wrote: > On Wed, Sep 07, 2016 at 04:39:54PM +0200, Michael Niedermayer wrote: > > On Wed, Sep 07, 2016 at 07:19:00AM -0700, Jonathan Campbell wrote: > > > On 09/07/2016 04:51 AM, Michael Niedermayer wrote: > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > doc/developer.texi | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/doc/developer.texi b/doc/developer.texi > > > > index 4d3a7ae..51e3da7 100644 > > > > --- a/doc/developer.texi > > > > +++ b/doc/developer.texi > > > > @@ -641,6 +641,12 @@ are notoriously left unchecked, which is a serious problem. > > > > @item > > > > Test your code with valgrind and or Address Sanitizer to ensure it's free > > > > of leaks, out of array accesses, etc. > > > > + > > > > +@item > > > > +Check that your submitted patch shows up on @url{https://patchwork.ffmpeg.org}. > > > > +Also make sure its status is updated, you can create an account and update it. > > > > +If your patch is incorrectly or not listed in patchwork then it might be > > > > +missed by developers using patchwork to find patches needing review or pushing. > > > > @end enumerate > > > > > > > > @section Patch review process > > > > > > > > > > I only see one of the 4 patches for AC-3 consistent noise generation (the libavutil version bump) on the patchwork site. Can I assume the other three will make their way into the patchwork list when the time is right, or did I submit the patch wrong? > > > > all 4 patches are there: > > https://patchwork.ffmpeg.org/project/ffmpeg/list/?submitter=82&state=%2A&archive=both > > in fact thats the 4 mails not the 4 patches from the last mail > patchwork does not support multiple patches per mail AFAIK and MUAs cant keep properly track of multiple patches per mail either a mail is either read or unread or otherwise flagged somehow, so patchwork is in fact not really worse than using a MUA here [...]
On Wed, 7 Sep 2016 14:27:07 +0200, Michael Niedermayer wrote: > yes one can ignore it but its alot more usefull if people keep in > mind that theres patchwork. The patch submission checklist is lengthy. I'm fine with an addition somewhere in doc/developer pointing out that it exists and how to use it if the patch submitter wants to, but I don't think it needs to be in the checklist.
On Thu, Sep 15, 2016 at 05:53:58PM -0800, Lou Logan wrote: > On Wed, 7 Sep 2016 14:27:07 +0200, Michael Niedermayer wrote: > > > yes one can ignore it but its alot more usefull if people keep in > > mind that theres patchwork. > > The patch submission checklist is lengthy. > > I'm fine with an addition somewhere in doc/developer pointing out that > it exists and how to use it if the patch submitter wants to, but I don't > think it needs to be in the checklist. where should it be put / where would you suggest to put it ? [...]
2016-09-16 15:07 GMT+08:00 Michael Niedermayer <michael@niedermayer.cc>: > On Thu, Sep 15, 2016 at 05:53:58PM -0800, Lou Logan wrote: > > On Wed, 7 Sep 2016 14:27:07 +0200, Michael Niedermayer wrote: > > > > > yes one can ignore it but its alot more usefull if people keep in > > > mind that theres patchwork. > > > > The patch submission checklist is lengthy. > > > > I'm fine with an addition somewhere in doc/developer pointing out that > > it exists and how to use it if the patch submitter wants to, but I don't > > think it needs to be in the checklist. > > where should it be put / where would you suggest to put it ? > I think this is very useful. Guys, what's the next step? > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Many that live deserve death. And some that die deserve life. Can you give > it to them? Then do not be too eager to deal out death in judgement. For > even the very wise cannot see all ends. -- Gandalf > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >
Hi, On Sun, Oct 9, 2016 at 7:32 AM, Steven Liu <lingjiujianke@gmail.com> wrote: > 2016-09-16 15:07 GMT+08:00 Michael Niedermayer <michael@niedermayer.cc>: > > > On Thu, Sep 15, 2016 at 05:53:58PM -0800, Lou Logan wrote: > > > On Wed, 7 Sep 2016 14:27:07 +0200, Michael Niedermayer wrote: > > > > > > > yes one can ignore it but its alot more usefull if people keep in > > > > mind that theres patchwork. > > > > > > The patch submission checklist is lengthy. > > > > > > I'm fine with an addition somewhere in doc/developer pointing out that > > > it exists and how to use it if the patch submitter wants to, but I > don't > > > think it needs to be in the checklist. > > > > where should it be put / where would you suggest to put it ? > > > I think this is very useful. > Guys, what's the next step? Put it somewhere that is not the checklist. Checklist is a list of required checks and allows us to block patches not adhering to it. For example, it mentions asan. If I send a patch that adds an asan error, that is grounds for blocking the patch. I'm pretty sure we unanimously agree that's a good thing. Patchwork is not something a patch adheres to. Rather, it's a method of patch tracking. As an analogy, trac is not a bug, or a bug report. Rather, it's a method of tracking bugs and bug reports. Aside from that linguistic difference, not all developers like patchwork and/or want to use it, so adding it in a list of required steps seems wrong to those developers that don't (want to) use it. In conclusion, mentioning patchwork somewhere on our website or documentation in the developer section is fine, but not in the checklist. Maybe add it to section 1.6 (Submitting patches)? Ronald
diff --git a/doc/developer.texi b/doc/developer.texi index 4d3a7ae..51e3da7 100644 --- a/doc/developer.texi +++ b/doc/developer.texi @@ -641,6 +641,12 @@ are notoriously left unchecked, which is a serious problem. @item Test your code with valgrind and or Address Sanitizer to ensure it's free of leaks, out of array accesses, etc. + +@item +Check that your submitted patch shows up on @url{https://patchwork.ffmpeg.org}. +Also make sure its status is updated, you can create an account and update it. +If your patch is incorrectly or not listed in patchwork then it might be +missed by developers using patchwork to find patches needing review or pushing. @end enumerate @section Patch review process
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- doc/developer.texi | 6 ++++++ 1 file changed, 6 insertions(+)