On Tue, Oct 02, 2012 at 07:39:32PM -0400, Ayal Baron wrote:

Ayal, thanks for your thorough treatment of this subject :)  I completely agree
with the framework that you have laid out here.  Hopefully, we can all come to
an agreement on a quasi-official project-wide policy based on this and then
place it up on the wiki in a gerrit workflow best practices document.

> First of all, in gerrit there is no immediately visible difference between 
> '0' and no review at all so someone might have serious issues with a patch 
> but if she did not mark it with -1 submitter might totally miss this fact.
> esp. if someone sent a new revision and the title of the cover comment for 
> previous version doesn't state a -1 (so maintainer doesn't know he needs to 
> go looking back to verify things were fixed).
> This is adding overhead on maintainer now to go back to each and every review 
> and make sure that there are no comments that should have been addressed in 0.
> Note that if someone gave a -1, normally I'd expect that person to make sure 
> and +1 a subsequent patch to flag to maintainer that all their problems with 
> the patch have been addressed.
> My take on this is:
> -2 - The approach taken to solve the problem is wrong and the whole thing 
> should either be abandoned or rewritten in a new way.  I can only accept this 
> though if the reviewer also suggests the alternative (i.e. just saying your 
> code isn't good is bad form imo).  e.g. stating things like 'circular 
> references is bad' and giving -2 but not suggesting alternatives and 
> explanations is bad form imo.
> -1 - I think there are some issues with the current patch that should be 
> addressed *prior* to merging it (bugs in the code that would affect many 
> people etc).  This would also include complex code which needs explaining (if 
> it's too complex for me to immediately understand then it's fine to delay 
> merge until either a good answer why this is mandatory is received and what 
> the code does or simplification of the code submitted or at worst case - 
> comment in the code.
>  -1 should only be given with proper explanation, otherwise imo it's bad form.
> 0 - I have some *personal* style problems, questions which do not affect the 
> validity of the patch *or* I think there are some changes that should be made 
> but can definitely be done in a future patch and should not prevent merging 
> the current version.  Note that I find this very important to actually 
> improve our current way of working.  This means that if a patch improves 
> current code but could in itself be further improved, it is valid imo to 
> accept current version and ask committer to submit another patch to further 
> improve it.
> Note that this would include things like (e.g.) discussions about spacing 
> which are not enforced by pep8 tools (i.e. preventing a patch which fixes 
> bugs from going in because of personal interpretation of pep8 about alignment 
> of parameters in function signature is wrong imo).
> +1 - I have reviewed the code and it looks correct to me but I'm not a 
> subject matter expert / the maintainer.
>      Note that for things like style review only '+1' should be accompanied 
> by a cover commit message stating - "+1 only for style" as Doron has 
> mentioned previously on this thred.
> +2 - I am a subject matter expert, I have reviewed the code and it looks good 
> to me (solves the problem properly and no serious issues left with it).
> As Doron mentioned, in our group (storage) the standard is to have at least 2 
> reviews (by different people) before committing unless the patch is *really* 
> trivial.
> This means that I try to avoid giving +2 if no else has given a +1 before.
> Alon, note that we apply this both to vdsm and engine.

Adam Litke <a...@us.ibm.com>
IBM Linux Technology Center

vdsm-devel mailing list

Reply via email to