On Thu, Aug 21, 2014 at 11:53 AM, Daniel P. Berrange <berra...@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 < > berra...@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 > :| >
_______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev