* [GSoC PATCH v3 0/1] Refactor SMTP Auth Error Handling
@ 2025-03-12 6:46 Zheng Yuting
2025-03-12 6:46 ` [GSoC PATCH v3 1/1] Unify SMTP auth error handling Zheng Yuting
0 siblings, 1 reply; 27+ messages in thread
From: Zheng Yuting @ 2025-03-12 6:46 UTC (permalink / raw)
To: git; +Cc: Zheng Yuting
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [GSoC PATCH v3 1/1] Unify SMTP auth error handling
2025-03-12 6:46 [GSoC PATCH v3 0/1] Refactor SMTP Auth Error Handling Zheng Yuting
@ 2025-03-12 6:46 ` Zheng Yuting
2025-03-13 19:58 ` Junio C Hamano
0 siblings, 1 reply; 27+ messages in thread
From: Zheng Yuting @ 2025-03-12 6:46 UTC (permalink / raw)
To: git; +Cc: Zheng Yuting
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(-)
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [GSoC PATCH v3 1/1] Unify SMTP auth error handling
2025-03-12 6:46 ` [GSoC PATCH v3 1/1] Unify SMTP auth error handling Zheng Yuting
@ 2025-03-13 19:58 ` Junio C Hamano
2025-03-14 12:55 ` Yuting Zheng
2025-03-16 5:09 ` [GSoC PATCH v4 0/2] smtp_auth_maybe: unified error capture and status code processing optimization Zheng Yuting
0 siblings, 2 replies; 27+ messages in thread
From: Junio C Hamano @ 2025-03-13 19:58 UTC (permalink / raw)
To: Zheng Yuting; +Cc: git
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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [GSoC PATCH v3 1/1] Unify SMTP auth error handling
2025-03-13 19:58 ` Junio C Hamano
@ 2025-03-14 12:55 ` Yuting Zheng
2025-03-16 5:09 ` [GSoC PATCH v4 0/2] smtp_auth_maybe: unified error capture and status code processing optimization Zheng Yuting
1 sibling, 0 replies; 27+ messages in thread
From: Yuting Zheng @ 2025-03-14 12:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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.
> 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.
> 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.
> 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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [GSoC PATCH v4 0/2] smtp_auth_maybe: unified error capture and status code processing optimization
2025-03-13 19:58 ` Junio C Hamano
2025-03-14 12:55 ` Yuting Zheng
@ 2025-03-16 5:09 ` Zheng Yuting
2025-03-16 5:09 ` [GSoC PATCH v4 1/2] Unify capture of SMTP errors Zheng Yuting
` (3 more replies)
1 sibling, 4 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-16 5:09 UTC (permalink / raw)
To: gitster; +Cc: 05zyt30, git, Zheng Yuting
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [GSoC PATCH v4 1/2] Unify capture of SMTP errors
2025-03-16 5:09 ` [GSoC PATCH v4 0/2] smtp_auth_maybe: unified error capture and status code processing optimization Zheng Yuting
@ 2025-03-16 5:09 ` Zheng Yuting
2025-03-16 5:09 ` [GSoC PATCH v4 2/2] Error handling for SMTP status codes Zheng Yuting
` (2 subsequent siblings)
3 siblings, 0 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-16 5:09 UTC (permalink / raw)
To: gitster; +Cc: 05zyt30, git, Zheng Yuting
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(-)
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [GSoC PATCH v4 2/2] Error handling for SMTP status codes
2025-03-16 5:09 ` [GSoC PATCH v4 0/2] smtp_auth_maybe: unified error capture and status code processing optimization Zheng Yuting
2025-03-16 5:09 ` [GSoC PATCH v4 1/2] Unify capture of SMTP errors Zheng Yuting
@ 2025-03-16 5:09 ` Zheng Yuting
2025-03-17 23:01 ` [GSoC PATCH v4 0/2] smtp_auth_maybe: unified error capture and status code processing optimization Junio C Hamano
2025-03-19 2:02 ` [GSoC PATCH v5 0/2] sendemail: improve error capture and status code handling Zheng Yuting
3 siblings, 0 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-16 5:09 UTC (permalink / raw)
To: gitster; +Cc: 05zyt30, git, Zheng Yuting
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(-)
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [GSoC PATCH v4 0/2] smtp_auth_maybe: unified error capture and status code processing optimization
2025-03-16 5:09 ` [GSoC PATCH v4 0/2] smtp_auth_maybe: unified error capture and status code processing optimization Zheng Yuting
2025-03-16 5:09 ` [GSoC PATCH v4 1/2] Unify capture of SMTP errors Zheng Yuting
2025-03-16 5:09 ` [GSoC PATCH v4 2/2] Error handling for SMTP status codes Zheng Yuting
@ 2025-03-17 23:01 ` Junio C Hamano
2025-03-19 2:02 ` [GSoC PATCH v5 0/2] sendemail: improve error capture and status code handling Zheng Yuting
3 siblings, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2025-03-17 23:01 UTC (permalink / raw)
To: Zheng Yuting; +Cc: git
Zheng Yuting <05zyt30@gmail.com> writes:
> 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.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [GSoC PATCH v5 0/2] sendemail: improve error capture and status code handling
2025-03-16 5:09 ` [GSoC PATCH v4 0/2] smtp_auth_maybe: unified error capture and status code processing optimization Zheng Yuting
` (2 preceding siblings ...)
2025-03-17 23:01 ` [GSoC PATCH v4 0/2] smtp_auth_maybe: unified error capture and status code processing optimization Junio C Hamano
@ 2025-03-19 2:02 ` Zheng Yuting
2025-03-19 2:02 ` [GSoC PATCH v5 1/2] sendemail: capture errors in an eval {} block Zheng Yuting
` (3 more replies)
3 siblings, 4 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-19 2:02 UTC (permalink / raw)
To: 05zyt30; +Cc: git, gitster, Zheng Yuting
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [GSoC PATCH v5 1/2] sendemail: capture errors in an eval {} block
2025-03-19 2:02 ` [GSoC PATCH v5 0/2] sendemail: improve error capture and status code handling Zheng Yuting
@ 2025-03-19 2:02 ` Zheng Yuting
2025-03-19 2:02 ` [GSoC PATCH v5 2/2] sendemail: finer-grained SMTP error handling Zheng Yuting
` (2 subsequent siblings)
3 siblings, 0 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-19 2:02 UTC (permalink / raw)
To: 05zyt30; +Cc: git, gitster, Zheng Yuting
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(-)
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [GSoC PATCH v5 2/2] sendemail: finer-grained SMTP error handling
2025-03-19 2:02 ` [GSoC PATCH v5 0/2] sendemail: improve error capture and status code handling Zheng Yuting
2025-03-19 2:02 ` [GSoC PATCH v5 1/2] sendemail: capture errors in an eval {} block Zheng Yuting
@ 2025-03-19 2:02 ` Zheng Yuting
2025-03-19 6:35 ` [GSoC PATCH v5 0/2] sendemail: improve error capture and status code handling Meet Soni
2025-03-21 2:51 ` [GSoC PATCH v6 0/2] send-email: " Zheng Yuting
3 siblings, 0 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-19 2:02 UTC (permalink / raw)
To: 05zyt30; +Cc: git, gitster, Zheng Yuting
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(-)
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [GSoC PATCH v5 0/2] sendemail: improve error capture and status code handling
2025-03-19 2:02 ` [GSoC PATCH v5 0/2] sendemail: improve error capture and status code handling Zheng Yuting
2025-03-19 2:02 ` [GSoC PATCH v5 1/2] sendemail: capture errors in an eval {} block Zheng Yuting
2025-03-19 2:02 ` [GSoC PATCH v5 2/2] sendemail: finer-grained SMTP error handling Zheng Yuting
@ 2025-03-19 6:35 ` Meet Soni
2025-03-21 2:51 ` [GSoC PATCH v6 0/2] send-email: " Zheng Yuting
3 siblings, 0 replies; 27+ messages in thread
From: Meet Soni @ 2025-03-19 6:35 UTC (permalink / raw)
To: Zheng Yuting; +Cc: git, gitster
On Wed, 19 Mar 2025 at 07:32, Zheng Yuting <05zyt30@gmail.com> wrote:
>
> 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`.
> git-send-email.perl | 62 ++++++++++++++++++++++++++++++++-------------
> 1 file changed, 45 insertions(+), 17 deletions(-)
>
> --
> 2.48.1
>
Thanks
^ permalink raw reply [flat|nested] 27+ messages in thread
* [GSoC PATCH v6 0/2] send-email: improve error capture and status code handling
2025-03-19 2:02 ` [GSoC PATCH v5 0/2] sendemail: improve error capture and status code handling Zheng Yuting
` (2 preceding siblings ...)
2025-03-19 6:35 ` [GSoC PATCH v5 0/2] sendemail: improve error capture and status code handling Meet Soni
@ 2025-03-21 2:51 ` Zheng Yuting
2025-03-21 2:51 ` [GSoC PATCH v6 1/2] send-email: capture errors in an eval {} block Zheng Yuting
` (4 more replies)
3 siblings, 5 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-21 2:51 UTC (permalink / raw)
To: git; +Cc: 05zyt30, meetsoni3017, gitster, Zheng Yuting
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [GSoC PATCH v6 1/2] send-email: capture errors in an eval {} block
2025-03-21 2:51 ` [GSoC PATCH v6 0/2] send-email: " Zheng Yuting
@ 2025-03-21 2:51 ` Zheng Yuting
2025-03-21 2:51 ` [GSoC PATCH v6 2/2] send-email: finer-grained SMTP error handling Zheng Yuting
` (3 subsequent siblings)
4 siblings, 0 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-21 2:51 UTC (permalink / raw)
To: git; +Cc: 05zyt30, meetsoni3017, gitster, Zheng Yuting
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(-)
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [GSoC PATCH v6 2/2] send-email: finer-grained SMTP error handling
2025-03-21 2:51 ` [GSoC PATCH v6 0/2] send-email: " Zheng Yuting
2025-03-21 2:51 ` [GSoC PATCH v6 1/2] send-email: capture errors in an eval {} block Zheng Yuting
@ 2025-03-21 2:51 ` Zheng Yuting
2025-03-21 15:38 ` [GSoC PATCH v6 0/2] send-email: improve error capture and status code handling Junio C Hamano
` (2 subsequent siblings)
4 siblings, 0 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-21 2:51 UTC (permalink / raw)
To: git; +Cc: 05zyt30, meetsoni3017, gitster, Zheng Yuting
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(-)
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [GSoC PATCH v6 0/2] send-email: improve error capture and status code handling
2025-03-21 2:51 ` [GSoC PATCH v6 0/2] send-email: " Zheng Yuting
2025-03-21 2:51 ` [GSoC PATCH v6 1/2] send-email: capture errors in an eval {} block Zheng Yuting
2025-03-21 2:51 ` [GSoC PATCH v6 2/2] send-email: finer-grained SMTP error handling Zheng Yuting
@ 2025-03-21 15:38 ` Junio C Hamano
2025-03-23 2:21 ` [GSoC PATCH v7 " Zheng Yuting
2025-03-24 14:53 ` [GSoC PATCH v8 0/2] send-email: improve error capture and status code handling Zheng Yuting
4 siblings, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2025-03-21 15:38 UTC (permalink / raw)
To: Zheng Yuting; +Cc: git, meetsoni3017
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [GSoC PATCH v7 0/2] send-email: improve error capture and status code handling
2025-03-21 2:51 ` [GSoC PATCH v6 0/2] send-email: " Zheng Yuting
` (2 preceding siblings ...)
2025-03-21 15:38 ` [GSoC PATCH v6 0/2] send-email: improve error capture and status code handling Junio C Hamano
@ 2025-03-23 2:21 ` Zheng Yuting
2025-03-23 2:21 ` [GSoC PATCH v7 1/2] send-email: capture errors in an eval {} block Zheng Yuting
2025-03-23 2:21 ` [GSoC PATCH v7 2/2] send-email: finer-grained SMTP error handling Zheng Yuting
2025-03-24 14:53 ` [GSoC PATCH v8 0/2] send-email: improve error capture and status code handling Zheng Yuting
4 siblings, 2 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-23 2:21 UTC (permalink / raw)
To: 05zyt30; +Cc: git, gitster, meetsoni3017, Zheng Yuting
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [GSoC PATCH v7 1/2] send-email: capture errors in an eval {} block
2025-03-23 2:21 ` [GSoC PATCH v7 " Zheng Yuting
@ 2025-03-23 2:21 ` Zheng Yuting
2025-03-23 2:21 ` [GSoC PATCH v7 2/2] send-email: finer-grained SMTP error handling Zheng Yuting
1 sibling, 0 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-23 2:21 UTC (permalink / raw)
To: 05zyt30; +Cc: git, gitster, meetsoni3017, Zheng Yuting
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(-)
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [GSoC PATCH v7 2/2] send-email: finer-grained SMTP error handling
2025-03-23 2:21 ` [GSoC PATCH v7 " Zheng Yuting
2025-03-23 2:21 ` [GSoC PATCH v7 1/2] send-email: capture errors in an eval {} block Zheng Yuting
@ 2025-03-23 2:21 ` Zheng Yuting
2025-03-24 6:00 ` Junio C Hamano
1 sibling, 1 reply; 27+ messages in thread
From: Zheng Yuting @ 2025-03-23 2:21 UTC (permalink / raw)
To: 05zyt30; +Cc: git, gitster, meetsoni3017, Zheng Yuting
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(-)
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [GSoC PATCH v7 2/2] send-email: finer-grained SMTP error handling
2025-03-23 2:21 ` [GSoC PATCH v7 2/2] send-email: finer-grained SMTP error handling Zheng Yuting
@ 2025-03-24 6:00 ` Junio C Hamano
0 siblings, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2025-03-24 6:00 UTC (permalink / raw)
To: Zheng Yuting; +Cc: git, meetsoni3017
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;
^ permalink raw reply [flat|nested] 27+ messages in thread
* [GSoC PATCH v8 0/2] send-email: improve error capture and status code handling
2025-03-21 2:51 ` [GSoC PATCH v6 0/2] send-email: " Zheng Yuting
` (3 preceding siblings ...)
2025-03-23 2:21 ` [GSoC PATCH v7 " Zheng Yuting
@ 2025-03-24 14:53 ` Zheng Yuting
2025-03-24 14:53 ` [GSoC PATCH v8 1/2] send-email: capture errors in an eval {} block Zheng Yuting
` (2 more replies)
4 siblings, 3 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-24 14:53 UTC (permalink / raw)
To: 05zyt30; +Cc: git, gitster, meetsoni3017, Zheng Yuting
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [GSoC PATCH v8 1/2] send-email: capture errors in an eval {} block
2025-03-24 14:53 ` [GSoC PATCH v8 0/2] send-email: improve error capture and status code handling Zheng Yuting
@ 2025-03-24 14:53 ` Zheng Yuting
2025-03-24 14:53 ` [GSoC PATCH v8 2/2] send-email: finer-grained SMTP error handling Zheng Yuting
2025-03-26 7:52 ` [GSoC PATCH v9 0/2] send-email: improve error capture and status code handling Zheng Yuting
2 siblings, 0 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-24 14:53 UTC (permalink / raw)
To: 05zyt30; +Cc: git, gitster, meetsoni3017, Zheng Yuting
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(-)
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [GSoC PATCH v8 2/2] send-email: finer-grained SMTP error handling
2025-03-24 14:53 ` [GSoC PATCH v8 0/2] send-email: improve error capture and status code handling Zheng Yuting
2025-03-24 14:53 ` [GSoC PATCH v8 1/2] send-email: capture errors in an eval {} block Zheng Yuting
@ 2025-03-24 14:53 ` Zheng Yuting
2025-03-25 15:34 ` Junio C Hamano
2025-03-26 7:52 ` [GSoC PATCH v9 0/2] send-email: improve error capture and status code handling Zheng Yuting
2 siblings, 1 reply; 27+ messages in thread
From: Zheng Yuting @ 2025-03-24 14:53 UTC (permalink / raw)
To: 05zyt30; +Cc: git, gitster, meetsoni3017, Zheng Yuting
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(-)
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [GSoC PATCH v8 2/2] send-email: finer-grained SMTP error handling
2025-03-24 14:53 ` [GSoC PATCH v8 2/2] send-email: finer-grained SMTP error handling Zheng Yuting
@ 2025-03-25 15:34 ` Junio C Hamano
0 siblings, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2025-03-25 15:34 UTC (permalink / raw)
To: Zheng Yuting; +Cc: git, meetsoni3017
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;
^ permalink raw reply [flat|nested] 27+ messages in thread
* [GSoC PATCH v9 0/2] send-email: improve error capture and status code handling
2025-03-24 14:53 ` [GSoC PATCH v8 0/2] send-email: improve error capture and status code handling Zheng Yuting
2025-03-24 14:53 ` [GSoC PATCH v8 1/2] send-email: capture errors in an eval {} block Zheng Yuting
2025-03-24 14:53 ` [GSoC PATCH v8 2/2] send-email: finer-grained SMTP error handling Zheng Yuting
@ 2025-03-26 7:52 ` Zheng Yuting
2025-03-26 7:52 ` [GSoC PATCH v9 1/2] send-email: capture errors in an eval {} block Zheng Yuting
2025-03-26 7:52 ` [GSoC PATCH v9 2/2] send-email: finer-grained SMTP error handling Zheng Yuting
2 siblings, 2 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-26 7:52 UTC (permalink / raw)
To: 05zyt30; +Cc: git, gitster, meetsoni3017, Zheng Yuting
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [GSoC PATCH v9 1/2] send-email: capture errors in an eval {} block
2025-03-26 7:52 ` [GSoC PATCH v9 0/2] send-email: improve error capture and status code handling Zheng Yuting
@ 2025-03-26 7:52 ` Zheng Yuting
2025-03-26 7:52 ` [GSoC PATCH v9 2/2] send-email: finer-grained SMTP error handling Zheng Yuting
1 sibling, 0 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-26 7:52 UTC (permalink / raw)
To: 05zyt30; +Cc: git, gitster, meetsoni3017, Zheng Yuting
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(-)
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [GSoC PATCH v9 2/2] send-email: finer-grained SMTP error handling
2025-03-26 7:52 ` [GSoC PATCH v9 0/2] send-email: improve error capture and status code handling Zheng Yuting
2025-03-26 7:52 ` [GSoC PATCH v9 1/2] send-email: capture errors in an eval {} block Zheng Yuting
@ 2025-03-26 7:52 ` Zheng Yuting
1 sibling, 0 replies; 27+ messages in thread
From: Zheng Yuting @ 2025-03-26 7:52 UTC (permalink / raw)
To: 05zyt30; +Cc: git, gitster, meetsoni3017, Zheng Yuting
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(-)
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
^ permalink raw reply related [flat|nested] 27+ messages in thread
end of thread, other threads:[~2025-03-26 7:53 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-12 6:46 [GSoC PATCH v3 0/1] Refactor SMTP Auth Error Handling Zheng Yuting
2025-03-12 6:46 ` [GSoC PATCH v3 1/1] Unify SMTP auth error handling Zheng Yuting
2025-03-13 19:58 ` Junio C Hamano
2025-03-14 12:55 ` Yuting Zheng
2025-03-16 5:09 ` [GSoC PATCH v4 0/2] smtp_auth_maybe: unified error capture and status code processing optimization Zheng Yuting
2025-03-16 5:09 ` [GSoC PATCH v4 1/2] Unify capture of SMTP errors Zheng Yuting
2025-03-16 5:09 ` [GSoC PATCH v4 2/2] Error handling for SMTP status codes Zheng Yuting
2025-03-17 23:01 ` [GSoC PATCH v4 0/2] smtp_auth_maybe: unified error capture and status code processing optimization Junio C Hamano
2025-03-19 2:02 ` [GSoC PATCH v5 0/2] sendemail: improve error capture and status code handling Zheng Yuting
2025-03-19 2:02 ` [GSoC PATCH v5 1/2] sendemail: capture errors in an eval {} block Zheng Yuting
2025-03-19 2:02 ` [GSoC PATCH v5 2/2] sendemail: finer-grained SMTP error handling Zheng Yuting
2025-03-19 6:35 ` [GSoC PATCH v5 0/2] sendemail: improve error capture and status code handling Meet Soni
2025-03-21 2:51 ` [GSoC PATCH v6 0/2] send-email: " Zheng Yuting
2025-03-21 2:51 ` [GSoC PATCH v6 1/2] send-email: capture errors in an eval {} block Zheng Yuting
2025-03-21 2:51 ` [GSoC PATCH v6 2/2] send-email: finer-grained SMTP error handling Zheng Yuting
2025-03-21 15:38 ` [GSoC PATCH v6 0/2] send-email: improve error capture and status code handling Junio C Hamano
2025-03-23 2:21 ` [GSoC PATCH v7 " Zheng Yuting
2025-03-23 2:21 ` [GSoC PATCH v7 1/2] send-email: capture errors in an eval {} block Zheng Yuting
2025-03-23 2:21 ` [GSoC PATCH v7 2/2] send-email: finer-grained SMTP error handling Zheng Yuting
2025-03-24 6:00 ` Junio C Hamano
2025-03-24 14:53 ` [GSoC PATCH v8 0/2] send-email: improve error capture and status code handling Zheng Yuting
2025-03-24 14:53 ` [GSoC PATCH v8 1/2] send-email: capture errors in an eval {} block Zheng Yuting
2025-03-24 14:53 ` [GSoC PATCH v8 2/2] send-email: finer-grained SMTP error handling Zheng Yuting
2025-03-25 15:34 ` Junio C Hamano
2025-03-26 7:52 ` [GSoC PATCH v9 0/2] send-email: improve error capture and status code handling Zheng Yuting
2025-03-26 7:52 ` [GSoC PATCH v9 1/2] send-email: capture errors in an eval {} block Zheng Yuting
2025-03-26 7:52 ` [GSoC PATCH v9 2/2] send-email: finer-grained SMTP error handling Zheng Yuting
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).