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

[Diego Biurrun]

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

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

> > > 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
> 
> And after that one quick mail either the submitter gets too annoyed by
> your nitpicking and the patch is lost,

Alternatively, I could just not look at it and it would get lost in the
void instead.  So much better...

> or he submits a new version and you have to recheck it completely
> again which takes more time.

Spotting cosmetic changes usually is very quick.  In this particular
case it took me less than a minute.  I doubt that this adds up to more
time total.

> > 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.
> 
> You can tell what you hope from future patches without rejecting the
> current one.

I could and I have done so in the past.  I just cannot always be
bothered to clean up patches for other people.  Why should I have to do
the boring work for them?

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

> > 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
determine this quickly.  For other parts of the code it takes much more
time or I simply cannot do it.  IOW patch submitters either have to hit
my area of maintenance or dumb down their submissions sufficiently for
my consumption.  But this goes for all reviewers, not just 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?

Of course patches for code with active maintainers have higher chances
of getting applied.  That's the definition of active maintenance.  If we
had more people willing to review patches outside their areas of
maintenance more patches would get applied.  Unfortunately this is not
the case.

Patches get applied when the patch sender manages to get somebody to
apply it.  As I said above this requires making the patch either simple
or interesting enough..

Diego