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 <agl(a)us.ibm.com
IBM Linux Technology Center