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

[Laszlo Ersek]

On 12/11/15 17:17, Eric Blake wrote:
> 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).

I'm very impressed that you are conscious of this. I also seek to remind
myself of it (in the reviewer role), frequently. Thank you.

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

I used to complain very loudly ("suffering in silence" is not what I'm
naturally inclined to do :)), but recently I've had zero problems on
qemu-devel, luckily. My only reason to go meta here was Samuel's emails,
which looked all too familiar to my own earlier ones.

(This is not to say that I take issue with whatever Thomas or other
reviewers may have said -- I have no clue what they said; I just noticed
the "pattern" in Samuel's emails.)

Thanks!
Laszlo