Re: [PATCH 1/1] Improve SMTP authentication error handling logic
On Tue Mar 11, 2025 at 6:14 AM CST, Karthik Nayak wrote:
Show 5 quoted lines
> 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.
Show 5 quoted lines
>> ->--->---# 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.
Show 5 quoted lines
>> >--->---'password' => $smtp_authpass
>> +
>
> Nit: unnecessary newline.
>
Show 16 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.
>
>> @@ -1441,13 +1441,27 @@ sub smtp_auth_maybe {
>> >--->--->--->--->---authname => $cred->{'username'},
>> >--->--->--->---}
>> >--->--->---);
>> -
>
> Nit: unnecessary newline removalI will adjust these suggestions in the next version.
Show 6 quoted lines
>
>> >--->--->---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.
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) {
>
> 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.
Show 6 quoted lines
>> +>--->---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:
Show 104 quoted lines
>
> 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
Zheng Yuting