patch review proceedings (was: Re: [MPlayer-dev-eng] [PATCH] make cache2.c readable)

[Diego Biurrun]

On Mon, Sep 11, 2006 at 03:43:29AM +0300, Uoti Urpala wrote:
> On Mon, 2006-09-11 at 01:10 +0200, Diego Biurrun wrote:
> > On Mon, Sep 11, 2006 at 01:54:33AM +0300, Uoti Urpala wrote:
> > > Yes, there are no reviewers for all patches. However I think the main
> > > problem is that in many cases most people are not using/interested
> > > in/familiar with the relevant code. The number of patches is not so high
> > > that dealing with some cosmetic changes in each would cause significant
> > > extra work.
> > 
> > I disagree completely.  I happen to be one of the (very few) active
> > reviewers, so I'm not making this up.  Look at what Michael says and
> > does, you'll see that he agrees.
> 
> I checked your commits during the last month which said they were
> patches from others. There were 10 such commits. 5 were one-line
> changes, one 2-line, one a deletion of 5 lines. Most of those were
> related to the build system. For those the submission format or
> cosmetics hardly matter. Of the remaining 3, 2 were larger changes to
> the build system and one was the libswscale -fPIC support patch which
> was mainly reviewed by Michael IIRC.

.. and Michael will immediately reject a patch that has whitespace
changes mixed in ..

> I think those numbers show that you do take care of the build system
> patches, but not that some cosmetic changes per patch would be a problem
> or that you would work much on patches related to code you don't
> otherwise maintain yourself either.

Those numbers don't count FFmpeg.

I give priority to code I maintain and try to review all the patches and
commits in those areas.  I also attempt to review some other patches as
time permits.  When this affects part of the code I don't understand well
it gets significantly easier if patches are small and well-contained.

The patch we started out debating in this thread is a good example.  It
mixes whitespace changes with non-whitespace changes.  You say it's
cosmetic, Rich says it is not.  There is obviously room for debate here,
something I have no interest in getting into.  However, had the patch
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...

> > Dealing with stray cosmetics and similar issues that could easily be
> > avoided does cause significant extra work.  Significant because it causes
> > me frustration and wastes my time that I could better spend reviewing
> > the next patch.  Significant because the bottleneck is at the reviewing
> > side, not at the submitting side.
> 
> However rejecting patches because of some minor issues doesn't save time
> on either side.

Rejecting patches just takes one quick mail.  There's also the hope that
it educates the patch sender so that the next time around this problem
will not show up anymore.  Judging from experience this works quite well
and does save time in the medium term.

It's also simply a good measure of experience and professionalism.  Not
properly separating whitespace changes from more substantive changes is
a beginners mistake and thus an indication that more problems may be
lurking.

> > I'll ask my question again: More reviewers are desperately needed.  Are
> > you up to the task?
> 
> If you mean reviewing patches for features which I don't personally use
> or care about, no.

Even for the ones that you do care about.  I have about 1500 mails in
the patch backlog mbox, there's bound to be something interesting.

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.

Diego