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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 25, 2025, 15:34 UTC
Message-ID: <xmqqmsd9m8e6.fsf@gitster.g>
In-Reply-To: <20250324145332.571813-3-05ZYT30@gmail.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 ...

Show 5 quoted lines

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

Show 27 quoted lines

> +	# 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;