Open Stack

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.

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