Zheng Yuting <05zyt30@xxxxxxxxx> writes:
> Refactored SMTP authentication to use a unified error capture block for
> both SASL and plain methods. Errors are now handled by parsing SMTP status
> codes (4yz for transient, 5yz for permanent) instead of relying on regex
> matching. This change improves clarity .
"improves clarity ." is (not well formatted and) a bit subjective
and does not apply to all three changes the patch is making here,
does it?
> Signed-off-by: Zheng Yuting <05ZYT30@xxxxxxxxx>
> ---
> git-send-email.perl | 72 +++++++++++++++++++++++++++------------------
> 1 file changed, 43 insertions(+), 29 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index a012d61abb..532dda264c 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1411,7 +1411,7 @@ sub smtp_auth_maybe {
> eval {
> require Authen::SASL;
> Authen::SASL->import(qw(Perl));
> - };
> + }
Hmph, the interpreter may tolerate the new block-eval "eval {}"
simple statement that lacks terminating ';' but is this an
improvement? The original look more kosher from syntactic point of
view. It seems to be totally unrelated change from the rest of the
patch.
> @@ -1426,42 +1426,56 @@ sub smtp_auth_maybe {
> '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
> -
We seem to already have the comment added by this hunk, since
4d31a44a (git-send-email: use git credential to obtain password,
2013-02-12). Am I looking at a wrong version of the source (or a
wrong version of the patch)?
> }, sub {
> my $cred = shift;
> my $result;
> my $error;
> - if ($smtp_auth) {
> - my $sasl = Authen::SASL->new(
> - mechanism => $smtp_auth,
> - callback => {
> - user => $cred->{'username'},
> - pass => $cred->{'password'},
> - authname => $cred->{'username'},
> - }
> - );
> - return !!$smtp->auth($sasl);
> - } else {
> - # Handle plain authentication errors
> - eval {
And curiously we do not seem to have this else clause with the
comment that is getting removed.
> + # catch all SMTP auth error
> + eval {
> + if ($smtp_auth) {
> + my $sasl = Authen::SASL->new(
> + mechanism => $smtp_auth,
> + callback => {
> + user => $cred->{'username'},
> + pass => $cred->{'password'},
> + authname => $cred->{'username'},
> + }
> + );
> + $result = $smtp->auth($sasl);
> + } else {
> $result = $smtp->auth($cred->{'username'}, $cred->{'password'});
> - 1; # Ensure true value is returned
> - } or do {
> - $error = $@ || 'Unknown error';
> - };
> - }
> - # Unified error handling logic
> + }
> + 1; # Ensure true value is returned if no exception is thrown.
> + } or do {
> + $error = $@ || 'Unknown error';
> + };
> +
> + #check if an error was captured
As I do not see two evals in our copy of git-send-email.perl source,
it may be moot at this point to comment on this patch, but if we did
have a eval block each of the if/else arms, moving the control
structure around and turning "if eval {} else eval {}" into "eval {
if ... else ...}" may make it cleaner to see what is going on,
especially if we plan to extend the choices and add elsif to the
chain later.
> 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;
> + #Parse SMTP status code from error message in:
> + #https://www.rfc-editor.org/rfc/rfc5321.html
Have a SP between "#" and the comment body.
Using the numeric error codes allows us to give more precise errors,
which should be a good change that can be done regardless of the
eval change. IOW, this part should be in a separate patch on its
own, either before or after the if-eval-else-eval change.
I'll stop here, as the patch does not seem to be designed to apply
to our source tree.
