Re: [PATCH] git-cvsserver: pserver-auth-script

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Jul 2, 2010 at 21:31, Jakub Narebski <jnareb@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> On Fri, Jul 2, 2010 at 07:54, László ÁSHIN <laszlo.ashin@neti.hu> wrote:
>>
>> > +
>> > +            open SCRIPTIN, '|' . $authscript or die $!;
>> > +            print SCRIPTIN $user . "\n";
>> > +            print SCRIPTIN descramble($password) . "\n";
>> > +            close SCRIPTIN;
>>
>> Nit: Nice use of three-arg open, but you should use lexical
>> filehandles instead. I.e.:
>>
>>     open my $script, '|' . $authscript or die $!;
>>     ...
>
> This is two-argument open, not three-argument magic open.  There is
> string concatenation operator '.' there, not a comma ',' delimiting
> arguments.

Well spotted, I misread that.

> It should be
>
>   open my $script_fd, '|-', $authscript
>        or die "Couldn't open authentication script '$authscript': $!";
>
>> > +        } else {
>> > +            if (not exists $cfg->{gitcvs}->{authdb}) {
>
> Why not elsif?
>
>> > +                print "E the repo config file needs a [gitcvs] section with an 'authdb' parameter set to the filename of the authentication database\n";
>
> Overly long line.  Perhaps it would be better to split it into
> concatenated parts.
>
>
>> > +            my $auth_ok;
>> > +            open my $passwd, "<", $authdb or die $!;
>
> And here you use three-argument form of (ordinary) open.

This and the code below were already part of the code. But the patch
would be better if it didn't move so much code around so that this
would be obvious.

>> > +            while (<$passwd>) {
>> > +                if (m{^\Q$user\E:(.*)}) {
>> > +                    if (crypt($user, descramble($password)) eq $1) {
>
> Why nested if, and not short-circuit &&?
>
>    +                if (/^\Q$user\E:(.*)/ &&
>    +                    crypt($user, descramble($password)) eq $1) {
>
> --
> Jakub Narebski
> Poland
> ShadeHawk on #git
>