| Message ID | 20250307123934.2064275-2-05ZYT30@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | smtp_auth_maybe: improve smtp authentication error handling logic | expand |
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. > --- > 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. > }, 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. > 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? > + } 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. > } > - > - 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. > + warn "SMTP temporary error: $error"; > + return 1; > + } > + return 0; > + } > + return !!$result; > }); > - Nit: unnecessary newline removal > return $auth; > } > > -- > 2.49.0.rc0.57.gdb91954e18
On Tue Mar 11, 2025 at 6:14 AM CST, Karthik Nayak wrote: > 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'. Thanks for pointing out my mistakes, I'm sorry to ignore the individual patch commits. I will add them in my next version. > > That said my perl knowledge is limited, but I'll ask some questions > anyways. > I am grateful for your support. As a new contributor, I’m still learning the best practices and will improve my process based on your feedback. >> ->--->---# 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. > I removed those comments because I intended to use the password directly at that point. I understand the confusion this caused and will restore or clarify the comments in my next revision. >> >--->---'password' => $smtp_authpass >> + > > Nit: unnecessary newline. > I’ll remove it. >> >---}, 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. > >> @@ -1441,13 +1441,27 @@ sub smtp_auth_maybe { >> >--->--->--->--->---authname => $cred->{'username'}, >> >--->--->--->---} >> >--->--->---); >> - > > Nit: unnecessary newline removal I will adjust these suggestions in the next version. > >> >--->--->---return !!$smtp->auth($sasl); > > Shouldn't we do the same kind of changes for this `smpt->auth()` as the > other below? > I’ll review both instances of the smtp->auth() call and update them so they follow a consistent approach in error handling and formatting. >> +>--->---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. > I agree with your assessment. Initially, I aimed to interpret the SMTP server's responses directly. But after considering your suggestion, I realized that I should use SMTP servers return standardized status codes for command outcomes according to RFC 5321 (4.2.2 Reply Codes by Function Groups). I will improve it. >> +>--->---return !!$result; >> >---}); >> - > > Nit: unnecessary newline removal > I will adjust these suggestions in the next version. On Tue, Mar 11, 2025 at 6:14 AM Karthik Nayak <karthik.188@gmail.com> wrote: > > 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. > > > --- > > 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. > > > }, 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. > > > 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? > > > + } 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. > > > } > > - > > - 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. > > > + warn "SMTP temporary error: $error"; > > + return 1; > > + } > > + return 0; > > + } > > + return !!$result; > > }); > > - > > Nit: unnecessary newline removal > > > return $auth; > > } > > > > -- > > 2.49.0.rc0.57.gdb91954e18
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. 'password' => $smtp_authpass + }, sub { my $cred = shift; - + my $result; + my $error; if ($smtp_auth) { my $sasl = Authen::SASL->new( mechanism => $smtp_auth, @@ -1441,13 +1441,27 @@ sub smtp_auth_maybe { authname => $cred->{'username'}, } ); - return !!$smtp->auth($sasl); + } else { + # Handle plain authentication errors + eval { + $result = $smtp->auth($cred->{'username'}, $cred->{'password'}); + 1; # Ensure true value is returned + } or do { + $error = $@ || 'Unknown error'; + }; } - - 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) { + warn "SMTP temporary error: $error"; + return 1; + } + return 0; + } + return !!$result; }); - return $auth; }