Re: [PATCH 2/3] git-cvsserver: protect against NULL in crypt(3)

Carlo Marcelo Arenas Belón  [off-list ref] writes:

-                if (crypt(descramble($password), $1) eq $1) {
-                    $auth_ok = 1;
+                my $hash = crypt(descramble($password), $1);
+                if (defined $hash) {
+                    $auth_ok = 1 if $hash eq $1;
                 }

It is not wrong per-se to separate the two checks into two separate
parts of the conditional, but because we check for definedness only
because comparison of it with $1 makes sense only when it is
defined, writing it either like this, 

		if (defined $hash and $hash eq $1) {
			$auth_ok = 1;
		}

or even like this,

		$auth_ok = (defined $hash and $hash eq $1);

may be easier to read, no?

On Thu, Sep 16, 2021 at 3:11 PM Junio C Hamano [off-list ref] wrote:

Carlo Marcelo Arenas Belón  [off-list ref] writes:

quoted

-                if (crypt(descramble($password), $1) eq $1) {
-                    $auth_ok = 1;
+                my $hash = crypt(descramble($password), $1);
+                if (defined $hash) {
+                    $auth_ok = 1 if $hash eq $1;
                 }

It is not wrong per-se to separate the two checks into two separate
parts of the conditional, but because we check for definedness only
because comparison of it with $1 makes sense only when it is
defined, writing it either like this,

                if (defined $hash and $hash eq $1) {
                        $auth_ok = 1;
                }

or even like this,

                $auth_ok = (defined $hash and $hash eq $1);

may be easier to read, no?

yes, let's go with the earlier; I was trying to mimic the original
code, but agree on a second read that it looks confusing.
assuming there are no more comments, would you want a reroll?

Carlo