I've been waiting for patch review of my work on http://bugs.python.org/issue13703 only to discover that people *have* been reviewing it.

It turns out that next to some of the patches in the issue tracker there's a "review" link, which takes me to http://bugs.python.org/review/13703/show and there's a whole second conversation going on about the issue there.  I haven't been getting any emails about it.

I've been looking through http://docs.python.org/devguide but I can't see any mention of the mechanics of patch review, or that this secondary site exists.  
Clearly I missed something big, but was it actually documented anywhere?

http://docs.python.org/devguide/patch.html mentions uploading patches for review, and talks about an iterative process of commenting and refining a patch, but when I read it, I assumed it was referring to discussion within the issue, rather than on this secondary site.

Some questions:
  * Do all patches go into this review site, or do I have to do something extra to get them to land there?
  * I have patches for both 2.6 and 3.1 - are they kept separate, or do they affect each other's "delta from patch set"?
  * Is there a way to enable notifications, e.g. for me to receive emails when someone comments on my patch?  or to put a comment into the *issue* noting that someone commented on the patch?

Thanks

At one time, reviews (or the fact of a review) were posted back here automatically (or perhaps it is an option?) just like push messages. But I am not much familiar with the process either.

AFAIK, all interested parties are supposed to automatically be sent email
notifications whenever anyone posts an update on the review. However,
I've run into a bug <http://psf.upfronthosting.co.za/roundup/meta/issue402>
that seems to be preventing this from working for me.

> * Do all patches go into this review site, or do I have to do something extra to get them to land there?

It should happen automatically, but only if the patch applies cleanly to
the head of the default branch.

> * I have patches for both 2.6 and 3.1 - are they kept separate, or do they affect each other's "delta from patch set"?

If the patch applies cleanly to default (unlikely), I imagine things
could get messy. Most of the time, though, they'll be ignored altogether.

Some notes from discussion with MvL at PyCon sprint:

The ideal is that:
  - for any patch attached to an issue: the patch is uploaded to a Rietveld instance colocated in the same db as Roundup (bugs.python.org)
    - if it works, than a "review" link is visible next to the patch, taking you to a patch review UI: unfortunately, it takes you to the most recent patch on the issue, rather than the patch you click on
    - unfortunately there are a few ways for the patch upload to fail, and if this happens, then currently there is no visible indication within Roundup.  If this happens, then patch review will need to be done within Roundup itself (i.e. the comment form or via email)
  - how can it fail: the importer needs to determine what the baseline to which the patch is to be applied.  It can figure this out for a "hg diff" (assuming that the baseline revision is known to hg.python.org/cpython), but not for git-style diffs (unless you do it on the current head of "default")
  - in theory the nosy list of a bug within Roundup is synchronized with that of the patch within Rietveld.  Unfortunately there is currently at least one bug with this (e.g. dmalcolm isnt on the nosy of http://bugs.python.org/review/13703/show despite being on the nosy of http://bugs.python.org/issue13703): FIXME: do we have a metabug about this?
See:
  http://psf.upfronthosting.co.za/roundup/meta/issue394
  http://psf.upfronthosting.co.za/roundup/meta/issue439

Some code links:
  http://svn.python.org/projects/tracker/instances/python-dev/rietveld/
  http://svn.python.org/projects/tracker/instances/python-dev/extensions/create_patch.py

It would appear that having:
  [diff]
  git = on
in ~/.hgrc breaks the rietveld integration, since "hg diff" then emits a diff that doesn't identify the hg revision number, and hence the importer can't determine the baseline.

It should be noted that diff in git supported format is recommended in the devguide -  http://docs.python.org/devguide/committing.html#minimal-configuration 

If it breaks the rietveld integration, then we should not be advising it.

This is highly debatable.  All Mercurial guides that I’ve seen recommend setting git=on because of its many advantages.  On the other side there is only one inconvenient.  I’m strongly opposed to removing that info from the devguide because one tool can’t cope with it.

See also http://psf.upfronthosting.co.za/roundup/meta/issue415

Yeah, the other option is to fix the tracker-reitveld integration.
(Which I assumed was harder because diff did not give hg info required
for it)

Most of the patches are against 3.2 or 2.7, so applying them to "default" might fail.
If we specify the correct branch, rietveld should be able to apply them cleanly to the head of the branch even without knowing the exact revision (so even with git-style diff).  This might still fail if the files affected by the patch changed in the meanwhile, but that shouldn't happen very often.
If this is true, we could simply add a way to specify the branch (either a dropdown in the roundup UI or an X.Y in the filename).  This might also be used to avoid conflicts when the same patch is submitted for different branches.

See also #11869.

> If this is true, we could simply add a way to specify the branch (either a dropdown in the roundup UI or an X.Y in the filename).

It would be cool if this could happen even without any user action (e.g. if Rietveld tried default, 3.2, and 2.7 in sequence until one succeeded).

I asked on the Mercurial tracker about suppressing git-style diffs when git is configured on, and there is a work-around:

http://bz.selenic.com/show_bug.cgi?id=3761

We could mention this in the devguide somewhere (e.g. in a FAQ about how to use Rietveld with patches to non-default branches).

I created issue 16931 to document a way to use Rietveld for 2.7 patches, while still keeping the Mercurial configuration we advise.

This strikes me as a sizable hole in our documentation.  Are there any plans to implement this as I quick glance at the devguide has no references to rietveld that I can find?

If someone familiar with rietveld wants to propose a patch I will review and apply it, otherwise it might take a while before I can get to it.

The patch should include these things:
1) how to make a patch that works with rietveld (this might already be mentioned somewhere in the devguide);
2) how to reach rietveld once the patch is uploaded (by clicking the review link);
3) how to use rietveld to review a patch.  This should briefly explain:
  a) the side-by-side view, the unified diff view, and the delta view;
  b) how to use the start review link and how to move between files using the keyboard shortcut or links;
  c) how to leave inline and general comments;
  d) how to go back to the issue (by clicking on the issue number on the main rietveld page)

Since we are moving away from rietveld, I'm going to close this as out of date.

FTR: The Mercurial bug has wandered to <https://bz.mercurial-scm.org/show_bug.cgi?id=3761>. It suggests using “hg --config diff.git=0 diff”.

Also, this 2011 post has some details of how the Git patch format is accepted or not (not sure if anything has changed since then):
https://mail.python.org/pipermail/python-dev/2011-March/110163.html