Open Stack

On Thu, Aug 21, 2014 at 11:53 AM, Daniel P. Berrange <berrange at redhat.com>
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.
>

I should have followed the above with a gently sprinkling of </sarcasm> and
</dogfooding>.


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

Valid theory, but I think you overestimate the amount of review bandwidth
the community actually has for this to be a problem :)


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

++ My comment is specifically with regard to nits. Real concerns should
always be discussed with the author.


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

++ As a reviewer submitting a followup patchset, you certainly need to take
care of the entire series.


>
>  - It does not scale to have reviewers take on much of the burden of
>    actually writing the fixes, running the tests & resubmitting.
>

Depends on what you're trying to scale for, I suppose. I'd like to see
valuable patches iterate and land more quickly, at the expense of lower
priority patches and my time as a reviewer.


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

+++ Even when a reviewer takes it upon themselves to contribute a patch,
I'd expect a normal -1 review explaining why things need to change, and
some sort of collaboration with the original author to make sure they're on
the same page.


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

This is a great general rule. But with enough code reviews, there will be
exceptions!


>
> Regards,
> Daniel
> --
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/
> :|
> |: http://libvirt.org              -o-             http://virt-manager.org
> :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/
> :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc
> :|
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140821/39f68509/attachment.html>