Re: [GSoC PATCH v7 2/2] send-email: finer-grained SMTP error handling

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

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

> +		if ($error) {

This block is full of overly long lines.  Would it make sense to
turn it into a helper sub and do just this here in the block

			return handle_smtp_error($error);

And then the remainder of the block would become a single helper
function that is called only from here, losing 2 levels of
indentation.

> +			# check if an error was captured
> +			# parse SMTP status code from error message in:
> +			# https://www.rfc-editor.org/rfc/rfc5321.html
> +			if ($error =~ /\b(\d{3})\b/) {
> +				my $status_code = $1;
> +				if ($status_code =~ /^4/) {
> +					# 4yz: Transient Negative Completion reply
> +					warn "SMTP temporary error (status code $status_code): $error";
> +					return 1;
> +				} elsif ($status_code =~ /^5/) {
> +					# 5yz: Permanent Negative Completion reply
> +					warn "SMTP permanent error (status code $status_code): $error";
> +					return 0;
> +				} else {
> +					# if no recognized status code is found, treat as transient error
> +					warn "SMTP unknown error: $error. Treating as permanent failure.";

The comment and warning message say different things.  I suspect
that the warning message is wrong, as the branch returns 1 to signal
the caller to do the same thing a the 4yz transient case?

> +					return 1;
> +				}
> +			} else {
> +				# if no status code is found, treat as transient error
> +				warn "SMTP generic error: $error";
> +				return 1;
> +			}
> +		} elsif (!defined $result) {
> +			# if no error and no result is returned, treat as transient error
> +			warn "SMTP no result error: $error";
> +		    return 1; 
> +		}
> +		else {
> +			return $result ? 1 : 0;
> +		}
>  	});
>  
>  	return $auth;