| Message ID | 20230824195615.19017-3-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | doc/developer patch review improvements | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Michael Niedermayer: > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > doc/developer.texi | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/doc/developer.texi b/doc/developer.texi > index 383120daaa..1c0091fc74 100644 > --- a/doc/developer.texi > +++ b/doc/developer.texi > @@ -856,6 +856,15 @@ way to get everyone's patches reviewed sooner. > Reviews must be constructive and when rejecting a patch the reviewer must explain > their reasons and ideally suggest an alternative approach. > > +If a change is pushed without being sent to ffmpeg-devel, the developer > +pushing it must annouce doing so on the ffmpeg-devel mailing list immedeatly. > +@example > +forgot a semicolon in this patch, pushed a seperate fix > +pushed my new autograd engine and stable diffusion filter. Didnt want to > +go through the bikeshed if that belongs in FFmpeg, go to the GA if you want > +it removed. Otherwise Just tell me what i should improve and ill look into it. > +@end example This encourages pushing patches (even completely new filters) without sending them to the ML. - Andreas
On 8/24/2023 4:56 PM, Michael Niedermayer wrote: > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > doc/developer.texi | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/doc/developer.texi b/doc/developer.texi > index 383120daaa..1c0091fc74 100644 > --- a/doc/developer.texi > +++ b/doc/developer.texi > @@ -856,6 +856,15 @@ way to get everyone's patches reviewed sooner. > Reviews must be constructive and when rejecting a patch the reviewer must explain > their reasons and ideally suggest an alternative approach. > > +If a change is pushed without being sent to ffmpeg-devel, the developer > +pushing it must annouce doing so on the ffmpeg-devel mailing list immedeatly. immediately. This chunk is ok, but... > +@example > +forgot a semicolon in this patch, pushed a seperate fix > +pushed my new autograd engine and stable diffusion filter. Didnt want to > +go through the bikeshed if that belongs in FFmpeg, go to the GA if you want > +it removed. Otherwise Just tell me what i should improve and ill look into it. > +@end example ...this one isn't. It's very passive aggressive and not informative at all. This instead should state that patches pushed without passing through the ML have to either be trivial, to push fixes to mistakes in recently pushed changes that did go through the ML, or for code you maintain. > + > @anchor{Regression tests} > @chapter Regression tests >
James Almer: > On 8/24/2023 4:56 PM, Michael Niedermayer wrote: >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> doc/developer.texi | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/doc/developer.texi b/doc/developer.texi >> index 383120daaa..1c0091fc74 100644 >> --- a/doc/developer.texi >> +++ b/doc/developer.texi >> @@ -856,6 +856,15 @@ way to get everyone's patches reviewed sooner. >> Reviews must be constructive and when rejecting a patch the reviewer >> must explain >> their reasons and ideally suggest an alternative approach. >> +If a change is pushed without being sent to ffmpeg-devel, the >> developer >> +pushing it must annouce doing so on the ffmpeg-devel mailing list >> immedeatly. > > immediately. > > This chunk is ok, but... > >> +@example >> +forgot a semicolon in this patch, pushed a seperate fix >> +pushed my new autograd engine and stable diffusion filter. Didnt want to >> +go through the bikeshed if that belongs in FFmpeg, go to the GA if >> you want >> +it removed. Otherwise Just tell me what i should improve and ill look >> into it. >> +@end example > > ...this one isn't. It's very passive aggressive and not informative at all. > This instead should state that patches pushed without passing through > the ML have to either be trivial, to push fixes to mistakes in recently > pushed changes that did go through the ML, or for code you maintain. > The maintainer exception has actually been removed in 6a3e174ad1921ba6aec473a2224c71610de3329b. - Andreas
On Thu, Aug 24, 2023 at 10:04:16PM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > doc/developer.texi | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/doc/developer.texi b/doc/developer.texi > > index 383120daaa..1c0091fc74 100644 > > --- a/doc/developer.texi > > +++ b/doc/developer.texi > > @@ -856,6 +856,15 @@ way to get everyone's patches reviewed sooner. > > Reviews must be constructive and when rejecting a patch the reviewer must explain > > their reasons and ideally suggest an alternative approach. > > > > +If a change is pushed without being sent to ffmpeg-devel, the developer > > +pushing it must annouce doing so on the ffmpeg-devel mailing list immedeatly. > > +@example > > +forgot a semicolon in this patch, pushed a seperate fix > > +pushed my new autograd engine and stable diffusion filter. Didnt want to > > +go through the bikeshed if that belongs in FFmpeg, go to the GA if you want > > +it removed. Otherwise Just tell me what i should improve and ill look into it. > > +@end example > > This encourages pushing patches (even completely new filters) without > sending them to the ML. That was not the intend but if you look at "cvslog" and ffmpeg-devel, you will notice that there are things being pushed that have not been seen on the ffmpeg-devel mailing list. thx [...]
On Fri, 25 Aug 2023, at 17:34, Michael Niedermayer wrote: > On Thu, Aug 24, 2023 at 10:04:16PM +0200, Andreas Rheinhardt wrote: >> Michael Niedermayer: >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> > --- >> > doc/developer.texi | 9 +++++++++ >> > 1 file changed, 9 insertions(+) >> > >> > diff --git a/doc/developer.texi b/doc/developer.texi >> > index 383120daaa..1c0091fc74 100644 >> > --- a/doc/developer.texi >> > +++ b/doc/developer.texi >> > @@ -856,6 +856,15 @@ way to get everyone's patches reviewed sooner. >> > Reviews must be constructive and when rejecting a patch the reviewer must explain >> > their reasons and ideally suggest an alternative approach. >> > >> > +If a change is pushed without being sent to ffmpeg-devel, the developer >> > +pushing it must annouce doing so on the ffmpeg-devel mailing list immedeatly. >> > +@example >> > +forgot a semicolon in this patch, pushed a seperate fix >> > +pushed my new autograd engine and stable diffusion filter. Didnt want to >> > +go through the bikeshed if that belongs in FFmpeg, go to the GA if you want >> > +it removed. Otherwise Just tell me what i should improve and ill look into it. >> > +@end example >> >> This encourages pushing patches (even completely new filters) without >> sending them to the ML. > > That was not the intend but if you look at "cvslog" and ffmpeg-devel, you will > notice that there are things being pushed that have not been seen on the > ffmpeg-devel mailing list. So that means mandatory sending to the mailing list :)
On Fri, Aug 25, 2023 at 5:36 PM Jean-Baptiste Kempf <jb@videolan.org> wrote: > > > On Fri, 25 Aug 2023, at 17:34, Michael Niedermayer wrote: > > On Thu, Aug 24, 2023 at 10:04:16PM +0200, Andreas Rheinhardt wrote: > >> Michael Niedermayer: > >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> > --- > >> > doc/developer.texi | 9 +++++++++ > >> > 1 file changed, 9 insertions(+) > >> > > >> > diff --git a/doc/developer.texi b/doc/developer.texi > >> > index 383120daaa..1c0091fc74 100644 > >> > --- a/doc/developer.texi > >> > +++ b/doc/developer.texi > >> > @@ -856,6 +856,15 @@ way to get everyone's patches reviewed sooner. > >> > Reviews must be constructive and when rejecting a patch the reviewer > must explain > >> > their reasons and ideally suggest an alternative approach. > >> > > >> > +If a change is pushed without being sent to ffmpeg-devel, the > developer > >> > +pushing it must annouce doing so on the ffmpeg-devel mailing list > immedeatly. > >> > +@example > >> > +forgot a semicolon in this patch, pushed a seperate fix > >> > +pushed my new autograd engine and stable diffusion filter. Didnt > want to > >> > +go through the bikeshed if that belongs in FFmpeg, go to the GA if > you want > >> > +it removed. Otherwise Just tell me what i should improve and ill > look into it. > >> > +@end example > >> > >> This encourages pushing patches (even completely new filters) without > >> sending them to the ML. > > > > That was not the intend but if you look at "cvslog" and ffmpeg-devel, > you will > > notice that there are things being pushed that have not been seen on the > > ffmpeg-devel mailing list. > > So that means mandatory sending to the mailing list :) > > So that means I will fork librempeg and stop sending patches/commits here. > -- > Jean-Baptiste Kempf - President > +33 672 704 734 > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
Jean-Baptiste Kempf (12023-08-25):
> So that means mandatory sending to the mailing list :)
And change the name to libav?
On Fri, 25 Aug 2023, at 18:27, Nicolas George wrote: > Jean-Baptiste Kempf (12023-08-25): >> So that means mandatory sending to the mailing list :) > > And change the name to libav? Trolling is not going to get you anywhere.
Jean-Baptiste Kempf (12023-08-25): > >> So that means mandatory sending to the mailing list :) > Trolling is not going to get you anywhere. What trolling? You mean the suggestion to enforce a rule that is known to have caused a fork to wither and die? That was not from me.
On 8/25/2023 2:16 PM, Nicolas George wrote: > Jean-Baptiste Kempf (12023-08-25): >>>> So that means mandatory sending to the mailing list :) > >> Trolling is not going to get you anywhere. > > What trolling? You mean the suggestion to enforce a rule that is known > to have caused a fork to wither and die? That was not from me. As i said on IRC, there's a difference between "Unless justified, please send things to the ML before pushing" and "Everything must have at least two reviews from long standing developers". This is not requiring everything to have been reviewed before being pushed, it's asking for non trivial things to hit the ML so people have at least a chance at finding problems.
James Almer (12023-08-25): > As i said on IRC, A medium that can only be accessed by a small fraction of the contributors due to timeliness constraints. > there's a difference between "Unless justified, please > send things to the ML before pushing" and "Everything must have at least two > reviews from long standing developers". > > This is not requiring everything to have been reviewed before being pushed, > it's asking for non trivial things to hit the ML so people have at least a > chance at finding problems. Faire enough. Just let us make it very clear it is the former and not the latter. The I-don't-know-the-code-“LGTM” mandatory review were a disgrace.
Hi, On Fri, Aug 25, 2023 at 1:42 PM Nicolas George <george@nsup.org> wrote: > James Almer (12023-08-25): > > As i said on IRC, > > A medium that can only be accessed by a small fraction of the > contributors due to timeliness constraints. > I suspect you misunderstand IRC. With matrix, a bouncer or a 24/7 client, most of us are online with 24/7 history. Chat logs even provide google searchability. I prefer IRC over mailinglist for most technical discussions. Ronald
diff --git a/doc/developer.texi b/doc/developer.texi index 383120daaa..1c0091fc74 100644 --- a/doc/developer.texi +++ b/doc/developer.texi @@ -856,6 +856,15 @@ way to get everyone's patches reviewed sooner. Reviews must be constructive and when rejecting a patch the reviewer must explain their reasons and ideally suggest an alternative approach. +If a change is pushed without being sent to ffmpeg-devel, the developer +pushing it must annouce doing so on the ffmpeg-devel mailing list immedeatly. +@example +forgot a semicolon in this patch, pushed a seperate fix +pushed my new autograd engine and stable diffusion filter. Didnt want to +go through the bikeshed if that belongs in FFmpeg, go to the GA if you want +it removed. Otherwise Just tell me what i should improve and ill look into it. +@end example + @anchor{Regression tests} @chapter Regression tests
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- doc/developer.texi | 9 +++++++++ 1 file changed, 9 insertions(+)