Open Stack

On 29/05/18 16:49, Slawomir Kaplonski wrote:
> Hi,
> 
>> Wiadomość napisana przez Jay S Bryant <jungleboyj at gmail.com> w dniu 29.05.2018, o godz. 22:25:
>> Maybe it would be different now that I am a Core/PTL but in the past I had been warned to be careful as it could be misinterpreted if I was changing other people's patches or that it could look like I was trying to pad my numbers. (I am a nit-picker though I do my best not to be.
> 
> Exactly. I remember when I was doing my first patch (or one of first patches) and someone pushed new PS with some very small nits fixed. I was a bit confused because of that and I was thinking why he did it instead of me?
> Now it’s of course much more clear for me but for someone who is new contributor I think that this might be confusing. Maybe such person should at least remember to explain in comment why he pushed new PS and that’s not „stealing” work of original author :)

Another issue is that if the original author needs to rev the patch 
again for any reason, they then need to figure out how to check out the 
modified patch. This requires a fairly sophisticated knowledge of both 
git and gerrit, which isn't a problem for those of us who have been 
using them for years but is potentially a nightmarish introduction for a 
relatively new contributor. Sometimes it's the right choice though 
(especially if the patch owner hasn't been seen for a while).

A follow-up patch is a good alternative, unless of course it conflicts 
with another patch in the series.

+1 with a comment can also get you a long way - it indicates that you've 
reviewed the whole patch and have found only nits to quibble with. If 
you're a core reviewer, another core could potentially +2/+A on a 
subsequent patch set with the nits addressed if they felt it 
appropriate, and even if they don't you'll have an easy re-review when 
you follow up.

We have lots of tools in our toolbox that are less blunt than -1. Let's 
save -1 for when major work is required and/or the patch as written 
would actually break something.


Since I am replying to this thread, Julia also mentioned the situation 
where two core reviewers are asking for opposite changes to a patch. It 
is never ever ever the contributor's responsibility to resolve a dispute 
between two core reviewers! If you see a core reviewer's advice on a 
patch and you want to give the opposite advice, by all means take it up 
immediately - with *the other core reviewer*. NOT the submitter. 
Preferably on IRC and not in the review. You work together every day, 
you can figure it out! A random contributor has no chance of parachuting 
into the middle of that dynamic and walking out unscathed, and they 
should never be asked to.

cheers,
Zane.