On Thu, Feb 21, 2008 at 12:42 PM, Jonathan Kamens
<jonathan.kamens_at_tamalesoftware.com> wrote:
> Does the fact that there has been no response to the email below mean
> that there is no interest in distributing this CGI script with
> Subversion or that I have failed to attract the attention of the people
> who would be involved in deciding whether to do so?
>
> If the latter, can anyone suggest how I might attract their attention?
>
> Thanks,
>
> jik
Hi Jonathan,
This script looks like it'd be a useful thing to include in the
contrib directory. To get script this into the contribution you need a
committer to sponsor you for partial commit access. I could probably
do that for you but I think we need to clean up the script to make it
a bit easier to review. My suggestions are included below.
I only noticed two functional issues with the script:
1) It'd be a good idea to unlock $fh when you are done with it, per
http://perldoc.perl.org/functions/flock.html
2) It'd probably be a good idea to die if %repos isn't set, rather
than defaulting to some test repository.
License compatibility: Could you release your program under a more
permissive license, such as BSD/MIT? Just asking. (GPL license
compatibility issues are often a pain, and if you use a more
permissive license then we don't need to worry about that.)
I also have a few stylistic suggestions:
1) I think that the locking of $fh should be moved from
lock_password_file function into the update_password_file function.
When I first read the script I missed the fact that the two functions
are operating on the same global variable. After you do this, could
you make the "$fh" variable a local variable for the
"update_password_file" function? This will get rid of a (potentially
confusing) global variable.
2) I found it a bit confusing that all of the functions modify the
global variable '$error'. Could we instead update the functions to
call an appropriate function which prints out an error message (in
helpful HTML format, with any appropriate forms included) and exits?
3) Can we split the program into functions which represent the tasks
which need to be done? Also, let's get rid of the 'doit' function. See
below.
Here's a list of the tasks your script does:
- read_config: Read the raw config from disk and validate it.
- read_form_input: Read the form input from the user and validate it.
- check_password: Make sure that the user password is correct
- update_password_file: Update the password file
- print_html: Print the HTML form (and any errors or messages).
Maybe you could create functions for each of the above? Include a
short comment (similar to the above) at the top of each function.
Here's an example of what your updated program (with functions) might look like:
my (%repos) = read_config();
my ($passwd_file, %input) = read_form_input(%repos);
my ($updated_password) = 0;
if ($passwd_file) {
check_password($passwd_file, %input);
update_password_file($passwd_file, %input);
$updated_password = 1;
}
print_html($updated_password, %input);
Of course your final program might look different -- I'm just trying
to give you an idea.
HTH,
David
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-02-22 17:04:43 CET