[bind10-dev] code reviews

[JINMEI Tatuya / 神明達哉]

At Thu, 15 Apr 2010 06:49:58 -0500,
Michael Graff <mgraff at isc.org> wrote:

> +1. :)

> >> So, I think we need to define a more specific and implementable
> >> procedure, rather than just saying "everything must be reviewed  
> >> before
> >> merge".
> >
> > let me go AOL here and simply say
> >
> > +1

Okay, then let me make a strawman proposal.  I experimentally tried
some of the ideas in my recent change:
- I adopted bullet 2 for a "very minor" change (r1722)
- I used the proposed approach #3 for a "minor" change for ticket
  #143.
The key point of the proposal is that we use branches for patches and
reviews more often.  Clearly, by this we can ensure "no (more)
unreviewed code in trunk".  The other point is to have a set of rules
on how to use branches to avoid make them unmanageable kitchen sink.

As I write down it, it became longer than I originally expected.  But
this is just a "strawman" idea.  I'm not necessarily pushing this and
would like to hear others' opinions.

1. if the change 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.

2. if the change is beyond the "very minor" level but is still minor
   and trivial enough that it wouldn't require explicit review/revise
   cycle, use your discretion and commit it to trunk.  but make sure
   it compiles and passes all 'make check' tests, and clearly indicate
   that it's directly committed to trunk as a minor change (and that
   it compiles and passes tests).  in general, we should be careful
   not to choose this option as an easy way out.

   basically, the change should be obvious from the commit
   notification email.  other developers should (at their best
   efforts) perform a quick check on it, and if anyone feels it beyond
   this level, simply say so.  then the change author needs to revert the
   change from the trunk and go to the normal review mode (#3 or #4).

3. if the change is relatively minor but it's better to be reviewed
   (many bug fix patches would belong to this category), the developer
   can still work on the patch on trunk, but shouldn't commit the
   change to trunk.  when the patch is ready for review, the developer
   opens a trac ticket for the change (if not yet opened) and creates
   a short-lived branch based on the ticket number
   (e.g. "branches/trac12345") and commit the change to that branch.
   Then the developer asks for review.

   The review process itself isn't so different from the "standard"
   review process, but the change should be "minor by definition",
   the review cycle is expected to be short and the patch branch is
   expected to be short-lived.  once the patch is approved it's
   committed to the trunk with noting in the commit message 
   the ticket number (that indicates the review).

   Alternatively, the developer may choose to just take a diff between
   the trunk and the working directory, and simply show the diff to
   the reviewer (e.g. by copying to the trac ticket).  It's up to the
   developer.  But, assuming we choose the right version control tool
   and use it correctly, making a branch should be quite lightweight
   (see some notes in the case of subversion below), so using a branch
   is generally preferable.  in fact, the developer may miss something
   trivial and the review may need a couple of revise-review cycles.
   in that case it's more convenient to all the revise work in a
   dedicated branch.

   We'll try to limit the number of open branches to a reasonably
   manageable model (say, up to 10 or so?).  one possible approach to
   loosely ensure this is to have a quick review of the branch status
   in our weekly call and if the number of open branches reaches (or
   worse, exceeds) the threshold we prioritize the review over other
   ongoing development tasks. (this would be another reason that
   explicit branches are preferable to bare diffs - we can easily see
   how well or poorly reviews are taken care of).

4. more substantial changes, e.g. a complicated bug fix or a new major
   feature, should generally be developed in a separate "work place".
   maybe we use the "experiments" branches in our svn repository or
   introduce a separate category such as "projects".  The key is we
   don't use "branches" for this purpose because we want to use them
   for short-lived patches mainly for review cycles.

   If the patch is ready for review in that separate work place, the
   developer opens a trac ticket for the change (if not yet opened)
   and moves it to "branches" (and maybe rename it to "tracNNNN"?),
   and ask review for it.

   The review process itself is basically the same, but since the
   change is expected to be non trivial, review may take more time and
   addressing review feedback may require more substantial work.  in
   that case, the developer once moves it back from the branches to
   the development work place, complete the major work again, and
   restart the review process.

Subversion specific notes:

The above proposal doesn't necessarily assume we use (or keep using)
subversion for version control (although some part of it use our
current repository layout as examples), but in case we keep using
subversion, I guess it would be useful to note about how to exploit
some specific features of subversion to implement the above process.

What made me write this part is Jeremy's comment on jabber, when I
experimentally tried this process for ticket #143:

"... checkout new branch, read logs, read ticket, realize it is
revision 1721. And the patch is only for a single line."

He seemed to checkout the whole new branch (and perhaps build and test
it).  As indicated in the comment it wouldn't make sense to do that
for a minor patch.  But it's not the expected work process.

The developer (that's me in this case) would do this:

- This is a minor fix, so I directly writes the patch on trunk,
  compiles and tests it.  since the change should be "relatively
  minor", it would normally affect up to several files in the same
  single module (directory), plus perhaps some files under the "tests"
  subdirectory for additional test cases.
- Now the patch is ready for review.  I create a new branch for it:
  % svn cp svn+ssh://bind10.isc.org/svn/bind10/trunk svn+ssh://bind10.isc.org/svn/bind10/branches/trac143
- Then I switch the current working directory (which is
  src/lib/auth/tests in this case.  all changes for this fix are in
  this directory) from the trunk to the new branch
  % svn switch svn+ssh://bind10.isc.org/svn/bind10/branches/trac143/src/lib/auth/tests
- and commit the change.  the change goes to the branch.
  % svn commit
- then I move back to trunk.  The temporary change will disappear
  automatically.
  % svn switch svn+ssh://bind10.isc.org/svn/bind10/trunk/src/lib/auth/tests

The reviewer would do this:

- update the trunk to the latest version
  % cd <somewhere>/trunk; svn update
- go down to the directory where the changes are to be applied (it
  should be noted in the ticket)
  % cd src/lib/auth/tests
- switch to the branch to be reviewed:
  % svn switch svn+ssh://bind10.isc.org/svn/bind10/branches/trac143/src/lib/auth/tests
  the changes will be incorporated automatically.
- build and test
  % make
  % make check
- to get the diff in that branch, there are two options:
  (a) since the diff should be minor and small, it's basically
  reasonable to just take diff between the trunk and the branch:
  % svn diff svn+ssh://bind10.isc.org/svn/bind10/trunk \
             svn+ssh://bind10.isc.org/svn/bind10/branches/trac143
  (b) identify the first revision of the branch and take a diff from
      that revision and the latest revision of the branch
  % svn log --stop-on-copy svn+ssh://bind10.isc.org/svn/bind10/branches/trac143
  [...]
  r1720 | jinmei | 2010-04-15 14:47:24 -0700 (Thu, 15 Apr 2010) | 2 lines
  (this means r1720 is the revision of branch point)
  % svn diff -r 1720 svn+ssh://bind10.isc.org/svn/bind10/branches/trac143
- when review is completed, either approving or disapproving the
  change, the reviewer updates the trac ticket with the feedback, and
  moves back to trunk:
  % svn switch svn+ssh://bind10.isc.org/svn/bind10/trunk/src/lib/auth/tests

Of course, the reviewer can checkout the entire branch for the purpose
of review, but the above procedure would be more handy because it's
faster for both checkout (update) and build/test: faster checkout
because it only updates a minimal set of files; faster build/test
because the reviewer would normally build/test on trunk regularly and
can reuse that environment for review.

---
JINMEI, Tatuya