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

Hi,

On Fri, Jul 2, 2010 at 23:34, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

On Fri, Jul 2, 2010 at 21:31, Jakub Narebski [off-list ref] wrote:

quoted

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

On Fri, Jul 2, 2010 at 07:54, László ÁSHIN [off-list ref] wrote:

quoted

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

quoted

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.

quoted

quoted

quoted

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

quoted

quoted

quoted

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

quoted

quoted

quoted

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

quoted

quoted

quoted

+            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