Thanks for the response.
I don't agree that unlocking is unnecessary, since locks are released
automatically when a file handle is closed.
I'll talk to the muckitymucks here about whether I can use the BSD or
MIT license instead of GPL. I don't think it'll be a problem, but I'll
confirm.
I agree with all of your other proposed changes and will try to get them
done and submit a revised version of the script to the list ASAP. It
might not be for a week or two since we're feverishly working on
finishing up our migration to Svn and there's a tight deadline on that,
so the work on that project needs to take precedence.
The two code reviews I've gotten for this script from people on the list
illustrate one of the most important reasons why I like giving away the
software that I write whenever possible. If it's interesting to people,
I can be sure that I'm going to get feedback telling me exactly what I
can and should do to improve it :-).
Thanks,
jik
-----Original Message-----
From: james82_at_gmail.com [mailto:james82_at_gmail.com] On Behalf Of David
James
Sent: Friday, February 22, 2008 11:04 AM
To: Jonathan Kamens
Cc: dev_at_subversion.tigris.org
Subject: Re: CGI script for self-administering password in svnserve
passwd files
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:16:43 CET