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

[Uoti Urpala]

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:
> > > 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
> > 
> > Rich says some compilers might generate different code, but it would
> > still be functionally equivalent. In principle compilers are free to
> > generate different code because of whitespace changes too...
> 
> I'm sure that in principle they are free to generate different code
> depending on the moon phase as well...

The point being that there is no debate about whether it would cause
functionality changes: it wouldn't.

> > > 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".

> > Not separating every whitespace change when it would mean creating a
> > separate patch just for reindenting a few lines is not "a beginner's
> > mistake", it's common sense. Even if you disagree about the desirability
> > of doing that it's still a fact that it's not something universally
> > accepted by non-beginners.
> 
> Mixing whitespace changes with non-whitespace changes *is* a beginners
> mistake.  Just look at the patches coming in from beginners..

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.

> > > 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? 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 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.