[MPlayer-dev-eng] Re: patch review proceedings

[Diego Biurrun]

On Mon, Sep 11, 2006 at 03:26:18PM +0300, Uoti Urpala wrote:
> On Mon, 2006-09-11 at 13:11 +0200, Diego Biurrun wrote:
> > On Mon, Sep 11, 2006 at 01:39:04PM +0300, Uoti Urpala wrote:
> > > On Mon, 2006-09-11 at 10:43 +0200, Diego Biurrun wrote:
> > > > just contained whitespace changes I would have quickly looked it over,
> > > > run 'diff -w' over the resulting file and applied it.  We wouldn't even
> > > > be having this discussion now...
> > > 
> > > Maybe you would have done that, but that doesn't show there was anything
> > > wrong with the patch now either.
> > 
> > It shows that it would have gotten the patch applied, which supposedly
> > was the original intention of the patch sender.
> 
> Supposedly, but it's not valid logic to say that rejecting patches based
> on some arbitrary criteria would in itself be proof of the correctness
> or sanity of those criteria "because patches following them would have
> already been accepted".

But this is not my point.  Most patch senders don't just get offended
and leave in a huff but instead do what was requested of them.

> I think my paragraph above explained it clearly enough, but I'll try
> again: separating even small whitespace changes into separate patches is
> not something that would be considered good practice by all experts, and
> so it's wrong to assume that someone is not an expert (or even that he's
> a beginner) because he doesn't do that.

It is, however, the currently accepted practice and our policy even
requires that behavior.  So if the patch sender does not split the patch
I would have to.  This is work I'd gladly skip.

So maybe this policy should be changed.  Your welcome to make an attempt
to do this.  You're also welcome to review and accept patches with laxer
criteria.

> > > > In any case the nitpicky reviewers like myself appear to be the only
> > > > game in town so for better or worse the patch senders will have to adapt
> > > > to it.
> > > 
> > > Patch senders don't seem to have much chance of getting multiline
> > > patches outside the build system applied by you either.
> > 
> > They have excellent chances of getting patches applied by me when I am
> > confident enough that they are correct.  For the parts I maintain I can
> 
> And how "excellent" are the chances of you becoming confident about any
> multiline patches that are not about build issues?

In the part you snipped is the answer:

  IOW patch submitters either have to hit my area of maintenance or dumb
  down their submissions sufficiently for my consumption.

A whitespace-only patch is great.  I can apply it without thinking.

> How many such patches have you applied this whole year? None during
> the last month (not counting the one applied by you after Michael
> reviewed it), and I think the last month is not exceptional.

I have no idea how many, but there were some.  If you're interested in
the exact numbers, find out.

To me reviewing patches is not an entirely unrewarding passtime as it
seems to be for most others.  I'd gladly get familiar with more areas of
MPlayer and review more patches in the process.  It's lack of time and a
matter of priorities that's keeping me.

> > > I still stand by what I said in the earlier mail: patches to
> > > code/features that have active maintainers get applied while other
> > > patches often won't unless they're completely trivial/obvious, and
> > > the proportion of unnecessarily dropped patches is more a function
> > > of which code parts have active maintainers rather than of how many
> > > "reviewers" there are.
> > 
> > .. and what is the relationship of this point with the current
> > discussion?
> 
> You talked about how "patch senders outnumber reviewers" and how patch
> handling would need to be tuned to minimize time use by reviewers to the
> absolute minimum. However maintainers do not seem to be overwhelmed by
> patches to the code they're interested in, and committing patches to
> other code is rare enough that it can hardly be limited by time required
> for things like cleaning up cosmetic issues.

But what if maintainers were overwhelmed just by the patches or simply
the work in their area?  There were 2500 commits to MPlayer this year,
600 from me, not counting the homepage and FFmpeg.

Michael sure is overwhelmed by patches to FFmpeg and I myself still have
some build system patches queued.  Reviewing just the patches to the
parts I maintain and keeping up-to-date with changes and suggestions is
keeping me pretty busy.

There's another argument I have brought up before: Why should I do the
boring work for the patch submitters.  I'd rather have them do it for
me.

Also, if I request changes and the submitter implements them, this is a
good indication that will be possible to work well with that person in
the future and that they will follow up if there are problems with their
patches.

Reviewers/maintainers *are* the scarce resource.  If for lack of time or
lack of interest or both is irrelevant.  The winning strategy to get
patches applied is to make things easy for the committers.

To get back to a slightly more constructive venue: What is your goal?
Do you wish me (everybody) to be less pedantic when reviewing patches?
Do you wish to change the policy regarding patches?  What do you hope
to gain from this?

(All of this regardless of the fact that it applies to work you cannot
be bothered to do yourself.  But regardless of your attitude your point
can be valid - even though less credible.)

Diego