On Wed, Feb 27 2013, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Michal Nazarewicz <mpn@google.com> writes:
>>
>>> +	$auth = Git::credential({
>>> +		'protocol' => 'smtp',
>>> +		'host' => join(':', $smtp_server, $smtp_server_port),
>>
>> At this point, $smtp_server_port is not always defined. I just tested
>> and got
>>
>> Use of uninitialized value $smtp_server_port in join or string at
>> git-send-email line 1077.
>>
>> Other than that, the whole series looks good.
>
> Given that there is another place that conditionally append ":$port"
> to the host string, I think we should follow suit here.  Perhaps
> like the attached diff?

Damn meetings, you beat me to it…  I was just about to send a patch. ;)

> Thanks for a review.
>
>
>  git-send-email.perl | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 76bbfc3..c3501d9 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1045,6 +1045,14 @@ sub maildomain {
>  	return maildomain_net() || maildomain_mta() || 'localhost.localdomain';
>  }
>  
> +sub smtp_host_string {
> +	if (defined $smtp_server_port) {
> +		return "$smtp_server:$smtp_server_port";
> +	} else {
> +		return $smtp_server;
> +	}
> +}
> +
>  # Returns 1 if authentication succeeded or was not necessary
>  # (smtp_user was not specified), and 0 otherwise.
>  
> @@ -1065,7 +1073,7 @@ sub smtp_auth_maybe {
>  	# reject credentials.
>  	$auth = Git::credential({
>  		'protocol' => 'smtp',
> -		'host' => join(':', $smtp_server, $smtp_server_port),
> +		'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.
> @@ -1188,9 +1196,7 @@ sub send_message {
>  		else {
>  			require Net::SMTP;
>  			$smtp_domain ||= maildomain();
> -			$smtp ||= Net::SMTP->new((defined $smtp_server_port)
> -						 ? "$smtp_server:$smtp_server_port"
> -						 : $smtp_server,
> +			$smtp ||= Net::SMTP->new(smtp_host_string(),
>  						 Hello => $smtp_domain,
>  						 Debug => $debug_net_smtp);
>  			if ($smtp_encryption eq 'tls' && $smtp) {

>From reading of SMTP.pm, it seems that this could be changed to:

-			$smtp ||= Net::SMTP->new((defined $smtp_server_port)
-						 ? "$smtp_server:$smtp_server_port"
-						 : $smtp_server,
+			$smtp ||= Net::SMTP->new($smtp_server,
+						 Port => $smtp_server_port,

and than the other part would become:

@@ -1060,12 +1060,17 @@ sub smtp_auth_maybe {
                Authen::SASL->import(qw(Perl));
        };
 
+       my $host = $smtp_server;
+       if (defined $smtp_server_port) {
+               $host .= ':' . $smtp_server_port;
+       }
+
        # TODO: 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' => join(':', $smtp_server, $smtp_server_port),
+               'host' => $host,
                'username' => $smtp_authuser,
                # if there's no password, "git credential fill" will
                # give us one, otherwise it'll just pass this one.

Either way, looks good to me.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--