From: Jakub Narebski <jnareb@gmail.com>
Date: Fri, 02 Jul 2010 21:31:48 GMT
Subject: Re: [PATCH] git-cvsserver: pserver-auth-script
Message-ID: <m31vbloa6m.fsf@localhost.localdomain>
In-Reply-To: <AANLkTinGZsCLF8CFKgQJaB63Jh9anGfQK4ZSHFPl4R4N@mail.gmail.com>

Æ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.

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.

> > +            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