View tracker patches with ViewVC?
To review a patch on the tracker, I have to read and try to make sense of the raw diff file. Sometimes that is easy, sometimes not. *After* a patch is applied, I can click the revxxxx link and then the 'text changed' link and see a nice, colored, side-by-side web-pace view created by ViewVC. Is there any way a 'view' button could be added, along with the current edit and remove buttons, to produce the same web page and make it easier to review patches *before* commitment? -- Terry Jan Reedy
Am 26.07.2010 02:24, schrieb Terry Reedy:
To review a patch on the tracker, I have to read and try to make sense of the raw diff file. Sometimes that is easy, sometimes not.
*After* a patch is applied, I can click the revxxxx link and then the 'text changed' link and see a nice, colored, side-by-side web-pace view created by ViewVC. Is there any way a 'view' button could be added, along with the current edit and remove buttons, to produce the same web page and make it easier to review patches *before* commitment?
You should be using Rietveld for that. Regards, Martin
On Mon, Jul 26, 2010 at 10:35 AM, "Martin v. Löwis" <martin@v.loewis.de> wrote:
Am 26.07.2010 02:24, schrieb Terry Reedy:
To review a patch on the tracker, I have to read and try to make sense of the raw diff file. Sometimes that is easy, sometimes not.
*After* a patch is applied, I can click the revxxxx link and then the 'text changed' link and see a nice, colored, side-by-side web-pace view created by ViewVC. Is there any way a 'view' button could be added, along with the current edit and remove buttons, to produce the same web page and make it easier to review patches *before* commitment?
You should be using Rietveld for that.
And I believe there's already an item in the meta-tracker about improving the integration between bugs.python.org and Rietveld - it just doesn't exist yet. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
Am 26.07.2010 14:41, schrieb Nick Coghlan:
On Mon, Jul 26, 2010 at 10:35 AM, "Martin v. Löwis" <martin@v.loewis.de> wrote:
Am 26.07.2010 02:24, schrieb Terry Reedy:
To review a patch on the tracker, I have to read and try to make sense of the raw diff file. Sometimes that is easy, sometimes not.
*After* a patch is applied, I can click the revxxxx link and then the 'text changed' link and see a nice, colored, side-by-side web-pace view created by ViewVC. Is there any way a 'view' button could be added, along with the current edit and remove buttons, to produce the same web page and make it easier to review patches *before* commitment?
You should be using Rietveld for that.
And I believe there's already an item in the meta-tracker about improving the integration between bugs.python.org and Rietveld - it just doesn't exist yet.
Sounds like something Ezio could easily do -- adapt Rietveld's upload.py to a Roundup extension that submits attachments as patches, adds people on nosy to Rietveld CC, &c. Georg -- Thus spake the Lord: Thou shalt indent with four spaces. No more, no less. Four shall be the number of spaces thou shalt indent, and the number of thy indenting shall be four. Eight shalt thou not indent, nor either indent thou two, excepting that thou then proceed to four. Tabs are right out.
Sounds like something Ezio could easily do -- adapt Rietveld's upload.py to a Roundup extension that submits attachments as patches, adds people on nosy to Rietveld CC, &c.
That may not be so easy - you'll have to authenticate to Rietveld from Roundup. The other way 'round actually works: if you put report@bugs.python.org into the reviewer list when uploading to Rietveld, Rietveld changes get automatically posted to the tracker. If you then also chose a sensible title (ie. one that includes [issueXXX]), Roundup will be able to associate it with the right issue. Regards, Martin
On 7/25/2010 8:35 PM, "Martin v. Löwis" wrote:
Am 26.07.2010 02:24, schrieb Terry Reedy:
To review a patch on the tracker, I have to read and try to make sense of the raw diff file. Sometimes that is easy, sometimes not.
*After* a patch is applied, I can click the revxxxx link and then the 'text changed' link and see a nice, colored, side-by-side web-pace view created by ViewVC. Is there any way a 'view' button could be added, along with the current edit and remove buttons, to produce the same web page and make it easier to review patches *before* commitment?
You should be using Rietveld for that.
There is no mention of Rietveld in the tracker docs that I could fine. Did I miss something? That fact that one needs to go to http://codereview.appspot.com/ rather than the rietveld page was not immediately obvious from 'use Rietveld'.
That may not be so easy - you'll have to authenticate to Rietveld from Roundup.
This is why I suggested ViewVC -- it already is at python.org and could potentially be directly accessed with a button.
The other way 'round actually works: if you put report@bugs.python.org into the reviewer list when uploading to Rietveld, Rietveld changes get automatically posted to the tracker. If you then also chose a sensible title (ie. one that includes [issueXXX]), Roundup will be able to associate it with the right issue.
These are less obvious. Here is what I have gathered so far. Open patch in browser tab to get its url. Go to http://codereview.appspot.com/ Login in with Google account Go to Create issue form Copy and paste url into url field To get comments mailed back to tracker, put Issuexxxx in title and put report@bugs.python.org in reviewer list Should I open a tracker issue to add something to the tracker doc? -- Terry Jan Reedy
On 7/27/2010 1:42 AM, "Martin v. Löwis" wrote:
Should I open a tracker issue to add something to the tracker doc?
I recommend that you use it for some time before changing anything.
How is someone suppose to use it without instructions?
I also suggest that, instead of uploading the patch to Rietveld yourself, you can ask the submitter to do it.
That adds another step. Let me repeat me original question: Would it be feasible to add a [view] button that I could click to get a nice view of a patch, such as provided by ViewVC? -- Terry Jan Reedy
On Tue, Jul 27, 2010 at 10:56 AM, Terry Reedy <tjreedy@udel.edu> wrote: ..
Let me repeat me original question: Would it be feasible to add a [view] button that I could click to get a nice view of a patch, such as provided by ViewVC?
I would at best +0 on such an addition. As I mentioned before, the largest obstacle for me in reviewing the patches is being unable to open them in the browser due to the use of a non-standard extension. For small patches, as long as they open and are in unified diff format, I don't have a problem reviewing them in plain text view. For larger patches, colored diff offers only a minor improvement over plain text compared to a large improvement I get from Rietveld. I believe this should be treated as any other RFE. AFAICT, tracker sources are available from SVN and metatracker is the place to post a patch. I don't think ViewVC module is available within roundup and there may be licensing issues that need to be looked into before it can be integrated. Other than that, writing viewvc-style diff viewer plugin for roundup does not strike me as a terribly difficult project, but my +0 vote means that I am not going to write it.
On Tue, Jul 27, 2010 at 7:56 AM, Terry Reedy <tjreedy@udel.edu> wrote:
I also suggest that, instead of uploading the patch to Rietveld yourself, you can ask the submitter to do it.
That adds another step.
Let me repeat me original question: Would it be feasible to add a [view] button that I could click to get a nice view of a patch, such as provided by ViewVC?
How are you proposing to use ViewVC to view the patch? I'd think that you'd have to commit it first, unless it has some functionality that I'm unaware of. Anyway, one uses Rietveld mostly via upload.py, not the form above. Instead of running 'svn diff' + uploading the patch file in a web browser and having several versions accumulate, you run `upload.py -i <rietveld issue #>` and it uploads the diff to rietveld. Rietveld's diff view is quite nice. Would the ViewVC functionality you are proposing look like this? http://svn.python.org/view/python/branches/release27-maint/Demo/classes/Vec.py?r1=82503&r2=83175&pathrev=83175 Rietveld's differ is smarter (it does intra-line diffs) and the inline comments there are a lot better than pasting the diff into an email. It's true that the workflow isn't really described anywhere, so I'll try to outline it in detail here. Author's steps to upload a patch and create an issue: - Discuss issue in the tracker - Hack away at solution in svn checkout - When done, run `upload.py` (no args creates a new issue and prints URL) - When prompted, enter Google account credentials - When prompted, enter the issue title you want to give it, probably by pasting in the tracker title plus IssueXXX - I always check the diff on Rietveld to make sure it looks good to me before sending - Go to the URL printed and click 'Start Review' to send mail Reviewer's steps to add review comments: - Receive mail, click URL to open issue - Click the link to the first file, and read through the colored diff, using 'n' to scroll down and 'j' to go to the next file. - To make a comment, double click the line you want to comment on. This is the most unintuitive part to beginners. - Enter the comment in the textbox that appears. - Repeat until done reading the diff, then go back to the issue page and click 'Publish+Mail Comments' Author's steps to respond to comments: - Open the files in the issue - Read through the comments ('N' skips from comment to comment) - Apply fixes, reply to each comment - Run `upload.py -i <issue#>` to add a new patch with your fixes. - Reply by clicking 'Publish+Mail Comments' to let your reviewer know that you've addressed the comments Repeat ad nauseum until reviewer is happy, then commit. === Not sure why I spelled that all out when these docs exist: http://code.google.com/p/rietveld/wiki/CodeReviewHelp Hopefully my outline reflects the Python workflow more accurately, though. :) Reid
On 7/27/2010 11:52 AM, Reid Kleckner wrote:
Let me repeat me original question: Would it be feasible to add a [view] button that I could click to get a nice view of a patch, such as provided by ViewVC?
How are you proposing to use ViewVC to view the patch? I'd think that you'd have to commit it first, unless it has some functionality that I'm unaware of.
The point of the tracker is to review patches *before* they are committed. The information is the same before and after it is committed. The question of 'feasible' asks whether ViewVC is inextricably linked to an svn repository. If it is, either technically or legally, then it is.
Anyway, one uses Rietveld mostly via upload.py, not the form above.
I had never heard of upload.py. Like Rietveld, it is not mentioned in the tracker docs. I see it mentioned on he rietveld page linked below. The page says it requires 2.5 (which newcomers will not have laying aroung)and only says how to run it on *nix, so I would not be surprised if it were to not run on Windows due to using *nix only functions.
Would the ViewVC functionality you are proposing look like this? http://svn.python.org/view/python/branches/release27-maint/Demo/classes/Vec.py?r1=82503&r2=83175&pathrev=83175
Yes, I presume that is what some people use to do post-commit reviews.
Rietveld's differ is smarter (it does intra-line diffs)
Ok, that would be even better.
and the inline comments there are a lot better
than out of context comments in a tracker message.
It's true that the workflow isn't really described anywhere,
So don't expect ignorant folks like me to follow it ;-).
so I'll try to outline it in detail here.
Like code patches, doc patches in here-today, gone-tomorrow email is not too useful. Better to put it the doc or in a tracker issue for further revision.
Author's steps to upload a patch and create an issue: - Discuss issue in the tracker - Hack away at solution in svn checkout - When done, run `upload.py` (no args creates a new issue and prints URL) - When prompted, enter Google account credentials - When prompted, enter the issue title you want to give it, probably by pasting in the tracker title plus IssueXXX - I always check the diff on Rietveld to make sure it looks good to me before sending - Go to the URL printed and click 'Start Review' to send mail
Reviewer's steps to add review comments: - Receive mail, click URL to open issue - Click the link to the first file, and read through the colored diff, using 'n' to scroll down and 'j' to go to the next file. - To make a comment, double click the line you want to comment on. This is the most unintuitive part to beginners. - Enter the comment in the textbox that appears. - Repeat until done reading the diff, then go back to the issue page and click 'Publish+Mail Comments'
Author's steps to respond to comments: - Open the files in the issue - Read through the comments ('N' skips from comment to comment) - Apply fixes, reply to each comment - Run `upload.py -i<issue#>` to add a new patch with your fixes. - Reply by clicking 'Publish+Mail Comments' to let your reviewer know that you've addressed the comments
Repeat ad nauseum until reviewer is happy, then commit.
===
Not sure why I spelled that all out when these docs exist: http://code.google.com/p/rietveld/wiki/CodeReviewHelp
Because a) someone reading http://wiki.python.org/moin/TrackerDocs/ would not know it exists and b) because the above is *much* cleared than that page. Let me try to explain better what my original post was about. There have been many posts on pydev about the need for more patch reviews. A couple of people have even made 5 for 1 offers to encourage more reviews and have emphasized that even partial reviews are more helpful than none. In response to these appeals, including the last point, I recently starting doing more patch reviews than in the past. Simple diffs that replace block A with block B I can read fine. Sufficiently complex diffs, say with 10 interlaced changed, I cannot. A couple of days ago, I got an email that a doc issue I opened was now closed with revxxxxx, patch never posted to the tracker. I followed the link, saw the [text] button, and got the page with the colored, side-by-side display. I thought, "Wow, I wish I could see patches like this while reviewing, before they are committed!". And to do so just as easily, with one click. (As it turns out, the patch needed review because it had a minor error, but that is another issue.) Rietveld may be a bit better, but it potentially is a lot of work to get that bit better, especially the firs time. And it may be overkill for small to medium patches. So what I am suggesting it this: to get more and better patch reviews, especially from new reviewers, make it as easy to get an improved view of diffs as it is now to view the raw file. -- Terry Jan Reedy
On Tue, Jul 27, 2010 at 1:19 PM, Terry Reedy <tjreedy@udel.edu> wrote: ..
A couple of days ago, I got an email that a doc issue I opened was now closed with revxxxxx, patch never posted to the tracker. I followed the link, saw the [text] button, and got the page with the colored, side-by-side display. I thought, "Wow, I wish I could see patches like this while reviewing, before they are committed!". And to do so just as easily, with one click. (As it turns out, the patch needed review because it had a minor error, but that is another issue.)
Multicolored diffs may look impressive the first time you see them, but they are not really that useful at the patch review stage. With a text link I often do curl <url> | patch -p0 with <url> pasted after "copy link". That would not work with a rev-style link. Copying code from side by side view may or may not work depending on your browser. Even with doc patches, for a serious review you need to apply and compile the patch (make html in case of the docs.) Post-commit rev NNNN link offers much more than a nice diff: it shows comments, allows you to explore the history etc. Colored diffs do help spotting spelling mistakes, but there is not much harm in finding them post-commit.
On 7/27/2010 1:48 PM, Alexander Belopolsky wrote:
Multicolored diffs may look impressive the first time you see them,
Side-by-side was the important part
Copying code from side by side view may or may not work depending on your browser.
It is a nuisance with FireFox. For a patch on the tracker, I would cut from the raw diff.
Even with doc patches, for a serious review you need to apply and compile the patch (make html in case of the docs.)
I know that and I would not commit an issue without doing that. I am talking about partial pre-commit reviews by a beginner who does not have the tools to do that. The kind that have been requested here on pydev. -- Terry Jan Reedy
On 7/27/10 2:31 PM, Terry Reedy wrote:
On 7/27/2010 1:48 PM, Alexander Belopolsky wrote:
Multicolored diffs may look impressive the first time you see them,
Side-by-side was the important part
Copying code from side by side view may or may not work depending on your browser.
It is a nuisance with FireFox. For a patch on the tracker, I would cut from the raw diff.
Even with doc patches, for a serious review you need to apply and compile the patch (make html in case of the docs.)
I know that and I would not commit an issue without doing that. I am talking about partial pre-commit reviews by a beginner who does not have the tools to do that. The kind that have been requested here on pydev.
I agree with Terry that this would be a useful feature to have integrated with the tracker. I'd use it. But until someone write it, it's an academic point. -- Eric.
On Tue, Jul 27, 2010 at 2:48 PM, Eric Smith <eric@trueblade.com> wrote: ..
I agree with Terry that this would be a useful feature to have integrated with the tracker. I'd use it. But until someone write it, it's an academic point.
I don't say it is useless. It is just not useful enough to justify the required development effort. A low hanging fruit in this area would be to teach the tracker to recognize diffs regardless of file extension and present them as text/plain. For me that would give 99% of the benefit a full-blown diff-to-html converter may bring.
On Tue, Jul 27, 2010 at 2:48 PM, Eric Smith <eric@trueblade.com> wrote: ..
I agree with Terry that this would be a useful feature to have integrated with the tracker. I'd use it. But until someone write it, it's an academic point.
I don't say it is useless.
And I never said you said that :)
It is just not useful enough to justify the required development effort. A low hanging fruit in this area would be to teach the tracker to recognize diffs regardless of file extension and present them as text/plain. For me that would give 99% of the benefit a full-blown diff-to-html converter may bring.
That would be sort of handy, but detecting a diff is of course problematic. I'd rather have the viewvc sort of view, but it's moot until someone does the work. I don't want it bad enough to write it.
Am 27.07.2010 16:56, schrieb Terry Reedy:
On 7/27/2010 1:42 AM, "Martin v. Löwis" wrote:
Should I open a tracker issue to add something to the tracker doc?
I recommend that you use it for some time before changing anything.
How is someone suppose to use it without instructions?
I'd expect that most committers would be able to learn using it quickly, without explicit instructions. However, I think there might also be a Rietveld manual somewhere.
Let me repeat me original question: Would it be feasible to add a [view] button that I could click to get a nice view of a patch, such as provided by ViewVC?
I think this would be very difficult to implement, plus it would be a wasted effort, since we are going to switch to Mercurial, and would stop using ViewVC. Regards, Martin
On Sun, Jul 25, 2010 at 8:24 PM, Terry Reedy <tjreedy@udel.edu> wrote:
Is there any way a 'view' button could be added, along with the current edit and remove buttons, to produce the same web page and make it easier to review patches *before* commitment?
One thing that patch submitters can do already is to make sure that their patches are recognized as text/plain by the tracker. This can be achieved using standard extensions such as .patch, .diff, or .txt and avoiding non-satndard ones such as .patch2. Note that if you accidentally uploaded a text file with a wrong extension, you can go to the edit page and change the file name and/or content type. I think the tracker may become a little more proactive in enforcing plain text attachments by rejecting unrecognized extensions or warning users about them. On a similar note, I wonder if tracker should simply reject binary attachments with a possible exception of well formed UTF-8 text.
On 26/07/2010 01:24, Terry Reedy wrote:
To review a patch on the tracker, I have to read and try to make sense of the raw diff file. Sometimes that is easy, sometimes not.
*After* a patch is applied, I can click the revxxxx link and then the 'text changed' link and see a nice, colored, side-by-side web-pace view created by ViewVC. Is there any way a 'view' button could be added, along with the current edit and remove buttons, to produce the same web page and make it easier to review patches *before* commitment?
Apologies if I've missed this from earlier parts of this thread. On my windows box I have maintainance versions for 2.6, 2.7, 3.1 and 3.2 plus tortoisesvn. Download the patch file, right click, select tortoisesvn then apply patch. Go to the version I'm interested in. Double click to select the unit test file to start things off. If I'm lucky get a coloured output in parallel highlighting removals, additions and conflicts. If I'm unlucky, get the message "The patch seems outdated". Is this what you're asking for? Can this be (simply) implemented on the issue tracker? What do *nix or OS X users do? Cheers. Mark Lawrence.
On my windows box I have maintainance versions for 2.6, 2.7, 3.1 and 3.2 plus tortoisesvn. Download the patch file, right click, select tortoisesvn then apply patch. Go to the version I'm interested in. Double click to select the unit test file to start things off. If I'm lucky get a coloured output in parallel highlighting removals, additions and conflicts. If I'm unlucky, get the message "The patch seems outdated". Is this what you're asking for? Can this be (simply) implemented on the issue tracker? What do *nix or OS X users do?
Automatically checking whether a patch still applies? That could certainly be implemented, although it might be expensive in terms of CPU time. Anything more frequent than daily runs is likely not feasible. In addition, it might be tricky to determine what branches a patch is meant to apply to. It would be nice if the patch author gets notified when a patch becomes stale, but that would require to be certain what branches a patch is targetted for. In any case: contributions are welcome. Regards, Martin
participants (8)
-
"Martin v. Löwis" -
Alexander Belopolsky -
Eric Smith -
Georg Brandl -
Mark Lawrence -
Nick Coghlan -
Reid Kleckner -
Terry Reedy