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 firstname.lastname@example.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel