On Thu, Jul 02, 2009 at 03:42:56PM +0100, Dave Page wrote: > On Thu, Jul 2, 2009 at 3:22 PM, Joshua Tolley<eggyk...@gmail.com> 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 {
signature.asc
Description: Digital signature
