Open Stack

On 08/21/2014 12:53 PM, Daniel P. Berrange wrote:
> On Thu, Aug 21, 2014 at 11:34:48AM -0500, Dolph Mathews wrote:
>> On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange <berrange at redhat.com>
>> wrote:
>>
>>> On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:
>>>> "I would prefer that you didn't merge this."
>>>>
>>>> i.e. The project is better off without it.
>>> A bit off topic, but I've never liked this message that gets added
>>> as it think it sounds overly negative. It would better written
>>> as
>>>
>>>    "This patch needs further work before it can be merged"
>>>
>> ++ "This patch needs further work before it can be merged, and as a
>> reviewer, I am either too lazy or just unwilling to checkout your patch and
>> fix those issues myself."
> I find the suggestion that reviewers are either "too lazy" or "unwilling"
> to fix it themselves rather distasteful to be honest.
That was from the Keystone PTL.  I think he was going for "vaguely self 
deprecating"  as opposed to dissing the reviewers.


>
> It is certainly valid for a code reviewer to fix an issue themselves &
> re-post the patch, but that is not something to encourage a general day
> to day practice for a number of reasons.
>
>   - When there are multiple people reviewing it would quickly become a
>     mess of conflicts if each & every reviewer took it upon themselves
>     to rework & repost the patch.
>
>   - The original submitter should generally always have the chance to
>     rebut any feedback from reviewers, since reviewers are not infallible,
>     nor always aware of the bigger picture or as familiar with the code
>     being changed.
>
>   - When a patch is a small part of a larger series, it can be a very
>     disruptive if someone else takes it, changes it & resubmits it,
>     as that invalidates all following patches in a series in gerrit.
>
>   - It does not scale to have reviewers take on much of the burden of
>     actually writing the fixes, running the tests & resubmitting.
>
>   - Having the original author deal with the review feedback actually
>     helps that contributor learn new things, so that they will be able
>     to do a better job for future patches they contribute
>
> I'd only recommend fixing & resubmitting someone else's patch if it is
> a really trivial thing that needed tweaking before approval for merge,
> or if they are known to be away for a prolonged time and the patch was
> blocking other important pending work.
>
> Regards,
> Daniel