Обсуждение: New CF app deployment

On Tue, Jan 13, 2015 at 2:35 PM, Magnus Hagander <magnus@hagander.net> wrote:
> Further status updates to come as we start working on it...

Things are looking good so far. All the information has been synced up
between both apps for the current CF and the next one. One the switch
is done, I would recommend to each patch author and reviewer to check
the patches they are registered on, and do the necessary modifications
if we missed something. Error is human.

Note as well that the new CF app has a couple of differences with the
old app regarding the patch status:
- When a patch is marked as "returned with feedback", it is
automatically added to the next CF
- When a patch is marked as "rejected", it is *not* added in the next
CF, but you can still re-add a new entry manually in the next app.
So "rejected" means as well that a patch is marked as such because the
author does not have time/resources to work on it for the next CF(s).
Thanks,
-- 
Michael

On Mon, Jan 19, 2015 at 3:57 PM, Magnus Hagander <magnus@hagander.net> wrote:
> The new site has been deployed and should now be usable.
>
> The old site is still available in readonly mode at
> https://commitfest-old.postgresql.org/.

These links, which were originally requested by Tom and which I have
bookmarked, used, and publicized extensively, no longer work:

https://commitfest.postgresql.org/action/commitfest_view/inprogress
https://commitfest.postgresql.org/action/commitfest_view/open

Any chance you could make those work, or at the very least provide
some equivalent?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jan 19, 2015 at 3:57 PM, Magnus Hagander <magnus@hagander.net> wrote:
> The new site has been deployed and should now be usable.

There are, for some reason, three copies of "Clarify need for memory
barriers in bgworkers" in the in-progress CF.  I don't know why that
happened, or how to fix it.

Also, the old app did this:

Status Summary. Needs Review: 34, Waiting on Author: 6, Ready for
Committer: 5, Committed: 16, Returned with Feedback: 12, Rejected: 5.
Total: 78.

...and you could click on the statuses to filter by that status.  I
see that this new app has a different way to filter by status, which
is fine, but I though the quick summary at the top was awfully useful,
so you could see how many total patches there were and how much
progress we were making.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jan 19, 2015 at 4:05 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Jan 19, 2015 at 3:57 PM, Magnus Hagander <magnus@hagander.net> wrote:
>> The new site has been deployed and should now be usable.
>
> There are, for some reason, three copies of "Clarify need for memory
> barriers in bgworkers" in the in-progress CF.  I don't know why that
> happened, or how to fix it.

There are also two copies of "ctidscan as an example of custom-scan".

Also, I can't figure out what I'm supposed to do with the "Author" and
"Reviewer" checkboxes.  Checking and unchecking them doesn't seem to
do anything.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jan 20, 2015 at 6:54 AM, Magnus Hagander <magnus@hagander.net> wrote:
> On Mon, Jan 19, 2015 at 10:10 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> On Mon, Jan 19, 2015 at 4:05 PM, Robert Haas <robertmhaas@gmail.com>
>> wrote:
>> > On Mon, Jan 19, 2015 at 3:57 PM, Magnus Hagander <magnus@hagander.net>
>> > wrote:
>> >> The new site has been deployed and should now be usable.
>> >
>> > There are, for some reason, three copies of "Clarify need for memory
>> > barriers in bgworkers" in the in-progress CF.  I don't know why that
>> > happened, or how to fix it.
>>
>> There are also two copies of "ctidscan as an example of custom-scan".
>
>
> Yeah, Michael pointed out he was unable to fix that one. I think I've fixed
> the permissions errors required for that, so I'll wait for him to try again
> to confirm that I managed to fix it.
Thanks for fixing the permission problem. I have removed the
duplicated entries, something actually caused by me when adding the
existing patches to the new app.
-- 
Michael

Hello, the new app's clean looking gets my favor and the
integrated operation will do good for me:)

By the way, I got the following notice when I tried to "Add
comment" on the new app.

"Note!: This form will generate an email to the publicmailinglist pgsql-hackers, with sender set
tohoriguchi.kyotaro@oss.ntt.co.jp!"

Hmm. The mail address indeed *was* mine but is now obsolete, so
that the email might bounce. But I haven't find how to change it
within the app itself, and the PostgreSQL community account page.

https://www.postgresql.org/account/

Could you tell me how do I change the sender address?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

On 2015-01-19 21:57:20 +0100, Magnus Hagander wrote:
> The new site has been deployed and should now be usable.

I think this unfortunately lost the RSS feature? I found that quite
useful to see who changed what recently (it's forwared to an imap
mailbox for me...).

What I'm also missing from the old app is that previously 'reviews'
could explicitly be linked from the app. Now there's a list of emails in
the thread, nice!, but in bigger threads that really doesn't help to
find the actual review.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Tue, Jan 20, 2015 at 10:09 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> Hi,
>
> On 2015-01-19 21:57:20 +0100, Magnus Hagander wrote:
>> The new site has been deployed and should now be usable.
>
> I think this unfortunately lost the RSS feature? I found that quite
> useful to see who changed what recently (it's forwared to an imap
> mailbox for me...).
Yep, added here:
https://commitfest.postgresql.org/3/109/
I linked it with 20140922230255.GD2521@awork2.anarazel.de. I also
recall that you and Robert were marked as reviewers, right?
-- 
Michael

On 2015-01-20 11:05:43 +0900, Michael Paquier wrote:
> On Tue, Jan 20, 2015 at 10:09 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> > Hi,
> >
> > On 2015-01-19 21:57:20 +0100, Magnus Hagander wrote:
> >> The new site has been deployed and should now be usable.
> >
> > I think this unfortunately lost the RSS feature? I found that quite
> > useful to see who changed what recently (it's forwared to an imap
> > mailbox for me...).
> Yep, added here:
> https://commitfest.postgresql.org/3/109/
> I linked it with 20140922230255.GD2521@awork2.anarazel.de. I also
> recall that you and Robert were marked as reviewers, right?

I think you misunderstood me ;). I was talking about the old CF
application providing a RSS feed of all changes to all entries.

I.e.
https://commitfest-old.postgresql.org/action/commitfest_activity.rss

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Tue, Jan 20, 2015 at 4:31 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2015-01-20 11:05:43 +0900, Michael Paquier wrote:
>> On Tue, Jan 20, 2015 at 10:09 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > Hi,
>> >
>> > On 2015-01-19 21:57:20 +0100, Magnus Hagander wrote:
>> >> The new site has been deployed and should now be usable.
>> >
>> > I think this unfortunately lost the RSS feature? I found that quite
>> > useful to see who changed what recently (it's forwared to an imap
>> > mailbox for me...).
>> Yep, added here:
>> https://commitfest.postgresql.org/3/109/
>> I linked it with 20140922230255.GD2521@awork2.anarazel.de. I also
>> recall that you and Robert were marked as reviewers, right?
>
> I think you misunderstood me ;). I was talking about the old CF
> application providing a RSS feed of all changes to all entries.
>
> I.e.
> https://commitfest-old.postgresql.org/action/commitfest_activity.rss
Oh, I didn't know this one. That's indeed useful.
-- 
Michael

>>>>> "Kyotaro" == Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> writes:
Kyotaro> Hmm. The mail address indeed *was* mine but is now obsolete,Kyotaro> so that the email might bounce. But I
haven'tfind how toKyotaro> change it within the app itself, and the PostgreSQL communityKyotaro> account page.
 

Just being able to change the email address on the community account
isn't enough; I for one am subscribed to the lists with a different
email address than the one associated with my community account (for
spam management reasons). There needs to be a way to have multiple
addresses or to specify which is to be used for the post.

-- 
Andrew (irc:RhodiumToad)

Michael Paquier <michael.paquier@gmail.com> writes:
> On Tue, Jan 20, 2015 at 4:31 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> I think you misunderstood me ;). I was talking about the old CF
>> application providing a RSS feed of all changes to all entries.
>> https://commitfest-old.postgresql.org/action/commitfest_activity.rss

> Oh, I didn't know this one. That's indeed useful.

Personally, I never used the RSS feed as such, but I did often consult the
"activity log" pages, which I think are equivalent:

https://commitfest-old.postgresql.org/action/commitfest_activity?id=25

That feature seems to be gone too :-(
        regards, tom lane

Magnus,

Now that I'm back on this side of the Pacific, is there any additional
data entry/cleanup which needs doing?

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Thu, Jan 22, 2015 at 10:47 AM, Josh Berkus <josh@agliodbs.com> wrote:
> Now that I'm back on this side of the Pacific, is there any additional
> data entry/cleanup which needs doing?
An extra look would be worth it. Magnus or I may have missed patch
entries between the old and new apps.
My2c.
-- 
Michael

Lovely!

> I've just pushed a change to the main website which now lets you change the
> email address there. That meas you can go to
> https://www.postgresql.org/account/profile/ and change the email. After you
> have confirmed the change (an email will be sent to your new address when
> you change it), the the new one should be valid on the cf app (if it
> doesn't show up, log out of the cf app and back in, and it should show up).
> 
> I will look into Andrews issue of being able to have multiple email
> addresses soon as well - but one feature per evening :)

It works perfectly. Thanks.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Hi,

Thanks for the excellent work on the new commitfest app.  It looks
awesome so far, though I'm betting the commitfest manager is the one who
reaps the most benefits.

Wanted to highlight this request:

Andres Freund wrote:
> What I'm also missing from the old app is that previously 'reviews'
> could explicitly be linked from the app. Now there's a list of emails in
> the thread, nice!, but in bigger threads that really doesn't help to
> find the actual review.

Note for instance the "BRIN inclusion operator class" patch here,
https://commitfest.postgresql.org/3/31/

The link points to the generic BRIN thread I started (you can see it in
the history).  If you list all attachments you can see that all the BRIN
patches are linked; Emre's patch is there, it seems, only because it's
the one most recently posted.

Not sure what to do here, but it's somewhat annoying.


Also, I was pretty used to offline operation with the old one: I could
load the page, for instance
https://commitfest-old.postgresql.org/action/commitfest_view?id=25
and the "patch" links alongside each patch had the message-ids with
which I could search my local mbox.  In the new one, the only way I can
get the message-id is by opening the patch page.  It would be pretty
useful to have a "copy message-id to clipboard" button, or something
similar, so that I could transfer operation from the web browser to my
mail client.  Having one message-id per patch in the commitfest summary
page is enough for my use case (probably pointing to the most recent
attachment in the linked threads.)

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

seems the CF app uses an invalid e-mail address when sending messages to
pgsql-hackers - I've added a comment to one of the patches and got this:
 pgsql-hackers-testing@localhost   Unrouteable address

Maybe that's expected as the CF app is new, but I haven't seen it
mentioned in this thread.

regards
Tomas

Is there any possibility of making it possible to "annotate"
particular messages (in particular, patch-related messages) with brief
comments? I would like to be able to highlight particular messages as
particular versions of the patch, and have it be apparent what
properties that version has at a glance.

The mailing list integration is good, but seems like it could often be
overkill. I just want to "tag" an existing message for readability
here, like with the old commitfest app. I like to make things easy to
find from the CF app, particularly for larger, more complicated
things.

Thanks
-- 
Peter Geoghegan

>>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes:
Peter> The mailing list integration is good, but seems like it couldPeter> often be overkill. I just want to "tag" an
existingmessage forPeter> readability here, like with the old commitfest app. I like toPeter> make things easy to find
fromthe CF app, particularly forPeter> larger, more complicated things.
 

There's a fairly serious readability problem when someone has posted a
patch as a subthread of some more general discussion. For example, look
at the "adaptive ndistinct estimator" patch: it's not obvious which
attachment is the actual patch, and whether the latest email has
anything to do with the patch is entirely arbitrary.

Either there needs to be an ironclad "always start a new thread for a
new patch" rule, or the app needs to be smarter about threading.

-- 
Andrew (irc:RhodiumToad)

On Sun, Jan 25, 2015 at 3:22 AM, Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
> There's a fairly serious readability problem when someone has posted a
> patch as a subthread of some more general discussion. For example, look
> at the "adaptive ndistinct estimator" patch: it's not obvious which
> attachment is the actual patch, and whether the latest email has
> anything to do with the patch is entirely arbitrary.

I think that the inability to put each message in context, with
"metadata" comments associated with individual messages is a serious
regression in functionality. I hope it is fixed soon. I raised this
issue at the earliest opportunity, when Magnus privately sought
feedback early last year.

-- 
Peter Geoghegan

On Mon, Jan 26, 2015 at 3:13 PM, Peter Geoghegan <pg@heroku.com> wrote:
> On Sun, Jan 25, 2015 at 3:22 AM, Andrew Gierth
> <andrew@tao11.riddles.org.uk> wrote:
>> There's a fairly serious readability problem when someone has posted a
>> patch as a subthread of some more general discussion. For example, look
>> at the "adaptive ndistinct estimator" patch: it's not obvious which
>> attachment is the actual patch, and whether the latest email has
>> anything to do with the patch is entirely arbitrary.
>
> I think that the inability to put each message in context, with
> "metadata" comments associated with individual messages is a serious
> regression in functionality. I hope it is fixed soon. I raised this
> issue at the earliest opportunity, when Magnus privately sought
> feedback early last year.

I agree.  Starting a new email thread for each patch version is, IMHO,
a complete non-starter.  It's 100% contrary to what has generally been
advocated as best-practice up until now, and it is basically saying we
should alter our workflow because the tool can't cope with the one
we've got.  The whole point of having home-grown tools for this stuff
is that they're supposed to work with the way we already like to do
things instead of forcing us to work in new ways.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jan 26, 2015 at 3:24 PM, Magnus Hagander <magnus@hagander.net> wrote:
> Yes, and the agreement after that feedback was to try it out and then figure
> out what changes were needed? As about half the feedback said it was better
> without and half said it was better with.

Well, I can't speak to anyone else's opinion, but I'm quite sure I
raised the issue that we need a way to call out which messages in the
thread are important, and I think that's pretty much what Peter is
saying, too.  I find the new tool essentially unusable - having one
link to the whole thread instead of individual links to just the
important messages in that thread is a huge regression for me, as is
the lack of the most recent activity on the summary page.  I don't
know how much more feedback than that you need to be convinced, but
I'm going to shut up now before I say something I will later regret.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jan 26, 2015 at 12:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> Well, I can't speak to anyone else's opinion, but I'm quite sure I
> raised the issue that we need a way to call out which messages in the
> thread are important, and I think that's pretty much what Peter is
> saying, too.

It is.

> I find the new tool essentially unusable - having one
> link to the whole thread instead of individual links to just the
> important messages in that thread is a huge regression for me, as is
> the lack of the most recent activity on the summary page.

Yes, the lack of the most recent activity is also a major omission.

I like some things about the new app. The general idea of having the
mailing list traffic drive the commitfest app is a good one. However,
it seems we've gone too far. I strongly feel that the two regressions
called out here are big problems.
-- 
Peter Geoghegan

On 2015-01-26 12:54:04 -0800, Peter Geoghegan wrote:
> On Mon, Jan 26, 2015 at 12:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> > Well, I can't speak to anyone else's opinion, but I'm quite sure I
> > raised the issue that we need a way to call out which messages in the
> > thread are important, and I think that's pretty much what Peter is
> > saying, too.
> 
> It is.

Agreed.

> > I find the new tool essentially unusable - having one
> > link to the whole thread instead of individual links to just the
> > important messages in that thread is a huge regression for me, as is
> > the lack of the most recent activity on the summary page.
> 
> Yes, the lack of the most recent activity is also a major omission.

That one is there now though, isn't it?

For specific CF:
https://commitfest.postgresql.org/3/activity/
All CFs:
https://commitfest.postgresql.org/activity/

Or do you mean something else?

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Mon, Jan 26, 2015 at 4:01 PM, Magnus Hagander <magnus@hagander.net> wrote:
> According to my mailbox, you didn't even respond on that thread. But it may
> well be that your email ended up on some other thread and therefor was not
> included when I went back and looked over all the responses I got on it. If
> that was the case, then I apologize for loosing track of the feedback.

I remember bringing it up at PGCon, I think.  I don't know whether I
wrote it in an email.

> The "most recent activity on the summary page" is on my short-term todo list
> to fix. The past couple of days have been a bit too busy to get that done
> though, mainly due to the upcoming FOSDEM and pgday events. But rest assured
> that part is definitely on the list, as it doesn't actually change any
> functionality, it's just a view. Same as that "quick stats numbers" thing on
> the frontpage of each cf.

OK.  What I'd like to understand is why this new app had to be rolled
out before these things were done.  We've been using my app for 5
years and it, like, worked.  So why the rush to roll it out with these
known issues unfixed?  I mean, it's not like you couldn't look at any
time and see what the new app lacked that the old app had.  The last
time you presented this app for feedback, which I remember to be
PGCon, it was so buggy that there was no point in trying to form any
considered opinion of it.  Now, it's rolled out, but with a bunch of
stuff that people use and rely on missing.  I grant that some of those
things you may not have realized anyone cared about, but it feels to
me like this got pushed into production without really getting it to
feature parity.  I could've spent more of my time complaining about
that than I did, but why should I have to do that?

> As for being able to flag more things on individual emails/patches, I am
> definitely not against that in principle, if that's what people prefer. But
> I don't think it's unreasonable to give it a few days and then collect
> feedback on that (and other things).

Suit yourself.

> Which of course also includes rolling back the whole thing if people prefer
> the older one - that has been an option on the table from the time we
> decided to give this one a try in the first place. (Though in that case, we
> really need to find a maintainer for that code, as it's we don't seem to
> have that now. But I'm sure that can get sorted)

I understand that having someone who has the time to maintain the code
is an important issue, and I admit I don't, and haven't for a while.
There is a lot of sense in replacing the app with something that uses
the same software framework as the rest of our infrastructure, which I
understand that yours does.  And it's not like what the new one does
is horribly different from what the old one did.  So I don't see that
there's any reason we can't make the new one work.  But I confess to
having no patience with the idea that I have to build a consensus to
get you to re-add features that you removed.  You can take that
position, and since you are the tool maintainer it's not exactly
unfair, but since I was willing to put in the work to add those
features in the first place, I probably think they were worth having.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Jan 26, 2015 at 4:07 PM, Magnus Hagander <magnus@hagander.net> wrote:
> I assume what was referred to was that the old cf app would show the  last 3
> (I think it was) comments/patches/whatnot on a patch on the summary page
> (and then clickthrough for more details).

Yep.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 01/26/2015 12:46 PM, Robert Haas wrote:
> I find the new tool essentially unusable - having one
> link to the whole thread instead of individual links to just the
> important messages in that thread is a huge regression for me, as is
> the lack of the most recent activity on the summary page.

Well, if it's essentially unusable, then we've reached parity with the
old app (yes, you deserved that).

The difference between the old and new apps is that it's actually
*possible* to improve things on the new app, which was never going to
happen on the old one.  There's also a significant advantage in knowing
that the entire email thread is available on a patch without someone
having to remember to manually paste in each message ID, something which
failed to happen at least 1/3 of the time for important messages.

What would be cool is a way to "flag" individual messages in the email
thread as being important.  Will give it some thought and suggest something.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Mon, Jan 26, 2015 at 4:16 PM, Josh Berkus <josh@agliodbs.com> wrote:
> Well, if it's essentially unusable, then we've reached parity with the
> old app (yes, you deserved that).

No, I didn't.  What we had before I wrote that tool was a bunch of
wiki pages you put together which were forever having problems with
multiple people editing the page at the same time, and not always
adhering to the formatting standards, and sometimes accidentally or
purposefully deleting things other people had done.  The tool was
written to mimic that, but without the edit-war chaos.  So, if sucked,
it sucked because it mimicked something designed by you.  Furthermore,
if it sucked so bad, why did it take anyone 5 years to get around to
rewriting it?  It took me less than a year to get around to replacing
what you wrote.

> The difference between the old and new apps is that it's actually
> *possible* to improve things on the new app, which was never going to
> happen on the old one.

That is probably a fair critique.

> There's also a significant advantage in knowing
> that the entire email thread is available on a patch without someone
> having to remember to manually paste in each message ID, something which
> failed to happen at least 1/3 of the time for important messages.

As far as I can see, the new app offers no real advantage in this
regard.  The thing that really made things better here is the work
that was done to make threading in our archives work properly.  In the
old app, you could click on the last message for which someone put the
message-ID and then go to the end of the thread.  In the new app, you
can... pretty much do the same thing.  It's not like every message in
the thread shows up in the app.  The latest one with an attachment
does, but that's not necessarily the latest patch version.

(While I'm complaining, the links only go to the "flat" version of the
thread, while I happen to prefer the version that shows one message at
a time with a message-ID selector to switch messages.)

> What would be cool is a way to "flag" individual messages in the email
> thread as being important.  Will give it some thought and suggest something.

You make that comment as if three people hadn't already +1'd that idea.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 01/26/2015 01:29 PM, Robert Haas wrote:
>  Furthermore,
> if it sucked so bad, why did it take anyone 5 years to get around to
> rewriting it?  It took me less than a year to get around to replacing
> what you wrote.

Because whoever replaced it knew they'd be facing a shitstorm of criticism?

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 2015-01-26 13:32:51 -0800, Josh Berkus wrote:
> On 01/26/2015 01:29 PM, Robert Haas wrote:
> >  Furthermore,
> > if it sucked so bad, why did it take anyone 5 years to get around to
> > rewriting it?  It took me less than a year to get around to replacing
> > what you wrote.
> 
> Because whoever replaced it knew they'd be facing a shitstorm of criticism?

Oh, comeon. Grow up.

A missing feature that several people commented on *before* the tool was
released, and that several people have commented upon since isn't a
"shitstorm of criticism".

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Mon, Jan 26, 2015 at 4:32 PM, Josh Berkus <josh@agliodbs.com> wrote:
> On 01/26/2015 01:29 PM, Robert Haas wrote:
>>  Furthermore,
>> if it sucked so bad, why did it take anyone 5 years to get around to
>> rewriting it?  It took me less than a year to get around to replacing
>> what you wrote.
>
> Because whoever replaced it knew they'd be facing a shitstorm of criticism?

Didn't stop me.  And actually, I didn't face a shitstorm of criticism.
The way I remember it, I got a pretty much unqualified positive
reaction at the time.  Only later, when you had conveniently forgotten
how bad things were before we had that tool, did you start routinely
dumping on it.  And, AFAICS, not because of anything it did wrong,
only because of things that it didn't do that were features you wanted
to have.

Most of those features were things that I could not possibly have
implemented anyway because they would have required deeper integration
with the authentication system than was possible with the access my
app had.  Automatically linking new emails?  Not possible: I had no
archives access.  Automatic emails to users?  Not possible: no access
to email addresses.  Place to put a link to a git branch?  Yeah, I
could have done that, and just didn't, but the new app doesn't do it
either, yet anyway.  I don't know how Magnus's app is connecting in to
the rest of the PG infrastructure, but it's obviously got more access
than mine had.  And I think that's great, because hopefully it will
eventually make this much nicer than what I had.  It isn't nicer yet,
though, and you showing up and tossing out insults based on
revisionist history won't fix that.

What particularly peeves me about your remarks is that you've
basically made no contribution to the CommitFest process in years.  I
am less active than I used to be, but I still do a lot of reviewing
and committing of patches and generally put a pretty significant
amount of time into it.  Despite whatever shortcomings that app I
wrote has, so do a lot of other people.  The CommitFest process has
got its issues, particularly a lack of reviewers, but it is still
better than having no process, and yet your only contributions seem to
be to denigrate the people who are putting time into it.  You're using
the poor quality of my app, and an almost total misinterpretation of
what was said about attracting new reviewers at PGCon, as excuses for
your non-participation, and that is certainly your prerogative.  You
have as much right not to participate as anyone.  But refusing to
participate except to throw bricks is not going to move this project
forward.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert,

> Didn't stop me.  And actually, I didn't face a shitstorm of criticism.
> The way I remember it, I got a pretty much unqualified positive
> reaction at the time.

Including from me, because it was a huge improvement on what we had
before.  As the new app is.

>  Only later, when you had conveniently forgotten
> how bad things were before we had that tool, did you start routinely
> dumping on it.  And, AFAICS, not because of anything it did wrong,
> only because of things that it didn't do that were features you wanted
> to have.

In order to get a consensus on moving to a new app I had to explain what
was wrong with the old app.  Eventually I had to use strong language to
do so, because nobody was paying attention otherwise.  While Magnus's
app isn't my original proposal, I'm 100% behind it because it gets us
moving forward and eliminates a lot of obstacles both to submitters and
CFMs.  If Magnus hasn't already, in the future we'll be able to add more
features to make things faster for major reviewers as well.

> And I think that's great, because hopefully it will
> eventually make this much nicer than what I had. It isn't nicer yet,
> though, and you showing up and tossing out insults based on
> revisionist history won't fix that.

You didn't previously say "isn't nicer".  You said "essentially
unusuable".  There's a big difference between those two phrases.  Your
emails came off as "the new app is a disaster, we should revert
immediately", and I'm not the only one who read them that way.

If you're going to throw around negative hyperbole, expect to get it
thrown back at you.

> What particularly peeves me about your remarks is that you've
> basically made no contribution to the CommitFest process in years. 

Aside from being a CFM for one CF each year, of course.

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Mon, Jan 26, 2015 at 5:36 PM, Josh Berkus <josh@agliodbs.com> wrote:
> In order to get a consensus on moving to a new app I had to explain what
> was wrong with the old app.  Eventually I had to use strong language to
> do so, because nobody was paying attention otherwise.  While Magnus's
> app isn't my original proposal, I'm 100% behind it because it gets us
> moving forward and eliminates a lot of obstacles both to submitters and
> CFMs.  If Magnus hasn't already, in the future we'll be able to add more
> features to make things faster for major reviewers as well.

Frankly, I don't believe that you had much to do with getting
consensus on moving to a new app.  I believe Magnus did that, mostly
by writing it.  And I'm not complaining about whatever strong language
you may have used in the past; I'm complaining about you showing up on
this thread now, after the switch has already been made, to lob
insults.

>> And I think that's great, because hopefully it will
>> eventually make this much nicer than what I had. It isn't nicer yet,
>> though, and you showing up and tossing out insults based on
>> revisionist history won't fix that.
>
> You didn't previously say "isn't nicer".  You said "essentially
> unusuable".  There's a big difference between those two phrases.  Your
> emails came off as "the new app is a disaster, we should revert
> immediately", and I'm not the only one who read them that way.

I stand by what I said.  I find it very hard to use in the present
state for the reasons that I set out.  That was not intended as a
request for a revert.  I have subsequently written several emails
clarifying my position on that point.  If Magnus *wants* to revert it,
that's fine.  But I suspect he doesn't, and that's fine too.  However,
I'd very much like the features that are missing added back, and,
yeah, I'm annoyed that they are missing.

> If you're going to throw around negative hyperbole, expect to get it
> thrown back at you.

Oh, give me a break.

>> What particularly peeves me about your remarks is that you've
>> basically made no contribution to the CommitFest process in years.
>
> Aside from being a CFM for one CF each year, of course.

OK, perhaps I'm overstating it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2015-02-06 11:51:50 +0100, Magnus Hagander wrote:
> So in an attempt to actually move this forward in a constructive way I'm
> going to ignore  a bunch of what happened after this email, and fork the
> discussion at this point.

Sounds good.

> First of all - assuming we'lI fix this particular thing. Are there *other*
> things that you would also say makes the tool "essentially unusable"?

I personally don't have any really fundamental complaints but this
one. I'd like to be able to add some CF app only annotations to the
whole patch, but that's not that important.

> What kind of annotation are we actually talking about here?

Well, to explain where, at least I, am coming from, let's have a look at
an example:
https://commitfest.postgresql.org/4/17/

a) The linked messages aren't going to help either the author, a  committer or prospective reviewers to find existing
reviews.And  looking through the whole thread isn't realistic at that message  count. And all of them need to.
 
b) The 'latest attachement' isn't actually the patch that's supposed to  be reviewed. The first patch is completely
outdated.So one would  need to grovel through all the linked patches.
 
c) By just looking at the CF entry I have no clue what the status of the  patch actually is. Sure it "Needs review",
butthat can mean minor  changes or major reworks. In the old app the comments to the patches  and reviews could say
thingslike "major issues identified,  significant work needed", "some cosmetic issues remain", "new version  of patch
withmajor redesign" or "rebased patch without other changes".
 

I don't think any of these questions can be answered by improved
automatisms alone.

This really doesn't matter much on a 5-10 message thread about a simple
patch. But we regularly have thread with hundreds of messages.


Makes sense?


> Are you looking for something that's basically "star a message" (like
> you'd do in a MUA in a lot of cases)? Or "assign tag" (from a set of -
> editable of course - pre-existing tags). Or a complete free-text
> field?

I think it has to be free form. If you look at the above, it's hard to
do that sanely with tags.

> Then, what do you need to be able to annotate on?

I think both individual messages and the CF entry itself, without an
email, make sense.

> Right now the app only shows messages that actually have attachments on
> them, assuming that you'd want to read the whole thread if you want to look
> at other things.

I don't think anybody can realistically do that on more complex
patches. Often the discussion in the beginning has little bearance on
later state, so even if you had the time to do so, it'd better be spent
doing something else.

> Are you saying you want to be able to attach an annotation to *any*
> message in the thread?

Yes.

> If so, we'd have to pull in and show every single message on the
> thread in the UI - I'm thinking that might become more than a little
> cluttered in a lot of cases.

Well, we only would need to show commented on messages in the UI when
viewing the entry.

> If that is what you're talking about, any
> suggestions for how to actually make that into an UI that's not that
> cluttered?

The simplest seems to be to simply allow comments using the message id
:(. I've no really good idea how to allow to select the message to
comment on. Even if we find something else, it should probably be at
least an option - it certainly is quicker for me to grab the message id
of an email I just read or wrote and paste it than pretty much any other
method.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

On Fri, Feb 6, 2015 at 5:55 AM, Magnus Hagander <magnus@hagander.net> wrote:
> On Mon, Jan 26, 2015 at 10:29 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> (While I'm complaining, the links only go to the "flat" version of the
>> thread, while I happen to prefer the version that shows one message at
>> a time with a message-ID selector to switch messages.)
>
> Then you're clicking the wrong link :) As long as you click the link for the
> message (first or last) you get the "regular" view. The "thread" link always
> goes to the same message as the "first" one, except the flat view. So you
> have a choice of both (and these links are both always visible, none of them
> is in the "collapsed" area which holds the attachments)

OK.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Feb 6, 2015 at 5:51 AM, Magnus Hagander <magnus@hagander.net> wrote:
> So in an attempt to actually move this forward in a constructive way I'm
> going to ignore  a bunch of what happened after this email, and fork the
> discussion at this point.

Thanks, and I probably owe you an apology for some of that, so, sorry
about that.

I think the core of the problem here is that the old application saw
its goal in life as *summarizing* the thread.  The idea is that people
would go in and add comments (which could be flagged as comment,
patch, or review) pointing to particularly important messages in the
discussion.  The problem with this is that it had to be manually
updated, and some people didn't like that.[1]  The new app attaches
the entire thread, which has the advantage that everything is always
there.  The problem with that is that the unimportant stuff is there,
too, and there's no way to mark the important stuff so that you can
distinguish between that and the unimportant stuff.  I think that's
the problem we need to solve.

One thing that would probably *help* is if the list of attachments
mentioned the names of the files that were attached to each message
rather than just noting that they have some kind of attachment.  If
people name their attachments sensibly, then you'll be able to
distinguish parallel-seqscan-v23.patch from
test-case-that-breaks-parallel-seqscan.sql, and that would be nice.
But that doesn't help with, say, distinguishing useful reviews from
general discussion on the thread.  I don't think there's any way to do
that in an automated way, so it's got to be something that somebody
does manually.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

[1] My personal opinion is that this complaint was misguided.  I have
yet to see an issue tracker that didn't require scads of manual work
to keep the information in it relevant, and ours had a higher
signal-to-noise ratio than many I've had to use professionally.  That
having been said, I understand the frustration.

On Fri, Feb 6, 2015 at 4:02 PM, Jeff Janes <jeff.janes@gmail.com> wrote:
> I liked to add comments which would point out some fact that was important
> to testing but which was non-obvious. Often this fact was mentioned
> somewhere in the 300 message thread, but it needs to be called out
> specifically for people interested in testing but not so interested in
> architectural debates.  Obviously adding another email to a overly-long
> thread is going the wrong way when it comes to making things stand out
> better.  (Also, if the comment is about a uncommitted dependency, then the
> comment can be deleted once the dependency is committed)

Good point. I think that your role in testing is very valuable, and
that we should make a particular effort to accommodate that sort of
thing.


-- 
Peter Geoghegan

Magnus Hagander <magnus@hagander.net> writes:
> On Sat, Feb 7, 2015 at 1:02 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
>> I'd like the ability to add a comment which does not get turned into an
>> email.

> I really don't ;)

> The reason I really don't like that is that this now makes it impossible to
> track the review status by just reading throught he mail thread. You have
> to context-switch back and forth between the app and the archives. We had
> this problem  in the old system every now and then where reviews were
> posted entirely in the old system...

Yeah, people did that sometimes and it sucked.  At the same time I see
Jeff's point: 300-email threads tend to contain a lot of dross.  Could we
address it by allowing only *very short* annotations?  The limiting case
would be 1-bit annotations, ie you could star the important messages in a
thread; but that might be too restrictive.
        regards, tom lane

On Sun, Feb 8, 2015 at 11:04 AM, Magnus Hagander <magnus@hagander.net> wrote:
> Filenames are now shown for attachments, including a direct link to the
> attachment itself.  I've also run a job to populate all old threads.

Thanks!

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Il 08/02/15 17:04, Magnus Hagander ha scritto:
>
> Filenames are now shown for attachments, including a direct link to the
> attachment itself.  I've also run a job to populate all old threads.
>

I wonder what is the algorithm to detect when an attachment is a patch.

If you look at https://commitfest.postgresql.org/4/94/ all the
attachments are marked as "Patch: no", but many of them are
clearly a patch.

Regards,
Marco

--
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciarini@2ndQuadrant.it | www.2ndQuadrant.it

On Mon, Feb 9, 2015 at 5:38 AM, Magnus Hagander <magnus@hagander.net> wrote:
> On Mon, Feb 9, 2015 at 11:09 AM, Marco Nenciarini
> <marco.nenciarini@2ndquadrant.it> wrote:
>>
>> Il 08/02/15 17:04, Magnus Hagander ha scritto:
>> >
>> > Filenames are now shown for attachments, including a direct link to the
>> > attachment itself.  I've also run a job to populate all old threads.
>> >
>>
>> I wonder what is the algorithm to detect when an attachment is a patch.
>>
>> If you look at https://commitfest.postgresql.org/4/94/ all the
>> attachments are marked as "Patch: no", but many of them are
>> clearly a patch.
>
> It uses the "magic" module, same as the "file" command. And that one claims:
>
> mha@mha-laptop:/tmp$ file 0003-File-based-incremental-backup-v9.patch
> 0003-File-based-incremental-backup-v9.patch: ASCII English text, with very
> long lines
>
> I think it doesn't consider it a patch because it's not actually a patch -
> it looks like a git-format actual email message that *contains* a patch. It
> even includes the unix From separator line. So if anything it should have
> detected that it's an email message, which it apparently doesn't.
>
> Picking from the very top patch on the cf, an actual patch looks like this:
>
> mha@mha-laptop:/tmp$ file psql_fix_uri_service_004.patch
> psql_fix_uri_service_004.patch: unified diff output, ASCII text, with very
> long lines

Can we make it smarter, so that the kinds of things people produce
intending for them to be patches are thought by the CF app to be
patches?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Feb 14, 2015 at 7:29 AM, Magnus Hagander <magnus@hagander.net> wrote:
> Ok, I've pushed an attempt at doing this.
>
> For each mailthread, you can now create annotations. Each annotation is
> connected to a mail in the thread, and has a free text comment field. The
> message will then automatically be "highlighted" out of the archives and a
> direct link to the message include, alongside the text comment.
>
> I *think* this is approximately what people wanted, so let's give it a shot.
> If you have a chance, please test it out and comment as soon as you can - if
> I'm completely off track with how it's done, we should back it out and try a
> different way before we start putting actual valuable data into it, so we
> don't end up with multiple different ways of doing it in the end...

Hmm.  This kind of looks like the right idea, but it's hard to use,
because all you've got to work with is the subject of the message
(which is the same for all), the time it was sent, and the author.  In
the old system, you could look at the message in the archives and then
copy-and-paste in the message ID.  That's obviously a bit manual, but
it worked.

I suppose what would be really spiffy is if the integration of the
archives and the CF app could be such that you could see the whole
message and then click "annotate this message".

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2/14/15 11:42 AM, Robert Haas wrote:
> On Sat, Feb 14, 2015 at 7:29 AM, Magnus Hagander <magnus@hagander.net> wrote:
>> Ok, I've pushed an attempt at doing this.
>>
>> For each mailthread, you can now create annotations. Each annotation is
>> connected to a mail in the thread, and has a free text comment field. The
>> message will then automatically be "highlighted" out of the archives and a
>> direct link to the message include, alongside the text comment.
>>
>> I *think* this is approximately what people wanted, so let's give it a shot.
>> If you have a chance, please test it out and comment as soon as you can - if
>> I'm completely off track with how it's done, we should back it out and try a
>> different way before we start putting actual valuable data into it, so we
>> don't end up with multiple different ways of doing it in the end...
>
> Hmm.  This kind of looks like the right idea, but it's hard to use,
> because all you've got to work with is the subject of the message
> (which is the same for all), the time it was sent, and the author.  In
> the old system, you could look at the message in the archives and then
> copy-and-paste in the message ID.  That's obviously a bit manual, but
> it worked.
>
> I suppose what would be really spiffy is if the integration of the
> archives and the CF app could be such that you could see the whole
> message and then click "annotate this message".

It would be nice if CF-related emails had a link at the bottom that took 
you to that email in the CF app. Another possibility is some kind of 
special markup you could add to an email to create an annotation.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Sun, Feb 15, 2015 at 8:36 AM, Magnus Hagander <magnus@hagander.net> wrote:
>> Hmm.  This kind of looks like the right idea, but it's hard to use,
>> because all you've got to work with is the subject of the message
>> (which is the same for all), the time it was sent, and the author.  In
>> the old system, you could look at the message in the archives and then
>> copy-and-paste in the message ID.  That's obviously a bit manual, but
>> it worked.
>
>
> Hmm. Agreed, I think my test-thread was just too short to do that.
>
> I've added a box where you can copy/paste the messageid. Back to the manual,
> but it works.


Thank you for correcting the issue in a timely manner. I appreciate the effort.

-- 
Peter Geoghegan

On 2/14/15 7:30 AM, Magnus Hagander wrote:
> On Mon, Feb 9, 2015 at 4:56 PM, Robert Haas <robertmhaas@gmail.com
>     Can we make it smarter, so that the kinds of things people produce
>     intending for them to be patches are thought by the CF app to be
>     patches?
> 
> 
> Doing it wouldn't be too hard, as the code right now is simply:
> 
>                 # Attempt to identify the file using magic information
>                 mtype = mag.buffer(contents)
>                 if mtype.startswith('text/x-diff'):
>                         a.ispatch = True
>                 else:
>                         a.ispatch = False
> 
> 
> (where mag is the API call into the magic module)
> 
> So we could easily add for example our own regexp parsing or so. The
> question is do we want to - because we'll have to maintain it as well.
> But I guess if we have a restricted enough set of rules, we can probably
> live with that.

As I had described in
<http://www.postgresql.org/message-id/54DD2413.8030201@gmx.net>, this is
all but impossible.  The above rule is certainly completely detached
from the reality of what people actually send in.  If you are just
ignoring patches that don't match your rule set, this is not going to
work very well.

I think the old system where the patch submitter declared, this message
contains my patch, is the only one that will work.

On Sun, Feb 15, 2015 at 4:59 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> I think the old system where the patch submitter declared, this message
> contains my patch, is the only one that will work.

I tend to agree. That being said, calling out latest attachments is
also useful (or highlighting that a particular mail has a particular
file attached in general).

Annotations-wise, I think that it would be nice to be able to modify
an annotation, in case a typo is made (the old app supported this). I
also think that it's a waste of screen space to show "who" within the
annotation view. Granted, the old app supported this, but I tend to
think that if I actually cared who added a certain annotation, I'd be
happy to drill down into history. I haven't cared so far, AFAICR.


-- 
Peter Geoghegan

Magnus Hagander <magnus@hagander.net> writes:
> On Sun, Feb 15, 2015 at 9:46 PM, Peter Geoghegan <pg@heroku.com> wrote:
>> also think that it's a waste of screen space to show "who" within the
>> annotation view. Granted, the old app supported this, but I tend to
>> think that if I actually cared who added a certain annotation, I'd be
>> happy to drill down into history. I haven't cared so far, AFAICR.

> Hmm. Personally, I definitely care about who made an annotation, but i
> guess I could be OK with going into the history as well. Anybody else have
> an opinion on the matter? I don't personally feel strongly either way.

Hmm ... in the old system, I agree with Peter, in fact I tended to find
the "who" annotations positively misleading, because people frequently
added other peoples' messages to the CF app.  It always looked to me like
this had the effect of attributing person A's patch to person B, who'd
actually only updated the CF entry.  I think the primary thing you usually
want to know is who is the author of a given email message.

With the new system of auto-adding messages, that particular problem
should be gone, and annotations should be mostly special events rather
than routine maintenance.  So maybe who made the annotation is of more
interest than it often was before.

I'm inclined to say you should leave it alone for the moment.  We don't
have enough experience with the new system to be sure how we'll use it,
so why do work you might just end up reverting?  We can adjust the display
later if we become convinced we don't need this info.
        regards, tom lane

On 2/22/15 3:12 PM, Magnus Hagander wrote:
> Would you suggest removing the automated system completely, or keep it
> around and just make it possible to override it (either by removing the
> note that something is a patch, or by making something that's not listed
> as a patch become marked as such)?

I would remove it completely.  It has been demonstrated to not work.

On Thu, Feb 26, 2015 at 9:15 PM, Asif Naeem <anaeem.it@gmail.com> wrote:
> This thread seems relevant, Please guide me to how can access older CF pages
>> The MSVC portion of this fix got completely lost in the void:
>> https://commitfest.postgresql.org/action/patch_view?id=1330
>
> Above link result in the following error i.e.
>
>> Not found
>> The specified URL was not found.
>
> Please do let me know if I missed something. Thanks.

Try commitfest-old instead, that is where the past CF app stores its
data, like that:
https://commitfest-old.postgresql.org/action/patch_view?id=1330
-- 
Michael

On 02/26/2015 01:59 PM, Michael Paquier wrote:
> On Thu, Feb 26, 2015 at 9:15 PM, Asif Naeem <anaeem.it@gmail.com> wrote:
>> This thread seems relevant, Please guide me to how can access older CF pages
>>> The MSVC portion of this fix got completely lost in the void:
>>> https://commitfest.postgresql.org/action/patch_view?id=1330
>>
>> Above link result in the following error i.e.
>>
>>> Not found
>>> The specified URL was not found.
>>
>> Please do let me know if I missed something. Thanks.
> 
> Try commitfest-old instead, that is where the past CF app stores its
> data, like that:
> https://commitfest-old.postgresql.org/action/patch_view?id=1330

hmm maybe we should have some sort of handler the redirects/reverse
proxies to the old commitfest app for this.



Stefan

<div dir="ltr">Thank you Michael, It works.<br /></div><div class="gmail_extra"><br /><div class="gmail_quote">On Thu,
Feb26, 2015 at 5:59 PM, Michael Paquier <span dir="ltr"><<a href="mailto:michael.paquier@gmail.com"
target="_blank">michael.paquier@gmail.com</a>></span>wrote:<br /><blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px#ccc solid;padding-left:1ex"><span class="">On Thu, Feb 26, 2015 at 9:15 PM, Asif Naeem <<a
href="mailto:anaeem.it@gmail.com">anaeem.it@gmail.com</a>>wrote:<br /> > This thread seems relevant, Please guide
meto how can access older CF pages<br /></span><span class="">>> The MSVC portion of this fix got completely lost
inthe void:<br /> >> <a href="https://commitfest.postgresql.org/action/patch_view?id=1330"
target="_blank">https://commitfest.postgresql.org/action/patch_view?id=1330</a><br/> ><br /> > Above link result
inthe following error i.e.<br /> ><br /> >> Not found<br /> >> The specified URL was not found.<br />
><br/> > Please do let me know if I missed something. Thanks.<br /><br /></span>Try commitfest-old instead, that
iswhere the past CF app stores its<br /> data, like that:<br /><a
href="https://commitfest-old.postgresql.org/action/patch_view?id=1330"
target="_blank">https://commitfest-old.postgresql.org/action/patch_view?id=1330</a><br/><span class="HOEnZb"><font
color="#888888">--<br/> Michael<br /></font></span></blockquote></div><br /></div> 

On Sun, Feb 22, 2015 at 03:12:12PM -0500, Magnus Hagander wrote:
>     >                 # Attempt to identify the file using magic information
>     >                 mtype = mag.buffer(contents)
>     >                 if mtype.startswith('text/x-diff'):
>     >                         a.ispatch = True
>     >                 else:
>     >                         a.ispatch = False
...
> 
>     I think the old system where the patch submitter declared, this message
>     contains my patch, is the only one that will work.
> 
> 
> 
> Would you suggest removing the automated system completely, or keep it around
> and just make it possible to override it (either by removing the note that
> something is a patch, or by making something that's not listed as a patch
> become marked as such)?

One counter-idea would be to assume every attachment is a patch _unless_
the attachment type matches a pattern that identifies it as not a patch.

However, I agree with Tom that we should go a little longer before
changing it.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On Tue, Mar  3, 2015 at 10:58:28AM -0500, Bruce Momjian wrote:
> > Would you suggest removing the automated system completely, or keep it around
> > and just make it possible to override it (either by removing the note that
> > something is a patch, or by making something that's not listed as a patch
> > become marked as such)?
> 
> One counter-idea would be to assume every attachment is a patch _unless_
> the attachment type matches a pattern that identifies it as not a patch.
> 
> However, I agree with Tom that we should go a little longer before
> changing it.

Also, can we look inside the attachment to see if it starts with
'diff<space>'.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + Everyone has their own god. +

On 3/3/15 11:11 AM, Bruce Momjian wrote:
> On Tue, Mar  3, 2015 at 10:58:28AM -0500, Bruce Momjian wrote:
>>> Would you suggest removing the automated system completely, or keep it around
>>> and just make it possible to override it (either by removing the note that
>>> something is a patch, or by making something that's not listed as a patch
>>> become marked as such)?
>>
>> One counter-idea would be to assume every attachment is a patch _unless_
>> the attachment type matches a pattern that identifies it as not a patch.
>>
>> However, I agree with Tom that we should go a little longer before
>> changing it.
> 
> Also, can we look inside the attachment to see if it starts with
> 'diff<space>'.

There are two different issues here:  One, how you detect a patch.  Two,
whether the latest "patch" is the patch of record for the commit fest item.

I think they are both currently wrong, but the second one is more
important.  Of course, if you punt on the first one, you have to do the
second one differently.