Re: [PATCH 1/1] Improve SMTP authentication error handling logic
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Mar 10, 2025, 22:14 UTC
- Message-ID
- <CAOLa=ZThgmKkYErnxUAGOhHyFcFrYDrcQ8KvqqkMBJ3wEckVPw@mail.gmail.com>
- In-Reply-To
- <20250307123934.2064275-2-05ZYT30@gmail.com>
Zheng Yuting <05zyt30@gmail.com> writes:
I see that you've added a cover message detailing the patch, but individual patches should include the relevant commit description and message. This is also outlined in 'Documentation/SubmittingPatches'.
That said my perl knowledge is limited, but I'll ask some questions anyways.
Show 22 quoted lines
> ---
> git-send-email.perl | 30 ++++++++++++++++++++++--------
> 1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 798d59b84f..a012d61abb 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1419,19 +1419,19 @@ 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({
> '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.I didn't understand why these comments were removed.
> 'password' => $smtp_authpass > +
Nit: unnecessary newline.
Show 5 quoted lines
> }, sub {
> my $cred = shift;
> -
> + my $result;
> + my $error;Nit: It is easier to read with a newline separating the variable declaration with the rest of the code.
Show 8 quoted lines
> if ($smtp_auth) {
> my $sasl = Authen::SASL->new(
> mechanism => $smtp_auth,
> @@ -1441,13 +1441,27 @@ sub smtp_auth_maybe {
> authname => $cred->{'username'},
> }
> );
> -Nit: unnecessary newline removal
> return !!$smtp->auth($sasl);
Shouldn't we do the same kind of changes for this `smpt->auth()` as the other below?
Show 8 quoted lines
> + } else {
> + # Handle plain authentication errors
> + eval {
> + $result = $smtp->auth($cred->{'username'}, $cred->{'password'});
> + 1; # Ensure true value is returned
> + } or do {
> + $error = $@ || 'Unknown error';
> + };Okay, so here we try to do the authentication and if there aren't any exceptions we return 1.
Show 7 quoted lines
> }
> -
> - return !!$smtp->auth($cred->{'username'}, $cred->{'password'});
> + # Unified error handling logic
> + 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) {Where do we get this list from? Regex matching errors for the type doesn't seem like the best way to go about this.
Show 8 quoted lines
> + warn "SMTP temporary error: $error"; > + return 1; > + } > + return 0; > + } > + return !!$result; > }); > -
Nit: unnecessary newline removal
Show 5 quoted lines
> return $auth; > } > > -- > 2.49.0.rc0.57.gdb91954e18