<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Aug 21, 2014 at 11:53 AM, Daniel P. Berrange <span dir="ltr"><<a href="mailto:berrange@redhat.com" target="_blank">berrange@redhat.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">On Thu, Aug 21, 2014 at 11:34:48AM -0500, Dolph Mathews wrote:<br>
> On Thu, Aug 21, 2014 at 11:21 AM, Daniel P. Berrange <<a href="mailto:berrange@redhat.com">berrange@redhat.com</a>><br>
> wrote:<br>
><br>
> > On Thu, Aug 21, 2014 at 05:05:04PM +0100, Matthew Booth wrote:<br>
> > > "I would prefer that you didn't merge this."<br>
> > ><br>
> > > i.e. The project is better off without it.<br>
> ><br>
> > A bit off topic, but I've never liked this message that gets added<br>
> > as it think it sounds overly negative. It would better written<br>
> > as<br>
> ><br>
> > "This patch needs further work before it can be merged"<br>
> ><br>
><br>
> ++ "This patch needs further work before it can be merged, and as a<br>
> reviewer, I am either too lazy or just unwilling to checkout your patch and<br>
> fix those issues myself." </div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">
<br>
</div>I find the suggestion that reviewers are either "too lazy" or "unwilling"<br>
to fix it themselves rather distasteful to be honest.<br></blockquote><div><br></div><div>I should have followed the above with a gently sprinkling of </sarcasm> and </dogfooding>.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
It is certainly valid for a code reviewer to fix an issue themselves &<br>
re-post the patch, but that is not something to encourage a general day<br>
to day practice for a number of reasons.<br>
<br>
- When there are multiple people reviewing it would quickly become a<br>
mess of conflicts if each & every reviewer took it upon themselves<br>
to rework & repost the patch.<br></blockquote><div><br></div><div>Valid theory, but I think you overestimate the amount of review bandwidth the community actually has for this to be a problem :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
- The original submitter should generally always have the chance to<br>
rebut any feedback from reviewers, since reviewers are not infallible,<br>
nor always aware of the bigger picture or as familiar with the code<br>
being changed.<br></blockquote><div><br></div><div>++ My comment is specifically with regard to nits. Real concerns should always be discussed with the author.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
- When a patch is a small part of a larger series, it can be a very<br>
disruptive if someone else takes it, changes it & resubmits it,<br>
as that invalidates all following patches in a series in gerrit.<br></blockquote><div><br></div><div>++ As a reviewer submitting a followup patchset, you certainly need to take care of the entire series.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
- It does not scale to have reviewers take on much of the burden of<br>
actually writing the fixes, running the tests & resubmitting.<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
- Having the original author deal with the review feedback actually<br>
helps that contributor learn new things, so that they will be able<br>
to do a better job for future patches they contribute<br></blockquote><div><br></div><div>+++ 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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
I'd only recommend fixing & resubmitting someone else's patch if it is<br>
a really trivial thing that needed tweaking before approval for merge,<br>
or if they are known to be away for a prolonged time and the patch was<br>
blocking other important pending work.<br></blockquote><div><br></div><div>This is a great general rule. But with enough code reviews, there will be exceptions!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><div class="h5"><br>
Regards,<br>
Daniel<br>
--<br>
|: <a href="http://berrange.com" target="_blank">http://berrange.com</a> -o- <a href="http://www.flickr.com/photos/dberrange/" target="_blank">http://www.flickr.com/photos/dberrange/</a> :|<br>
|: <a href="http://libvirt.org" target="_blank">http://libvirt.org</a> -o- <a href="http://virt-manager.org" target="_blank">http://virt-manager.org</a> :|<br>
|: <a href="http://autobuild.org" target="_blank">http://autobuild.org</a> -o- <a href="http://search.cpan.org/~danberr/" target="_blank">http://search.cpan.org/~danberr/</a> :|<br>
|: <a href="http://entangle-photo.org" target="_blank">http://entangle-photo.org</a> -o- <a href="http://live.gnome.org/gtk-vnc" target="_blank">http://live.gnome.org/gtk-vnc</a> :|<br>
</div></div></blockquote></div><br></div></div>