a guide to reviewing

[Justin Mason <jm...@jmason.org>]

actually, it occurs to me that a good few of the recent committers
probably don't know the details on reviewing patches.
http://wiki.apache.org/spamassassin/DevelopmentMode has the details; I'll
cut and paste:


  Review-Then-Commit

  R-T-C mode is short for "[WWW] Review-then-Commit".

  Non-trivial patches (see below for a definition of what's considered a
  'trivial' change) must be reviewed by committers, and need consensus
  approval before being committed into the development tree. This is done
  by opening a Bugzilla ticket, setting the Target Milestone to the
  correct release version of the tree, attaching the suggested patch to
  the ticket via the web interface, and putting the ticket in "review"
  status (indicated by adding [review] as a prefix to the ticket summary).

  The patch is then [WWW] voted upon, and if gets a [WWW] consensus
  approval and is not [WWW] vetoed, can be applied to the tree. Votes
  should generally be permitted to run for at least 24 hours to provide an
  opportunity for all concerned persons to participate regardless of their
  geographic locations. "Consensus approval" refers to a vote which has
  completed with at least three binding +1 votes and no -1 vetos.

  The author of a patch is allowed to vote as long as they're a committer.
  Typically, if a committer uploaded the patch, it's assumed they're
  implicitly voting +1 on their own patch, unless otherwise specified.

  Trivial Patches: When R-T-C is Optional

  Trivial patches include:

      * documentation
      * finishing off pre-existing T_ tests
      * changes to sandbox rules
      * non-controversial non-semantic style changes (fixing indentation, adding comments, but not actual code)
      * very simple, non-controversial, and absolutely safe bug fixes (i.e.: removing repetitive my() enclosing sections)

  These can be applied without a vote.

  Time Delays for Code Modifications

  Votes should generally be permitted to run for at least 72 hours to
  provide an opportunity for all concerned persons to participate
  regardless of their geographic locations.

  (Since it does say "generally", it seems reasonable exceptions to the 72
  hour rule are allowed if you specify such in your vote, but let's always
  allow at least 24 hours or at least 48 hours if the weekend is involved.
  Remember, though, that if someone later vetos with a technical
  explanation, then the code gets pulled.)

  How To Review A Patch

  Please don't vote +1 unless you actually did something to check the
  patch. That means some form of testing or code review. You do not
  necessarily need to apply the patch to your local copy of SA, but do
  take a look at it before voting +1.

  It's not your responsibility as the reviewer to run 'make test'. It is
  assumed that a review patch already passes this, so don't worry about
  it.

  If you don't understand the workings of the code you're voting on, don't
  worry about it too much. Do your best, and if all else fails, vote on
  the structural aspects of the patch, and its code quality. It's not
  expected that every committer knows how all of SpamAssassin works -- but
  it is important that R-T-C has enough people voting!

  Running 'make test'

  'make test' should be run before committing anything, unless the change
  doesn't modify the code in any way (such as a documentation change). If
  you check in something that breaks 'make test', you have Done A Bad
  Thing.

  If you send out a patch for C-T-R, and your patch manages to break 'make
  test' on its own (ie. not through interaction with other current review
  patches), that is Also Bad. However, it's not the fault of the reviewers
  -- it's yours ;)


Hope that helps,

--j.

Re: a guide to reviewing

[Sidney Markowitz <si...@sidney.com>]

Michael Parker wrote, On 5/7/07 8:53 AM:
> but I did want to highlight the 24 hr clause. I'm seeing patches
> enter bugzilla, get voted on and get committed in < 24 hrs

Oops, even worse than my faux pas at committing other people's fixes, I
see that I included bug 5545 when I was blasting through the queue of
3.2.2 bugs that had been finally reviewed after sitting for a long time.
Mea culpa, mea culpa, mea culpa. No excuse for that one.

 -- sidney

Re: a guide to reviewing

[Sidney Markowitz <si...@sidney.com>]

Daryl C. W. O'Shea wrote, On 5/7/07 8:59 AM:
> Michael Parker wrote:
>> Also, it is polite to allow patch creators, who are also committers, to
[...]
> Yeah, I find this quite annoying when others immediately commit other 
> people's patches

I apologize -- I would not have done that if I had thought anyone would
be annoyed by it. It must be a cultural difference. I won't do that again.

 -- sidney

Re: a guide to reviewing

["Daryl C. W. O'Shea" <sp...@dostech.ca>]

Michael Parker wrote:

> Also, it is polite to allow patch creators, who are also committers, to
> apply and commit their own patches unless a) they pass that off to
> someone else or b) enough time has passed with sufficient votes for the
> patch to be committed.

Yeah, I find this quite annoying when others immediately commit other 
people's patches.  Just now I spent a good chunk of time looking for a 
change I made 6 months ago to finally find that someone else committed 
it as soon as they added their (the third) +1 vote.

Daryl

Re: a guide to reviewing

[Michael Parker <pa...@pobox.com>]

Justin Mason wrote:
> 
>   The patch is then [WWW] voted upon, and if gets a [WWW] consensus
>   approval and is not [WWW] vetoed, can be applied to the tree. Votes
>   should generally be permitted to run for at least 24 hours to provide an
>   opportunity for all concerned persons to participate regardless of their
>   geographic locations. "Consensus approval" refers to a vote which has
>   completed with at least three binding +1 votes and no -1 vetos.
> 

Not that I have an issue with any recent commits, and with me being less
than active perhaps I should just shut up, but I did want to highlight
the 24 hr clause.  I'm seeing patches enter bugzilla, get voted on and
get committed in < 24 hrs.  While there are good reasons to bypass the
24 hr pause, it is there for a reason.

Also, it is polite to allow patch creators, who are also committers, to
apply and commit their own patches unless a) they pass that off to
someone else or b) enough time has passed with sufficient votes for the
patch to be committed.

Michael