Date: Mon, 5 Mar 2007 14:56:13 -0800
Message-Id: <200703052256.l25MuDoA000712@recluse.mozilla.org>
From: bugzilla-daemon@mozilla.org
To: bhearsum@wittydomain.com
Subject: [Bug 370241] Buildbot patch uploader
In-Reply-To: <bug-370241-259016@https.bugzilla.mozilla.org/>
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0

Do not reply to this email.  You can add comments to this bug at
https://bugzilla.mozilla.org/show_bug.cgi?id=370241


J. Paul Reed [:preed] <preed@mozilla.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #257155|review?(preed@mozilla.com)  |review+
               Flag|                            |




--- Comment #22 from J. Paul Reed [:preed] <preed@mozilla.com>  2007-03-05 14:56:10 PST ---
(From update of attachment 257155)

In general, it may be useful to look over these:

https://intranet.mozilla.org/Build:PerlStyleGuide

(Most notably for captilization/indentation and function call semantics.)

># the size limit for the file, in bytes
>my $SIZELIMIT = 10*1024*1024;

Might add to the comment that this is 10 megs.

>sub write_success_page {
>    print "Content-type: text/html\n\n";
>    print <<ENDOFHTML;

I tend to write these as:

print <<__END_OF_HTML__; so they're easier to pick out from the code.

>sub process {
>    my $time = time();
>    # get the parameters
>    my $name = "";
>    $name = $ENV{'REMOTE_USER'};
>    if ($name eq "") {
>        $name = "Noname";
>    }

Isn't it a requirement that people log in? Or are we just doing that for the
trial? If it's a requirement, we should error out if $name eq "".

But I can't remember which we decided.

>    # only allow alphanumeric, hyphens, and single dots
>    if ($patchfile !~ /^([\w-]|\.[\w-])+$/) {
>        # ends the script

This is in the filename, right? Why do we allow single dots? So people can name
them patch.foo.bar?

>    # write the patch
>    my $filename = "$PATCH_DIR/$patchfile";
>    open PATCH, ">$filename" or write_page($description,
>      "Server error - Could not open file for writing.") and return;
>    binmode PATCH;

We probably don't want to blindly overwrite files; there's a race-condition if
you use -e; hrm... something to think about.

For the architectural stuff, I'm mostly ok with the patch as is; I'd like to
see some of the formatting stuff fixed, though.

In general, r=preed. Good job on seeing this through, Ben!


-- 
Configure bugmail: https://bugzilla.mozilla.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
You are the assignee for the bug.
You reported the bug.