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

[Uoti Urpala]

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

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

> > 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, or he submits a new version and
you have to recheck it completely again which takes more time.

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

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

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