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

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

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

> -		# NOTE: SMTP status code handling will be added in a subsequent commit,
> -		# return 1 when failed due to non-credential reasons
> -		return $error ? 1 : ($result ? 1 : 0);
> +		return handle_smtp_error($error, $result);

It was a bit surprising that the new handle-smtp-error sub handles
the case without an error.  I would actually have expected for it to
be something like:

		return ($error
                        ? handle_smtp_error($error)
			: ($result ? 1 : 0));

I.e., we used to unconditionally return 1 upon error, and the only
change introduced by this step is to classify $error with the helper
function better and behave differently depending on the error.

Having said that ...

> +sub handle_smtp_error {
> +	my ($error, $result) = @_;
> +
> +	# If no error is present, return the result directly
> +	return $result ? 1 : 0 unless $error;

... as the "no error" case is implemented as an early return, the
mental burden on the readers is not so bad.  They can concentrate on
the error case when reading the remainder of the function.

Still, it would be with even less mental burden if the no-error case
is handled by the caller to make this function only about error cases.

> +	# 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 transient 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;
> +		}
> +		# If no recognized status code is found, treat as transient error
> +		warn "SMTP unknown error: $error. Treating as transient failure.";
> +		return 1;
> +	}
> +
> +	# If no status code is found, treat as transient error
> +	warn "SMTP generic error: $error";
> +	return 1;
> +}
> +
>  sub ssl_verify_params {
>  	eval {
>  		require IO::Socket::SSL;