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.
|
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) * |
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) * |
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) * |
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
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:26 | admin | set | github: 58171 |
2016-07-25 13:35:55 | BreamoreBoy | set | nosy:
- BreamoreBoy
|
2016-07-25 12:46:21 | martin.panter | set | nosy:
+ martin.panter messages:
+ msg271260
|
2016-01-10 15:42:33 | ezio.melotti | set | status: open -> closed resolution: out of date messages:
+ msg257915
stage: needs patch -> resolved |
2014-06-29 09:41:04 | ezio.melotti | set | messages:
+ msg221834 |
2014-06-12 22:29:09 | BreamoreBoy | set | nosy:
+ BreamoreBoy messages:
+ msg220392
|
2013-09-21 07:07:17 | akitada | set | nosy:
+ akitada
|
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 |
2012-09-23 17:44:44 | chris.jerdonek | set | nosy:
+ chris.jerdonek messages:
+ msg171067
|
2012-09-23 13:29:50 | ezio.melotti | set | assignee: ezio.melotti type: enhancement 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 |
2012-03-14 21:19:31 | orsenthil | set | nosy:
+ orsenthil messages:
+ msg155795
|
2012-03-14 00:21:01 | dmalcolm | set | nosy:
+ loewis
|
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 |
2012-02-12 04:36:47 | terry.reedy | set | nosy:
+ terry.reedy messages:
+ msg153166
|
2012-02-08 15:51:54 | eric.araujo | set | nosy:
+ eric.araujo
|
2012-02-07 16:42:03 | tshepang | set | nosy:
+ tshepang
|
2012-02-07 16:39:42 | nadeem.vawda | set | nosy:
+ nadeem.vawda
|
2012-02-07 16:24:38 | dmalcolm | create | |