First CommitFest: July 15th

[Zdenek Kotala]

Peter Eisentraut píše v pá 03. 07. 2009 v 09:19 +0300:

> On Friday 03 July 2009 05:16:41 Robert Haas wrote:
> > On Thu, Jul 2, 2009 at 3:42 PM, Zdenek Kotala<Zdenek.Kotala@...> wrote:
> > > Josh Berkus píše v st 01. 07. 2009 v 17:21 -0700:
> > >> Folks,
> > >>
> > >> There's been a lot of discussion/argument around how to handle the last
> > >> commitfest, but there seems to be a total consensus that we want to have
> > >> the first CF on July 15th.
> > >
> > > Can we add flags like bump catalog version, bump page layout version,
> > > modify AM for each patch? It should help to track pg_upgrade changes.
> >
> > That's not a bad idea, and it wouldn't be hard to add various flags
> > and things to the CommitFest app I wrote, but how would we maintain
> > the information and keep it correct?  It seems like there might be a
> > danger that patch authors wouldn't know whether or not they were doing
> > those things.  Also, how would we handle changes by committers, who
> > don't always go through the CommitFest process?
>
> I think this information could be computed automatically, if someone cared
> enough.  It shouldn't be necessary to bother every single participant in the
> process with this.

I think that developer is responsible for his patch. He should know what
he doing. When he will send a patch and see checkbox like "modified AM?"
then he should know what he modified? It is also warning for commiter
that catalog version has to be bumped during a commit.

I don't see any method how to check automatically. Something could be
possible - like structure checker. But when meaning of data is going to
be different.

        Zdenek







--
Sent via pgsql-hackers mailing list (pgsql-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[Zdenek Kotala]

Robert Haas píše v čt 02. 07. 2009 v 22:16 -0400:
> On Thu, Jul 2, 2009 at 3:42 PM, Zdenek Kotala<Zdenek.Kotala@...> wrote:

> Also, how would we handle changes by committers, who
> don't always go through the CommitFest process?

I think that all head patches should go to through a new tool for
recording also in case when developer is commiter itself.

        Zdenek


--
Sent via pgsql-hackers mailing list (pgsql-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[Kevin Grittner]

"Dickson S. Guedes" <listas@...> wrote:
 
> pgcommitfest tables sctructure [1]?
>
> [1]  
>
http://git.postgresql.org/gitweb?p=pgcommitfest.git;a=blob;f=etc/table.sql;h=c60a298c863ef3e88dcfd16572781d2b435ca629;hb=HEAD
 
On minor quibble with this schema:  I believe that session.login_time
should be TIMESTAMP WITH TIME ZONE.
 
-Kevin

--
Sent via pgsql-hackers mailing list (pgsql-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[Robert Haas]

On Fri, Jul 3, 2009 at 3:22 PM, Zdenek Kotala<Zdenek.Kotala@...> wrote:
> Robert Haas píše v čt 02. 07. 2009 v 22:16 -0400:
>> On Thu, Jul 2, 2009 at 3:42 PM, Zdenek Kotala<Zdenek.Kotala@...> wrote:
>
>> Also, how would we handle changes by committers, who
>> don't always go through the CommitFest process?
>
> I think that all head patches should go to through a new tool for
> recording also in case when developer is commiter itself.

You'll have to put that argument to the committers; but I expect a
cool reception.  I think what would be more useful is if we could
somehow associated metadata with each commit.  Right now, for example,
the author of a patch is not stored with the patch in any structured
way; it's just typed in, usually but not always as the last line of
the commit.  So you can't easily find out what lines of code a certain
person has touched, for example.  The sorts of problems that you're
talking about seem broadly in the same vein.

...Robert

--
Sent via pgsql-hackers mailing list (pgsql-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[Ron Mayer]

Josh Berkus wrote:
> Folks,...the first CF on July 15th.

Would it make the CommitFest easier if there were an additional
column which indicates what CVS-version of Postgres the patch
cleanly applies to?

Perhaps a patch submitter could indicate the CVS date/time
with which he developed the patch.  If a reviewer happens
to apply the patch on a later version he could update it as
cleanly applying at that later date.

Commiters could feel free to ignore patches that are
sufficiently far off of HEAD, so it might make work easier
for them too.



--
Sent via pgsql-hackers mailing list (pgsql-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[Andrew Dunstan]

Ron Mayer wrote:

> Josh Berkus wrote:
>> Folks,...the first CF on July 15th.
>
> Would it make the CommitFest easier if there were an additional
> column which indicates what CVS-version of Postgres the patch
> cleanly applies to?
>
> Perhaps a patch submitter could indicate the CVS date/time
> with which he developed the patch.  If a reviewer happens
> to apply the patch on a later version he could update it as
> cleanly applying at that later date.
>
> Commiters could feel free to ignore patches that are
> sufficiently far off of HEAD, so it might make work easier
> for them too.
>
>

I think the patch should apply cleanly to HEAD at the time it is
submitted. The actual CVS versions should be visible in the patch.
Normally we will try to take care of any subsequent bitrot - I don't
think developers should have to pay too high a price for our processes.
Occasionally a developer will be asked to help in removing the bitrot,
but that is usually the first thing I try to do in a review, after
applying the simple eyeballs test.

In theory this is an area where a more sophisticated SCM system will
help us some.

(Actually, you often learn a lot that way - it's a good exercise for all
of us.)

cheers

andrew

--
Sent via pgsql-hackers mailing list (pgsql-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[Peter Eisentraut-2]

On Saturday 04 July 2009 00:54:11 Robert Haas wrote:
> I think what would be more useful is if we could
> somehow associated metadata with each commit.  Right now, for example,
> the author of a patch is not stored with the patch in any structured
> way; it's just typed in, usually but not always as the last line of
> the commit.  So you can't easily find out what lines of code a certain
> person has touched, for example.  The sorts of problems that you're
> talking about seem broadly in the same vein.

I have been trying to follow a convention on-and-off to put the author of the
patch in the last line of the commit message, like

Author: First Last <name@...>

A tool such as git-cvsimport will actually parse that and put it into the
author field of a git commit.  (The tool we use, fromcvs, doesn't do that, but
it could conceivably be patched easily to do it.)

I also found the following resource helpful in crafting commit messages:
http://www.tpope.net/node/106

--
Sent via pgsql-hackers mailing list (pgsql-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[Bruce Momjian-5]

Peter Eisentraut wrote:

> On Saturday 04 July 2009 00:54:11 Robert Haas wrote:
> > I think what would be more useful is if we could
> > somehow associated metadata with each commit.  Right now, for example,
> > the author of a patch is not stored with the patch in any structured
> > way; it's just typed in, usually but not always as the last line of
> > the commit.  So you can't easily find out what lines of code a certain
> > person has touched, for example.  The sorts of problems that you're
> > talking about seem broadly in the same vein.
>
> I have been trying to follow a convention on-and-off to put the author of the
> patch in the last line of the commit message, like
>
> Author: First Last <name@...>

Sure, I can use that format if we decide to be consistent.

> A tool such as git-cvsimport will actually parse that and put it into the
> author field of a git commit.  (The tool we use, fromcvs, doesn't do that, but
> it could conceivably be patched easily to do it.)
>
> I also found the following resource helpful in crafting commit messages:
> http://www.tpope.net/node/106

Interesting idea to have a subject line for the commit message.

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[David Fetter]

On Mon, Jul 06, 2009 at 09:12:55AM -0400, Bruce Momjian wrote:

> Peter Eisentraut wrote:
> > On Saturday 04 July 2009 00:54:11 Robert Haas wrote:
> > > I think what would be more useful is if we could
> > > somehow associated metadata with each commit.  Right now, for example,
> > > the author of a patch is not stored with the patch in any structured
> > > way; it's just typed in, usually but not always as the last line of
> > > the commit.  So you can't easily find out what lines of code a certain
> > > person has touched, for example.  The sorts of problems that you're
> > > talking about seem broadly in the same vein.
> >
> > I have been trying to follow a convention on-and-off to put the author of the
> > patch in the last line of the commit message, like
> >
> > Author: First Last <name@...>
>
> Sure, I can use that format if we decide to be consistent.
>
> > A tool such as git-cvsimport will actually parse that and put it into the
> > author field of a git commit.  (The tool we use, fromcvs, doesn't do that, but
> > it could conceivably be patched easily to do it.)
> >
> > I also found the following resource helpful in crafting commit messages:
> > http://www.tpope.net/node/106
>
> Interesting idea to have a subject line for the commit message.

It would help me a lot when putting together the patches section in
the PostgreSQL Weekly News.

Cheers,
David.
--
David Fetter <david@...> http://fetter.org/
Phone: [phone]  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@...

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

--
Sent via pgsql-hackers mailing list (pgsql-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[Bruce Momjian-5]

David Fetter wrote:

> On Mon, Jul 06, 2009 at 09:12:55AM -0400, Bruce Momjian wrote:
> > Peter Eisentraut wrote:
> > > On Saturday 04 July 2009 00:54:11 Robert Haas wrote:
> > > > I think what would be more useful is if we could
> > > > somehow associated metadata with each commit.  Right now, for example,
> > > > the author of a patch is not stored with the patch in any structured
> > > > way; it's just typed in, usually but not always as the last line of
> > > > the commit.  So you can't easily find out what lines of code a certain
> > > > person has touched, for example.  The sorts of problems that you're
> > > > talking about seem broadly in the same vein.
> > >
> > > I have been trying to follow a convention on-and-off to put the author of the
> > > patch in the last line of the commit message, like
> > >
> > > Author: First Last <name@...>
> >
> > Sure, I can use that format if we decide to be consistent.
> >
> > > A tool such as git-cvsimport will actually parse that and put it into the
> > > author field of a git commit.  (The tool we use, fromcvs, doesn't do that, but
> > > it could conceivably be patched easily to do it.)
> > >
> > > I also found the following resource helpful in crafting commit messages:
> > > http://www.tpope.net/node/106
> >
> > Interesting idea to have a subject line for the commit message.
>
> It would help me a lot when putting together the patches section in
> the PostgreSQL Weekly News.

OK, someone want to write a wiki that explains our new preferred commit
message format?

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[Joshua Tolley]

On Thu, Jul 02, 2009 at 03:42:56PM +0100, Dave Page wrote:

> On Thu, Jul 2, 2009 at 3:22 PM, Joshua Tolley<eggyknap@...> wrote:
> > On Thu, Jul 02, 2009 at 08:41:27AM +0100, Dave Page wrote:
> >> As far as I'm aware, there's been no code
> >> review yet either, which would probably be a good idea.
> >
> > I don't have loads of time in the coming days, but IIRC I've taken a glance at
> > a past version of the code, and would be willing to do so again, if it would
> > be useful.
>
> If you can look over it, that would be great. i'm not really qualified
> to review perl code, and we always prefer to have at least two sets of
> eyeballs on anything that we put into production for obvious reasons.

On the assumption that other folks' testing has included bug hunting and the
like, I've spent the review time I was able to muster up figuring out the code
and looking for stuff that scared me. I didn't find anything that jumped out.
I did wonder if the %ACTION hash in Handler.pm ought not perhaps include a
flag to indicate that that action needs authentication, and have the handler
take care of that instead of the individual actions. Perhaps a similar
technique could be profitably employed for the titles. Oh, and in Patch.pm,
s/with/which in "patches with have been Committed".

Finally, I ran Perl::Critic, and attached an (admittedly untested) patch to
clean up the things it whined about.

--
Joshua Tolley / eggyknap
End Point Corporation
http://www.endpoint.com

---

diff --git a/perl-lib/PgCommitFest/CommitFestTopic.pm b/perl-lib/PgCommitFest/CommitFestTopic.pm
index 3e101fc..92113ac 100644
--- a/perl-lib/PgCommitFest/CommitFestTopic.pm
+++ b/perl-lib/PgCommitFest/CommitFestTopic.pm
@@ -80,7 +80,8 @@ EOM
 sub search {
  my ($r) = @_;
  my $id = $r->cgi_id();
- my $d = $r->db->select_one(<<EOM, $id) if defined $id;
+ my $d;
+    $d = $r->db->select_one(<<EOM, $id) if defined $id;
 SELECT id, name FROM commitfest_view WHERE id = ?
 EOM
  $r->error_exit('CommitFest not found.') if !defined $d;
diff --git a/perl-lib/PgCommitFest/DB.pm b/perl-lib/PgCommitFest/DB.pm
index 4719007..adeecdb 100644
--- a/perl-lib/PgCommitFest/DB.pm
+++ b/perl-lib/PgCommitFest/DB.pm
@@ -118,7 +118,7 @@ sub select_one {
 sub tidy {
  my ($self) = @_;
  $self->{'dbh'}->rollback() if $self->{'dirty'};
- return undef;
+ return;
 }
 
 sub update {
diff --git a/perl-lib/PgCommitFest/Handler.pm b/perl-lib/PgCommitFest/Handler.pm
index d94e042..19c4424 100644
--- a/perl-lib/PgCommitFest/Handler.pm
+++ b/perl-lib/PgCommitFest/Handler.pm
@@ -103,9 +103,9 @@ EOM
  $pg_login_db->disconnect;
  if (defined $u) {
  my $random_bits;
- open(RANDOM_BITS, '</dev/urandom') || die "/dev/urandom: $!";
- sysread(RANDOM_BITS, $random_bits, 16);
- close(RANDOM_BITS);
+ open(my $RANDOM_BITS, '<', '/dev/urandom') || die "/dev/urandom: $!";
+ sysread($RANDOM_BITS, $random_bits, 16);
+ close($RANDOM_BITS);
  my $session_cookie = unpack("H*", $random_bits);
  $r->db->insert('session', { 'id' => $session_cookie,
  'userid' => $u->{'userid'} });
diff --git a/perl-lib/PgCommitFest/Patch.pm b/perl-lib/PgCommitFest/Patch.pm
index fe9a720..aebec55 100644
--- a/perl-lib/PgCommitFest/Patch.pm
+++ b/perl-lib/PgCommitFest/Patch.pm
@@ -161,7 +161,8 @@ sub view {
  my ($r) = @_;
  my $aa = $r->authenticate();
  my $id = $r->cgi_id();
- my $d = $r->db->select_one(<<EOM, $id) if defined $id;
+    my $d;
+ $d = $r->db->select_one(<<EOM, $id) if defined $id;
 SELECT id, name, commitfest_id, commitfest, commitfest_topic_id,
  commitfest_topic, patch_status, author, reviewers, date_closed
 FROM patch_view WHERE id = ?
diff --git a/perl-lib/PgCommitFest/Request.pm b/perl-lib/PgCommitFest/Request.pm
index de6d32f..c1042ba 100644
--- a/perl-lib/PgCommitFest/Request.pm
+++ b/perl-lib/PgCommitFest/Request.pm
@@ -22,7 +22,7 @@ $CGI::DISABLE_UPLOADS = 1;  # No need for uploads at present.
 sub new {
  my ($class, $db) = @_;
  my $cgi = CGI::Fast->new();
- return undef if !defined $cgi;
+ return if !defined $cgi;
  bless {
  'cgi' => $cgi,
  'control' => {},
diff --git a/perl-lib/PgCommitFest/WebControl.pm b/perl-lib/PgCommitFest/WebControl.pm
index 7443a60..11af6bd 100644
--- a/perl-lib/PgCommitFest/WebControl.pm
+++ b/perl-lib/PgCommitFest/WebControl.pm
@@ -42,7 +42,7 @@ sub choice {
  $self->{'value_key'} = 'id' if !defined $self->{'value_key'};
  $self->{'text_key'} = 'name' if !defined $self->{'text_key'};
  $self->{'choice'} = $choice_list;
- return undef;
+ return;
 }
 
 sub db_value {

---

[Robert Haas]

On Wed, Jul 8, 2009 at 1:11 AM, Joshua Tolley<eggyknap@...> wrote:

> On Thu, Jul 02, 2009 at 03:42:56PM +0100, Dave Page wrote:
>> On Thu, Jul 2, 2009 at 3:22 PM, Joshua Tolley<eggyknap@...> wrote:
>> > On Thu, Jul 02, 2009 at 08:41:27AM +0100, Dave Page wrote:
>> >> As far as I'm aware, there's been no code
>> >> review yet either, which would probably be a good idea.
>> >
>> > I don't have loads of time in the coming days, but IIRC I've taken a glance at
>> > a past version of the code, and would be willing to do so again, if it would
>> > be useful.
>>
>> If you can look over it, that would be great. i'm not really qualified
>> to review perl code, and we always prefer to have at least two sets of
>> eyeballs on anything that we put into production for obvious reasons.
>
> On the assumption that other folks' testing has included bug hunting and the
> like, I've spent the review time I was able to muster up figuring out the code
> and looking for stuff that scared me. I didn't find anything that jumped out.
> I did wonder if the %ACTION hash in Handler.pm ought not perhaps include a
> flag to indicate that that action needs authentication, and have the handler
> take care of that instead of the individual actions.

Possibly so.  We may also find that it needs to be a bit more
fine-grained than that, as there are already three levels of access
(public, login required, administrator login required) and there could
theoretically be more in the future.

> Perhaps a similar
> technique could be profitably employed for the titles. Oh, and in Patch.pm,
> s/with/which in "patches with have been Committed".

Fixed, thanks.

> Finally, I ran Perl::Critic, and attached an (admittedly untested) patch to
> clean up the things it whined about.

As usual, I'm unimpressed by the whining emitted by Perl::Critic.  I
can understand that if a function is really intended to return void
(but perl doesn't have that concept) then you probably ought to write
just "return" rather than "return undef".  But if the function
sometimes returns a value and sometimes returns "undef", insisting
that the word "undef" not be spelled out explicitly seems pretty
silly.

The other changes have marginally more merit, though some of them
break with surrounding whitespace conventions.

...Robert

--
Sent via pgsql-hackers mailing list (pgsql-hackers@...)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers