* [FFmpeg-devel] [PATCH 0/2] [RFC] doc/developer patch review improvements @ 2023-08-24 19:56 Michael Niedermayer 2023-08-24 19:56 ` [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive Michael Niedermayer 2023-08-24 19:56 ` [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML Michael Niedermayer 0 siblings, 2 replies; 28+ messages in thread From: Michael Niedermayer @ 2023-08-24 19:56 UTC (permalink / raw) To: FFmpeg development discussions and patches This set contains 2 additions to the patch review and commit policy/guidlines _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive 2023-08-24 19:56 [FFmpeg-devel] [PATCH 0/2] [RFC] doc/developer patch review improvements Michael Niedermayer @ 2023-08-24 19:56 ` Michael Niedermayer 2023-08-25 1:56 ` Vittorio Giovara 2023-08-25 14:22 ` Rémi Denis-Courmont 2023-08-24 19:56 ` [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML Michael Niedermayer 1 sibling, 2 replies; 28+ messages in thread From: Michael Niedermayer @ 2023-08-24 19:56 UTC (permalink / raw) To: FFmpeg development discussions and patches Suggested text is from Anton Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- doc/developer.texi | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/developer.texi b/doc/developer.texi index 0c2f2cd7d1..383120daaa 100644 --- a/doc/developer.texi +++ b/doc/developer.texi @@ -853,6 +853,9 @@ Everyone is welcome to review patches. Also if you are waiting for your patch to be reviewed, please consider helping to review other patches, that is a great 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. + @anchor{Regression tests} @chapter Regression tests -- 2.17.1 _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive 2023-08-24 19:56 ` [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive Michael Niedermayer @ 2023-08-25 1:56 ` Vittorio Giovara 2023-08-25 6:46 ` Nicolas George 2023-08-25 14:06 ` Anton Khirnov 2023-08-25 14:22 ` Rémi Denis-Courmont 1 sibling, 2 replies; 28+ messages in thread From: Vittorio Giovara @ 2023-08-25 1:56 UTC (permalink / raw) To: FFmpeg development discussions and patches On Thu, Aug 24, 2023 at 9:56 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > Suggested text is from Anton > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > doc/developer.texi | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/doc/developer.texi b/doc/developer.texi > index 0c2f2cd7d1..383120daaa 100644 > --- a/doc/developer.texi > +++ b/doc/developer.texi > @@ -853,6 +853,9 @@ Everyone is welcome to review patches. Also if you are > waiting for your patch > to be reviewed, please consider helping to review other patches, that is > a great > 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. > NAK we shouldn't put extra burden on reviewers, nor guilt trap them into suggesting an alternative approach offlist and irc discussion is of course recommended, but writing this rules in stone will only deter good reviews, in my opinion -- Vittorio _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive 2023-08-25 1:56 ` Vittorio Giovara @ 2023-08-25 6:46 ` Nicolas George 2023-08-25 9:22 ` Paul B Mahol 2023-08-25 17:23 ` Vittorio Giovara 2023-08-25 14:06 ` Anton Khirnov 1 sibling, 2 replies; 28+ messages in thread From: Nicolas George @ 2023-08-25 6:46 UTC (permalink / raw) To: FFmpeg development discussions and patches Vittorio Giovara (12023-08-25): > NAK > we shouldn't put extra burden on reviewers, nor guilt trap them into > suggesting an alternative approach It is hilarious, in a very sad way, that you prefer put extra burden on people who do things than on people who block them. -- Nicolas George _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive 2023-08-25 6:46 ` Nicolas George @ 2023-08-25 9:22 ` Paul B Mahol 2023-08-25 17:23 ` Vittorio Giovara 1 sibling, 0 replies; 28+ messages in thread From: Paul B Mahol @ 2023-08-25 9:22 UTC (permalink / raw) To: FFmpeg development discussions and patches On Fri, Aug 25, 2023 at 8:46 AM Nicolas George <george@nsup.org> wrote: > Vittorio Giovara (12023-08-25): > > NAK > > we shouldn't put extra burden on reviewers, nor guilt trap them into > > suggesting an alternative approach > > It is hilarious, in a very sad way, that you prefer put extra burden on > people who do things than on people who block them. > It is easier to post dubious solutions instead of doing proper research or/and review. > > -- > Nicolas George > _______________________________________________ > 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". > _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive 2023-08-25 6:46 ` Nicolas George 2023-08-25 9:22 ` Paul B Mahol @ 2023-08-25 17:23 ` Vittorio Giovara 1 sibling, 0 replies; 28+ messages in thread From: Vittorio Giovara @ 2023-08-25 17:23 UTC (permalink / raw) To: FFmpeg development discussions and patches On Fri, Aug 25, 2023 at 8:46 AM Nicolas George <george@nsup.org> wrote: > Vittorio Giovara (12023-08-25): > > NAK > > we shouldn't put extra burden on reviewers, nor guilt trap them into > > suggesting an alternative approach > > It is hilarious, in a very sad way, that you prefer put extra burden on > people who do things than on people who block them. > I am confused, your email is the epitome of not suggesting any alternative approach, so you're agreeing with it..? -- Vittorio _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive 2023-08-25 1:56 ` Vittorio Giovara 2023-08-25 6:46 ` Nicolas George @ 2023-08-25 14:06 ` Anton Khirnov 1 sibling, 0 replies; 28+ messages in thread From: Anton Khirnov @ 2023-08-25 14:06 UTC (permalink / raw) To: FFmpeg development discussions and patches Quoting Vittorio Giovara (2023-08-25 03:56:44) > On Thu, Aug 24, 2023 at 9:56 PM Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > Suggested text is from Anton > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > doc/developer.texi | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/doc/developer.texi b/doc/developer.texi > > index 0c2f2cd7d1..383120daaa 100644 > > --- a/doc/developer.texi > > +++ b/doc/developer.texi > > @@ -853,6 +853,9 @@ Everyone is welcome to review patches. Also if you are > > waiting for your patch > > to be reviewed, please consider helping to review other patches, that is > > a great > > 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. > > > > NAK > we shouldn't put extra burden on reviewers, nor guilt trap them into > suggesting an alternative approach I don't understand this argument at all. First, "ideally suggest an alternative approach" is an aspiration, not a hard requirement. Second, I don't think reviewers should be able to reject patches with no explanation. The author/submitter spent time and effort on writing and submitting the patch - it is only fair that if it's to be rejected, it should be done for a clear reason. > offlist and irc discussion is of course recommended, I absolutely do not recommend offlist discussion, as it is not visible to other developers or preserved in the archives. > but writing this rules in stone will only deter good reviews, in my > opinion Non-constructive reviews without an explanation are never good reviews. -- Anton Khirnov _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive 2023-08-24 19:56 ` [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive Michael Niedermayer 2023-08-25 1:56 ` Vittorio Giovara @ 2023-08-25 14:22 ` Rémi Denis-Courmont 2023-08-25 14:58 ` Anton Khirnov 1 sibling, 1 reply; 28+ messages in thread From: Rémi Denis-Courmont @ 2023-08-25 14:22 UTC (permalink / raw) To: FFmpeg development discussions and patches Le torstaina 24. elokuuta 2023, 22.56.14 EEST Michael Niedermayer a écrit : > Suggested text is from Anton > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > doc/developer.texi | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/doc/developer.texi b/doc/developer.texi > index 0c2f2cd7d1..383120daaa 100644 > --- a/doc/developer.texi > +++ b/doc/developer.texi > @@ -853,6 +853,9 @@ Everyone is welcome to review patches. Also if you are > waiting for your patch to be reviewed, please consider helping to review > other patches, that is a great way to get everyone's patches reviewed > sooner. > > +Reviews must be constructive Well, frankly, no. You're really confusing code reviews with teaching here. A code review is first and foremost meant to find problems and avoid adding bugs or bad designs into the code base. It is not meant to solve problems. Of course, it is preferable for a review to be constructive, but it is not always possible or reasonable. Sometimes code just does not belong in. Sometimes the reviewer can prove or make strong arguments that a patch is broken or bad, without having constructive feedback to give. This is just like mathematical proofs. Some are constructive, some aren't. And then sometimes an argument has been argued to death previously and there is really no point to rehash it again and again. If people cannot agree, they should refer to the TC, not brute force the review through overwhelming insistance. Also what Vittorio already pointed out. Ultimately, this is also a question of what to optimise for. And in my 20+ years experience with software development and open-source, I think it's abundantly clear that available skilled reviewers are an ever scarcer resource than skilled available developers, so you should not optimise for the later. So -1 as far as I am concerned. -- 雷米‧德尼-库尔蒙 http://www.remlab.net/ _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive 2023-08-25 14:22 ` Rémi Denis-Courmont @ 2023-08-25 14:58 ` Anton Khirnov 2023-08-25 15:09 ` Rémi Denis-Courmont 0 siblings, 1 reply; 28+ messages in thread From: Anton Khirnov @ 2023-08-25 14:58 UTC (permalink / raw) To: FFmpeg development discussions and patches Quoting Rémi Denis-Courmont (2023-08-25 16:22:45) > Le torstaina 24. elokuuta 2023, 22.56.14 EEST Michael Niedermayer a écrit : > > Suggested text is from Anton > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > doc/developer.texi | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/doc/developer.texi b/doc/developer.texi > > index 0c2f2cd7d1..383120daaa 100644 > > --- a/doc/developer.texi > > +++ b/doc/developer.texi > > @@ -853,6 +853,9 @@ Everyone is welcome to review patches. Also if you are > > waiting for your patch to be reviewed, please consider helping to review > > other patches, that is a great way to get everyone's patches reviewed > > sooner. > > > > +Reviews must be constructive > > Well, frankly, no. You're really confusing code reviews with teaching here. A > code review is first and foremost meant to find problems and avoid adding bugs > or bad designs into the code base. It is not meant to solve problems. Of > course, it is preferable for a review to be constructive, but it is not always > possible or reasonable. > > Sometimes code just does not belong in. > > Sometimes the reviewer can prove or make strong arguments that a patch is > broken or bad, without having constructive feedback to give. This is just like > mathematical proofs. Some are constructive, some aren't. > > And then sometimes an argument has been argued to death previously and there > is really no point to rehash it again and again. If people cannot agree, they > should refer to the TC, not brute force the review through overwhelming > insistance. I think we just have different interpretations of the word 'constructive' here. I certainly agree that some patches are just not acceptable - I certainly did not mean to imply that there must be a way forward for all patches. The intent is rather that every review (other than OK) should be based on technical arguments and not be a semantic equivalent of 'no'. In case you did not notice, we have a persistent problem with some people who are sending "reviews" of exactly this type. I don't think that should be acceptable. Would you be happier with some reformulation of the text that makes this more clear? -- Anton Khirnov _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive 2023-08-25 14:58 ` Anton Khirnov @ 2023-08-25 15:09 ` Rémi Denis-Courmont 2023-08-25 15:23 ` Anton Khirnov 2023-08-25 17:34 ` Leo Izen 0 siblings, 2 replies; 28+ messages in thread From: Rémi Denis-Courmont @ 2023-08-25 15:09 UTC (permalink / raw) To: FFmpeg development discussions and patches Le perjantaina 25. elokuuta 2023, 17.58.40 EEST Anton Khirnov a écrit : > > And then sometimes an argument has been argued to death previously and > > there is really no point to rehash it again and again. If people cannot > > agree, they should refer to the TC, not brute force the review through > > overwhelming insistance. > > I think we just have different interpretations of the word > 'constructive' here. > I certainly agree that some patches are just not acceptable - I certainly > did not mean to imply that there must be a way forward for all patches. I think that you do not agree with the generally accepted meaning of "constructive" in this context. By definition a review cannot be constructive, as in helpful or conducive of a way forward, if it argues that there are no ways forward. Maybe you meant "supported" or "corroborated". -- 雷米‧德尼-库尔蒙 http://www.remlab.net/ _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive 2023-08-25 15:09 ` Rémi Denis-Courmont @ 2023-08-25 15:23 ` Anton Khirnov 2023-08-25 17:26 ` Vittorio Giovara 2023-08-25 17:34 ` Leo Izen 1 sibling, 1 reply; 28+ messages in thread From: Anton Khirnov @ 2023-08-25 15:23 UTC (permalink / raw) To: FFmpeg development discussions and patches Quoting Rémi Denis-Courmont (2023-08-25 17:09:55) > Le perjantaina 25. elokuuta 2023, 17.58.40 EEST Anton Khirnov a écrit : > > > And then sometimes an argument has been argued to death previously and > > > there is really no point to rehash it again and again. If people cannot > > > agree, they should refer to the TC, not brute force the review through > > > overwhelming insistance. > > > > I think we just have different interpretations of the word > > 'constructive' here. > > I certainly agree that some patches are just not acceptable - I certainly > > did not mean to imply that there must be a way forward for all patches. > > I think that you do not agree with the generally accepted meaning of > "constructive" in this context. By definition a review cannot be constructive, > as in helpful or conducive of a way forward, if it argues that there are no > ways forward. Explaining why a patch is not acceptable is helpful IMO. Saying 'no', on the other hand, is not. > Maybe you meant "supported" or "corroborated". Might as well describe it in more than one word, since apparently it's so unclear. Would you be in favor of something along the lines of Nontrivial (i.e. other than cosmetics or accepting the patch) reviews must be based on technical arguments. If the reviewer fails to provide arguments for rejecting the patch or requesting changes, then the review may be disregarded. -- Anton Khirnov _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive 2023-08-25 15:23 ` Anton Khirnov @ 2023-08-25 17:26 ` Vittorio Giovara 2023-08-25 17:35 ` Anton Khirnov 0 siblings, 1 reply; 28+ messages in thread From: Vittorio Giovara @ 2023-08-25 17:26 UTC (permalink / raw) To: FFmpeg development discussions and patches On Fri, Aug 25, 2023 at 5:24 PM Anton Khirnov <anton@khirnov.net> wrote: > Quoting Rémi Denis-Courmont (2023-08-25 17:09:55) > > Le perjantaina 25. elokuuta 2023, 17.58.40 EEST Anton Khirnov a écrit : > > > > And then sometimes an argument has been argued to death previously > and > > > > there is really no point to rehash it again and again. If people > cannot > > > > agree, they should refer to the TC, not brute force the review > through > > > > overwhelming insistance. > > > > > > I think we just have different interpretations of the word > > > 'constructive' here. > > > I certainly agree that some patches are just not acceptable - I > certainly > > > did not mean to imply that there must be a way forward for all patches. > > > > I think that you do not agree with the generally accepted meaning of > > "constructive" in this context. By definition a review cannot be > constructive, > > as in helpful or conducive of a way forward, if it argues that there are > no > > ways forward. > > Explaining why a patch is not acceptable is helpful IMO. > Saying 'no', on the other hand, is not. > that is true, but saying "no" and preventing some bad code from making it in the codebase is better than not saying anything > Maybe you meant "supported" or "corroborated". > > Might as well describe it in more than one word, since apparently it's > so unclear. Would you be in favor of something along the lines of > > Nontrivial (i.e. other than cosmetics or accepting the patch) reviews > must be based on technical arguments. If the reviewer fails to provide > arguments for rejecting the patch or requesting changes, then the > review may be disregarded. > I agree with the text suggested, but I don't understand why it needs to be set in stone in the first place... -- Vittorio _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive 2023-08-25 17:26 ` Vittorio Giovara @ 2023-08-25 17:35 ` Anton Khirnov 0 siblings, 0 replies; 28+ messages in thread From: Anton Khirnov @ 2023-08-25 17:35 UTC (permalink / raw) To: FFmpeg development discussions and patches Quoting Vittorio Giovara (2023-08-25 19:26:21) > On Fri, Aug 25, 2023 at 5:24 PM Anton Khirnov <anton@khirnov.net> wrote: > > > Quoting Rémi Denis-Courmont (2023-08-25 17:09:55) > > > Le perjantaina 25. elokuuta 2023, 17.58.40 EEST Anton Khirnov a écrit : > > > > > And then sometimes an argument has been argued to death previously > > and > > > > > there is really no point to rehash it again and again. If people > > cannot > > > > > agree, they should refer to the TC, not brute force the review > > through > > > > > overwhelming insistance. > > > > > > > > I think we just have different interpretations of the word > > > > 'constructive' here. > > > > I certainly agree that some patches are just not acceptable - I > > certainly > > > > did not mean to imply that there must be a way forward for all patches. > > > > > > I think that you do not agree with the generally accepted meaning of > > > "constructive" in this context. By definition a review cannot be > > constructive, > > > as in helpful or conducive of a way forward, if it argues that there are > > no > > > ways forward. > > > > Explaining why a patch is not acceptable is helpful IMO. > > Saying 'no', on the other hand, is not. > > > > that is true, but saying "no" and preventing some bad code from making it > in the codebase is better than not saying anything If the code is so bad that it should not go in then surely someone can find it in themselves to write two sentences about the reason why it is so bad. Nobody is saying you have to produce a 10-page manifesto. > > Maybe you meant "supported" or "corroborated". > > > > Might as well describe it in more than one word, since apparently it's > > so unclear. Would you be in favor of something along the lines of > > > > Nontrivial (i.e. other than cosmetics or accepting the patch) reviews > > must be based on technical arguments. If the reviewer fails to provide > > arguments for rejecting the patch or requesting changes, then the > > review may be disregarded. > > > > I agree with the text suggested, but I don't understand why it needs to be > set in stone in the first place... There is a persistent problem with certain people rejecting patches for no clear reason, and then refusing to elaborate. -- Anton Khirnov _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive 2023-08-25 15:09 ` Rémi Denis-Courmont 2023-08-25 15:23 ` Anton Khirnov @ 2023-08-25 17:34 ` Leo Izen 2023-08-25 17:39 ` Nicolas George 1 sibling, 1 reply; 28+ messages in thread From: Leo Izen @ 2023-08-25 17:34 UTC (permalink / raw) To: ffmpeg-devel On 8/25/23 11:09, Rémi Denis-Courmont wrote: > Le perjantaina 25. elokuuta 2023, 17.58.40 EEST Anton Khirnov a écrit : >>> And then sometimes an argument has been argued to death previously and >>> there is really no point to rehash it again and again. If people cannot >>> agree, they should refer to the TC, not brute force the review through >>> overwhelming insistance. >> >> I think we just have different interpretations of the word >> 'constructive' here. >> I certainly agree that some patches are just not acceptable - I certainly >> did not mean to imply that there must be a way forward for all patches. > > I think that you do not agree with the generally accepted meaning of > "constructive" in this context. By definition a review cannot be constructive, > as in helpful or conducive of a way forward, if it argues that there are no > ways forward. > > Maybe you meant "supported" or "corroborated". > FWIW I read it the same way Anton did but if it's unclear then perhaps it could be modified. Essentially, I think what's going on is we don't want "NAK" without a reason. If you want to say a patch shouldn't make it in, there should at least be a reason. Even if the reason is "this API/module has no place in FFmpeg." - Leo Izen (Traneptora) _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive 2023-08-25 17:34 ` Leo Izen @ 2023-08-25 17:39 ` Nicolas George 0 siblings, 0 replies; 28+ messages in thread From: Nicolas George @ 2023-08-25 17:39 UTC (permalink / raw) To: FFmpeg development discussions and patches [-- Attachment #1.1: Type: text/plain, Size: 819 bytes --] Leo Izen (12023-08-25): > FWIW I read it the same way Anton did but if it's unclear then perhaps it > could be modified. Essentially, I think what's going on is we don't want > "NAK" without a reason. If you want to say a patch shouldn't make it in, > there should at least be a reason. I agree on this too. > Even if the reason is "this API/module has no place in FFmpeg." But not on this example: what has place in FFmpeg or not is anybody's arbitrary opinion, saying “no place in FFmpeg” alone is just a fancy way of saying “NAK” with no reason. It must be substantiated too, for example “the same feature is already possible [like that]”. And if the same feature is *not* already possible, then it surely means the code *does* belong in FFmpeg. Regards, -- Nicolas George [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 251 bytes --] _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML 2023-08-24 19:56 [FFmpeg-devel] [PATCH 0/2] [RFC] doc/developer patch review improvements Michael Niedermayer 2023-08-24 19:56 ` [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive Michael Niedermayer @ 2023-08-24 19:56 ` Michael Niedermayer 2023-08-24 20:04 ` Andreas Rheinhardt 2023-08-24 20:06 ` James Almer 1 sibling, 2 replies; 28+ messages in thread From: Michael Niedermayer @ 2023-08-24 19:56 UTC (permalink / raw) To: FFmpeg development discussions and patches 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 + @anchor{Regression tests} @chapter Regression tests -- 2.17.1 _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML 2023-08-24 19:56 ` [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML Michael Niedermayer @ 2023-08-24 20:04 ` Andreas Rheinhardt 2023-08-25 15:34 ` Michael Niedermayer 2023-08-24 20:06 ` James Almer 1 sibling, 1 reply; 28+ messages in thread From: Andreas Rheinhardt @ 2023-08-24 20:04 UTC (permalink / raw) To: ffmpeg-devel 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 _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML 2023-08-24 20:04 ` Andreas Rheinhardt @ 2023-08-25 15:34 ` Michael Niedermayer 2023-08-25 15:36 ` Jean-Baptiste Kempf 0 siblings, 1 reply; 28+ messages in thread From: Michael Niedermayer @ 2023-08-25 15:34 UTC (permalink / raw) To: FFmpeg development discussions and patches [-- Attachment #1.1: Type: text/plain, Size: 1789 bytes --] 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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Old school: Use the lowest level language in which you can solve the problem conveniently. New school: Use the highest level language in which the latest supercomputer can solve the problem without the user falling asleep waiting. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] [-- Attachment #2: Type: text/plain, Size: 251 bytes --] _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML 2023-08-25 15:34 ` Michael Niedermayer @ 2023-08-25 15:36 ` Jean-Baptiste Kempf 2023-08-25 15:47 ` Paul B Mahol 2023-08-25 16:27 ` Nicolas George 0 siblings, 2 replies; 28+ messages in thread From: Jean-Baptiste Kempf @ 2023-08-25 15:36 UTC (permalink / raw) To: ffmpeg-devel 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 :) -- 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML 2023-08-25 15:36 ` Jean-Baptiste Kempf @ 2023-08-25 15:47 ` Paul B Mahol 2023-08-25 16:27 ` Nicolas George 1 sibling, 0 replies; 28+ messages in thread From: Paul B Mahol @ 2023-08-25 15:47 UTC (permalink / raw) To: FFmpeg development discussions and patches 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". > _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML 2023-08-25 15:36 ` Jean-Baptiste Kempf 2023-08-25 15:47 ` Paul B Mahol @ 2023-08-25 16:27 ` Nicolas George 2023-08-25 16:33 ` Jean-Baptiste Kempf 1 sibling, 1 reply; 28+ messages in thread From: Nicolas George @ 2023-08-25 16:27 UTC (permalink / raw) To: FFmpeg development discussions and patches [-- Attachment #1.1: Type: text/plain, Size: 152 bytes --] Jean-Baptiste Kempf (12023-08-25): > So that means mandatory sending to the mailing list :) And change the name to libav? -- Nicolas George [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 251 bytes --] _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML 2023-08-25 16:27 ` Nicolas George @ 2023-08-25 16:33 ` Jean-Baptiste Kempf 2023-08-25 17:16 ` Nicolas George 0 siblings, 1 reply; 28+ messages in thread From: Jean-Baptiste Kempf @ 2023-08-25 16:33 UTC (permalink / raw) To: ffmpeg-devel 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 - 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML 2023-08-25 16:33 ` Jean-Baptiste Kempf @ 2023-08-25 17:16 ` Nicolas George 2023-08-25 17:25 ` James Almer 0 siblings, 1 reply; 28+ messages in thread From: Nicolas George @ 2023-08-25 17:16 UTC (permalink / raw) To: FFmpeg development discussions and patches [-- Attachment #1.1: Type: text/plain, Size: 308 bytes --] 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. -- Nicolas George [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 251 bytes --] _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML 2023-08-25 17:16 ` Nicolas George @ 2023-08-25 17:25 ` James Almer 2023-08-25 17:42 ` Nicolas George 0 siblings, 1 reply; 28+ messages in thread From: James Almer @ 2023-08-25 17:25 UTC (permalink / raw) To: ffmpeg-devel 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. _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML 2023-08-25 17:25 ` James Almer @ 2023-08-25 17:42 ` Nicolas George 2023-08-25 21:41 ` Ronald S. Bultje 0 siblings, 1 reply; 28+ messages in thread From: Nicolas George @ 2023-08-25 17:42 UTC (permalink / raw) To: FFmpeg development discussions and patches [-- Attachment #1.1: Type: text/plain, Size: 722 bytes --] 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. -- Nicolas George [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 251 bytes --] _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML 2023-08-25 17:42 ` Nicolas George @ 2023-08-25 21:41 ` Ronald S. Bultje 0 siblings, 0 replies; 28+ messages in thread From: Ronald S. Bultje @ 2023-08-25 21:41 UTC (permalink / raw) To: FFmpeg development discussions and patches 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 _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML 2023-08-24 19:56 ` [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML Michael Niedermayer 2023-08-24 20:04 ` Andreas Rheinhardt @ 2023-08-24 20:06 ` James Almer 2023-08-24 20:23 ` Andreas Rheinhardt 1 sibling, 1 reply; 28+ messages in thread From: James Almer @ 2023-08-24 20:06 UTC (permalink / raw) To: ffmpeg-devel 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 > _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML 2023-08-24 20:06 ` James Almer @ 2023-08-24 20:23 ` Andreas Rheinhardt 0 siblings, 0 replies; 28+ messages in thread From: Andreas Rheinhardt @ 2023-08-24 20:23 UTC (permalink / raw) To: ffmpeg-devel 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 _______________________________________________ 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". ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2023-08-25 21:41 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-08-24 19:56 [FFmpeg-devel] [PATCH 0/2] [RFC] doc/developer patch review improvements Michael Niedermayer 2023-08-24 19:56 ` [FFmpeg-devel] [PATCH 1/2] doc/developer: Reviews must be constructive Michael Niedermayer 2023-08-25 1:56 ` Vittorio Giovara 2023-08-25 6:46 ` Nicolas George 2023-08-25 9:22 ` Paul B Mahol 2023-08-25 17:23 ` Vittorio Giovara 2023-08-25 14:06 ` Anton Khirnov 2023-08-25 14:22 ` Rémi Denis-Courmont 2023-08-25 14:58 ` Anton Khirnov 2023-08-25 15:09 ` Rémi Denis-Courmont 2023-08-25 15:23 ` Anton Khirnov 2023-08-25 17:26 ` Vittorio Giovara 2023-08-25 17:35 ` Anton Khirnov 2023-08-25 17:34 ` Leo Izen 2023-08-25 17:39 ` Nicolas George 2023-08-24 19:56 ` [FFmpeg-devel] [PATCH 2/2] doc/developer: Code pushed without patches on ffmpeg-devel must be announced on the ML Michael Niedermayer 2023-08-24 20:04 ` Andreas Rheinhardt 2023-08-25 15:34 ` Michael Niedermayer 2023-08-25 15:36 ` Jean-Baptiste Kempf 2023-08-25 15:47 ` Paul B Mahol 2023-08-25 16:27 ` Nicolas George 2023-08-25 16:33 ` Jean-Baptiste Kempf 2023-08-25 17:16 ` Nicolas George 2023-08-25 17:25 ` James Almer 2023-08-25 17:42 ` Nicolas George 2023-08-25 21:41 ` Ronald S. Bultje 2023-08-24 20:06 ` James Almer 2023-08-24 20:23 ` Andreas Rheinhardt
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel This inbox may be cloned and mirrored by anyone: git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \ ffmpegdev@gitmailbox.com public-inbox-index ffmpegdev Example config snippet for mirrors. AGPL code for this site: git clone https://public-inbox.org/public-inbox.git