Patch review

Gregory Stark wrote:

so.... I went to look for the held patches queue to start reviewing patches.
There are over 2000 messages in the queue in 300 separate threads. At that
rate it would probably be just as easy to scan the patches and hackers mailing
list.

Pretty unwieldy, yes. I'm not sure -patchers of -hackers is really
"just as easy" though.

Is someone working on dumping the list into a table on the wiki? I could
download the mbox files from the web site and filter them into a table.

Some part of me thinks this data should be in a postgres database so I can do
SQL queries against it to find a good patch to review.

It's hard to put this stuff in a database. Truth is that it's highly
unstructured.

IMHO an mbox is not the right interface either, though. I guess there
must be something in the middle, like a *cough*patch manager*cough*. At
least there should be a way to mark patches: a "is this a patch" (or
merely discussion) boolean; and a free-form field where other people can
make comments. Well, I guess that's what Review Board is for. I think
we should start asking patch submitters to load their patches on RB.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

On Fri, Feb 08, 2008 at 12:22:08PM -0300, Alvaro Herrera wrote:

Gregory Stark wrote:

Is someone working on dumping the list into a table on the wiki? I could
download the mbox files from the web site and filter them into a table.

Some part of me thinks this data should be in a postgres database so I can do
SQL queries against it to find a good patch to review.

It's hard to put this stuff in a database. Truth is that it's highly
unstructured.

IMHO an mbox is not the right interface either, though. I guess there
must be something in the middle, like a *cough*patch manager*cough*. At
least there should be a way to mark patches: a "is this a patch" (or
merely discussion) boolean; and a free-form field where other people can
make comments. Well, I guess that's what Review Board is for. I think
we should start asking patch submitters to load their patches on RB.

I agree with the need of some kind of patch tracking thats nicer than what
we have now, but let's at least try out the product all the way before we
ask people to use it! :-)

//Magnus

Alvaro Herrera wrote:

Gregory Stark wrote:

so.... I went to look for the held patches queue to start reviewing patches.
There are over 2000 messages in the queue in 300 separate threads. At that
rate it would probably be just as easy to scan the patches and hackers mailing
list.

Pretty unwieldy, yes. I'm not sure -patchers of -hackers is really
"just as easy" though.

Is someone working on dumping the list into a table on the wiki? I could
download the mbox files from the web site and filter them into a table.

Some part of me thinks this data should be in a postgres database so I can do
SQL queries against it to find a good patch to review.

It's hard to put this stuff in a database. Truth is that it's highly
unstructured.

Yep, _unstructured_ is the word for it.

We have a commit-fest coming March 1 I think so I will have the hold
patch queue cleaned up by then and only valid items will be left there.

IMHO an mbox is not the right interface either, though. I guess there
must be something in the middle, like a *cough*patch manager*cough*. At
least there should be a way to mark patches: a "is this a patch" (or
merely discussion) boolean; and a free-form field where other people can
make comments. Well, I guess that's what Review Board is for. I think
we should start asking patch submitters to load their patches on RB.

Frankly I think the structuring of the data is the hard part. For 8.3
we had that web page that tracked the outstanding patches and that was
very useful because the patches were addressed over a 4-5 month period.

Ideally we could have that status for all patches all the time, but the
time required to structure/categorize them often isn't worth it. We
could have the submitters do the categorizing or the patch appliers, but
in many cases the structuring is more work than just getting the patch
applied and completed.

If you think it would be easy to get patch submitters to categorize,
realize we have some very skilled people who don't even send email
reports of bugs, they just report them on IRC and can't be bothered with
email. If email is a burden for them, imagine filling in a web form.

If patch appliers categorize, why would then do that if they are just
going to go an apply the patch and complete it?

Perhaps we can have two ways of submitting patches --- one that
categorizes and another that is more freeform via email? Patch appliers
would have to deal with both.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce@momjian.us> writes:

If you think it would be easy to get patch submitters to categorize,
realize we have some very skilled people who don't even send email
reports of bugs, they just report them on IRC and can't be bothered with
email. If email is a burden for them, imagine filling in a web form.
...
Perhaps we can have two ways of submitting patches --- one that
categorizes and another that is more freeform via email? Patch appliers
would have to deal with both.

Let's not swat flies with steam hammers. There are various sizes of
problem here, and they need different sizes of solution. In particular,
we see a lot of bugs that can be fixed-the-same-day, and those shouldn't
get into the patch queue mechanism at all. It's the stuff that needs
some extended thought and discussion that needs tracking.

regards, tom lane

Bruce Momjian wrote:

IMHO an mbox is not the right interface either, though. I guess there
must be something in the middle, like a *cough*patch manager*cough*. At
least there should be a way to mark patches: a "is this a patch" (or
merely discussion) boolean; and a free-form field where other people can
make comments. Well, I guess that's what Review Board is for. I think
we should start asking patch submitters to load their patches on RB.

Not sure if this is platform-specific, but I keep being annoyed by not
being able to actually *view* the patches in the queue. I have to
download them, and then open them separately. I can't just view them in
the browser, because they're all named ".bin" and come out as mime type
"application/octet-stream".
(That particular problem would be fixed if they were accessible through
for example IMAP)

Frankly I think the structuring of the data is the hard part. For 8.3
we had that web page that tracked the outstanding patches and that was
very useful because the patches were addressed over a 4-5 month period.

As Tom already said, we need to differ between the long-review patches
and the quick fixes. The quick fixes work fine the way we do it now, the
more advanced long-review patches don't really.

Ideally we could have that status for all patches all the time, but the
time required to structure/categorize them often isn't worth it. We
could have the submitters do the categorizing or the patch appliers, but
in many cases the structuring is more work than just getting the patch
applied and completed.

It may not be worth it for the quick-fixes, but it certainly is for the
longer ones. The wiki page was very useful.

It would also be better to be able to off-load it to more than one
person. For example, I would like to be able to get into the unapplied
patches list and remove the email about events on 8.3RC1. First of all,
it's not a patch, but it's listed under it. But more importantly, it has
been fixed and should just be removed. So I now have to email you to ask
you to remove it, and then you have to do the actual work, which means
double work.

In fact, those two mbox archives need to be renamed - they're not
actually patches, from what I can tell, they're more "patches and
discussions that appear interesting for this release", no?

If you think it would be easy to get patch submitters to categorize,
realize we have some very skilled people who don't even send email
reports of bugs, they just report them on IRC and can't be bothered with
email. If email is a burden for them, imagine filling in a web form.

A whole lot of people find it much easier, and less of a burden, to
submit something like this through a web interface, than through an
email. For several reasons, one being that almost everything else they
do "on the net" is through web interfaces of different kinds. Another
being that they don't have to subscribe to anything, they can just post it.

//Magnus

Magnus Hagander <magnus@hagander.net> writes:

Not sure if this is platform-specific, but I keep being annoyed by not
being able to actually *view* the patches in the queue. I have to
download them, and then open them separately. I can't just view them in
the browser, because they're all named ".bin" and come out as mime type
"application/octet-stream".

Yeah, it happens that way for me too. The other huge, huge problem with
it is the lack of stability of URLs for the items in the list, which
makes it difficult to identify which item you're talking about.

Personally I'd be happier with an editable wiki page consisting of links
to the original messages in the mail list archives, plus free-format
annotations (such as status). This should be trivial to set up and
reasonably flexible. With experience we might find we need something
fancier, but let's not overdesign our solution at the start.

regards, tom lane

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

Not sure if this is platform-specific, but I keep being annoyed by not
being able to actually *view* the patches in the queue. I have to
download them, and then open them separately. I can't just view them in
the browser, because they're all named ".bin" and come out as mime type
"application/octet-stream".

Yeah, it happens that way for me too. The other huge, huge problem with
it is the lack of stability of URLs for the items in the list, which
makes it difficult to identify which item you're talking about.

yeah that is fairly annoying :-(

Personally I'd be happier with an editable wiki page consisting of links
to the original messages in the mail list archives, plus free-format
annotations (such as status). This should be trivial to set up and
reasonably flexible. With experience we might find we need something
fancier, but let's not overdesign our solution at the start.

I will see if I can come up with a proposal on the developer wiki for a
list that looks like that tomorrow.

_IF_ we are looking for an (over)designed solution I just resurrected
what was left over from the last years discussion on that topic (and the
creating of the now defunct tracker mailinglist).
What was discussed back then was a set of requirements for a
(bug|feature) tracker and based on that I hacked up a prototype using a
modified/enhanced version of bugzilla 3.0.

some of the discussions/criterias from back than are summarized on:

http://developer.postgresql.org/index.php/TrackerDiscussion

the demo installation itself is on:

http://trackerdemo.postgresql.org

with a modified bug submission form on

http://pgweb.kaltenbrunner.cc/support/submitbug

basic concepts and docs are on the wiki too.

Stefan

"Stefan Kaltenbrunner" <stefan@kaltenbrunner.cc> writes:

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

Not sure if this is platform-specific, but I keep being annoyed by not being
able to actually *view* the patches in the queue. I have to download them,
and then open them separately. I can't just view them in the browser,
because they're all named ".bin" and come out as mime type
"application/octet-stream".

Yeah, it happens that way for me too. The other huge, huge problem with
it is the lack of stability of URLs for the items in the list, which
makes it difficult to identify which item you're talking about.

yeah that is fairly annoying :-(

Strangely enough I have the opposite complaint. Sometimes the patch is
displayed inline (seems to be pretty unpredictable) and that makes it hard to
download and apply. If you download the page you have to un-html-escape it and
if you copy-paste the contents the whitespace is messed up.

It would be nice if the patches list were fed into reviewboard or viewcvs or
something like it. Even if we only used it for viewing patches it would at
least make that nice.

I couldn't see how to make it work with reviewboard at first glance. It seems
to assume you're starting form a working tree and wants to generate the diff
and send it in itself. If you generate it yourself then you have to tell it
precisely what it's a diff against. It doesn't seem to expect to be used as a
general purpose patch viewer.

Personally I'd be happier with an editable wiki page consisting of links
to the original messages in the mail list archives, plus free-format
annotations (such as status). This should be trivial to set up and
reasonably flexible. With experience we might find we need something
fancier, but let's not overdesign our solution at the start.

I will see if I can come up with a proposal on the developer wiki for a list
that looks like that tomorrow.

How powerful are these wiki things? Can you write a script which automatically
inserts a new line in a table in the wiki whenever an email to the patches
list containing a patch comes in?

Can you add buttons on the wiki itself to do common actions like removing the
item from the table or joining it up with another item? That way we would only
have to actually edit the wiki content if we have something substantive to add
such as review comments.

_IF_ we are looking for an (over)designed solution

I'm against overdesigned solutions. If we want more than a wiki I think it
would be reviewboard. I think something which fills a specific hole like
reviewboard is much more likely to work in the long term than an overarching
solution whose problem description is as vague as a "tracker". As soon as our
process changes the "tracker" will be obsolete whereas a problem-specific
solution like reviewboard will always be useful as long as there are patches
to review.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Get trained by Bruce Momjian - ask me about EnterpriseDB's PostgreSQL training!

Gregory Stark wrote:

"Stefan Kaltenbrunner" <stefan@kaltenbrunner.cc> writes:

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

Not sure if this is platform-specific, but I keep being annoyed by not being
able to actually *view* the patches in the queue. I have to download them,
and then open them separately. I can't just view them in the browser,
because they're all named ".bin" and come out as mime type
"application/octet-stream".

Yeah, it happens that way for me too. The other huge, huge problem with
it is the lack of stability of URLs for the items in the list, which
makes it difficult to identify which item you're talking about.

yeah that is fairly annoying :-(

Strangely enough I have the opposite complaint. Sometimes the patch is
displayed inline (seems to be pretty unpredictable) and that makes it hard to
download and apply. If you download the page you have to un-html-escape it and
if you copy-paste the contents the whitespace is messed up.

It would be nice if the patches list were fed into reviewboard or viewcvs or
something like it. Even if we only used it for viewing patches it would at
least make that nice.

hmm that could be made to work but why not submit them to reviewboard in
the first place and let it send an mail to the list ?

I couldn't see how to make it work with reviewboard at first glance. It seems
to assume you're starting form a working tree and wants to generate the diff
and send it in itself. If you generate it yourself then you have to tell it
precisely what it's a diff against. It doesn't seem to expect to be used as a
general purpose patch viewer.

yeah - I think that dave was looking into checking if something like
that would be possible.

Personally I'd be happier with an editable wiki page consisting of links
to the original messages in the mail list archives, plus free-format
annotations (such as status). This should be trivial to set up and
reasonably flexible. With experience we might find we need something
fancier, but let's not overdesign our solution at the start.

I will see if I can come up with a proposal on the developer wiki for a list
that looks like that tomorrow.

How powerful are these wiki things? Can you write a script which automatically
inserts a new line in a table in the wiki whenever an email to the patches
list containing a patch comes in?

yes

Can you add buttons on the wiki itself to do common actions like removing the
item from the table or joining it up with another item? That way we would only
have to actually edit the wiki content if we have something substantive to add
such as review comments.

not sure on that one

_IF_ we are looking for an (over)designed solution

I'm against overdesigned solutions. If we want more than a wiki I think it
would be reviewboard. I think something which fills a specific hole like
reviewboard is much more likely to work in the long term than an overarching
solution whose problem description is as vague as a "tracker". As soon as our
process changes the "tracker" will be obsolete whereas a problem-specific
solution like reviewboard will always be useful as long as there are patches
to review.

well the trackerdemo installation could do something very similiar to
what you are describing (I only considered that process for -bugs up to
know but it could be adapted to patches I guess).

* change the bugreporting for to one that posts to the tracker and have
it generate the (bug)id
* subscribe the tracker to the list (say pgsql-bugs)
* have people discuss stuff on either the list or via the GUI of the
tracker - the tracker will be able automatically track the mails and add
them as comments to the "bug" or not
* have that tracker generate a short summary report based on "open"
issues on either the wiki (plugins are available for that) or use one of
the integrated reporting features

Has anybody actually looked into how suitable reviewboard is for
reviewing large patches ? it seems to have some limitations based on the
observation that patches are only commented on but are not expected to
be changed/modified by the reviewer (in fact I think the reviewer cannot
even upload a modified version of the patch but I could be wrong there).

Stefan

Stefan Kaltenbrunner wrote:

Tom Lane wrote:

[...]

Personally I'd be happier with an editable wiki page consisting of links
to the original messages in the mail list archives, plus free-format
annotations (such as status). This should be trivial to set up and
reasonably flexible. With experience we might find we need something
fancier, but let's not overdesign our solution at the start.

I will see if I can come up with a proposal on the developer wiki for a
list that looks like that tomorrow.

I have simply converted the old "8.3 patch status" page for now by
removing all the applied and rejected patches:

http://developer.postgresql.org/index.php/Todo:PatchStatus

I'm slowly adding some more stuff from the patches_hold list there but
given the way that list is structured this is a fairly difficult and
tendious task(a lot there is only discussion/design sketches and in
effect one has to check every single mail in there to see if should be
added) :-(

Stefan

On Feb 9, 2008, at 1:08 AM, Tom Lane wrote:

Let's not swat flies with steam hammers.

What the heck is a steam hammer? :P
--
Decibel!, aka Jim C. Nasby, Database Architect decibel@decibel.org
Give your computer some brain candy! www.distributed.net Team #1828

Moving to -www

On Feb 9, 2008, at 11:27 AM, Tom Lane wrote:

Yeah, it happens that way for me too. The other huge, huge problem
with
it is the lack of stability of URLs for the items in the list, which
makes it difficult to identify which item you're talking about.

Wasn't someone going to look into having the mailing list generate a
header that would be universally unique and un-changing?
--
Decibel!, aka Jim C. Nasby, Database Architect decibel@decibel.org
Give your computer some brain candy! www.distributed.net Team #1828

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Mon, Feb 11, 2008 at 05:09:45PM -0600, Decibel! wrote:

On Feb 9, 2008, at 1:08 AM, Tom Lane wrote:

Let's not swat flies with steam hammers.

What the heck is a steam hammer? :P

<http://en.wikipedia.org/wiki/Creusot_steam_hammer&gt;

The difficult part would be to convince the fly to stay in place...

Regrds
- -- tomás
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFHsSVoBcgs9XrR2kYRAq/ZAJ9yx58rlPZaLklbR4gr7YVMUHjbOACfVWxJ
JUSd7OVhxi0PPeAS18Ku6Uk=
=dexU
-----END PGP SIGNATURE-----

Magnus Hagander wrote:

It would also be better to be able to off-load it to more than one
person. For example, I would like to be able to get into the unapplied
patches list and remove the email about events on 8.3RC1. First of all,
it's not a patch, but it's listed under it. But more importantly, it has
been fixed and should just be removed. So I now have to email you to ask
you to remove it, and then you have to do the actual work, which means
double work.

True. Those web pages are emails pulled from the stream of emails that
I think are worthy of discussion during 8.4 development.

If we assume we want to continue communicating via email I need a way to
pull items out and collect them, and unfortunately right now it is hard
for others to help in that.

I have a few ideas. First I could easily create an email address that
would allow others to _add_ emails to the web page (via bounce) but that
doesn't solve the issue of allowing people to comment on and delete
items.

Do most email readers support bouncing emails to another address, so the
to/from fields are not modified? (Seems there is a Thunderbird
extension to do it,
http://blog.mecworks.com/articles/2005/04/20/bouncing-mail-in-thunderbird/.)

There is no reason I have to host the list here. I can _bounce_ emails
to any address. Is there a service we can use that allows emails to be
accepted and displayed on a web site and that allows deletions and
comments, and has stable URLs for every email message? Is there
software I can install on my server to do this?

If we want to communicate via a web interface, I would still need a way
to collect specific messages (not always entire threads).

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Tue, Feb 12, 2008 at 05:49:12AM -0500, Bruce Momjian wrote:

There is no reason I have to host the list here. I can _bounce_ emails
to any address. Is there a service we can use that allows emails to be
accepted and displayed on a web site and that allows deletions and
comments, and has stable URLs for every email message? Is there
software I can install on my server to do this?

debbugs? *duck*

Personally I like trac for its simplicity. You can add comments and
post patches and track status. Which seems more than enough for what we
do.

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

"Bruce Momjian" <bruce@momjian.us> writes:

If we want to communicate via a web interface, I would still need a way
to collect specific messages (not always entire threads).

You're talking about collecting messages. Everyone else wants to collect
patches or proposed changes. There's not a one-to-one mapping between the two.

One example of what we would want is a table in a wiki where it was easy to
insert a reference to the most current patch, a few pointers to messages where
it was discussed, and the current status.

Or something like the review-board dashboard which lists the patches under
consideration with their current status. Ideally with pgsql-patches gatewayed
to create new patches in the list and comments gatewayed back to the list.

I'm sure we want to communicate by email. We also want to have an interface we
can all use to maintain a queue of patches under consideration. They're two
separate, but related, things.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's On-Demand Production Tuning

Gregory Stark wrote:

"Bruce Momjian" <bruce@momjian.us> writes:

If we want to communicate via a web interface, I would still need a way
to collect specific messages (not always entire threads).

You're talking about collecting messages. Everyone else wants to collect
patches or proposed changes. There's not a one-to-one mapping between the two.

One example of what we would want is a table in a wiki where it was easy to
insert a reference to the most current patch, a few pointers to messages where
it was discussed, and the current status.

yeah but generating that list is not so easy - in a perfect world that
short overview list would be generated by whatever tool we use.

Or something like the review-board dashboard which lists the patches under
consideration with their current status. Ideally with pgsql-patches gatewayed
to create new patches in the list and comments gatewayed back to the list.

most modern trackers can do that more or less (like BZ can track
conversations provided it has "some" way to infer what mail might be
related to what bug/patch/featurerequest).
I'm trying to hack that feature up for the demo install to simply do
that transparently for -bugs so that we can see this working for the
simple case of -bugs without having to do any chances ...

I'm sure we want to communicate by email. We also want to have an interface we
can all use to maintain a queue of patches under consideration. They're two
separate, but related, things.

yes well but that interface could very well be a more featureful tracker
that we use to generate that list ...

Stefan

Martijn van Oosterhout wrote:

On Tue, Feb 12, 2008 at 05:49:12AM -0500, Bruce Momjian wrote:

There is no reason I have to host the list here. I can _bounce_ emails
to any address. Is there a service we can use that allows emails to be
accepted and displayed on a web site and that allows deletions and
comments, and has stable URLs for every email message? Is there
software I can install on my server to do this?

debbugs? *duck*

debbugs is nice but it is more or less developed purely for debian with
rarely any regular releases afaik ?

Stefan

Decibel! wrote:

Moving to -www

On Feb 9, 2008, at 11:27 AM, Tom Lane wrote:

Yeah, it happens that way for me too. The other huge, huge problem with
it is the lack of stability of URLs for the items in the list, which
makes it difficult to identify which item you're talking about.

Wasn't someone going to look into having the mailing list generate a
header that would be universally unique and un-changing?

The archive URLs *are* stable. It's the ones in Bruces patch-queue that
aren't, and that's because he sometimes deletes mail. In the archives we
don't delete, so they stay stable.

//Magnus