[FFmpeg-devel] [RFC] Reviewing merges

[James Almer]

On 4/24/2017 3:27 PM, Michael Niedermayer wrote:
> On Mon, Apr 24, 2017 at 11:23:16AM -0300, James Almer wrote:
>> On 4/23/2017 11:07 PM, Michael Niedermayer wrote:
>>> Hi all
>>>
>>> Should changes ported from libav (what we call merges) be reviewed
>>> before being pushed?
>>
>> The lot of merges are simple things like "Fix this bug that was already
>> fixed in ffmpeg months ago", "K&R", etc. Lately we are even no-oping a
>> good amount of them as they don't even apply.
> 
>> Only a small set of merges are big API changes, and those are always
>> handled by more than one developer, either by reviewing the changes,
>> testing them or by helping getting the thing working. And of course, any
>> bug that was not caught before pushing is fixed afterwards.
>> In fact, it should be noted that if they are initially skipped for
>> requiring more thought and for blocking unrelated merges, when we get
>> them working at a latter point we always send them to the ML instead of
>> simply pushing them.
> 
> yes
> 
> could you send more changes to the ML instead of just ones
> initially skiped ? (and of course i dont mean trivial / clearly correct
> stuff)

If I'm the one merging some change that has considerable chances of
introducing regressions, then sure.

> 
> More eyes may catch more issues also i belive its important that we
> all see the changes being done, we all have to maintain the code
> which interacts with these changes.
> 
> 
> [...]
> 
>> We have recently been able to go through six hundred or so commits in a
>> month or two this way after being stuck for the longest time by a few of
>> those big API changes. If we start requiring every commit to go through
>> a review process on the ML then we will never catch up with the backlog.
>> In short, things as they are right now are smooth. Changing it will only
>> make this slower.
> 
> Maybe, but is merging more faster also better for FFmpeg ?

Not being one thousand commits behind is. If someone wants to submit
something to both projects and find code or a framework they depend on
isn't in one, then they will not submit it to that project.

> I did not analyze the bugs on our bug tracker but subjectivly the
> number of regressions seems much larger than a year or 2 ago.
> and i just yesterday found 2 issues in a merge (which you fixed)
> 
> 
>>
>> That aside, may i ask what prompted this question? Did someone complain
>> to you privately? No merge recently seems to have broken anything that
>> hasn't been already fixed, beyond one threading bug that's being
>> investigated right now, so i wonder why ask this now.
> 
> It was on my mind since a long time. I guess the recent surge of merges
> and the uneasiness of doing the 3.3 release in the middle kind of was
> bringing it up into relevance again.
> Before that with the merges falling behind, multiple libav developers
> did sent patches to ffmpeg-devel which passed through review on our
> codebase before application. The merges restarting stoped this it
> seems.
> I strongly belive that libav developers sending patches to ffmpeg
> and thus the author of a change testing it on top of ffmpeg results
> in better code than if its merged by a 3rd party.

Of course, and said developers should ideally be encouraged to keep
doing it instead of saying "oh, they'll merge it". If they wanted to
contribute when merges were almost at a standstill, they can still
contribute and make the job of the people handling the merges easier.