[darcs-devel] review policy revisited

[Petr Rockai]

Hi,

[heads-up: the mail is not as coherent as I'd like, but I am pretty
tired and I started drafting yesterday and who knows how long it'll take
before I am happy about it, so sending it now, anyway]

we had another of those review discussions on IRC today (Eric will
surely have a handy link to the logs). Executive summary (so far, Eric
and myself agree in principle, details to be worked out):

- make reviews optional; the rationale is that if review is done purely
  as an obligation, it is unlikely to serve its purpose very well
- patches from core team members come with a one-week timeout: if a
  patch is idle in needs-review for at least a week, it will move to an
  "approved" status; "approved" patches can be pushed without further
  review (a status like accepted-pending-tests or such)

Some rationale:

- "my non-activity should not beat your activity" -- others not doing
  anything should not block an active contributor
- 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
- the review backlog is a witness of the review process not working very
  well: patches are being blocked in review for a long time, leading to
  all sorts of complications... review and code discussion in general is
  best done while the author has the code in fresh memory

The policy would also shift the responsibility from the code author (who
now needs a reviewer to get code in) to the reviewers (who now need to
actively review code to prevent stuff they disagree with from entering
the code-base). I think a week delay is reasonable, but is of course
tweakable...

Hopefully, this will also motivate people to review more: more people
care about general stability than about particular feature or refactor,
so the motivation to get things in is usually rare. On the other hand,
the motivation to keep (bad) things out should be universal: once it
breaks, it's broken. I understand it's much more comfortable to have no
breakage by default, but the same effect is even more comfortably
achieved by freezing the codebase altogether... and I suppose we also
want to make progress -- better served by making changes than by not
making changes.

Yours,
   Petr