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

From: Áshin László <ashinlaszlo@gmail.com>
Date: Jul 3, 2010, 09:28 UTC
Message-ID: <AANLkTinLgbrd-tOLntZt-FZkGVtTHXU2lteB-t7NAjSm@mail.gmail.com>
In-Reply-To: <AANLkTimts_MlrWGKaiu3frk7vOnK9B70RE7It2Wml3on@mail.gmail.com>

Hi,

On Fri, Jul 2, 2010 at 23:34, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 27 quoted lines

> 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': $!";
>>

It will be fixed, I will resend the patch.

>>> > +        } else {
>>> > +            if (not exists $cfg->{gitcvs}->{authdb}) {
>>
>> Why not elsif?

Because the else branch continues below. But good point, it can be done that way too. Will be fixed.

Show 6 quoted lines

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

OK. Next patch will be fixed from this point of view as well.

Show 10 quoted lines

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

I wanted to keep the old functionality (authdb), so I moved all of its code to this else branch. That move meant I had to increase indentation level. But as far as I see now I could have solved it somehow else to keep the indentation level. I will fix this too.

Show 9 quoted lines

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

Good point, but it is not my code.

So, I will resend this patch - the corrected one without the mistakes I taken in the first version of it. Should I make one patch for each of the code, doc, and test case, or can they all go into one patch?

Regards, László ÁSHIN