threads / patch / 63097

v3Refactor SMTP Auth Error Handling

Subject: [GSoC PATCH v3 0/1] Refactor SMTP Auth Error Handling

## tl;dr

27 messages between Mar 12, 2025 and Mar 26, 2025. Diffs are folded; open one to read it.

replies: 26people: 3as markdown or json

Zheng Yuting· Mar 12, 2025, 06:46 UTC · lore

This patch unifies error capture for both SASL and plain SMTP authentication. It replaces regex-based error detection with SMTP status code parsing, differentiating transient (retryable) errors from permanent failures.

Zheng Yuting (1):
  SMTP Auth: Use status codes to differentiate transient vs. permanent
    errors
 git-send-email.perl | 72 +++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 29 deletions(-)

-- 2.49.0.rc0.57.gdb91954e18

Zheng Yuting· Mar 12, 2025, 06:46 UTC · re: Zheng Yuting · lore

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

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 .

Signed-off-by: Zheng Yuting <05ZYT30@gmail.com>
---
 git-send-email.perl | 72 +++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 29 deletions(-)
Show changes to git-send-email.perl +43 −30
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));
-	};
+	}

 	# Check mechanism naming as defined in:
 	# https://tools.ietf.org/html/rfc4422#page-8
@@ -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
-
 	}, 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 {
+
+		# 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
 		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
+			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;
+				}
+				# If no status code is found, treat as permanent error
+				warn "SMTP unknown error: $error";
+				return 0;
 			}
-			return 0;
-		}
-		return !!$result;
-	});
+			return $result ? 1 : 0;
+		});
+
 	return $auth;
 }

--
2.49.0.rc0.57.gdb91954e18
Junio C Hamano· Mar 13, 2025, 19:58 UTC · re: Zheng Yuting · lore

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

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?

Show 15 quoted lines
> 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.

Show 8 quoted lines
> @@ -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)?

Show 17 quoted lines
>  	}, 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.

Show 27 quoted lines
> +		# 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.

Show 7 quoted lines
>  		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.

Yuting Zheng· Mar 14, 2025, 12:55 UTC · re: Junio C Hamano · lore

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

On Fri Mar 14, 2025 at 3:58 AM CST, Junio C Hamano wrote:

Thank you for the thorough review. As a newcomer, I really appreciate you taking time to help me improve.

> "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?

I'll reformat the commit message and split the patch into more detailed parts.

Show 5 quoted lines
> 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.
I'll revert it to the original state.
Show 7 quoted lines
> 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)?
>
> And curiously we do not seem to have this else clause with the
> comment that is getting removed.

You're correct - this was caused by my failure to rebase before submission.I'll clean up all duplicate comments.

Show 7 quoted lines
> 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.
I'll use if/else structure which is more extensible.
> Have a SP between "#" and the comment body.

Understood. I'll rigorously adhere to code style guidelines by adding space after comment markers.

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

This was caused by my local branch being several commits behind upstream. I've now synchronized and will resubmit properly.

Zheng Yuting· Mar 16, 2025, 05:09 UTC · re: Junio C Hamano · lore

[GSoC PATCH v4 0/2] smtp_auth_maybe: unified error capture and status code processing optimization

This v4 patch series includes two improvements:
1. Unified error capture:
Consolidate exception handling within a single eval block by introducing
local variables to store results and error states, thereby streamlining
code structure and enabling future extensibility.
2. Status code processing optimization:
After catching the authentication exception, parse the three-digit status
code in the error message, For temporary errors (4yz), only print warnings
and return success, while for permanent errors (5xx), return failure,
Unrecognized status codes are treated as permanent errors by default.
Zheng Yuting (2):
  Unify capture of SMTP errors
  Error handling for SMTP status codes
 git-send-email.perl | 62 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 17 deletions(-)

-- 2.48.1

Zheng Yuting· Mar 16, 2025, 05:09 UTC · re: Zheng Yuting · lore

[GSoC PATCH v4 1/2] Unify capture of SMTP errors

This change adds local variables $result and $error to store authentication return results and exception information respectively.

In the eval block, different auth methods are called depending on whether the SMTP authentication mechanism is specified, and true is returned when there is no exception.

After catching the exception, the error information is saved. This makes the error capture logic more centralized and easier to understand, and lays the foundation for subsequent expansion.

Signed-off-by: Zheng Yuting <05ZYT30@gmail.com>
---
 git-send-email.perl | 45 ++++++++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 19 deletions(-)
Show changes to git-send-email.perl +26 −20
diff --git a/git-send-email.perl b/git-send-email.perl
index 798d59b84f..8feb43e9f7 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1419,7 +1419,7 @@ sub smtp_auth_maybe {
 		die "invalid smtp auth: '${smtp_auth}'";
 	}

-	# TODO: Authentication may fail not because credentials were
+	# Authentication may fail not because credentials were
 	# invalid but due to other reasons, in which we should not
 	# reject credentials.
 	$auth = Git::credential({
@@ -1431,25 +1431,32 @@ sub smtp_auth_maybe {
 		'password' => $smtp_authpass
 	}, sub {
 		my $cred = shift;
+		my $result;
+		my $error;
+
+		# catch all SMTP auth error in a unified eval block
+		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 if no exception is thrown
+		} or do {
+			$error = $@ || 'Unknown 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);
-		}
-
-		return !!$smtp->auth($cred->{'username'}, $cred->{'password'});
-	});
-
-	return $auth;
-}
+		# NOTE: SMTP status code handling will be added in a subsequent commit
+		return $result ? 1 : 0;
+	}

 sub ssl_verify_params {
 	eval {
--
2.48.1
Zheng Yuting· Mar 16, 2025, 05:09 UTC · re: Zheng Yuting · lore

[GSoC PATCH v4 2/2] Error handling for SMTP status codes

This change further parses and processes the captured exception information based on the previous patch's unified error capture. Specifically, a three-digit status code is extracted from the error information through a regular expression, and judged according to the definition of RFC 5321:

- If the status code starts with "4" (temporary error), only a warning is
printed and success (1) is returned to allow subsequent retries;
- If the status code starts with "5" (permanent error), a warning is
printed and failure (0) is returned;
- If the status code is not recognized in the error, it is considered a
permanent error, and an unknown error message is printed and failure is
returned.

In the absence of an error, the authentication result is still returned according to the original logic. This change makes SMTP authentication error handling more refined,

Signed-off-by: Zheng Yuting <05ZYT30@gmail.com>
---
 git-send-email.perl | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)
Show changes to git-send-email.perl +24 −4
diff --git a/git-send-email.perl b/git-send-email.perl
index 8feb43e9f7..69ba328653 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1454,9 +1454,30 @@ sub smtp_auth_maybe {
 			$error = $@ || 'Unknown error';
 		};

-		# NOTE: SMTP status code handling will be added in a subsequent commit
-		return $result ? 1 : 0;
-	}
+		# check if an error was captured
+		if ($error) {
+			# 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;
+				}
+				# if no recognized status code is found, treat as permanent error
+				warn "SMTP unknown error: $error";
+				return 0;
+			}
+			return $result ? 1 : 0;
+		} else {
+			return $result ? 1 : 0;
+		}
+}

 sub ssl_verify_params {
 	eval {
--
2.48.1
Junio C Hamano· Mar 17, 2025, 23:01 UTC · re: Zheng Yuting · lore

Re: [GSoC PATCH v4 0/2] smtp_auth_maybe: unified error capture and status code processing optimization

Zheng Yuting <05zyt30@gmail.com> writes:
Show 16 quoted lines
> This v4 patch series includes two improvements:
>
> 1. Unified error capture:
> Consolidate exception handling within a single eval block by introducing
> local variables to store results and error states, thereby streamlining
> code structure and enabling future extensibility.
>
> 2. Status code processing optimization:
> After catching the authentication exception, parse the three-digit status
> code in the error message, For temporary errors (4yz), only print warnings
> and return success, while for permanent errors (5xx), return failure,
> Unrecognized status codes are treated as permanent errors by default.
>
> Zheng Yuting (2):
>   Unify capture of SMTP errors
>   Error handling for SMTP status codes

Give title your commits following the project convention (Documentation/SubmittingPatches:summary-section).

I think these two can share "sendemail:" as their "<area>:" part.
	sendemail: capture errors in an eval {} block
	sendemail: finer-grained SMTP error handling
or something like that, perhaps.

For both patches, the usual way to compose a log message of this project is to

 - Give an observation on how the current system work in the present
   tense (so no need to say "Currently X is Y", just "X is Y"), and
   discuss what you perceive as a problem in it.
 - Propose a solution (optional---often, problem description
   trivially leads to an obvious solution in reader's minds).
 - Give commands to the codebase to "become like so".

in this order. I got an impression that at least your 1/2 it was unclear which part was explaining the state before the patch and which part was about the state after the patch.

Thanks.
Zheng Yuting· Mar 19, 2025, 02:02 UTC · re: Zheng Yuting · lore

[GSoC PATCH v5 0/2] sendemail: improve error capture and status code handling

This patch series improves SMTP authentication error handling.

Auth relied solely on return values without capturing exceptions, misjudging non-credential errors as authentication failures.

Patch v5 1/2 wraps the auth process in an eval {} block to catch all exceptions, adds var error for future handling, and var result to return auth state.

Patch v5 2/2 introduces finer-grained SMTP error handling, extracting status codes per RFC 5321 to differentiate between temporary (4yz) and permanent (5yz) errors. Unrecognized codes are treated as permanent failures. Otherwise return the authentication result.

Zheng Yuting (2):
  sendemail: capture errors in an eval {} block
  sendemail: finer-grained SMTP error handling
 git-send-email.perl | 62 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 17 deletions(-)

-- 2.48.1

Zheng Yuting· Mar 19, 2025, 02:02 UTC · re: Zheng Yuting · lore

[GSoC PATCH v5 1/2] sendemail: capture errors in an eval {} block

Auth relied solely on return values without catching errors, misjudging non-credential errors as auth failures without details.

Wrap the entire auth process in an eval {} block to catch all exceptions, including non-credential errors.

Add $error to store exceptions for future handling and $result for auth results.

Uses 'or do' to replace direct returns.

Merges if/else branches, integrates SASL and basic auth, with comments for future status code handling.

Signed-off-by: Zheng Yuting <05ZYT30@gmail.com>
---
 git-send-email.perl | 45 ++++++++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 19 deletions(-)
Show changes to git-send-email.perl +26 −20
diff --git a/git-send-email.perl b/git-send-email.perl
index 798d59b84f..8feb43e9f7 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1419,7 +1419,7 @@ sub smtp_auth_maybe {
 		die "invalid smtp auth: '${smtp_auth}'";
 	}

-	# TODO: Authentication may fail not because credentials were
+	# Authentication may fail not because credentials were
 	# invalid but due to other reasons, in which we should not
 	# reject credentials.
 	$auth = Git::credential({
@@ -1431,25 +1431,32 @@ sub smtp_auth_maybe {
 		'password' => $smtp_authpass
 	}, sub {
 		my $cred = shift;
+		my $result;
+		my $error;
+
+		# catch all SMTP auth error in a unified eval block
+		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 if no exception is thrown
+		} or do {
+			$error = $@ || 'Unknown 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);
-		}
-
-		return !!$smtp->auth($cred->{'username'}, $cred->{'password'});
-	});
-
-	return $auth;
-}
+		# NOTE: SMTP status code handling will be added in a subsequent commit
+		return $result ? 1 : 0;
+	}

 sub ssl_verify_params {
 	eval {
--
2.48.1
Zheng Yuting· Mar 19, 2025, 02:02 UTC · re: Zheng Yuting · lore

[GSoC PATCH v5 2/2] sendemail: finer-grained SMTP error handling

Code captured errors but did not process them further. This treated all failures the same without distinguishing SMTP status.

Add a regex to extract status codes as defined in RFC 5321:
- For 4yz (temporary errors), return 1 and allow retries.
- For 5yz (permanent errors), return 0 as failure.
- For unrecognized codes, treat as permanent errors.
- If no error occurs, return the authentication result.
Signed-off-by: Zheng Yuting <05ZYT30@gmail.com>
---
 git-send-email.perl | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)
Show changes to git-send-email.perl +24 −4
diff --git a/git-send-email.perl b/git-send-email.perl
index 8feb43e9f7..69ba328653 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1454,9 +1454,30 @@ sub smtp_auth_maybe {
 			$error = $@ || 'Unknown error';
 		};

-		# NOTE: SMTP status code handling will be added in a subsequent commit
-		return $result ? 1 : 0;
-	}
+		# check if an error was captured
+		if ($error) {
+			# 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;
+				}
+				# if no recognized status code is found, treat as permanent error
+				warn "SMTP unknown error: $error";
+				return 0;
+			}
+			return $result ? 1 : 0;
+		} else {
+			return $result ? 1 : 0;
+		}
+}

 sub ssl_verify_params {
 	eval {
--
2.48.1
Meet Soni· Mar 19, 2025, 06:35 UTC · re: Zheng Yuting · lore

Re: [GSoC PATCH v5 0/2] sendemail: improve error capture and status code handling

On Wed, 19 Mar 2025 at 07:32, Zheng Yuting <05zyt30@gmail.com> wrote:
Show 20 quoted lines
>
> This patch series improves SMTP authentication error handling.
>
> Auth relied solely on return values without capturing exceptions,
> misjudging non-credential errors as authentication failures.
>
> Patch v5 1/2 wraps the auth process in an eval {} block to catch all
> exceptions, adds var error for future handling, and var result to return
> auth state.
>
> Patch v5 2/2 introduces finer-grained SMTP error handling, extracting
> status codes per RFC 5321 to differentiate between temporary (4yz) and
> permanent (5yz) errors. Unrecognized codes are treated as permanent
> failures. Otherwise return the authentication result.
>
>
> Zheng Yuting (2):
>   sendemail: capture errors in an eval {} block
>   sendemail: finer-grained SMTP error handling
>
I'm not sure if this is worth a re-roll but, `sendemail` should be `send-email`.
Show 6 quoted lines
>  git-send-email.perl | 62 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 45 insertions(+), 17 deletions(-)
>
> --
> 2.48.1
>
Thanks
Zheng Yuting· Mar 21, 2025, 02:51 UTC · re: Zheng Yuting · lore

[GSoC PATCH v6 0/2] send-email: improve error capture and status code handling

This patch series improves SMTP authentication error handling.

Auth relied solely on return values without capturing exceptions, misjudging non-credential errors as authentication failures.

Patch v6 1/2 wraps the auth process in an eval {} block to catch all exceptions, adds var error for future handling, and var result to return auth state.

Patch v6 2/2 introduces finer-grained SMTP error handling, extracting status codes per RFC 5321 to differentiate between temporary (4yz) and permanent (5yz) errors. Unrecognized codes are treated as permanent failures. Otherwise return the authentication result.

Zheng Yuting (2):
  send-email: capture errors in an eval {} block
  send-email: finer-grained SMTP error handling
 git-send-email.perl | 62 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 17 deletions(-)

-- 2.48.1

Zheng Yuting· Mar 21, 2025, 02:51 UTC · re: Zheng Yuting · lore

[GSoC PATCH v6 1/2] send-email: capture errors in an eval {} block

Auth relied solely on return values without catching errors. This misjudges non-credential errors as auth failure without error info.

Patch wraps the entire auth process in an eval {} block to catch all exceptions, including non-credential errors. It adds a new $error var, uses 'or do' to prevent flow break, and returns $result ? 1 : 0. And merges if/else branches, integrates SASL and basic auth, with comments for future status code handling.

Signed-off-by: Zheng Yuting <05ZYT30@gmail.com>
---
 git-send-email.perl | 45 ++++++++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 19 deletions(-)
Show changes to git-send-email.perl +26 −20
diff --git a/git-send-email.perl b/git-send-email.perl
index 798d59b84f..8feb43e9f7 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1419,7 +1419,7 @@ sub smtp_auth_maybe {
 		die "invalid smtp auth: '${smtp_auth}'";
 	}

-	# TODO: Authentication may fail not because credentials were
+	# Authentication may fail not because credentials were
 	# invalid but due to other reasons, in which we should not
 	# reject credentials.
 	$auth = Git::credential({
@@ -1431,25 +1431,32 @@ sub smtp_auth_maybe {
 		'password' => $smtp_authpass
 	}, sub {
 		my $cred = shift;
+		my $result;
+		my $error;
+
+		# catch all SMTP auth error in a unified eval block
+		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 if no exception is thrown
+		} or do {
+			$error = $@ || 'Unknown 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);
-		}
-
-		return !!$smtp->auth($cred->{'username'}, $cred->{'password'});
-	});
-
-	return $auth;
-}
+		# NOTE: SMTP status code handling will be added in a subsequent commit
+		return $result ? 1 : 0;
+	}

 sub ssl_verify_params {
 	eval {
--
2.48.1
Zheng Yuting· Mar 21, 2025, 02:51 UTC · re: Zheng Yuting · lore

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

Code captured errors but did not process them further. This treated all failures the same without distinguishing SMTP status.

Add a regex to extract status codes as defined in RFC 5321:
- For 4yz (temporary errors), return 1 and allow retries.
- For 5yz (permanent errors), return 0 as failure.
- For unrecognized codes, treat as permanent errors.
- If no error occurs, return the authentication result.
Signed-off-by: Zheng Yuting <05ZYT30@gmail.com>
---
 git-send-email.perl | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)
Show changes to git-send-email.perl +24 −3
diff --git a/git-send-email.perl b/git-send-email.perl
index 8feb43e9f7..69ba328653 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1454,9 +1454,30 @@ sub smtp_auth_maybe {
 			$error = $@ || 'Unknown error';
 		};
 
-		# NOTE: SMTP status code handling will be added in a subsequent commit
-		return $result ? 1 : 0;
-	}
+		# check if an error was captured
+		if ($error) {
+			# 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;
+				}
+				# if no recognized status code is found, treat as permanent error
+				warn "SMTP unknown error: $error";
+				return 0;
+			}
+			return $result ? 1 : 0;
+		} else {
+			return $result ? 1 : 0;
+		}
+}
 
 sub ssl_verify_params {
 	eval {
-- 
2.48.1
Junio C Hamano· Mar 21, 2025, 15:38 UTC · re: Zheng Yuting · lore

Re: [GSoC PATCH v6 0/2] send-email: improve error capture and status code handling

Does not pass t9001 when applied to Git 2.49.0.
Test Summary Report
-------------------
t9001-send-email.sh (Wstat: 256 (exited 1) Tests: 215 Failed: 169)
  Failed tests:  4-7, 9-10, 12-13, 15, 17, 19, 21-28, 31-35
                37, 39-60, 62, 64-66, 68, 70, 72, 74, 76
                78, 80, 82, 84-91, 94-99, 101-112, 114-127
                130, 132-135, 138, 140-142, 144, 146, 148
                151, 153-167, 169-189, 194-203, 205-215
  Non-zero exit status: 1
Files=1, Tests=215, 24 wallclock secs ( 0.19 usr  0.02 sys +  9.31 cusr 10.44 csys = 19.96 CPU)
Result: FAIL
Zheng Yuting· Mar 23, 2025, 02:21 UTC · re: Zheng Yuting · lore

[GSoC PATCH v7 0/2] send-email: improve error capture and status code handling

This patch series improves SMTP authentication error handling.

Auth relied solely on return values without capturing exceptions, misjudging non-credential errors as authentication failures.

Patch v7 1/2 wraps the auth process in an eval {} block to catch all exceptions, adds var error for future handling, and var result to return auth state.

Patch v7 2/2 introduces finer-grained SMTP error handling, extracting status codes per RFC 5321 to differentiate between temporary (4yz) and permanent (5yz) errors. For 4yz (transient errors), return 1 and allow retries. For 5yz (permanent errors), return 0 as failure. Unrecognized codes are treated as transient errors by returning 1. If the status code is not caught or no error occurs but no result is defined, return 1 as a transient error. Otherwise, return the authentication result.

Zheng Yuting (2):
  send-email: capture errors in an eval {} block
  send-email: finer-grained SMTP error handling
 git-send-email.perl | 68 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 54 insertions(+), 14 deletions(-)
-- 
2.49.0
Zheng Yuting· Mar 23, 2025, 02:21 UTC · re: Zheng Yuting · lore

[GSoC PATCH v7 1/2] send-email: capture errors in an eval {} block

Auth relied solely on return values without catching errors. This misjudges non-credential errors as auth failure without error info.

Patch wraps the entire auth process in an eval {} block to catch all exceptions, including non-credential errors. It adds a new $error var, uses 'or do' to prevent flow break, and returns $result ? 1 : 0. And merges if/else branches, integrates SASL and basic auth, with comments for future status code handling.

Signed-off-by: Zheng Yuting <05ZYT30@gmail.com>
---
 git-send-email.perl | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)
Show changes to git-send-email.perl +27 −16
diff --git a/git-send-email.perl b/git-send-email.perl
index 798d59b84f..0f05f55e50 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1419,7 +1419,7 @@ sub smtp_auth_maybe {
 		die "invalid smtp auth: '${smtp_auth}'";
 	}
 
-	# TODO: Authentication may fail not because credentials were
+	# Authentication may fail not because credentials were
 	# invalid but due to other reasons, in which we should not
 	# reject credentials.
 	$auth = Git::credential({
@@ -1431,21 +1431,32 @@ sub smtp_auth_maybe {
 		'password' => $smtp_authpass
 	}, sub {
 		my $cred = shift;
-
-		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);
-		}
-
-		return !!$smtp->auth($cred->{'username'}, $cred->{'password'});
+		my $result;
+		my $error;
+
+		# catch all SMTP auth error in a unified eval block
+		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 if no exception is thrown
+		} or do {
+			$error = $@ || 'Unknown error';
+		};
+
+		# 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 $auth;
-- 
2.49.0
Zheng Yuting· Mar 23, 2025, 02:21 UTC · re: Zheng Yuting · lore

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

Code captured errors but did not process them further. This treated all failures the same without distinguishing SMTP status.

Add a regex to extract status codes as defined in RFC 5321:
- For 4yz (transient errors), return 1 and allow retries.
- For 5yz (permanent errors), return 0 as failure.
- For unrecognized codes, return 1 as transient errors.
- For errors where the status code was not caught, return 1 as transient
errors.
- If no error and no result is returned, return 1 as a transient error.
- If no error occurs with result defined, return the authentication result.
Signed-off-by: Zheng Yuting <05ZYT30@gmail.com>
---
 git-send-email.perl | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)
Show changes to git-send-email.perl +32 −3
diff --git a/git-send-email.perl b/git-send-email.perl
index 0f05f55e50..e09a4a316f 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1454,9 +1454,38 @@ sub smtp_auth_maybe {
 			$error = $@ || 'Unknown error';
 		};
 
-		# 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);
+		if ($error) {
+			# 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.";
+					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;
-- 
2.49.0
Junio C Hamano· Mar 24, 2025, 06:00 UTC · re: Zheng Yuting · lore

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

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.

Show 16 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 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?

Show 18 quoted lines
> +					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;
Zheng Yuting· Mar 24, 2025, 14:53 UTC · re: Zheng Yuting · lore

[GSoC PATCH v8 0/2] send-email: improve error capture and status code handling

This patch series improves SMTP authentication error handling.

Auth relied solely on return values without capturing exceptions, misjudging non-credential errors as authentication failures.

Patch v8 1/2 wraps the auth process in an eval {} block to catch all exceptions, adds var error for future handling, and var result to return auth state.

Patch v8 2/2 introduces finer-grained SMTP error handling by extracting status codes per RFC 5321. For 4yz (transient) errors, return 1 and allow retries; for 5yz (permanent) errors, return 0. Unrecognized or uncaught status codes are treated as transient errors (return 1). If no error is present and no result is defined, return 1 as a transient error; otherwise, return the authentication result.

 Zheng Yuting (2):
  send-email: capture errors in an eval {} block
  send-email: finer-grained SMTP error handling
 git-send-email.perl | 69 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 54 insertions(+), 15 deletions(-)

-- 2.49.0

Zheng Yuting· Mar 24, 2025, 14:53 UTC · re: Zheng Yuting · lore

[GSoC PATCH v8 1/2] send-email: capture errors in an eval {} block

Auth relied solely on return values without catching errors. This misjudges non-credential errors as auth failure without error info.

Patch wraps the entire auth process in an eval {} block to catch all exceptions, including non-credential errors. It adds a new $error var, uses 'or do' to prevent flow break, and returns $result ? 1 : 0. And merges if/else branches, integrates SASL and basic auth, with comments for future status code handling.

Signed-off-by: Zheng Yuting <05ZYT30@gmail.com>
---
 git-send-email.perl | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)
Show changes to git-send-email.perl +27 −16
diff --git a/git-send-email.perl b/git-send-email.perl
index 798d59b84f..0f05f55e50 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1419,7 +1419,7 @@ sub smtp_auth_maybe {
 		die "invalid smtp auth: '${smtp_auth}'";
 	}
 
-	# TODO: Authentication may fail not because credentials were
+	# Authentication may fail not because credentials were
 	# invalid but due to other reasons, in which we should not
 	# reject credentials.
 	$auth = Git::credential({
@@ -1431,21 +1431,32 @@ sub smtp_auth_maybe {
 		'password' => $smtp_authpass
 	}, sub {
 		my $cred = shift;
-
-		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);
-		}
-
-		return !!$smtp->auth($cred->{'username'}, $cred->{'password'});
+		my $result;
+		my $error;
+
+		# catch all SMTP auth error in a unified eval block
+		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 if no exception is thrown
+		} or do {
+			$error = $@ || 'Unknown error';
+		};
+
+		# 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 $auth;
-- 
2.49.0
Zheng Yuting· Mar 24, 2025, 14:53 UTC · re: Zheng Yuting · lore

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

Code captured errors but did not process them further. This treated all failures the same without distinguishing SMTP status.

Add handle-smtp_error to extract SMTP status codes using a regex (as defined in RFC 5321) and handle errors as follows:

- No error present:
	- If a result is provided, return 1 to indicate success.
	- Otherwise, return 0 to indicate failure.
- Error present with a captured three-digit status code:
	- For 4yz (transient errors), return 1 and allow retries.
	- For 5yz (permanent errors), return 0 to indicate failure.
	- For any other recognized status code, return 1, treating it as
	a transient error.
- Error present but no status code found:
	- Return 1 as a transient error.
Signed-off-by: Zheng Yuting <05ZYT30@gmail.com>
---
 git-send-email.perl | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)
Show changes to git-send-email.perl +31 −3
diff --git a/git-send-email.perl b/git-send-email.perl
index 0f05f55e50..12b1a7c7de 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1454,14 +1454,42 @@ sub smtp_auth_maybe {
 			$error = $@ || 'Unknown error';
 		};
 
-		# 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);
 	});
 
 	return $auth;
 }
 
+sub handle_smtp_error {
+	my ($error, $result) = @_;
+
+	# If no error is present, return the result directly
+	return $result ? 1 : 0 unless $error;
+
+	# 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;
-- 
2.49.0
Junio C Hamano· Mar 25, 2025, 15:34 UTC · re: Zheng Yuting · lore

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

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;
Zheng Yuting· Mar 26, 2025, 07:52 UTC · re: Zheng Yuting · lore

[GSoC PATCH v9 0/2] send-email: improve error capture and status code handling

This patch series improves SMTP authentication error handling.

Auth relied solely on return values without capturing exceptions, misjudging non-credential errors as authentication failures.

Patch v9 1/2 wraps the auth process in an eval {} block to catch all exceptions, adds var error for future handling, and var result to return auth state.

Patch v9 2/2 introduces finer-grained SMTP error handling by extracting status codes per RFC 5321. For 4yz (transient) errors, return 1 and allow retries; for 5yz (permanent) errors, return 0. Unrecognized or uncaught status codes are treated as transient errors (return 1). If no error is present and no result is defined, return 1 as a transient error; otherwise, return the authentication result.

 Zheng Yuting (2):
  send-email: capture errors in an eval {} block
  send-email: finer-grained SMTP error handling
 git-send-email.perl | 69 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 54 insertions(+), 15 deletions(-)

-- 2.49.0

Zheng Yuting· Mar 26, 2025, 07:52 UTC · re: Zheng Yuting · lore

[GSoC PATCH v9 1/2] send-email: capture errors in an eval {} block

Auth relied solely on return values without catching errors. This misjudges non-credential errors as auth failure without error info.

Patch wraps the entire auth process in an eval {} block to catch all exceptions, including non-credential errors. It adds a new $error var, uses 'or do' to prevent flow break, and returns $result ? 1 : 0. And merges if/else branches, integrates SASL and basic auth, with comments for future status code handling.

Signed-off-by: Zheng Yuting <05ZYT30@gmail.com>
---
 git-send-email.perl | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)
Show changes to git-send-email.perl +27 −16
diff --git a/git-send-email.perl b/git-send-email.perl
index 798d59b84f..0f05f55e50 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1419,7 +1419,7 @@ sub smtp_auth_maybe {
 		die "invalid smtp auth: '${smtp_auth}'";
 	}
 
-	# TODO: Authentication may fail not because credentials were
+	# Authentication may fail not because credentials were
 	# invalid but due to other reasons, in which we should not
 	# reject credentials.
 	$auth = Git::credential({
@@ -1431,21 +1431,32 @@ sub smtp_auth_maybe {
 		'password' => $smtp_authpass
 	}, sub {
 		my $cred = shift;
-
-		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);
-		}
-
-		return !!$smtp->auth($cred->{'username'}, $cred->{'password'});
+		my $result;
+		my $error;
+
+		# catch all SMTP auth error in a unified eval block
+		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 if no exception is thrown
+		} or do {
+			$error = $@ || 'Unknown error';
+		};
+
+		# 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 $auth;
-- 
2.49.0
Zheng Yuting· Mar 26, 2025, 07:52 UTC · re: Zheng Yuting · lore

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

Code captured errors but did not process them further. This treated all failures the same without distinguishing SMTP status.

Add handle-smtp_error to extract SMTP status codes using a regex (as defined in RFC 5321) and handle errors as follows:

- No error present:
	- If a result is provided, return 1 to indicate success.
	- Otherwise, return 0 to indicate failure.
- Error present with a captured three-digit status code:
	- For 4yz (transient errors), return 1 and allow retries.
	- For 5yz (permanent errors), return 0 to indicate failure.
	- For any other recognized status code, return 1, treating it as
	a transient error.
- Error present but no status code found:
	- Return 1 as a transient error.
Signed-off-by: Zheng Yuting <05ZYT30@gmail.com>
---
 git-send-email.perl | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)
Show changes to git-send-email.perl +29 −3
diff --git a/git-send-email.perl b/git-send-email.perl
index 0f05f55e50..1f613fa979 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1454,14 +1454,40 @@ sub smtp_auth_maybe {
 			$error = $@ || 'Unknown error';
 		};
 
-		# 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 ($error
+			? handle_smtp_error($error)
+			: ($result ? 1 : 0));
 	});
 
 	return $auth;
 }
 
+sub handle_smtp_error {
+	my ($error) = @_;
+
+	# 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;
-- 
2.49.0

← back to recent threads