Re: [GSoC PATCH v3 1/1] Unify SMTP auth error handling

[Junio C Hamano <gitster@pobox.com>]

Zheng Yuting <05zyt30@gmail.com> writes:

> Refactored SMTP authentication to use a unified error capture block for
> both SASL and plain methods. Errors are now handled by parsing SMTP status
> codes (4yz for transient, 5yz for permanent) instead of relying on regex
> matching. This change improves clarity .

"improves clarity ." is (not well formatted and) a bit subjective
and does not apply to all three changes the patch is making here,
does it?

> Signed-off-by: Zheng Yuting <05ZYT30@gmail.com>
> ---
>  git-send-email.perl | 72 +++++++++++++++++++++++++++------------------
>  1 file changed, 43 insertions(+), 29 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index a012d61abb..532dda264c 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1411,7 +1411,7 @@ sub smtp_auth_maybe {
>  	eval {
>  		require Authen::SASL;
>  		Authen::SASL->import(qw(Perl));
> -	};
> +	}

Hmph, the interpreter may tolerate the new block-eval "eval {}"
simple statement that lacks terminating ';' but is this an
improvement?  The original look more kosher from syntactic point of
view.  It seems to be totally unrelated change from the rest of the
patch.

> @@ -1426,42 +1426,56 @@ sub smtp_auth_maybe {
>  		'protocol' => 'smtp',
>  		'host' => smtp_host_string(),
>  		'username' => $smtp_authuser,
> +		# if there's no password, "git credential fill" will
> +		# give us one, otherwise it'll just pass this one.
>  		'password' => $smtp_authpass
> -

We seem to already have the comment added by this hunk, since
4d31a44a (git-send-email: use git credential to obtain password,
2013-02-12).  Am I looking at a wrong version of the source (or a
wrong version of the patch)?

>  	}, sub {
>  		my $cred = shift;
>  		my $result;
>  		my $error;
> -		if ($smtp_auth) {
> -			my $sasl = Authen::SASL->new(
> -				mechanism => $smtp_auth,
> -				callback => {
> -					user => $cred->{'username'},
> -					pass => $cred->{'password'},
> -					authname => $cred->{'username'},
> -				}
> -			);
> -			return !!$smtp->auth($sasl);
> -		} else {
> -			# Handle plain authentication errors
> -			eval {

And curiously we do not seem to have this else clause with the
comment that is getting removed.

> +		# catch all SMTP auth error
> +		eval {
> +			if ($smtp_auth) {
> +				my $sasl = Authen::SASL->new(
> +					mechanism => $smtp_auth,
> +					callback => {
> +						user => $cred->{'username'},
> +						pass => $cred->{'password'},
> +						authname => $cred->{'username'},
> +					}
> +				);
> +				$result = $smtp->auth($sasl);
> +			} else {
>  				$result = $smtp->auth($cred->{'username'}, $cred->{'password'});
> -				1; # Ensure true value is returned
> -			} or do {
> -				$error = $@ || 'Unknown error';
> -			};
> -		}
> -		# Unified error handling logic
> +			}
> +			1; # Ensure true value is returned if no exception is thrown.
> +		} or do {
> +			$error = $@ || 'Unknown error';
> +		};
> +
> +		#check if an error was captured

As I do not see two evals in our copy of git-send-email.perl source,
it may be moot at this point to comment on this patch, but if we did
have a eval block each of the if/else arms, moving the control
structure around and turning "if eval {} else eval {}" into "eval {
if ... else ...}" may make it cleaner to see what is going on,
especially if we plan to extend the choices and add elsif to the
chain later.

>  		if ($error) {
> -			# Match temporary errors
> -			if ($error =~ /timeout|temporary|greylist|throttled|quota\s+exceeded|queue|overload|try\s+again|connection\s+lost|network\s+error/i) {
> -				warn "SMTP temporary error: $error";
> -				return 1;
> +			#Parse SMTP status code from error message in:
> +			#https://www.rfc-editor.org/rfc/rfc5321.html

Have a SP between "#" and the comment body.

Using the numeric error codes allows us to give more precise errors,
which should be a good change that can be done regardless of the
eval change.  IOW, this part should be in a separate patch on its
own, either before or after the if-eval-else-eval change. 

I'll stop here, as the patch does not seem to be designed to apply
to our source tree.