Re: [PATCH 1/1] Improve SMTP authentication error handling logic

[Yuting Zheng <05zyt30@gmail.com>]

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