[darcs-devel] review policy revisited

[Stephen J. Turnbull]

Guillaume Hoffmann writes:

 > One remark:
 > 
 > > - it enables "silent" reviewing -- for me, personally, formulating
 > >  feedback is a significant distraction from actually reading and
 > >  understanding code; I like to be able to approve by silence
 > 
 > following this idea, one-liner reviews should be also possible in
 > order to accelerate this one-week wait? Or shall we only accept
 > detailed reviews?

As an experienced release manager in another project, I really dislike
the "silent review" idea.  In my experience, in certain kinds of
developer, it encourages quantity over quality ("nobody else reviews
my code, why should I wait a day and review my own code? commit now
and get on with the next task!").  Others will take advantage of it to
push controversial or risky changes without enough discussion.
Finally, from a personnel point of view, silence is ambiguous.  What
is Petr doing?  Is he reviewing or not?  Release managers can't know,
and that increases uncertainty about quality of the release.

As a personal matter, I disagree strongly with Petr's evaluation that
feedback is a "distraction from" reading and understanding code.  IMO
in a multideveloper project, feedback is the *main purpose* of reading
and understanding code.  If it's so hard to understand the code, then
probably it is not well documented, etc.  But that's IMHO, YMMV, etc.,
not something that I would say has been proved by experience.

On the other hand, it is *not* necessary that reviewers give detailed
and coherent feedback in every review, or that every reviewer weigh in
on every important change (let alone all the typos fixes!)  It seems
to me that a minimum report should be made on every review, which
consists of a -1, 0, or +1 "vote" on the idea (even for apparent bug
fixes, you may consider that the behavior isn't really a bug, and that
the inconvenience should be addressed in a different way), and a -1,
0, +1 vote on the implementation.  Of course, in the case of a (+1,-1)
review, the submitter will be *very* frustrated if you don't give
detailed feedback.  However, many reviews will be  (-1,X) or (+1,+1),
and in those cases little and no (respectively) feedback is usually
necessary.

I also think that every change should be reviewed for readability,
especially of comments and other documentation.  While some people
think that comments that only the writer can really understand are
useful for followup, in my own experience more than half of such
comments persist for long enough that I forget what they mean unless
they are quite detailed.  And in a large code base employing complex
algorithms, many such comments may persist for a decade or more.
Better that they not be written at all in such cases, I think.  Again,
IMHO, YMMV.

The Bazaar project (which you should note has several fulltime
employees on it, so cost is much higher for Darcs) has an interesting
approach.  They appoint a rotating Patch Pilot (changes every two
weeks), whose job is to help newbies (and sometimes not-so-new-bies)
negotiate their (quite elaborate) review process.  This has been very
successful in (1) keeping the review queue short (for two reasons: the
pilot puts priority on reviewing so gets to it quickly, and the pilot
will often direct a difficult review to the attention of the person
who will eventually have the most influence on the fate of that
patch), and (2) recruiting new active developers.  With four active
core developers, two of whom are managing releases, a full-bore patch
pilot is too expensive for Darcs, I suppose.  But maybe there's an
idea there that somebody can pick up and run with.

HTH

Steve