Development/PatchReview

Basic Process

The Core Team is responsible for reviewing patches incoming on the mailing list. The basic rules are:

  • all patches go to the list first
  • all patches have to be reviewed
  • you never push your own patches to darcs.net

Exceptions to this rule

  • obviously trivial patches (eg. typos in the comment) (if in doubt, it’s not obvious enough)
  • patches that do not require formal review [whitelist below]

    • contrib
    • homepage
    • distribution/packaging
    • build system
    • test cases
  • the “hello, anybody? rule” : if a core team patch has not received review in a week, the submitter can push it directly to reviewed. Common-sense dictates that you avoid doing this if you think the patch really needs review.

Workflow

  1. Somebody sends a patch to patches@darcs.net The patch will have status needs-screening.

  2. If the submitter is on the core team, he can optionally consider his patches to be self-screened. Patches you GPG sign will be automatically applied to the screened repository. If you want to say “please screen this”, just don’t sign the patch.

    Patches from outside the core team should be screened by a core team member. Screening simply consists of checking that the patch is sane in principle, with no attempt at thorough review. When you have screened a patch, please push it to http://darcs.net/screened

  3. When a patch bundle has been screened, the patch tracker status should be set to needs-review.

  4. Somebody should review the patch and push it to http://darcs.net/reviewed

General info

  1. To review a bundle, for now

    darcs get http://darcs.net/screened --context foo.dpatch
    darcs apply

    In the near future, Darcs will have some extra features to make it easier to avoid accidentally getting screened patches you didn’t review.

  2. If you would like modifications to the bundle, say so and also set the status to “followup-requested” (formerly known as amend-requested). This is a slight cultural shift; we no longer request patch amendments so much as follow on patches.

    Note: please pay attention to who submitted the patch. The ideal is to gradually raise the bar of expectations. That means that first time submitters should have as easy a time as possible: can you the core team member just make the follow-up changes yourself and tell the submitter about them? Then just do it. But as people start submitting more patches, you can start to gradually raise your expectations about their patch standards.

  3. If you are happy with the patch bundle, you can push it to the reviewed branch at http://darcs.net/reviewed

Submitting patches

Sending an amended version of an already submitted patch

darcs send –subject ‘[patchNNN]’

Reviewing patches

Try to make it easier for the maintainer to understand both the patch and your review.

  • cut out superfluous context
  • make each patch visually distinct

Finding out which bundle to review exactly (most likely the latest):

  • scroll down to a message that talks about the bundle you want
  • select “msgXXXX (view message)” (example)
  • see the list of attachments for that individual message

Reduce friction!

The following tips are not be interpreted as policy but tricks for cutting down on unnecessary delays:

  • discuss patches on IRC so you get immediate feedback, but be sure to summarise and link the conversation afterwards
  • if there are patches to amend, push the largest safe/acceptable subset of the bundle you can
  • do not let trivial amendments block a patch; instead, follow up by pushing corrections on your own

Pushing Patches

All people on the Core Team posses a “commit bit”: they have the permission to push patches to darcs.net. When a reviewer is satisfied with a patch and reasonably confident it is good to go, they can push it to the central repository… The best way is to maintain a 1:1 mirror of darcs.net on your local filesystem. To push a patch, apply or pull it into your local mirror, and then darcs push darcs-unstable@darcs.net:screened there. The “darcs” repository is an alias for the “screened” repository. One can similarly push to ...:reviewed, etc.

Please also consult BuildBot after pushing changes, and possibly take steps necessary to get it back to green if anything breaks (but don’t panic, anyway).

For help setting up your ssh keys, try this guide, which explains how to manage multiple key identities per host.

Helping out review

If you are not (yet…) a member of the review team you can still help doing patch reviews. First, you need to find patches to review.

  • For this, go to the bugtracker, get an account and login. Then, look in the open patches section for unassigned patches which need review.

  • Pick your favorite. First, assign it to yourself and set the status to review-in-progress. Then do the review proper: test and read the patches; reply to the patch as you would reply to a usenet message, interleaving your comments with the body of the patch. Then you need to send the review back to the list and patch-tracker. Either reply via the patch-tracker’s web interface, or reply to the mailing list message. Your mail’s to: should be bugs@darcs.net, and the list will automatically get a copy of the message.

    • If you think the patch should be applied, set the status to accepted-pending-tests (if you reply by mail, add [status=accepted-pending-tests] to the subject-line), and notify in the body that you have not applied yet. Please point out that you need a committer to push this patch.
    • If you think the patch is inappropriate for darcs in a definitive way, set the status to rejected.
    • If some thing should be changed before application, set the status to amend-requested.

Review tools

Darcs-roundup-watch See where your repositories differ with the state of the darcs patch tracker

Darcs-team scripts: Mutt and similar e-mail clients can be used in conjunction with the following scripts: darcs get http://code.haskell.org/darcs/darcs-team

For instance, I have vim programmed to call it so that when I reply to a patch in mutt, it gets a bit more nicely formatted:

map ,dr        :%! sed -f ~/custom/darcs-templates/patch-review.sed<CR>

A tip from Eric, when patches involve a lot of hunk-moving, it is helpful to use:

darcs diff --diff-command="mv %1 %2 /tmp" --last=1
darcs changes --summary --last=1
cd /tmp
meld old-patch484/src/foo.hs new-patch484/src/bar.hs