Re: [Qemu-devel] [PATCH 02/18] slirp: Generalizing and neutralizing code before adding IPv6 stuff

[Eric Blake]

On 12/11/2015 08:40 AM, Laszlo Ersek wrote:
> meta
> 
> On 12/11/15 16:09, Thomas Huth wrote:
>> On 11/12/15 15:55, Samuel Thibault wrote:
>>> Thomas Huth, on Fri 11 Dec 2015 15:32:48 +0100, wrote:
>>>> So maybe it's better to do smaller steps instead: Would it for example
>>>> make sense to split the whole series into two parts - first a series
>>>> that does all the preparation and cleanup patches. And then once that
>>>> has been reviewed and merged, send the second series that adds the real
>>>> new IPv6 code.
>>>
>>> Ok, that's what we already have: patches 1-9 are refactoring and
>>> support, and 10-18 are ipv6 code.
>>
>> Sounds good, ... then I'd suggest to sent the preparation patches
>> separately next time and get them accepted first.
> 
> And then the next reviewer will say, "nice, but it would be even nicer
> to see what *motivates* these preparatory patches!" :)
> 
> Disclaimer: I don't have any technical context for this thread; I just
> noticed Samuel's email / frustration. I know that all too well, first
> hand, from this list and elsewhere. I don't know how it can be fixed,
> but I'm positive it is a *systemic* problem with this development model.

The best defense you have against this is to put more information in a
commit message - any time you have split work to ease review, make sure
to mention it in the commit message.  Any time you choose to merge work
into a single patch for less churn, mention it.  The commit message is
your greatest chance of explaining to reviewers WHY you did it the way
you did; and a reasonable reviewer should be happy enough that you did
the work without making you redo it to match their 'ivory tower'
alternative approach that meets the same end.  If you changed a patch
from an earlier version due to a particular reviewer, feel free to
mention that reviewer in your explanation of changes (although in this
case, doing it in the cover letter or after the --- marker may be better
than in the commit body proper).

Yes, I've also been on the receiving end of frustration of my patch not
being perfect enough, and try to be relatively forgiving of different
styles when they aren't outright forbidden by the code base's written
standards (there's a huge difference between a patch not being
technically correct, and merely not being stylistically ideal according
to my ideals).  Also, projects that automate standards checks (such as
our scripts/checkpatch.pl) are nicer than ones that require contributors
to comply with unwritten rules - but even then you can only encode so
much into an automated checker, and still need to leave room for human
judgment on when to bend the rules.

At any rate, if you ever feel like you are being asked to do too much
churn for no real benefit, please call attention to that fact.  Burn out
is real, and suffering in silence is not beneficial to the project.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature