Code Review with Rietveld and Mercurial Queues

UPD 2014-07: On Windows, thanks to bug in subprocess that doesn't escape ^ character (caret) you need to escape it manually in all commands, like this ^^.

Teal Deer

    $ hg qser -v
    0 A supported-vcs
    1 U noul

    $ python upload.py --rev "'supported-vcs'^1:'supported-vcs'"

Also FAQ.

Rietveld, pre-commit and post-commit reviews

You probably know what Rietveld is - it allows you to send uncommitted changes for review with upload.py script that you grab from review server:

    $ python upload.py
    Upload server: codereview.appspot.com (change with -s/--server)
    New issue subject: Test
    Issue created. URL: http://codereview.appspot.com/10864043
    Uploading base file for README

This is a pre-commit review, where you discuss changes before they are committed, and add a link to passed review into commit message. In post-commit reviews you usually comment on existing revisions in external project history browser service, such as Google Code or GitHub. I you need a thorough examination of every change, then post-commit review process can become a challenge of catching a running train. If you just need to skim over the committed code and express doubts if anything is unclear for further discussion, the post-commit is ok. In fact we use this way for Spyder IDE development.

Post review is nice in the sense that development is not stopped while you're offline for a vacation, or your braincells are overwhelmed with more time pressing matters. Pre review is good if you want to ensure maximum stability for your system. Ideal development process should have convenient means to support both simultaneously.

--rev hack

But back to the Rietveld. Little is know that it supports reviewing existing changesets and revision ranges with the --rev argument of upload.py For example:

    $ python upload.py --rev "2869^1:2869"
    Upload server: codereview.appspot.com (change with -s/--server)
    New issue subject: Spyder IDE changeset 2869
    Issue created. URL: http://codereview.appspot.com/10866043
    Uploading base file for spyderlib/plugins/configdialog.py
    Uploading base file for spyderlib/utils/external/lockfile.py

This will upload revision 2869 (5a3d6821eabe) from Spyder repository for review. Why --rev is a hack? As you may see there is no original commit message, no revision number, no parent revision info. Actually, Rietveld doesn't know anything about that. What is gets from upload.py is a patchset as a plain SVN diff. Data loss could be prevented with extensible changeset format, which will greatly improve tool interoperability, but I don't know any entity that could support the time required for that development.

The --rev is a hack not only because of information loss, but also because it serves double purpose. The purpose is either:

1. --rev REV specify the base revision to compare current change to
2. --rev REV1:REV2 specify revision range to create the diff

To implement it the proper way, the upload.py needs -c CSET, --changeset CSET argument that will extract revision diff from the current version control history, propose to supply commit message as an issue description, save parent revision and changeset hash info and maybe cache the issue number for that changeset locally.

Uploading stuff from Mercurial Queue

Mercurial Queue is just a bunch of patch files in .hg/patches/ directory that can be applied or reverted in order specified by series file. This may change in future, but everybody knows and uses this fact. There is also a less obvious fact (that took long time for me to discover) that every applied patch is also a full fledged Mercurial revision. While it can be applied/reverted with qpush/qpop, as long as it applied, all other operations are working on it as well. In addition, every such revision gets a tag that is essentially this patch name.

So, to upload a patch from Mercurial Queue, make sure it is applied:

    $ hg qser -v
    0 A supported-vcs
    1 U noul

And use its name as --rev range argument:

    $ python upload.py --rev "'supported-vcs'^1:'supported-vcs'"

Make sure it is escaped as shown, because otherwise Mercurial will read is as a subtraction of tag named vcs from tag supported.

With Mercurial Queues it is also much-much easier to send patch updates. For that make sure your patch is active:

    $ hg qseries -v
    0 U supported-vcs
    1 U noul

It is not, so apply it:

    $ hg qpush 
    applying supported-vcs
    now at: supported-vcs

Do your modifications according to review comments and refresh the patch:

    $ hg qrefresh

Remember Rietveld issue number and use upload.py to update existing issue:

    $ python upload.py -i 10822044 --rev "'supported-vcs'^1:'supported-vcs'"

Stuff ToDo and Bitcoin Extortion

As I said Mercurial Queue integration can be improved. It is the 9th most requested feature in Rietveld at the moment and at least for me there is a clear reason why. The workflow for upload.py should look like the following:

(no arguments specified)

• check that current VCS is Mercurial
• check that hg diff output is empty
• check that there is MQ patch applied
• check if there is Rietveld issue number for the patch
  (needs local storage for this number)
• check if the patch is not already uploaded
  (actual if issue number is found)
• propose to upload a patch
• propose to use commit message for patch description
  (propose to open editor to edit description)
• submit

This is just an entrypoint for further enhancement to reach the ideal workflow, which will require more fixes not only in upload.py, but also on server side.

I am not sure I will be able to code this given the fact that I am to be sold into full time job slavery for my debts. That's why I leave this research here for further development. With that decomposition it should be easy to implement it and send a patch in exchange for some credits.

For those who'd appreciate the feature, but whose time/money ratio is leaning towards 0, there is a way to show the interest through a timeless fund (tracked here) with a total goal limit of 4 BTC for improved MQ support in Rietveld. I could name a few reasons why you should do this including the chance to trade money back for some life time in future, not only for yourself, but for the whole lot of open source projects that use Mercurial with Rietveld (such as core Python development). While all these are argument good, this is not a sponsored development, as I will likely to do this stuff sooner or later regardless of the goals met. I just need some Bitcoins to experiment with, and because I don't have anything else to trade, here is a good opportunity to give something back in exchange.