Patchwork patch tracking system

Patchwork patch tracking system

[Gary Benson]

Hi all,

Patchwork is a patch tracking system designed to help manage
contributions to open-source projects.  The glibc maintainers have
been experimenting to see if it can meet their needs, and they've very
kindly added GDB to their test instance to allow us to trial it too.

The way it works is that patches sent to gdb-patches are caught by
Patchwork and appear on a web page, currently
http://patchwork.siddhesh.in/.  Replies to patch emails are also
caught, and are appended to the patch pages.

The prototype workflow the glibc maintainers are using is
https://sourceware.org/glibc/wiki/Patch%20Review%20Workflow.  If you'd
like to have a go with the trial instance please register an account
on http://patchwork.siddhesh.in/ and then get hold of me by email or
on IRC (I'm gbenson on Freenode) and I'll add you to the maintainers
list.  You won't be able to update anything without this.

Patchwork has been subscribed to gdb-patches for a couple of weeks
now, but I haven't updated any patches' statuses so there will be some
stale ones in there.  Please feel free to update patches as you see
fit, this is a trial instance so don't worry about breaking things!

As well as the web interface there is a command line client.
You can download the client script and a sample ~/.pwclientrc
from here:

  http://patchwork.siddhesh.in/project/binutils-gdb/

The ~/.pwclientrc file just needs your password adding, then you
can do things like:

 pwclient list -s new

to see a list of all patches with the state "new".

Let me know what you think!

Thanks,
Gary

-- 
http://gbenson.net/

Re: Patchwork patch tracking system

[Stan Shebs]

On 4/2/14 3:08 AM, Gary Benson wrote:
> Hi all,
> 
> Patchwork is a patch tracking system designed to help manage
> contributions to open-source projects.  The glibc maintainers have
> been experimenting to see if it can meet their needs, and they've very
> kindly added GDB to their test instance to allow us to trial it too.

I just heard about this last week, and it looks like a good idea!

> Patchwork has been subscribed to gdb-patches for a couple of weeks
> now, but I haven't updated any patches' statuses so there will be some
> stale ones in there.  Please feel free to update patches as you see
> fit, this is a trial instance so don't worry about breaking things!

So if we try it and like it, how does one go about transitioning
from "trial" to "real"?

> Let me know what you think!

Thanks for setting this up!

Stan
stan@codesourcery.com

Re: Patchwork patch tracking system

[Gary Benson]

Hi Stan,

Sorry for the late reply, your message got lost in a bunch of
demangler issues :(

Stan Shebs wrote:
> On 4/2/14 3:08 AM, Gary Benson wrote:
> > Patchwork is a patch tracking system designed to help manage
> > contributions to open-source projects.  The glibc maintainers have
> > been experimenting to see if it can meet their needs, and they've
> > very kindly added GDB to their test instance to allow us to trial
> > it too.
> 
> I just heard about this last week, and it looks like a good idea!
> 
> > Patchwork has been subscribed to gdb-patches for a couple of weeks
> > now, but I haven't updated any patches' statuses so there will be
> > some stale ones in there.  Please feel free to update patches as
> > you see fit, this is a trial instance so don't worry about
> > breaking things!
> 
> So if we try it and like it, how does one go about transitioning
> from "trial" to "real"?

I guess by the people doing the reviewing deciding to use it.
It may be it is useful even with only a subset of reviewers
using it.  I can't determine this myself, I need feedback from
people who are reviewing regularly.

There's work underway to transfer glibc's trial instance to
sourceware.org, and I've asked for the GDB instance to be
moved at the same time.  Apart from anything else, it's probably
easier that way.

> > Let me know what you think!
> 
> Thanks for setting this up!

No worries, Siddhesh did all the hard work :D

Cheers,
Gary

-- 
http://gbenson.net/

Re: Patchwork patch tracking system

[Joel Brobecker]

> > So if we try it and like it, how does one go about transitioning
> > from "trial" to "real"?
> 
> I guess by the people doing the reviewing deciding to use it.
> It may be it is useful even with only a subset of reviewers
> using it.  I can't determine this myself, I need feedback from
> people who are reviewing regularly.

In my opinion, the GDB project is in dire need of a way to track
patches. Using one's mailbox to track patches just does not work.
But I think that we would need full commitment to the tool from
the project, or else it'd quickly start overflowing with stale
info.

There is a tool that we use internally at AdaCore which I was starting
to think of proposing for GDB, called geritt. From what I have been
able to see from patchwork's webpage, geritt seems like a much more
advanced system compared to patchwork. But the tradeoff is that using
geritt requires a bit more work as well, and that part or all of
the review process would happen on geritt, rather than the mailing-list.
It's not very intuitive at first, but it is very easy and lightweight.

I personally believe geritt's approach to be better in the long run.
But, while I am worried about having communication and patch handling
be done via two distinct systems, patchwork's simpler approach might be
working well enough without requiring the big shift in patch-reviewing
paradigm.

-- 
Joel

Re: Patchwork patch tracking system

[Eric Christopher]

On Tue, Apr 22, 2014 at 6:06 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> > So if we try it and like it, how does one go about transitioning
>> > from "trial" to "real"?
>>
>> I guess by the people doing the reviewing deciding to use it.
>> It may be it is useful even with only a subset of reviewers
>> using it.  I can't determine this myself, I need feedback from
>> people who are reviewing regularly.
>
> In my opinion, the GDB project is in dire need of a way to track
> patches. Using one's mailbox to track patches just does not work.
> But I think that we would need full commitment to the tool from
> the project, or else it'd quickly start overflowing with stale
> info.
>
> There is a tool that we use internally at AdaCore which I was starting
> to think of proposing for GDB, called geritt. From what I have been
> able to see from patchwork's webpage, geritt seems like a much more
> advanced system compared to patchwork. But the tradeoff is that using
> geritt requires a bit more work as well, and that part or all of
> the review process would happen on geritt, rather than the mailing-list.
> It's not very intuitive at first, but it is very easy and lightweight.
>
> I personally believe geritt's approach to be better in the long run.
> But, while I am worried about having communication and patch handling
> be done via two distinct systems, patchwork's simpler approach might be
> working well enough without requiring the big shift in patch-reviewing
> paradigm.
>

FWIW we (some of the google folk) looked at geritt for LLVM and
discarded in favor of phabricator. It seemed to solve a lot of the
problems that we had and allowed communication to and from the mailing
lists for patches which was key for us as we have a similar review
style to gcc/gdb/binutils. We didn't want to remove the ability for
people to send patches to the mailing lists, but yet get a better
review mechanism for large patches/queuing/etc.

Just piping in since we recently did some of this work. Feel free to
let me know if you have any questions on our experiences.

-eric

Re: Patchwork patch tracking system

[Joel Brobecker]

> FWIW we (some of the google folk) looked at geritt for LLVM and
> discarded in favor of phabricator. It seemed to solve a lot of the
> problems that we had and allowed communication to and from the mailing
> lists for patches which was key for us as we have a similar review
> style to gcc/gdb/binutils. We didn't want to remove the ability for
> people to send patches to the mailing lists, but yet get a better
> review mechanism for large patches/queuing/etc.

Thanks for the suggestion! I have to say, from the outside,
phabricator looks like a pretty interesting option.

-- 
Joel

Re: Patchwork patch tracking system

[Siva Chandra]

On Tue, Apr 22, 2014 at 8:51 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> FWIW we (some of the google folk) looked at geritt for LLVM and
>> discarded in favor of phabricator. It seemed to solve a lot of the
>> problems that we had and allowed communication to and from the mailing
>> lists for patches which was key for us as we have a similar review
>> style to gcc/gdb/binutils. We didn't want to remove the ability for
>> people to send patches to the mailing lists, but yet get a better
>> review mechanism for large patches/queuing/etc.
>
> Thanks for the suggestion! I have to say, from the outside,
> phabricator looks like a pretty interesting option.

I am essentially an outsider here, and I have not used phabricator.
However, the Chromium project uses Rietveld for codereview:
https://code.google.com/p/rietveld/ and
https://codereview.chromium.org/

IIRC, Diego Novillo setup a rietveld instance for GCC:
http://gcc.gnu.org/wiki/rietveld

Chromium-OS project uses gerrit and hence I have used both these tools
and my personal choice is rietveld over gerrit by many a mile.
Rietveld can be configured to send mails to a watchlist for every
patch sent out for review and also for every review posting. Hence,
all review logs can still be saved in a mailing list archive.

Thanks,
Siva Chandra

Re: Patchwork patch tracking system

[Tom Tromey]

Joel> There is a tool that we use internally at AdaCore which I was starting
Joel> to think of proposing for GDB, called geritt. From what I have been
Joel> able to see from patchwork's webpage, geritt seems like a much more
Joel> advanced system compared to patchwork. But the tradeoff is that using
Joel> geritt requires a bit more work as well, and that part or all of
Joel> the review process would happen on geritt, rather than the mailing-list.
Joel> It's not very intuitive at first, but it is very easy and lightweight.

I was keen on gerrit until I read this:

http://lists.nongnu.org/archive/html/qemu-devel/2014-02/msg00232.html

Maybe phabricator is better for our needs?

I've been trying the patchworks install as well.  I don't find it all
that useful myself, but maybe it would be better if more people were
using it.

Tom

Re: Patchwork patch tracking system

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Cc: Gary Benson <gbenson@redhat.com>, Stan Shebs <stanshebs@earthlink.net>,        gdb@sourceware.org
> Date: Tue, 29 Apr 2014 11:07:32 -0600
> 
> I was keen on gerrit until I read this:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2014-02/msg00232.html
> 
> Maybe phabricator is better for our needs?
> 
> I've been trying the patchworks install as well.  I don't find it all
> that useful myself, but maybe it would be better if more people were
> using it.

Perhaps we should begin by formulating a set of minimum requirements
for the tool we need and want.

Re: Patchwork patch tracking system

[Joel Brobecker]

> Maybe phabricator is better for our needs?

I have to say that phabricator looks like a pretty nice tool.
I'd be happy trying it out as an experiment.

-- 
Joel

Re: Patchwork patch tracking system

[Pedro Alves]

On 04/29/2014 06:07 PM, Tom Tromey wrote:

> I've been trying the patchworks install as well.  I don't find it all
> that useful myself, but maybe it would be better if more people were
> using it.

I've been trying it out too.  I've already found it useful to keep
track of which of my own patches I have pending.

I've been absent a little while from review, but I'm heading back,
and I'm using patchwork to guide me.

I like that it doesn't make the mailing list a second class
citizen.  I'd be willing to continue giving it a try, but indeed I
think it'd be better if more people were using it.  That'll be true
for any tool we end up with.

In the past week, I've been cleaning it up whenever I see that
patches have been pushed, even those that I didn't approve myself,
but of course it'd be better if who approves the patch or
the submitter themselves take care of their own patches.

non-maintainers shouldn't hold back from creating an account
and updating the state of their own patches.  Whatever helps
bringing the load down from maintainers should help your own
patches.  :-)

Here's the current list of who-has-how-many-pending:

$ ~/bin/pwclient-hacked list -s New | sort | uniq -c | sort -nr
     35 Andy Wingo <wingo@igalia.com>
     24 Andreas Arnez <arnez@linux.vnet.ibm.com>
     20 Yao Qi <yao@codesourcery.com>
     17 Jan Kratochvil <jan.kratochvil@redhat.com>
     16 Pedro Alves <palves@redhat.com>
     14 Siva Chandra <sivachandra@google.com>
     13 Keith Seitz <keiths@redhat.com>
     13 David Blaikie <dblaikie@gmail.com>
     12 Andrew Burgess <aburgess@broadcom.com>
      9 Doug Evans <dje@google.com>
      4 Kyle McMartin <kmcmarti@redhat.com>
      4 Hui Zhu <hui_zhu@mentor.com>
      3 Simon Marchi <simon.marchi@ericsson.com>
      3 Eli Zaretskii <eliz@gnu.org>
      3 Alan Modra <amodra@gmail.com>
      2 Ulrich Weigand <uweigand@de.ibm.com>
      2 Mike Frysinger <vapier@gentoo.org>
      2 Doug Evans <xdje42@gmail.com>
      2 Alexander Smundak <asmundak@google.com>
      2 Agovic, Sanimir <sanimir.agovic@intel.com>
      1 Vladimir Nikulichev <nvs@tbricks.com>
      1 Tom Tromey <tromey@redhat.com>
      1 Sandra Loosemore <sandra@codesourcery.com>
      1 Pierre Langlois <pierre.langlois@embecosm.com>
      1 Nick Clifton <nickc@redhat.com>
      1 Mateusz Tabaka <8tab@wp.pl>
      1 Mark Wielaard <mjw@redhat.com>
      1 Marcus Shawcroft <marcus.shawcroft@arm.com>
      1 Marc Khouzam <marc.khouzam@ericsson.com>
      1 Maciej W. Rozycki <macro@codesourcery.com>
      1 Julian Brown <julian@codesourcery.com>
      1 John Marino <gnugcc@marino.st>
      1 Gary Benson <gbenson@redhat.com>
      1 David Taylor <dtaylor@emc.com>
      1 Daniel Gutson <daniel.gutson@tallertechnologies.com>

As you see, most of the patches so far, since we began tracking a
few weeks back, came from a small set of people.  And I suspect
many of those are actually already in.

-- 
Pedro Alves

RE: Patchwork patch tracking system

[Breazeal, Don]

> -----Original Message-----
> From: gdb-owner@sourceware.org [mailto:gdb-owner@sourceware.org] On
> Behalf Of Pedro Alves
> Sent: Tuesday, April 29, 2014 11:58 AM
> To: Tom Tromey
> Cc: Joel Brobecker; Gary Benson; Stan Shebs; gdb@sourceware.org
> Subject: Re: Patchwork patch tracking system
> 
> On 04/29/2014 06:07 PM, Tom Tromey wrote:
> 
> > I've been trying the patchworks install as well.  I don't find it all
> > that useful myself, but maybe it would be better if more people were
> > using it.
> 
> I've been trying it out too.  I've already found it useful to keep track
> of which of my own patches I have pending.
> 
> I've been absent a little while from review, but I'm heading back, and
> I'm using patchwork to guide me.
> 
> I like that it doesn't make the mailing list a second class citizen.
> I'd be willing to continue giving it a try, but indeed I think it'd be
> better if more people were using it.  That'll be true for any tool we
> end up with.
> 
> In the past week, I've been cleaning it up whenever I see that patches
> have been pushed, even those that I didn't approve myself, but of course
> it'd be better if who approves the patch or the submitter themselves
> take care of their own patches.
> 
> non-maintainers shouldn't hold back from creating an account and
> updating the state of their own patches.  Whatever helps bringing the
> load down from maintainers should help your own patches.  :-)
> 
> Here's the current list of who-has-how-many-pending:
> 
> $ ~/bin/pwclient-hacked list -s New | sort | uniq -c | sort -nr
>      35 Andy Wingo <wingo@igalia.com>
>      24 Andreas Arnez <arnez@linux.vnet.ibm.com>
>      20 Yao Qi <yao@codesourcery.com>
>      17 Jan Kratochvil <jan.kratochvil@redhat.com>
>      16 Pedro Alves <palves@redhat.com>
>      14 Siva Chandra <sivachandra@google.com>
>      13 Keith Seitz <keiths@redhat.com>
>      13 David Blaikie <dblaikie@gmail.com>
>      12 Andrew Burgess <aburgess@broadcom.com>
>       9 Doug Evans <dje@google.com>
>       4 Kyle McMartin <kmcmarti@redhat.com>
>       4 Hui Zhu <hui_zhu@mentor.com>
>       3 Simon Marchi <simon.marchi@ericsson.com>
>       3 Eli Zaretskii <eliz@gnu.org>
>       3 Alan Modra <amodra@gmail.com>
>       2 Ulrich Weigand <uweigand@de.ibm.com>
>       2 Mike Frysinger <vapier@gentoo.org>
>       2 Doug Evans <xdje42@gmail.com>
>       2 Alexander Smundak <asmundak@google.com>
>       2 Agovic, Sanimir <sanimir.agovic@intel.com>
>       1 Vladimir Nikulichev <nvs@tbricks.com>
>       1 Tom Tromey <tromey@redhat.com>
>       1 Sandra Loosemore <sandra@codesourcery.com>
>       1 Pierre Langlois <pierre.langlois@embecosm.com>
>       1 Nick Clifton <nickc@redhat.com>
>       1 Mateusz Tabaka <8tab@wp.pl>
>       1 Mark Wielaard <mjw@redhat.com>
>       1 Marcus Shawcroft <marcus.shawcroft@arm.com>
>       1 Marc Khouzam <marc.khouzam@ericsson.com>
>       1 Maciej W. Rozycki <macro@codesourcery.com>
>       1 Julian Brown <julian@codesourcery.com>
>       1 John Marino <gnugcc@marino.st>
>       1 Gary Benson <gbenson@redhat.com>
>       1 David Taylor <dtaylor@emc.com>
>       1 Daniel Gutson <daniel.gutson@tallertechnologies.com>
> 
> As you see, most of the patches so far, since we began tracking a few
> weeks back, came from a small set of people.  And I suspect many of
> those are actually already in.

A patch series that I posted to gdb-patches at the beginning of April doesn't seem to show up in patchwork.  It would be good to understand why that is and how to fix it.
https://sourceware.org/ml/gdb-patches/2014-04/msg00037.html
https://sourceware.org/ml/gdb-patches/2014-04/msg00040.html
https://sourceware.org/ml/gdb-patches/2014-04/msg00072.html
https://sourceware.org/ml/gdb-patches/2014-04/msg00071.html
thanks
--Don

Re: Patchwork patch tracking system

[Pedro Alves]

On 04/29/2014 08:33 PM, Breazeal, Don wrote:
> 
> 
>> -----Original Message-----
>> From: gdb-owner@sourceware.org [mailto:gdb-owner@sourceware.org] On
>> Behalf Of Pedro Alves
>> Sent: Tuesday, April 29, 2014 11:58 AM
>> To: Tom Tromey
>> Cc: Joel Brobecker; Gary Benson; Stan Shebs; gdb@sourceware.org
>> Subject: Re: Patchwork patch tracking system
>>
>> On 04/29/2014 06:07 PM, Tom Tromey wrote:
>>
>>> I've been trying the patchworks install as well.  I don't find it all
>>> that useful myself, but maybe it would be better if more people were
>>> using it.
>>
>> I've been trying it out too.  I've already found it useful to keep track
>> of which of my own patches I have pending.
>>
>> I've been absent a little while from review, but I'm heading back, and
>> I'm using patchwork to guide me.
>>
>> I like that it doesn't make the mailing list a second class citizen.
>> I'd be willing to continue giving it a try, but indeed I think it'd be
>> better if more people were using it.  That'll be true for any tool we
>> end up with.
>>
>> In the past week, I've been cleaning it up whenever I see that patches
>> have been pushed, even those that I didn't approve myself, but of course
>> it'd be better if who approves the patch or the submitter themselves
>> take care of their own patches.
>>
>> non-maintainers shouldn't hold back from creating an account and
>> updating the state of their own patches.  Whatever helps bringing the
>> load down from maintainers should help your own patches.  :-)
>>
>> Here's the current list of who-has-how-many-pending:
>>
>> $ ~/bin/pwclient-hacked list -s New | sort | uniq -c | sort -nr
>>      35 Andy Wingo <wingo@igalia.com>
>>      24 Andreas Arnez <arnez@linux.vnet.ibm.com>
>>      20 Yao Qi <yao@codesourcery.com>
>>      17 Jan Kratochvil <jan.kratochvil@redhat.com>
>>      16 Pedro Alves <palves@redhat.com>
>>      14 Siva Chandra <sivachandra@google.com>
>>      13 Keith Seitz <keiths@redhat.com>
>>      13 David Blaikie <dblaikie@gmail.com>
>>      12 Andrew Burgess <aburgess@broadcom.com>
>>       9 Doug Evans <dje@google.com>
>>       4 Kyle McMartin <kmcmarti@redhat.com>
>>       4 Hui Zhu <hui_zhu@mentor.com>
>>       3 Simon Marchi <simon.marchi@ericsson.com>
>>       3 Eli Zaretskii <eliz@gnu.org>
>>       3 Alan Modra <amodra@gmail.com>
>>       2 Ulrich Weigand <uweigand@de.ibm.com>
>>       2 Mike Frysinger <vapier@gentoo.org>
>>       2 Doug Evans <xdje42@gmail.com>
>>       2 Alexander Smundak <asmundak@google.com>
>>       2 Agovic, Sanimir <sanimir.agovic@intel.com>
>>       1 Vladimir Nikulichev <nvs@tbricks.com>
>>       1 Tom Tromey <tromey@redhat.com>
>>       1 Sandra Loosemore <sandra@codesourcery.com>
>>       1 Pierre Langlois <pierre.langlois@embecosm.com>
>>       1 Nick Clifton <nickc@redhat.com>
>>       1 Mateusz Tabaka <8tab@wp.pl>
>>       1 Mark Wielaard <mjw@redhat.com>
>>       1 Marcus Shawcroft <marcus.shawcroft@arm.com>
>>       1 Marc Khouzam <marc.khouzam@ericsson.com>
>>       1 Maciej W. Rozycki <macro@codesourcery.com>
>>       1 Julian Brown <julian@codesourcery.com>
>>       1 John Marino <gnugcc@marino.st>
>>       1 Gary Benson <gbenson@redhat.com>
>>       1 David Taylor <dtaylor@emc.com>
>>       1 Daniel Gutson <daniel.gutson@tallertechnologies.com>
>>
>> As you see, most of the patches so far, since we began tracking a few
>> weeks back, came from a small set of people.  And I suspect many of
>> those are actually already in.
> 

> A patch series that I posted to gdb-patches at the beginning of April doesn't seem to show up in patchwork.  It would be good to understand why that is and how to fix it.
> https://sourceware.org/ml/gdb-patches/2014-04/msg00037.html
> https://sourceware.org/ml/gdb-patches/2014-04/msg00040.html
> https://sourceware.org/ml/gdb-patches/2014-04/msg00072.html
> https://sourceware.org/ml/gdb-patches/2014-04/msg00071.html

Well, I suspect it's the same reason your patch doesn't show
inline in those urls -- follow the "raw" link and we see:

--_002_DA279C53C4A5884A907135DFCD7A059A0E1D95BDNAMBX02mgcmento_
Content-Type: application/octet-stream; name="0001-remote-exit.patch"
Content-Description: 0001-remote-exit.patch
Content-Disposition: attachment; filename="0001-remote-exit.patch"; size=9688;
	creation-date="Wed, 02 Apr 2014 21:17:10 GMT";
	modification-date="Wed, 02 Apr 2014 21:35:13 GMT"
Content-Transfer-Encoding: base64

You need to either inline the patch in the body of the email,
or make Content-Type be some kind of "text".  For ".patch" files,
that's usually text/x-patch.

Compare with Sandra's, which is also sent as an attachment:

 https://sourceware.org/ml/gdb-patches/2014-03/msg00602.html


I don't have access to patchwork's logs, but looking around current
git mainline patchwork's sources, I see,
in apps/patchwork/bin/parsemail.py:

 150 def find_content(project, mail):
 151     patchbuf = None
 152     commentbuf = ''
 153     pullurl = None
 154
 155     for part in mail.walk():
 156         if part.get_content_maintype() != 'text':
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 157             continue
 158
 159         payload = part.get_payload(decode=True)
 160         charset = part.get_content_charset()
 161         subtype = part.get_content_subtype()
 162
 163         # if we don't have a charset, assume utf-8
 164         if charset is None:
 165             charset = 'utf-8'
 166
 167         if not isinstance(payload, unicode):
 168             payload = unicode(payload, charset)
 169
 170         if subtype in ['x-patch', 'x-diff']:
 171             patchbuf = payload
 172
 173         elif subtype == 'plain':
 174             c = payload
 175
 176             if not patchbuf:
 177                 (patchbuf, c) = parse_patch(payload)
 178
 179             if not pullurl:
 180                 pullurl = find_pull_request(payload)
 181
 182             if c is not None:
 183                 commentbuf += c.strip() + '\n'
 184
 185     patch = None
 186     comment = None
 187
 188     if pullurl or patchbuf:
 189         name = clean_subject(mail.get('Subject'), [project.linkname])
 190         patch = Patch(name = name, pull_url = pullurl, content = patchbuf,
 191                     date = mail_date(mail), headers = mail_headers(mail))
 192
 193     if commentbuf:
 194         if patch:
 195             cpatch = patch
 196         else:
 197             cpatch = find_patch_for_comment(project, mail)
 198             if not cpatch:
 199                 return (None, None)
 200         comment = Comment(patch = cpatch, date = mail_date(mail),
 201                 content = clean_content(commentbuf),
 202                 headers = mail_headers(mail))
 203
 204     return (patch, comment)


So it looks like patchwork just skips your attachments, (rightfully) considering them blobs.

Full source here:

 http://git.ozlabs.org/?p=patchwork;a=blob;f=apps/patchwork/bin/parsemail.py;h=b6eb97ad827a8f499e763dc99c297e2c0b6e4a8f;hb=HEAD

I suggest just using "git send-email" to send patches.  It makes sending
patch series _so_ much easier, it enforces following good practices
commit log practices, and makes sure the receiving end has it easy too -- one
can just save the emails as mbox files (from the mail client, or patchwork's
web frontend -- see e.g., the "mbox" link at http://patchwork.siddhesh.in/patch/660/)
and then simply use "git am" to import the result.  Or using patchwork's
command line tool, do that in one step with "pwclient git-am $patch_id".

-- 
Pedro Alves

Re: Patchwork patch tracking system

[Breazeal, Don]

On 4/29/2014 3:32 PM, Pedro Alves wrote:
> On 04/29/2014 08:33 PM, Breazeal, Don wrote:
>>
>>
>>> -----Original Message-----
>>> From: gdb-owner@sourceware.org [mailto:gdb-owner@sourceware.org] On
>>> Behalf Of Pedro Alves
>>> Sent: Tuesday, April 29, 2014 11:58 AM
>>> To: Tom Tromey
>>> Cc: Joel Brobecker; Gary Benson; Stan Shebs; gdb@sourceware.org
>>> Subject: Re: Patchwork patch tracking system
>>>
>>> On 04/29/2014 06:07 PM, Tom Tromey wrote:
>>>
>>>> I've been trying the patchworks install as well.  I don't find it all
>>>> that useful myself, but maybe it would be better if more people were
>>>> using it.
>>>
>>> I've been trying it out too.  I've already found it useful to keep track
>>> of which of my own patches I have pending.
>>>
>>> I've been absent a little while from review, but I'm heading back, and
>>> I'm using patchwork to guide me.
>>>
>>> I like that it doesn't make the mailing list a second class citizen.
>>> I'd be willing to continue giving it a try, but indeed I think it'd be
>>> better if more people were using it.  That'll be true for any tool we
>>> end up with.
>>>
>>> In the past week, I've been cleaning it up whenever I see that patches
>>> have been pushed, even those that I didn't approve myself, but of course
>>> it'd be better if who approves the patch or the submitter themselves
>>> take care of their own patches.
>>>
>>> non-maintainers shouldn't hold back from creating an account and
>>> updating the state of their own patches.  Whatever helps bringing the
>>> load down from maintainers should help your own patches.  :-)
>>>
>>> Here's the current list of who-has-how-many-pending:
>>>
>>> $ ~/bin/pwclient-hacked list -s New | sort | uniq -c | sort -nr
>>>       35 Andy Wingo <wingo@igalia.com>
>>>       24 Andreas Arnez <arnez@linux.vnet.ibm.com>
>>>       20 Yao Qi <yao@codesourcery.com>
>>>       17 Jan Kratochvil <jan.kratochvil@redhat.com>
>>>       16 Pedro Alves <palves@redhat.com>
>>>       14 Siva Chandra <sivachandra@google.com>
>>>       13 Keith Seitz <keiths@redhat.com>
>>>       13 David Blaikie <dblaikie@gmail.com>
>>>       12 Andrew Burgess <aburgess@broadcom.com>
>>>        9 Doug Evans <dje@google.com>
>>>        4 Kyle McMartin <kmcmarti@redhat.com>
>>>        4 Hui Zhu <hui_zhu@mentor.com>
>>>        3 Simon Marchi <simon.marchi@ericsson.com>
>>>        3 Eli Zaretskii <eliz@gnu.org>
>>>        3 Alan Modra <amodra@gmail.com>
>>>        2 Ulrich Weigand <uweigand@de.ibm.com>
>>>        2 Mike Frysinger <vapier@gentoo.org>
>>>        2 Doug Evans <xdje42@gmail.com>
>>>        2 Alexander Smundak <asmundak@google.com>
>>>        2 Agovic, Sanimir <sanimir.agovic@intel.com>
>>>        1 Vladimir Nikulichev <nvs@tbricks.com>
>>>        1 Tom Tromey <tromey@redhat.com>
>>>        1 Sandra Loosemore <sandra@codesourcery.com>
>>>        1 Pierre Langlois <pierre.langlois@embecosm.com>
>>>        1 Nick Clifton <nickc@redhat.com>
>>>        1 Mateusz Tabaka <8tab@wp.pl>
>>>        1 Mark Wielaard <mjw@redhat.com>
>>>        1 Marcus Shawcroft <marcus.shawcroft@arm.com>
>>>        1 Marc Khouzam <marc.khouzam@ericsson.com>
>>>        1 Maciej W. Rozycki <macro@codesourcery.com>
>>>        1 Julian Brown <julian@codesourcery.com>
>>>        1 John Marino <gnugcc@marino.st>
>>>        1 Gary Benson <gbenson@redhat.com>
>>>        1 David Taylor <dtaylor@emc.com>
>>>        1 Daniel Gutson <daniel.gutson@tallertechnologies.com>
>>>
>>> As you see, most of the patches so far, since we began tracking a few
>>> weeks back, came from a small set of people.  And I suspect many of
>>> those are actually already in.
>>
>
>> A patch series that I posted to gdb-patches at the beginning of April doesn't seem to show up in patchwork.  It would be good to understand why that is and how to fix it.
>> https://sourceware.org/ml/gdb-patches/2014-04/msg00037.html
>> https://sourceware.org/ml/gdb-patches/2014-04/msg00040.html
>> https://sourceware.org/ml/gdb-patches/2014-04/msg00072.html
>> https://sourceware.org/ml/gdb-patches/2014-04/msg00071.html
>
> Well, I suspect it's the same reason your patch doesn't show
> inline in those urls -- follow the "raw" link and we see:
>
> --_002_DA279C53C4A5884A907135DFCD7A059A0E1D95BDNAMBX02mgcmento_
> Content-Type: application/octet-stream; name="0001-remote-exit.patch"
> Content-Description: 0001-remote-exit.patch
> Content-Disposition: attachment; filename="0001-remote-exit.patch"; size=9688;
> 	creation-date="Wed, 02 Apr 2014 21:17:10 GMT";
> 	modification-date="Wed, 02 Apr 2014 21:35:13 GMT"
> Content-Transfer-Encoding: base64
>
> You need to either inline the patch in the body of the email,
> or make Content-Type be some kind of "text".  For ".patch" files,
> that's usually text/x-patch.
>
> Compare with Sandra's, which is also sent as an attachment:
>
>   https://sourceware.org/ml/gdb-patches/2014-03/msg00602.html
>
>
> I don't have access to patchwork's logs, but looking around current
> git mainline patchwork's sources, I see,
> in apps/patchwork/bin/parsemail.py:
>
>   150 def find_content(project, mail):
>   151     patchbuf = None
>   152     commentbuf = ''
>   153     pullurl = None
>   154
>   155     for part in mail.walk():
>   156         if part.get_content_maintype() != 'text':
>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   157             continue
>   158
>   159         payload = part.get_payload(decode=True)
>   160         charset = part.get_content_charset()
>   161         subtype = part.get_content_subtype()
>   162
>   163         # if we don't have a charset, assume utf-8
>   164         if charset is None:
>   165             charset = 'utf-8'
>   166
>   167         if not isinstance(payload, unicode):
>   168             payload = unicode(payload, charset)
>   169
>   170         if subtype in ['x-patch', 'x-diff']:
>   171             patchbuf = payload
>   172
>   173         elif subtype == 'plain':
>   174             c = payload
>   175
>   176             if not patchbuf:
>   177                 (patchbuf, c) = parse_patch(payload)
>   178
>   179             if not pullurl:
>   180                 pullurl = find_pull_request(payload)
>   181
>   182             if c is not None:
>   183                 commentbuf += c.strip() + '\n'
>   184
>   185     patch = None
>   186     comment = None
>   187
>   188     if pullurl or patchbuf:
>   189         name = clean_subject(mail.get('Subject'), [project.linkname])
>   190         patch = Patch(name = name, pull_url = pullurl, content = patchbuf,
>   191                     date = mail_date(mail), headers = mail_headers(mail))
>   192
>   193     if commentbuf:
>   194         if patch:
>   195             cpatch = patch
>   196         else:
>   197             cpatch = find_patch_for_comment(project, mail)
>   198             if not cpatch:
>   199                 return (None, None)
>   200         comment = Comment(patch = cpatch, date = mail_date(mail),
>   201                 content = clean_content(commentbuf),
>   202                 headers = mail_headers(mail))
>   203
>   204     return (patch, comment)
>
>
> So it looks like patchwork just skips your attachments, (rightfully) considering them blobs.
>
> Full source here:
>
>   http://git.ozlabs.org/?p=patchwork;a=blob;f=apps/patchwork/bin/parsemail.py;h=b6eb97ad827a8f499e763dc99c297e2c0b6e4a8f;hb=HEAD
>
> I suggest just using "git send-email" to send patches.  It makes sending
> patch series _so_ much easier, it enforces following good practices
> commit log practices, and makes sure the receiving end has it easy too -- one
> can just save the emails as mbox files (from the mail client, or patchwork's
> web frontend -- see e.g., the "mbox" link at http://patchwork.siddhesh.in/patch/660/)
> and then simply use "git am" to import the result.  Or using patchwork's
> command line tool, do that in one step with "pwclient git-am $patch_id".
>
Thanks Pedro.  I'll re-post, following your suggestions.
--Don

Re: Patchwork patch tracking system

[Gary Benson]

Joel Brobecker wrote:
> > Maybe phabricator is better for our needs?
> 
> I have to say that phabricator looks like a pretty nice tool.
> I'd be happy trying it out as an experiment.

I started installing phabricator on a local machine, but various
things distracted me.  I planned to pick it up again later this
week after this discussion, but I'd be happy to help/collaborate/
bounce ideas with you somewhere more public if you like.

Cheers,
Gary

Re: Patchwork patch tracking system

[Joel Brobecker]

> I started installing phabricator on a local machine, but various
> things distracted me.  I planned to pick it up again later this
> week after this discussion, but I'd be happy to help/collaborate/
> bounce ideas with you somewhere more public if you like.

Thanks for doing that!

Any way works for me. I'd like to give it a try over patchwork, because
phabricator looks a lot more exiting that patchwork. I know it sounds
like it's putting the shine ahead of functionality, but if it does
the job and isn't too much of a maintenance issue, I think we'll have
an easier time getting people to adopt it. And regardless of what we
end up choosing in the end, it'll be an interesting experiment.

-- 
Joel

Re: Patchwork patch tracking system

[Tom Tromey]

Tom> I've been trying the patchworks install as well.  I don't find it all
Tom> that useful myself, but maybe it would be better if more people were
Tom> using it.

Let me walk that back a little.

I've been trying it again and it is useful.  I've been using it every
day.

At the very least it gives us a global list of what has not been
reviewed.  And, the patchwork command-line client seems quite nice;
"pwclient git-am" seems truly useful, though I haven't had cause to use
it yet.

A few things would make it nicer for use with gdb:

1. If it knew about patch series.  Right now it drops (the usually quite
   useful) "patch 0" mail and treats each patch as a separate entity.

   I looked at the patchwork mailing list and while this topic has come
   up, nobody has implemented the needed features.

2. Building on #1, if it could tell when a patch series obsoletes an
   older series.

   I think this would work best with patch series support, because it's
   common when resubmitting for patches to change their subject and
   otherwise be "untrackable", whereas we could easily adopt a
   convention that the new series have the same title for the cover
   letter.

3. If it ignored "FYI" patches.  This step could be applied after #2 so
   that the final courtesy copy would zap the old series from the UI.

4. If we ran the existing patchwork automatic zapper regularly so that
   commits could remove patches from the UI.


One final thing that would be useful is if a mention of a PR in a
gdb-patches mail caused some note to be posted to Bugzilla -- ideally a
link to the email in our archives, but a link to the appropriate entry
in patchwork would be an acceptable substitute.

Tom

Re: Patchwork patch tracking system

[Pedro Alves]

On 05/21/2014 03:47 PM, Tom Tromey wrote:
> Tom> I've been trying the patchworks install as well.  I don't find it all
> Tom> that useful myself, but maybe it would be better if more people were
> Tom> using it.
> 
> Let me walk that back a little.
> 
> I've been trying it again and it is useful.  I've been using it every
> day.

Just to reinforce, in case people wonder whether previous users
backed out -- I still use it, and find it useful.

One useful thing even if you're not interested in others' patches
is getting the list of your own patches that are pending.
For example, I get mine here:

 http://patchwork.siddhesh.in/project/binutils-gdb/list/?submitter=37

(I just have that bookmarked)

It's easy to find one's URL, by hitting the "Filters" link at the
top of the patches table, and filtering by submitter.  That'll
take you to such an url.

> 2. Building on #1, if it could tell when a patch series obsoletes an
>    older series.

Obviously, even for single patches.  But I think any patch tracking
system will require some sort of action to have a patch obsolete some
other as opposed to adding more on top.

>    I think this would work best with patch series support, because it's
>    common when resubmitting for patches to change their subject and
>    otherwise be "untrackable", whereas we could easily adopt a
>    convention that the new series have the same title for the cover
>    letter.

Yeah.  I think this would be a nice to have, but if people (at least
frequent submitters) care for their own patches, then this really
isn't a big problem.

> 3. If it ignored "FYI" patches.  This step could be applied after #2 so
>    that the final courtesy copy would zap the old series from the UI.

I know you know this, but for others -- I'm getting around this by
running this script once in a while:

pwclient list -s New |
grep -i "\[.*commit.*\]\|\[.*pushed.*\]\|\[.*fyi.*\]" |
cut -f 1 -d ' ' |
while read patch; do
printf "updating %d: " $patch
    pwclient info $patch | grep " name *:" | sed 's/^- name *: //'
    pwclient update -s Accepted $patch
done

That uses the command line client to "Accept" "New" patches
patches that have [commit], [commit] or [fyi] in its subject.

You can find it at:

 https://github.com/palves/misc/blob/master/patchwork/pwupdate-already-pushed


> 4. If we ran the existing patchwork automatic zapper regularly so that
>    commits could remove patches from the UI.

I'm not certain that would work for us, unless we change some process.
I am under the impression that that matches commits by git hash.
The fact that the ChangeLog is usually not a part of the patch
and then is added before push changes the hash.  Also,
given we only allow fast-forward, it's very frequent that patches
need to be rebased when pushed, which changes hash as well.

-- 
Pedro Alves

Re: Patchwork patch tracking system

[Joel Brobecker]

> I'm not certain that would work for us, unless we change some process.
> I am under the impression that that matches commits by git hash.
> The fact that the ChangeLog is usually not a part of the patch
> and then is added before push changes the hash.  Also,
> given we only allow fast-forward, it's very frequent that patches
> need to be rebased when pushed, which changes hash as well.

I think we should be open to the idea of changing our process to
some degree. To me, keeping track of patch series, in terms of
grouping as well as iterations of the patch series, would be
a great feature worth some changes in our procedures. To go
even further, I am not necessarily attached to making email-based
interaction the primary way of discussing an email. I know many
people are, and I am not advocating for one way or the other, but
just trying to show that this isn't an obvious requirement to me.

-- 
Joel

Re: Patchwork patch tracking system

[Eli Zaretskii]

> Date: Wed, 21 May 2014 18:18:06 +0100
> From: Pedro Alves <palves@redhat.com>
> CC: Gary Benson <gbenson@redhat.com>, Stan Shebs <stanshebs@earthlink.net>,        gdb@sourceware.org
> 
> The fact that the ChangeLog is usually not a part of the patch
> and then is added before push changes the hash.

But that's just for historical reasons, right?  If we use
git-changelog-merge, there should be no reason not to make ChangeLog
changes part of the patch, right?

> given we only allow fast-forward, it's very frequent that patches
> need to be rebased when pushed, which changes hash as well.

Does this mean I cannot push after merging from my local branch into
master in my repository?  IOW, if I have local feature branches, do I
have to rebase commits from those branches before pushing them?

Re: Patchwork patch tracking system

[Joel Brobecker]

> Does this mean I cannot push after merging from my local branch into
> master in my repository?  IOW, if I have local feature branches, do I
> have to rebase commits from those branches before pushing them?

We do not have any hook that would reject such a push, but we would
_really_ like you to (this is going back to a discussion we had
about allowing merge commits or not).

-- 
Joel

Re: Patchwork patch tracking system

[Eli Zaretskii]

> Date: Wed, 21 May 2014 11:23:49 -0700
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Pedro Alves <palves@redhat.com>, tromey@redhat.com, gbenson@redhat.com,
> 	stanshebs@earthlink.net, gdb@sourceware.org
> 
> > Does this mean I cannot push after merging from my local branch into
> > master in my repository?  IOW, if I have local feature branches, do I
> > have to rebase commits from those branches before pushing them?
> 
> We do not have any hook that would reject such a push, but we would
> _really_ like you to

Too bad.

> (this is going back to a discussion we had about allowing merge
> commits or not).

AFAIR, that discussion was about merging from the (public) release
branch to master.  Now I asked about my private branches, which is
slightly different.

Re: Patchwork patch tracking system

[Joel Brobecker]

> > (this is going back to a discussion we had about allowing merge
> > commits or not).
> 
> AFAIR, that discussion was about merging from the (public) release
> branch to master.  Now I asked about my private branches, which is
> slightly different.

The thing is that there is no real difference between pushing
a merge of a public branch, and a merge of a private branch.
We'll still end up with a merge commit in the public repository.

-- 
Joel

Re: Patchwork patch tracking system

[Pedro Alves]

On 05/21/2014 07:03 PM, Eli Zaretskii wrote:
>> Date: Wed, 21 May 2014 18:18:06 +0100
>> From: Pedro Alves <palves@redhat.com>
>> CC: Gary Benson <gbenson@redhat.com>, Stan Shebs <stanshebs@earthlink.net>,        gdb@sourceware.org
>>
>> The fact that the ChangeLog is usually not a part of the patch
>> and then is added before push changes the hash.
> 
> But that's just for historical reasons, right?  If we use
> git-changelog-merge, there should be no reason not to make ChangeLog
> changes part of the patch, right?

Right, but still, but if that merge actually changes the patch,
because it puts the ChangeLog entry at the top of the file, and
the top commit is no longer the same as was when the patch was
generated for submission (frequently true), then the hash changes.

-- 
Pedro Alves

Re: Patchwork patch tracking system

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1744 bytes --]

On Tue 22 Apr 2014 11:37:37 Siva Chandra wrote:
> On Tue, Apr 22, 2014 at 8:51 AM, Joel Brobecker wrote:
> >> FWIW we (some of the google folk) looked at geritt for LLVM and
> >> discarded in favor of phabricator. It seemed to solve a lot of the
> >> problems that we had and allowed communication to and from the mailing
> >> lists for patches which was key for us as we have a similar review
> >> style to gcc/gdb/binutils. We didn't want to remove the ability for
> >> people to send patches to the mailing lists, but yet get a better
> >> review mechanism for large patches/queuing/etc.
> > 
> > Thanks for the suggestion! I have to say, from the outside,
> > phabricator looks like a pretty interesting option.
> 
> I am essentially an outsider here, and I have not used phabricator.
> However, the Chromium project uses Rietveld for codereview:
> https://code.google.com/p/rietveld/ and
> https://codereview.chromium.org/
> 
> IIRC, Diego Novillo setup a rietveld instance for GCC:
> http://gcc.gnu.org/wiki/rietveld

rietveld is terrible imo ;).  it has a lot of custom tooling and has 
historically been svn-centric, and it isn't as well maintained as gerrit.

> Chromium-OS project uses gerrit and hence I have used both these tools
> and my personal choice is rietveld over gerrit by many a mile.
> Rietveld can be configured to send mails to a watchlist for every
> patch sent out for review and also for every review posting. Hence,
> all review logs can still be saved in a mailing list archive.

the new gerrit-on-borg is much nicer than the old, but it still lacks mailing 
list integration ... could post a feature request, but i'm not sure 
realistically it'd be solved in a timely manner
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]