Zheng Yuting <05zyt30@xxxxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
