classification
Title: dev guide has no mention of mechanics of patch review
Type: enhancement Stage: resolved
Components: Devguide Versions:
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: ezio.melotti Nosy List: akitada, chris.jerdonek, dmalcolm, eric.araujo, ezio.melotti, loewis, martin.panter, nadeem.vawda, orsenthil, terry.reedy, tshepang
Priority: normal Keywords:

Created on 2012-02-07 16:24 by dmalcolm, last changed 2016-07-25 13:35 by BreamoreBoy. This issue is now closed.

Messages (17)
msg152813 - (view) Author: Dave Malcolm (dmalcolm) (Python committer) Date: 2012-02-07 16:24
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
msg153166 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2012-02-12 04:36
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.
msg153194 - (view) Author: Nadeem Vawda (nadeem.vawda) * (Python committer) Date: 2012-02-12 10:19
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.
msg155691 - (view) Author: Dave Malcolm (dmalcolm) (Python committer) Date: 2012-03-14 00:10
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
msg155692 - (view) Author: Dave Malcolm (dmalcolm) (Python committer) Date: 2012-03-14 00:20
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.
msg155795 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2012-03-14 21:19
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.
msg155822 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-03-15 00:14
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
msg155825 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2012-03-15 00:16
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)
msg155830 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-03-15 00:37
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.
msg171036 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-09-23 13:29
See also #11869.
msg171067 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-23 17:44
> 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).
msg179633 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-01-11 05:50
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).
msg179658 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-01-11 09:01
I created issue 16931 to document a way to use Rietveld for 2.7 patches, while still keeping the Mercurial configuration we advise.
msg220392 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-12 22:29
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?
msg221834 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2014-06-29 09:41
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)
msg257915 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2016-01-10 15:42
Since we are moving away from rietveld, I'm going to close this as out of date.
msg271260 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-07-25 12:46
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
History
Date User Action Args
2016-07-25 13:35:55BreamoreBoysetnosy: - BreamoreBoy
2016-07-25 12:46:21martin.pantersetnosy: + martin.panter
messages: + msg271260
2016-01-10 15:42:33ezio.melottisetstatus: open -> closed
resolution: out of date
messages: + msg257915

stage: needs patch -> resolved
2014-06-29 09:41:04ezio.melottisetmessages: + msg221834
2014-06-12 22:29:09BreamoreBoysetnosy: + BreamoreBoy
messages: + msg220392
2013-09-21 07:07:17akitadasetnosy: + akitada
2013-03-14 07:51:42ezio.melottilinkissue11869 superseder
2013-01-11 09:01:35chris.jerdoneksetmessages: + msg179658
2013-01-11 05:50:37chris.jerdoneksetmessages: + msg179633
2012-09-23 17:44:44chris.jerdoneksetnosy: + chris.jerdonek
messages: + msg171067
2012-09-23 13:29:50ezio.melottisetassignee: ezio.melotti
type: enhancement
messages: + msg171036
stage: needs patch
2012-03-15 00:37:28ezio.melottisetmessages: + msg155830
2012-03-15 00:16:56orsenthilsetmessages: + msg155825
2012-03-15 00:14:12eric.araujosetmessages: + msg155822
2012-03-14 21:19:31orsenthilsetnosy: + orsenthil
messages: + msg155795
2012-03-14 00:21:01dmalcolmsetnosy: + loewis
2012-03-14 00:20:29dmalcolmsetmessages: + msg155692
2012-03-14 00:10:53dmalcolmsetmessages: + msg155691
2012-02-12 10:19:22nadeem.vawdasetmessages: + msg153194
2012-02-12 04:36:47terry.reedysetnosy: + terry.reedy
messages: + msg153166
2012-02-08 15:51:54eric.araujosetnosy: + eric.araujo
2012-02-07 16:42:03tshepangsetnosy: + tshepang
2012-02-07 16:39:42nadeem.vawdasetnosy: + nadeem.vawda
2012-02-07 16:24:38dmalcolmcreate