[bind10-dev] code reviews

[JINMEI Tatuya / 神明達哉]

At Wed, 14 Apr 2010 07:48:02 -0500 (CDT),
"Jeremy C. Reed" <jreed at isc.org> wrote:

> The plan is have trunk used for development, but all changes to it 
> reviewed first. If it is very minor (like comment changes or obvious 
> typo fix), use your discretion and commit fix without review but clearly 
> indicate this in your commit message. If the change is minor, then use 
> jabber or email to ask for an okay and mention that in your commit 
> message. Else use the review policy as documented at 
> http://bind10.isc.org/wiki/CodeReviewProcedure
> Be sure to include the ticket number in your svn commit message.

I agree on the policy for "very minor" changes.

For (not very) "minor" changes, I'm not so sure if the proposed
approach works well.  A review request via email or even via jabber
can be missed if others are busy (and jabber has its own problem of
short-lived logs - while the separate permanent archive will help we
can't rely on it because the consensus is that we are not requested to
check the archive not to miss past conversations).

I'm also not sure how we effectively implement this policy:

> - all code reviewed before merged to trunk

The easiest way to get code reviewed is to commit the change to
"somewhere" and ask the reviewer to get diff from that "somewhere".
For a relatively large set of change we probably use a separate branch
for the "somewhere".  That's fine.  But what do we do for a not so
large, but not so minor changes, if we don't use the trunk or branch?
Especially when the changes are made on multiple files?  If the change
is a few lines of diff, we might send a copy of "svn diff" to the
reviewer via email or jabber, but if the change is larger than that
level I doubt that approach is effective.

What I've done recently is to commit the change to the trunk anyway,
and ask for review specifying the corresponding svn changeset.  But
I'm not very happy with this approach, because, as easily expected,
these review requests are not so quickly taken on (much less
completed), and more and more new changes have been committed to the
trunk, waiting fore review.  As a result, the trunk is having more and
more unreviewed changes.  Also, unlike a separate branch, it's not
very easy to specify the svn changeset(s) on trunk for the change to
be reviewed if it consists of multiple commits because the trunk often
contains irrelevant changes as well.

So, I think we need to define a more specific and implementable
procedure, rather than just saying "everything must be reviewed before
merge".

---
JINMEI, Tatuya