Patch Review Process

This page explains the Mercurial patch review process and how (anyone) can help.

1. Generic Fact

• Reviews happen on MailingLists#The_Mercurial-Devel_list and Phabricator

• Contributors follow the ContributingChanges and send their patch(es) to the list

• Reviews are just email replies to the emailed patch, or Phabricator comments

• Everyone is welcome to do review

2. Simple Review Checklist

• The patch should conform to the ContributingChanges bullet list.

• You understand the change
• The change seems correct
• The change seems efficient

2.1. For email patches

If any concerns raised, reply to the email asking questions.

If everything sounds good, reply to the email too. Just state it looks good to you in your reply. To make your pre-review even more useful, don't forget to go to Patchwork and mark the patches as "Pre-Reviewed".

2.2. For Phabricator patches

Comment on the web page. Feel free to use "Accept Revision" action even if you're not a core reviewer. If you want to comment without using a browser, follow steps on Phabricator and use "hg phabupdate".

3. Things we commonly miss

(we should probably move these recommendation in other page and just link to them)

• Good 'topic'. (The part is the one before colon in the first line topic: short desc). It is unvaluable to sort thing out when scanning through commit, especially when building a release changelog. A common mistake is to pick a very general work like "commands".

• Config section. All debatable/temporary/unsure-we-want-this config should go to the [experimental] config section. For other option, try to avoid adding a new section if we can't foresee more than one option in it and another pre-existing section would be a good fit.

• Deprecation warning, if a major internal API get killed, encourage the preservation of the old version for 1 version with a deprecation warning (if it is easy to implement). This makes third party extensions maintainer life easier.

4. Accepters Review Checklist

Some people are blessed to accept patches and push them to a repo where Matt Mackall ultimately pulls from.

Here is another check list for them

• Run check code on all patches
• Run the whole test suites
• Reply to the list saying that you took care of the patch

• you can get the patches files directly from http://hgpatches.durin42.com/patches/<node> Appropriate hg alias would be:

  • [alias]
    getpatch=import --partial --obsolete http://hgpatches.durin42.com/patches/$1

• Make sure you created obsolescence marker between the node in the patch and the one you created, e.g.

  • hg import --partial --obsolete <patches>:

• use the drophack extension if you need to drop a changeset you queued

• Rebase your queue on top of main's @

• Move @ with the changeset you pushed.

• Update Patchwork once you have pushed

5. Patchwork States

6. Review Tooling

• Phabricator

• Patchwork (backlog plot)

• The Patches Bot (sources)

• Various data collection http://review.octopoid.net/ (STALED)

• Matt Mackall Inbox Metrix (nb email, nb patches, oldest email (in day)) and Content.

7. The Committers Group

Current list with push access to the hg-committed repository

• Kevin Bullock
• Augie Fackler
• Matt Mackall
• Yuya Nishihara
• Martin von Zweigbergk
• Gregory Szorc
• Sean Farley

• RyanMcElroy

• Pulkit Goyal

CategoryDeveloper