[Qemu-devel] Fwd: Proposal: Improving patch tracking and review using Rietveld

[Qemu-devel] Fwd: Proposal: Improving patch tracking and review using Rietveld

[Paolo Bonzini]

Forwarding this from the GCC mailing list.  Since patchwork isn't more 
than a mail archive the way it's implemented in QEMU, this may be a more 
interesting possibility.

Paolo

> At Google we use a code review tool which was open sourced a couple of
> years ago: Rietveld
> (http://code.google.com/appengine/articles/rietveld.html).
>
> The best way of thinking about it is "bugzilla for patches".  The
> system creates an entry for every patch submitted, provides a web tool
> for manipulating the patch (comments, different views of the diff,
> highlighting, etc) and it also has an email gateway.
>
> We have discussed patch tracking mechanisms in the past, and none so
> far has taken hold.  The reason why I like Rietveld is that it doesn't
> really matter whether we all switch to using it at once:
>
> 1- Rietveld always send the patch sent to it to gcc-patches@ (provided
> the submitter added gcc-patches to the CC list).
> 2- The whole trail of discussion on the patch also get sent to
> gcc-patches and everyone else is CC'd in it.
> 3- Reviewers do not need to use the web tool to reply to the patch.
> One can simply respond to the e-mail, and it will get added to the
> patch discussion trail.
>
> So, for people who do not want to use the tool, Rietveld will not get
> in the way.  They can simply respond to the patch as usual, and as
> long as they keep the rietveld email address in the CC list, the patch
> trail will be updated automatically.
>
> At Google we will start using Rietveld to send patches.  The only
> difference folks will notice is that Rietveld-generated email has some
> extra text.
>
> I have created a wiki page that explains the basics of using Rietveld
> (thanks Jeffrey for the instructions):
> http://gcc.gnu.org/wiki/rietveld
>
> Once again, I'd like to underscore the fact that if a patch submitter
> chooses to use Rietveld for tracking their patches, this should not
> affect in any way the traditional mail-based review.  All I ask is
> that reviewers maintain the CC and Subject line intact in order to not
> confuse the tool.
>
> Jeffrey, would you mind looking over the instructions I've written to
> make sure they're correct?
>
> Richard, this is the tool I mentioned in today's chat.
>
>
> Thanks.  Diego.

Re: [Qemu-devel] Fwd: Proposal: Improving patch tracking and review using Rietveld

[Stefan Hajnoczi]

On Thu, Jan 27, 2011 at 08:55:30AM +0100, Paolo Bonzini wrote:
> Forwarding this from the GCC mailing list.  Since patchwork isn't
> more than a mail archive the way it's implemented in QEMU, this may
> be a more interesting possibility.

What features are you looking for beyond archiving?

It would be nice to have a dashboard of currently unapplied patches
(with old versions of patch series ignored).  There should be an
automatic way of letting the review system know which patches have been
applied to the code repository.

BTW the email integration sounds good and is critical.  If we have an
isolated system that requires separate log in and manual syncing of
status between mailing list, code repository, bug tracker, and review
tool then it will probably just wither away.

Stefan

Re: [Qemu-devel] Fwd: Proposal: Improving patch tracking and review using Rietveld

[Paolo Bonzini]

On 01/27/2011 11:19 AM, Stefan Hajnoczi wrote:
> On Thu, Jan 27, 2011 at 08:55:30AM +0100, Paolo Bonzini wrote:
>> Forwarding this from the GCC mailing list.  Since patchwork isn't
>> more than a mail archive the way it's implemented in QEMU, this may
>> be a more interesting possibility.
>
> What features are you looking for beyond archiving?

Well, you pretty much nailed it.

> It would be nice to have a dashboard of currently unapplied patches
> (with old versions of patch series ignored).

and this:

> BTW the email integration sounds good and is critical.

:)

The only tricky point is whether email integration includes ignoring 
older versions of patch series.  I'll ask on gcc@gcc.gnu.org how/whether 
that works.

Paolo

Re: [Qemu-devel] Fwd: Proposal: Improving patch tracking and review using Rietveld

[Stefan Hajnoczi]

On Thu, Jan 27, 2011 at 10:23 AM, Paolo Bonzini <bonzini@gnu.org> wrote:
> On 01/27/2011 11:19 AM, Stefan Hajnoczi wrote:
>>
>> On Thu, Jan 27, 2011 at 08:55:30AM +0100, Paolo Bonzini wrote:
>>>
>>> Forwarding this from the GCC mailing list.  Since patchwork isn't
>>> more than a mail archive the way it's implemented in QEMU, this may
>>> be a more interesting possibility.
>>
>> What features are you looking for beyond archiving?
>
> Well, you pretty much nailed it.

Glad we're on the same page.  I wanted to see if you are looking for
code review or patch tracking.

I actually like the code review process used today - inline patches to
the mailing list.  It's hard to add value to that without making it
more cumbersome.

But patch tracking is definitely lacking and would benefit from
something that aggregates and presents an accurate view of the state
of unapplied patches.

Stefan

[Qemu-devel] Re: Fwd: Proposal: Improving patch tracking and review using Rietveld

[Paolo Bonzini]

On 01/27/2011 11:23 AM, Paolo Bonzini wrote:
> On 01/27/2011 11:19 AM, Stefan Hajnoczi wrote:
>> On Thu, Jan 27, 2011 at 08:55:30AM +0100, Paolo Bonzini wrote:
>>> Forwarding this from the GCC mailing list. Since patchwork isn't
>>> more than a mail archive the way it's implemented in QEMU, this may
>>> be a more interesting possibility.
>>
>> What features are you looking for beyond archiving?
>
> Well, you pretty much nailed it.
>
>> It would be nice to have a dashboard of currently unapplied patches
>> (with old versions of patch series ignored).
>
> and this:
>
>> BTW the email integration sounds good and is critical.
>
> :)
>
> The only tricky point is whether email integration includes ignoring
> older versions of patch series. I'll ask on gcc@gcc.gnu.org how/whether
> that works.

Here's the outcome of my conversation with the GCC developer who 
proposed it.

- the tool would require adoption of a special tool for patch submitter. 
  The tool is a self-contained Python script that could be included in 
the QEMU repository (upload.py).

- reviewers (and submitters discussing the issue) can either reply 
directly to email, or use the web tool.  Diego said that either way can 
be used, but the web tool is actually pretty addictive.  It produces 
basically the equivalent of the "inline comment" mails we use, and has 
decent keyboard bindings (that said, creating a comment always requires 
a double click).


That said, it looks like the integration with git is (still?) a bit too 
rough to be usable.  In particular, you can more or less track a patch 
series but not the commit messages of each series.

I created two issues in the tracker as examples:
- http://code.google.com/p/rietveld/issues/detail?id=267
- http://code.google.com/p/rietveld/issues/detail?id=268

http://code.google.com/p/rietveld/issues/detail?id=262 is also of 
interest, even though it refers to Mercurial.

It's possible that if these are fixed, the remaining problems can be 
worked around by hacking upload.py or wrapping a custom script around 
it, that would be more similar to git-send-email in appearance and behavior.

Paolo

[Qemu-devel] Re: Fwd: Proposal: Improving patch tracking and review using Rietveld

[Diego Novillo]

On Thu, Jan 27, 2011 at 11:26, Paolo Bonzini <pbonzini@redhat.com> wrote:

> - reviewers (and submitters discussing the issue) can either reply directly
> to email, or use the web tool.  Diego said that either way can be used, but
> the web tool is actually pretty addictive.  It produces basically the
> equivalent of the "inline comment" mails we use, and has decent keyboard
> bindings (that said, creating a comment always requires a double click).

Right.

Given that you folks are just starting to look for code review tools,
you may want to consider other options we have listed in
http://gcc.gnu.org/wiki/GCC_Patch_Tracking.  In particular,
http://www.reviewboard.org/ (though it seemed too much to me, and I
was looking something similar to our internal review tool).


Diego.

Re: [Qemu-devel] Fwd: Proposal: Improving patch tracking and review using Rietveld

[Anthony Liguori]

On 01/27/2011 01:55 AM, Paolo Bonzini wrote:
> Forwarding this from the GCC mailing list.  Since patchwork isn't more 
> than a mail archive the way it's implemented in QEMU, this may be a 
> more interesting possibility.

Patchwork is a nice tool but I found a few issues with it that really 
deterred me from using it:

1) it's all or nothing in terms of whether maintainers use it.  if 
everyone isn't on top of keeping it clean, you end up with a terrible 
backlog

2) it doesn't understand patches series.  A 20 patch series gets applied 
all at once, yet you have to update status for each patch.  That's annoying.

3) it doesn't understand new revisions of the same patch/series.  This 
is really a deal breaker.  Having to go and update the status 
particularly when you have patch series that see multiple revs in 24 
hours creates an awful lot of work

> Paolo
>
>> At Google we use a code review tool which was open sourced a couple of
>> years ago: Rietveld
>> (http://code.google.com/appengine/articles/rietveld.html).
>>
>> The best way of thinking about it is "bugzilla for patches".  The
>> system creates an entry for every patch submitted, provides a web tool
>> for manipulating the patch (comments, different views of the diff,
>> highlighting, etc) and it also has an email gateway.
>>
>> We have discussed patch tracking mechanisms in the past, and none so
>> far has taken hold.  The reason why I like Rietveld is that it doesn't
>> really matter whether we all switch to using it at once:
>>
>> 1- Rietveld always send the patch sent to it to gcc-patches@ (provided
>> the submitter added gcc-patches to the CC list).
>> 2- The whole trail of discussion on the patch also get sent to
>> gcc-patches and everyone else is CC'd in it.
>> 3- Reviewers do not need to use the web tool to reply to the patch.
>> One can simply respond to the e-mail, and it will get added to the
>> patch discussion trail.
>>
>> So, for people who do not want to use the tool, Rietveld will not get
>> in the way.  They can simply respond to the patch as usual, and as
>> long as they keep the rietveld email address in the CC list, the patch
>> trail will be updated automatically.
>>
>> At Google we will start using Rietveld to send patches.  The only
>> difference folks will notice is that Rietveld-generated email has some
>> extra text.
>>
>> I have created a wiki page that explains the basics of using Rietveld
>> (thanks Jeffrey for the instructions):
>> http://gcc.gnu.org/wiki/rietveld

Interesting.  This seems to have nice characteristics compared to 
patchworks.

Regards,

Anthony Liguori

>> Once again, I'd like to underscore the fact that if a patch submitter
>> chooses to use Rietveld for tracking their patches, this should not
>> affect in any way the traditional mail-based review.  All I ask is
>> that reviewers maintain the CC and Subject line intact in order to not
>> confuse the tool.
>>
>> Jeffrey, would you mind looking over the instructions I've written to
>> make sure they're correct?
>>
>> Richard, this is the tool I mentioned in today's chat.
>>
>>
>> Thanks.  Diego.
>
>

Re: [Qemu-devel] Fwd: Proposal: Improving patch tracking and review using Rietveld

[Peter Maydell]

On 27 January 2011 17:31, Anthony Liguori <aliguori@linux.vnet.ibm.com> wrote:
> Patchwork is a nice tool but I found a few issues with it that really
> deterred me from using it:

> 2) it doesn't understand patches series.  A 20 patch series gets applied all
> at once, yet you have to update status for each patch.  That's annoying.

I submitted that as a feature request to the patchwork mailing
list earlier this month:
http://lists.ozlabs.org/pipermail/patchwork/2011-January/000348.html
and it didn't get shot down in flames so maybe if we procrastinate
long enough patchwork will improve :-)

-- PMM

Re: [Qemu-devel] Fwd: Proposal: Improving patch tracking and review using Rietveld

[Stefan Hajnoczi]

On Thu, Jan 27, 2011 at 5:31 PM, Anthony Liguori
<aliguori@linux.vnet.ibm.com> wrote:
> Patchwork is a nice tool but I found a few issues with it that really
> deterred me from using it:
>
> 1) it's all or nothing in terms of whether maintainers use it.  if everyone
> isn't on top of keeping it clean, you end up with a terrible backlog
>
> 2) it doesn't understand patches series.  A 20 patch series gets applied all
> at once, yet you have to update status for each patch.  That's annoying.

You shouldn't have to update patch status manually at all.  If there
is a manual step involved then it is unlikely that patchwork's view
will ever stay in sync with the actual repository.

Stefan