Created on 2012-02-07 16:24 by dmalcolm, last changed 2013-01-11 09:01 by chris.jerdonek.
|msg152813 - (view)||Author: Dave Malcolm (dmalcolm)||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) *||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) *||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)||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)||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) *||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) *||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) *||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) *||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) *||Date: 2012-09-23 13:29|
See also #11869.
|msg171067 - (view)||Author: Chris Jerdonek (chris.jerdonek) *||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) *||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) *||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.
|2013-03-14 07:51:42||ezio.melotti||link||issue11869 superseder|
|2013-01-11 09:01:35||chris.jerdonek||set||messages: + msg179658|
|2013-01-11 05:50:37||chris.jerdonek||set||messages: + msg179633|
messages: + msg171067
|2012-09-23 13:29:50||ezio.melotti||set||assignee: ezio.melotti|
messages: + msg171036
stage: needs patch
|2012-03-15 00:37:28||ezio.melotti||set||messages: + msg155830|
|2012-03-15 00:16:56||orsenthil||set||messages: + msg155825|
|2012-03-15 00:14:12||eric.araujo||set||messages: + msg155822|
messages: + msg155795
|2012-03-14 00:20:29||dmalcolm||set||messages: + msg155692|
|2012-03-14 00:10:53||dmalcolm||set||messages: + msg155691|
|2012-02-12 10:19:22||nadeem.vawda||set||messages: + msg153194|
messages: + msg153166