Discussion:
Patchwork patch tracking system
(too old to reply)
Gary Benson
2014-04-02 10:08:42 UTC
Permalink
Raw Message
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/
Stan Shebs
2014-04-04 22:49:55 UTC
Permalink
Raw Message
Post by 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.
I just heard about this last week, and it looks like a good idea!
Post by Gary Benson
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"?
Post by Gary Benson
Let me know what you think!
Thanks for setting this up!

Stan
***@codesourcery.com
Gary Benson
2014-04-17 13:50:40 UTC
Permalink
Raw Message
Hi Stan,

Sorry for the late reply, your message got lost in a bunch of
demangler issues :(
Post by Stan Shebs
Post by Gary Benson
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!
Post by Gary Benson
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.
Post by Stan Shebs
Post by Gary Benson
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/
Joel Brobecker
2014-04-22 13:06:52 UTC
Permalink
Raw Message
Post by Gary Benson
Post by Stan Shebs
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
Eric Christopher
2014-04-22 15:40:17 UTC
Permalink
Raw Message
Post by Joel Brobecker
Post by Gary Benson
Post by Stan Shebs
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
Joel Brobecker
2014-04-22 15:51:32 UTC
Permalink
Raw Message
Post by Eric Christopher
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
Siva Chandra
2014-04-22 18:37:37 UTC
Permalink
Raw Message
Post by Joel Brobecker
Post by Eric Christopher
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
Mike Frysinger
2014-08-02 14:25:53 UTC
Permalink
Raw Message
Post by Siva Chandra
Post by Joel Brobecker
Post by Eric Christopher
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.
https://code.google.com/p/rietveld/ and
https://codereview.chromium.org/
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.
Post by Siva Chandra
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

Tom Tromey
2014-04-29 17:07:32 UTC
Permalink
Raw Message
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
Eli Zaretskii
2014-04-29 17:25:42 UTC
Permalink
Raw Message
Date: Tue, 29 Apr 2014 11:07:32 -0600
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.
Joel Brobecker
2014-04-29 17:36:29 UTC
Permalink
Raw Message
Post by Tom Tromey
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
Gary Benson
2014-04-30 08:44:03 UTC
Permalink
Raw Message
Post by Joel Brobecker
Post by Tom Tromey
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
Joel Brobecker
2014-04-30 12:20:23 UTC
Permalink
Raw Message
Post by Gary Benson
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
Pedro Alves
2014-04-29 18:57:59 UTC
Permalink
Raw Message
Post by Tom Tromey
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 <***@igalia.com>
24 Andreas Arnez <***@linux.vnet.ibm.com>
20 Yao Qi <***@codesourcery.com>
17 Jan Kratochvil <***@redhat.com>
16 Pedro Alves <***@redhat.com>
14 Siva Chandra <***@google.com>
13 Keith Seitz <***@redhat.com>
13 David Blaikie <***@gmail.com>
12 Andrew Burgess <***@broadcom.com>
9 Doug Evans <***@google.com>
4 Kyle McMartin <***@redhat.com>
4 Hui Zhu <***@mentor.com>
3 Simon Marchi <***@ericsson.com>
3 Eli Zaretskii <***@gnu.org>
3 Alan Modra <***@gmail.com>
2 Ulrich Weigand <***@de.ibm.com>
2 Mike Frysinger <***@gentoo.org>
2 Doug Evans <***@gmail.com>
2 Alexander Smundak <***@google.com>
2 Agovic, Sanimir <***@intel.com>
1 Vladimir Nikulichev <***@tbricks.com>
1 Tom Tromey <***@redhat.com>
1 Sandra Loosemore <***@codesourcery.com>
1 Pierre Langlois <***@embecosm.com>
1 Nick Clifton <***@redhat.com>
1 Mateusz Tabaka <***@wp.pl>
1 Mark Wielaard <***@redhat.com>
1 Marcus Shawcroft <***@arm.com>
1 Marc Khouzam <***@ericsson.com>
1 Maciej W. Rozycki <***@codesourcery.com>
1 Julian Brown <***@codesourcery.com>
1 John Marino <***@marino.st>
1 Gary Benson <***@redhat.com>
1 David Taylor <***@emc.com>
1 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
Breazeal, Don
2014-04-29 19:33:04 UTC
Permalink
Raw Message
-----Original Message-----
Behalf Of Pedro Alves
Sent: Tuesday, April 29, 2014 11:58 AM
To: Tom Tromey
Subject: Re: Patchwork patch tracking system
Post by Tom Tromey
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. :-)
$ ~/bin/pwclient-hacked list -s New | sort | uniq -c | sort -nr
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
Pedro Alves
2014-04-29 22:32:26 UTC
Permalink
Raw Message
Post by Breazeal, Don
-----Original Message-----
Behalf Of Pedro Alves
Sent: Tuesday, April 29, 2014 11:58 AM
To: Tom Tromey
Subject: Re: Patchwork patch tracking system
Post by Tom Tromey
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. :-)
$ ~/bin/pwclient-hacked list -s New | sort | uniq -c | sort -nr
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
Breazeal, Don
2014-04-30 01:08:25 UTC
Permalink
Raw Message
Post by Pedro Alves
Post by Breazeal, Don
-----Original Message-----
Behalf Of Pedro Alves
Sent: Tuesday, April 29, 2014 11:58 AM
To: Tom Tromey
Subject: Re: Patchwork patch tracking system
Post by Tom Tromey
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. :-)
$ ~/bin/pwclient-hacked list -s New | sort | uniq -c | sort -nr
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
--_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.
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,
151 patchbuf = None
152 commentbuf = ''
153 pullurl = None
154
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
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
165 charset = 'utf-8'
166
168 payload = unicode(payload, charset)
169
171 patchbuf = payload
172
174 c = payload
175
177 (patchbuf, c) = parse_patch(payload)
178
180 pullurl = find_pull_request(payload)
181
183 commentbuf += c.strip() + '\n'
184
185 patch = None
186 comment = None
187
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
195 cpatch = patch
197 cpatch = find_patch_for_comment(project, mail)
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.
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
Tom Tromey
2014-05-21 14:47:44 UTC
Permalink
Raw Message
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
Pedro Alves
2014-05-21 17:18:06 UTC
Permalink
Raw Message
Post by 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.
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.
Post by Tom Tromey
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.
Post by Tom Tromey
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.
Post by Tom Tromey
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
Post by Tom Tromey
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
Joel Brobecker
2014-05-21 17:49:08 UTC
Permalink
Raw Message
Post by Pedro Alves
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
Eli Zaretskii
2014-05-21 18:03:14 UTC
Permalink
Raw Message
Date: Wed, 21 May 2014 18:18:06 +0100
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?
Joel Brobecker
2014-05-21 18:23:49 UTC
Permalink
Raw Message
Post by Eli Zaretskii
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
Eli Zaretskii
2014-05-21 18:48:26 UTC
Permalink
Raw Message
Date: Wed, 21 May 2014 11:23:49 -0700
Post by Eli Zaretskii
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.
Joel Brobecker
2014-05-21 19:45:43 UTC
Permalink
Raw Message
Post by Eli Zaretskii
(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
Pedro Alves
2014-05-21 21:35:04 UTC
Permalink
Raw Message
Post by Eli Zaretskii
Date: Wed, 21 May 2014 18:18:06 +0100
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
Loading...