Re: [PATCH v2] send-email: Fix tls AUTH when sending batch

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 16, 2018, 22:19 UTC
Message-ID: <xmqqwoturfh0.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180714085848.4031-1-jules.maselbas@grenoble-inp.org>

Jules Maselbas <jules.maselbas@grenoble-inp.org> writes:

Show 21 quoted lines

> The variable smtp_encryption must keep it's value between two batches.
> Otherwise the authentication is skipped after the first batch.
>
> Signed-off-by: Jules Maselbas <jules.maselbas@grenoble-inp.org>
> ---
>  git-send-email.perl | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 8ec70e58e..1f9a73f74 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1479,7 +1479,7 @@ sub send_message {
>  							 SSL => 1);
>  			}
>  		}
> -		else {
> +		elsif (!$smtp) {
>  			$smtp_server_port ||= 25;
>  			$smtp ||= Net::SMTP->new($smtp_server,
>  						 Hello => $smtp_domain,

Puzzled... The code is prepared to deal with the case where $smtp is still active at this point by using "$smtp ||=", but this hunk forces any caller that still has $smtp active from entering this block.

So the conditional assignment "$smtp ||= Net::SMTP->new()" is meaningless after this patch---we always do "new" if we reach this statement.

This hunk did not exist in your v1, yet the proposed log message has not changed since v1. With this hunk, it appears that the problem and the solution are not about $smtp_encryption but is about not calling ->starttls() or ->command('STARTTLS') when we reuse $smtp that has been established earlier, and the description in the log message feels a bit off. Some exaplantion on why this hunk is a good thing may be missing, I guess.

Dropping the assignment to $smtp_encryption in this block (i.e. hunk below) makes sense, as we do not call read_config() to re-read its value after the batching code quits and unsets $smtp.

Show 6 quoted lines

> @@ -1501,7 +1501,6 @@ sub send_message {
>  					$smtp->starttls(ssl_verify_params())
>  						or die sprintf(__("STARTTLS failed! %s"), IO::Socket::SSL::errstr());
>  				}
> -				$smtp_encryption = '';
>  				# Send again to receive fresh

By the way, the patch claims that it is against 8ec70e58e, but has an edit on this context line and does not apply. Did you hand-edit your patch after generating it? Please don't.

>  				# supported commands
>  				$smtp->hello($smtp_domain);