[PATCH] Do not strip empty lines / trailing spaces from a commit message template

[PATCH] Do not strip empty lines / trailing spaces from a commit message template

[Sebastian Schuberth]

Templates should be just that: Forms that the user fills out, and forms
have blanks. If people are attached to not having extra whitespace in the
editor, they can simply clean up their templates.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 builtin-commit.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index f4c7344..8a68dd3 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -584,7 +584,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (fp == NULL)
 		die_errno("could not open '%s'", git_path(commit_editmsg));
 
-	if (cleanup_mode != CLEANUP_NONE)
+	if (cleanup_mode != CLEANUP_NONE && strcmp(hook_arg1, "template"))
 		stripspace(&sb, 0);
 
 	if (signoff) {
-- 
1.7.0.2.msysgit.0.8.g888e.dirty

Re: [PATCH] Do not strip empty lines / trailing spaces from a commit message template

[Jeff King]

On Wed, Mar 10, 2010 at 04:57:11PM +0100, Sebastian Schuberth wrote:

> Templates should be just that: Forms that the user fills out, and forms
> have blanks. If people are attached to not having extra whitespace in the
> editor, they can simply clean up their templates.

Rationale makes sense to me...

>  	if (fp == NULL)
>  		die_errno("could not open '%s'", git_path(commit_editmsg));
>  
> -	if (cleanup_mode != CLEANUP_NONE)
> +	if (cleanup_mode != CLEANUP_NONE && strcmp(hook_arg1, "template"))
>  		stripspace(&sb, 0);

And the code looks OK, though admittedly I am not too familiar with this
chunk of code (at first I was confused that you would have to look at
hook_arg1, but apparently there is no other variable that contains the
result of that big if-else chain).

How about a test to check the new behavior?

-Peff

Re: [PATCH] Do not strip empty lines / trailing spaces from a commit message template

[Jeff King]

On Thu, Mar 11, 2010 at 03:12:13AM -0500, Jeff King wrote:

> >  	if (fp == NULL)
> >  		die_errno("could not open '%s'", git_path(commit_editmsg));
> >  
> > -	if (cleanup_mode != CLEANUP_NONE)
> > +	if (cleanup_mode != CLEANUP_NONE && strcmp(hook_arg1, "template"))
> >  		stripspace(&sb, 0);
> 
> And the code looks OK, though admittedly I am not too familiar with this
> chunk of code (at first I was confused that you would have to look at
> hook_arg1, but apparently there is no other variable that contains the
> result of that big if-else chain).

BTW, a subtle point for anyone else reviewing this patch: we also call
stripspace in message_is_empty to skip over an untouched template. But
that code path is stil OK, because we stripspace the whole message that
comes back from the user before calling message_is_empty(), so the
result should be the same for an untouched template.

-Peff

Re: [PATCH] Do not strip empty lines / trailing spaces from a commit message template

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 11, 2010 at 03:12:13AM -0500, Jeff King wrote:
>
>> >  	if (fp == NULL)
>> >  		die_errno("could not open '%s'", git_path(commit_editmsg));
>> >  
>> > -	if (cleanup_mode != CLEANUP_NONE)
>> > +	if (cleanup_mode != CLEANUP_NONE && strcmp(hook_arg1, "template"))
>> >  		stripspace(&sb, 0);
>> 
>> And the code looks OK, though admittedly I am not too familiar with this
>> chunk of code (at first I was confused that you would have to look at
>> hook_arg1, but apparently there is no other variable that contains the
>> result of that big if-else chain).


I suspect that the attached would be much easier to read and understand.

> BTW, a subtle point for anyone else reviewing this patch: we also call
> stripspace in message_is_empty to skip over an untouched template. But
> that code path is stil OK, because we stripspace the whole message that
> comes back from the user before calling message_is_empty(), so the
> result should be the same for an untouched template.
>
> -Peff

Thanks for a patch and a review.

> How about a test to check the new behavior?

Speaking of tests, t2203 will segfault with your patch.  I don't think the
following does, though.

 builtin-commit.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 8a68dd3..14488d5 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -534,6 +534,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	const char *hook_arg1 = NULL;
 	const char *hook_arg2 = NULL;
 	int ident_shown = 0;
+	int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
 
 	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
 		return 0;
@@ -571,6 +572,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		if (strbuf_read_file(&sb, template_file, 0) < 0)
 			die_errno("could not read '%s'", template_file);
 		hook_arg1 = "template";
+		clean_message_contents = 0;
 	}
 
 	/*
@@ -584,7 +586,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (fp == NULL)
 		die_errno("could not open '%s'", git_path(commit_editmsg));
 
-	if (cleanup_mode != CLEANUP_NONE && strcmp(hook_arg1, "template"))
+	if (clean_message_contents)
 		stripspace(&sb, 0);
 
 	if (signoff) {

Re: [PATCH] Do not strip empty lines / trailing spaces from a commit message template

[Jeff King]

On Thu, Mar 11, 2010 at 12:46:51PM -0800, Junio C Hamano wrote:

> > How about a test to check the new behavior?
> 
> Speaking of tests, t2203 will segfault with your patch.  I don't think the
> following does, though.

I thought I ran the tests, but obviously not. I see the segfault here.
It is not just t2203, but any "git commit" with no message will cause
it.

Your patch looks right, and is more readable, too, I think.

-Peff

Re: [PATCH] Do not strip empty lines / trailing spaces from a commit message template

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 11, 2010 at 12:46:51PM -0800, Junio C Hamano wrote:
>
>> > How about a test to check the new behavior?
>> 
>> Speaking of tests, t2203 will segfault with your patch.  I don't think the
>> following does, though.
>
> I thought I ran the tests, but obviously not. I see the segfault here.
> It is not just t2203, but any "git commit" with no message will cause
> it.
>
> Your patch looks right, and is more readable, too, I think.

Ok, Sebastian can use that code in his reroll with tests, then.

Thanks.

Re: [PATCH] Do not strip empty lines / trailing spaces from a commit message template

[Sebastian Schuberth]

On 11.03.2010 21:46, Junio C Hamano wrote:

> I suspect that the attached would be much easier to read and understand.

It is, indeed, easier to read. Thanks.

>> How about a test to check the new behavior?
>
> Speaking of tests, t2203 will segfault with your patch.  I don't think the
> following does, though.

Hmm, t2203 does neither segfault here with my nor your patch, but I'm 
running the test on msysGit.

Anyway, Junio, do you think it would be necessary to introduce a new 
test for this, or can I resend with just your improvements applied?

-- 
Sebastian Schuberth

Re: [PATCH] Do not strip empty lines / trailing spaces from a commit message template

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> Anyway, Junio, do you think it would be necessary to introduce a new
> test for this, or can I resend with just your improvements applied?

If you can promise that you will around on this list forever, and that
every time somebody posts patches to builtin-commit.c and the code in
related areas, you will make sure that the changes do not inadvertently
break this feature and respond to the patches that do break it before they
hit my tree, then theoretically we do not need to have any test to make
sure this feature keeps working as advertised.

If you cannot make such a time committment, one alternative that is
presumably easier for you is to have me run the test as part of the
regular patch acceptance test cycle.

Your choice ;-)

[PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Jari Aalto]

Add new function maildomain() which returns FQDN for use in
send_message(). The value is passed to Net::SMTP HELO/EHLO handshake.

The default value in Net::SMTP may not get through:

  Net::SMTP=GLOB(0x267ec28)>>> EHLO localhost.localdomain
  Net::SMTP=GLOB(0x267ec28)<<< 550 EHLO argument does not match calling host

whereas using the FQDN, the result is:

  Net::SMTP=GLOB(0x15b8e80)>>> EHLO host.example.com
  Net::SMTP=GLOB(0x15b8e80)<<< 250-host.example.com Hello host.example.com [192.168.1.7]

Signed-off-by: Jari Aalto <jari.aalto@cante.net>
---
 git-send-email.perl |   52 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 50 insertions(+), 2 deletions(-)


        This is a reworked patch

        - In maildomain() start with "use Net::SMTP". Had "use Net::Domain" twice.
        - In maildomain() lose the "()" prototype.


diff --git a/git-send-email.perl b/git-send-email.perl
index 6af7bd3..f743d9e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -834,6 +834,46 @@ sub sanitize_address
 
 }
 
+# Returns the local Fully Qualified Domain Name (FQDN) if available,
+# If this is not given to EHLO, the receiving SMTP may deny connection
+# Here is an example of Net::SMTP without explicit Helo: it
+# uses by default "localhost.localdomain"
+#
+# Net::SMTP=GLOB(0x267ec28)>>> EHLO localhost.localdomain
+# Net::SMTP=GLOB(0x267ec28)<<< 550 EHLO argument does not match calling host
+
+sub maildomain
+{
+	my $maildomain;
+	eval "use Net::SMTP";
+
+	unless ( $@ ) {
+		for my $host ( qw(mailhost localhost) ) {
+			my $smtp = Net::SMTP->new($host);
+			if (defined $smtp) {
+				my $domain = $smtp->domain;
+				$smtp->quit;
+
+				$maildomain = $domain
+					unless $^O eq 'darwin' && $domain =~ /\.local$/;
+
+				last if $maildomain;
+			}
+		}
+	}
+
+	unless ($maildomain) {
+		eval "use Net::Domain";
+		unless ( $@ ) {
+		    my $domain = Net::Domain::domainname();
+		    $maildomain = $domain
+			    unless $^O eq 'darwin' && $domain =~ /\.local$/;
+		}
+	}
+
+	$maildomain;
+}
+
 # Returns 1 if the message was sent, and 0 otherwise.
 # In actuality, the whole program dies when there
 # is an error sending a message.
@@ -917,6 +957,8 @@ X-Mailer: git-send-email $gitversion
 		}
 	}
 
+	my $maildomain;
+
 	if ($dry_run) {
 		# We don't want to send the email.
 	} elsif ($smtp_server =~ m#^/#) {
@@ -936,13 +978,18 @@ X-Mailer: git-send-email $gitversion
 		if ($smtp_encryption eq 'ssl') {
 			$smtp_server_port ||= 465; # ssmtp
 			require Net::SMTP::SSL;
-			$smtp ||= Net::SMTP::SSL->new($smtp_server, Port => $smtp_server_port);
+			$maildomain = maildomain() || "localhost.localdomain";
+			$smtp ||= Net::SMTP::SSL->new($smtp_server,
+						      Hello => $maildomain,
+						      Port => $smtp_server_port);
 		}
 		else {
 			require Net::SMTP;
+			$maildomain = maildomain() || "localhost.localdomain";
 			$smtp ||= Net::SMTP->new((defined $smtp_server_port)
 						 ? "$smtp_server:$smtp_server_port"
 						 : $smtp_server,
+						 Hello => $maildomain,
 						 Debug => $debug_net_smtp);
 			if ($smtp_encryption eq 'tls' && $smtp) {
 				require Net::SMTP::SSL;
@@ -962,9 +1009,10 @@ X-Mailer: git-send-email $gitversion
 		}
 
 		if (!$smtp) {
-			die "Unable to initialize SMTP properly. Check config. ",
+			die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
 			    "VALUES: server=$smtp_server ",
 			    "encryption=$smtp_encryption ",
+			    "maildomain=$maildomain",
 			    defined $smtp_server_port ? "port=$smtp_server_port" : "";
 		}
 
-- 
1.7.0

Re: [PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Junio C Hamano]

Jari Aalto <jari.aalto@cante.net> writes:

> Add new function maildomain() which returns FQDN for use in
> send_message(). The value is passed to Net::SMTP HELO/EHLO handshake.
>
> The default value in Net::SMTP may not get through:
>
>   Net::SMTP=GLOB(0x267ec28)>>> EHLO localhost.localdomain
>   Net::SMTP=GLOB(0x267ec28)<<< 550 EHLO argument does not match calling host
>
> whereas using the FQDN, the result is:
>
>   Net::SMTP=GLOB(0x15b8e80)>>> EHLO host.example.com
>   Net::SMTP=GLOB(0x15b8e80)<<< 250-host.example.com Hello host.example.com [192.168.1.7]

I think you identified a good issue to tackle.  But is it really the
optimal solution?

 - Is it the best we can do to always make an empty connection only to
   check if the we have hosts locally known as mailhost or localhost
   listens to SMTP port?  And calling this function again and again, even
   after sending one message to the same $smtp_server successfully ($smtp
   in the global scope is already set in that case)?

 - You are trying to improve the chance that $smtp_server likes the name
   your side identifies as; what does it have to do with your local
   "mailhost" or "localhost" listening to the SMTP port?  These local MTA
   may be configured for local-only delivery after all.

[PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Jari Aalto]

Add new function maildomain() which returns FQDN for use in
send_message(). The value is passed to Net::SMTP HELO/EHLO handshake.
The domain name can also be set via --smtp-domain option.

The default value in Net::SMTP may not get through:

  Net::SMTP=GLOB(0x267ec28)>>> EHLO localhost.localdomain
  Net::SMTP=GLOB(0x267ec28)<<< 550 EHLO argument does not match calling host

whereas using the FQDN, the result is:

  Net::SMTP=GLOB(0x15b8e80)>>> EHLO host.example.com
  Net::SMTP=GLOB(0x15b8e80)<<< 250-host.example.com Hello host.example.com [192.168.1.7]

Signed-off-by: Jari Aalto <jari.aalto@cante.net>
---
 git-send-email.perl |   57 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 55 insertions(+), 2 deletions(-)

> Junio C Hamano <gitster@pobox.com> writes:
> I think you identified a good issue to tackle.  But is it really the
> optimal solution?
>
>  - Is it the best we can do to always make an empty connection only to
>    check if the we have hosts locally known as mailhost or localhost
>    listens to SMTP port?  And calling this function again and again, even
>    after sending one message to the same $smtp_server successfully ($smtp
>    in the global scope is already set in that case)?

A new patch below. I made the value global, so that it's set only once
for the duration of program.

>  - You are trying to improve the chance that $smtp_server likes the name
>    your side identifies as; what does it have to do with your local
>    "mailhost" or "localhost" listening to the SMTP port?  These local MTA
>    may be configured for local-only delivery after all.

There is now explicit option to set the name with option --mail-domain.
Hope this address the issue.

    ** applies after the other two patches **

Jari

diff --git a/git-send-email.perl b/git-send-email.perl
index 6af7bd3..ef7cc30 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -64,6 +64,7 @@ git send-email [options] <file | directory | rev-list options >
     --smtp-pass             <str>  * Password for SMTP-AUTH; not necessary.
     --smtp-encryption       <str>  * tls or ssl; anything else disables.
     --smtp-ssl                     * Deprecated. Use '--smtp-encryption ssl'.
+    --smtp-domain           <str>  * The domain name sent to HELO/EHLO handshake
     --smtp-debug            <0|1>  * Disable, enable Net::SMTP debug.
 
   Automating:
@@ -131,6 +132,7 @@ my $have_email_valid = eval { require Email::Valid; 1 };
 my $have_mail_address = eval { require Mail::Address; 1 };
 my $smtp;
 my $auth;
+my $MAIL_DOMAIN;			# See maildomain()
 
 sub unique_email_list(@);
 sub cleanup_compose_files();
@@ -274,6 +276,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "smtp-ssl" => sub { $smtp_encryption = 'ssl' },
 		    "smtp-encryption=s" => \$smtp_encryption,
 		    "smtp-debug:i" => \$debug_net_smtp,
+		    "smtp-domain:s" => \$MAIL_DOMAIN,
 		    "identity=s" => \$identity,
 		    "annotate" => \$annotate,
 		    "compose" => \$compose,
@@ -834,6 +837,48 @@ sub sanitize_address
 
 }
 
+# Returns the local Fully Qualified Domain Name (FQDN) if available,
+# If this is not given to EHLO, the receiving SMTP may deny connection
+# Here is an example of Net::SMTP without explicit Helo: it
+# uses by default "localhost.localdomain"
+#
+# Net::SMTP=GLOB(0x267ec28)>>> EHLO localhost.localdomain
+# Net::SMTP=GLOB(0x267ec28)<<< 550 EHLO argument does not match calling host
+
+sub maildomain
+{
+        return $MAIL_DOMAIN if $MAIL_DOMAIN;
+
+	my $maildomain;
+	eval "use Net::SMTP";
+
+	unless ( $@ ) {
+		for my $host ( qw(mailhost localhost) ) {
+			my $smtp = Net::SMTP->new($host);
+			if (defined $smtp) {
+				my $domain = $smtp->domain;
+				$smtp->quit;
+
+				$maildomain = $domain
+					unless $^O eq 'darwin' && $domain =~ /\.local$/;
+
+				last if $maildomain;
+			}
+		}
+	}
+
+	unless ($maildomain) {
+		eval "use Net::Domain";
+		unless ( $@ ) {
+		    my $domain = Net::Domain::domainname();
+		    $maildomain = $domain
+			    unless $^O eq 'darwin' && $domain =~ /\.local$/;
+		}
+	}
+
+	$MAIL_DOMAIN = $maildomain;
+}
+
 # Returns 1 if the message was sent, and 0 otherwise.
 # In actuality, the whole program dies when there
 # is an error sending a message.
@@ -917,6 +962,8 @@ X-Mailer: git-send-email $gitversion
 		}
 	}
 
+	my $maildomain;
+
 	if ($dry_run) {
 		# We don't want to send the email.
 	} elsif ($smtp_server =~ m#^/#) {
@@ -936,13 +983,18 @@ X-Mailer: git-send-email $gitversion
 		if ($smtp_encryption eq 'ssl') {
 			$smtp_server_port ||= 465; # ssmtp
 			require Net::SMTP::SSL;
-			$smtp ||= Net::SMTP::SSL->new($smtp_server, Port => $smtp_server_port);
+			$maildomain = maildomain() || "localhost.localdomain";
+			$smtp ||= Net::SMTP::SSL->new($smtp_server,
+						      Hello => $maildomain,
+						      Port => $smtp_server_port);
 		}
 		else {
 			require Net::SMTP;
+			$maildomain = maildomain() || "localhost.localdomain";
 			$smtp ||= Net::SMTP->new((defined $smtp_server_port)
 						 ? "$smtp_server:$smtp_server_port"
 						 : $smtp_server,
+						 Hello => $maildomain,
 						 Debug => $debug_net_smtp);
 			if ($smtp_encryption eq 'tls' && $smtp) {
 				require Net::SMTP::SSL;
@@ -962,9 +1014,10 @@ X-Mailer: git-send-email $gitversion
 		}
 
 		if (!$smtp) {
-			die "Unable to initialize SMTP properly. Check config. ",
+			die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
 			    "VALUES: server=$smtp_server ",
 			    "encryption=$smtp_encryption ",
+			    "maildomain=$maildomain",
 			    defined $smtp_server_port ? "port=$smtp_server_port" : "";
 		}
 
-- 
1.7.0

Re: [PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Junio C Hamano]

Jari Aalto <jari.aalto@cante.net> writes:

>>  - You are trying to improve the chance that $smtp_server likes the name
>>    your side identifies as; what does it have to do with your local
>>    "mailhost" or "localhost" listening to the SMTP port?  These local MTA
>>    may be configured for local-only delivery after all.
>
> There is now explicit option to set the name with option --mail-domain.
> Hope this address the issue.

Not quite.  Imagine if you were a maintainer of a system who received a
patch that looks like adding a piece of code that computes random rubbish.

    sub a_function () {
	compute some value that looked not so useful nor reliable;
        return it to be used;
    }

When you ask the contributor why the code computes and uses it, the
contributor sends an updated patch that lets the user override the logic
and specify the exact value to be used.  That is:

    sub a_function () {
+	if (the user gave a value)
+        	return that value;
	compute some value that looked not so useful nor reliable;
        return it to be used;
    }

Does that updated patch answer the question you asked?

Configurability does not alleviate the puzzlement about the logic. A
better explanation would be to describe how the computation produces
reliable and correct value for the most sane installations; then the user
configurability becomes only "just in case" way to give them the last
resort.

If you don't have a good explanation, the patch should instead be like
this:

    sub a_function () {
-	compute some value that looked not so useful nor reliable;
-       return it to be used;
+	if (the user gave a value)
+        	return that value;
+	die "please configure me";
    }

So please explain why asking your local MTA (either mailhost or localhost)
how it is configured to identify to other MTA is a good way to obtain the
HELO domain you should give $smtp_server.  Your answer could be "I expect
that most people use $smtp_server set to 'mailhost' or 'localhost' and
have the MTA configured to talk with the outside world." (which by the way
I do not think is true for most people).  Or it could be "Xsmtp MTA that
is used by many people to send out mails through their ISP's mailserver
uses the same trick to solve this issue".

Side note.  Apparently this seems to be a common issue.  For example,
msmtp has this configurable via command line and configuration option:

   domain argument
          Use  this  command to set the argument of the SMTP EHLO (or LMTP
          LHLO) command.  The default is localhost (stupid, but  working).
          Possible  choices  are  the  domain  part  of  your mail address
          (provider.example for joe@provider.example) or the fully  quali‐
          fied domain name of your host (if available).

Interestingly, one of the suggestions above is to derive it from the From
address.

In any case, I want to hear your justification.

> +    --smtp-domain           <str>  * The domain name sent to HELO/EHLO handshake

I think this addition makes sense. I suspect that we would also want to
have sendemail.smtpdomain configuration variable.  They of course need to
be documented.

> @@ -131,6 +132,7 @@ my $have_email_valid = eval { require Email::Valid; 1 };
>  my $have_mail_address = eval { require Mail::Address; 1 };
>  my $smtp;
>  my $auth;
> +my $MAIL_DOMAIN;			# See maildomain()

Why uppercase?  The lifetime rule for this looks exactly the same as
existing $smtp to me, so it would be more sensible to spell it in
lowercase, $mail_domain.

> +sub maildomain
> +{
> +        return $MAIL_DOMAIN if $MAIL_DOMAIN;
> +
> +	my $maildomain;

Looks like a funny indent here...

> +	eval "use Net::SMTP";
> +
> +	unless ( $@ ) {
> +		for my $host ( qw(mailhost localhost) ) {

Please drop extra SP immediately after '(' and before ')'; they are
distracting.

But as I already said, I tend to think the logic implemented by this part
is of dubious validity.

> @@ -917,6 +962,8 @@ X-Mailer: git-send-email $gitversion
>  		}
>  	}
>  
> +	my $maildomain;
> +
>  	if ($dry_run) {
>  		# We don't want to send the email.
>  	} elsif ($smtp_server =~ m#^/#) {
> @@ -936,13 +983,18 @@ X-Mailer: git-send-email $gitversion
> ...
> +			$maildomain = maildomain() || "localhost.localdomain";
> +			$smtp ||= Net::SMTP::SSL->new($smtp_server,
> +						      Hello => $maildomain,
> +						      Port => $smtp_server_port);

Why not use the same structure as how lifetime is defined for $smtp
variable?  IOW,

			$mail_domain || = maildomain();
			$smtp ||= ...

without an extra local $maildomain variable?  If you do so, then

 - You can lose the $maildomain variable local to this function;

 - "return what the user configured" does not have to be in the maildomain
   sub;

 - the maildomain sub can return "localhost.localdomain" as a fallback
   default; and

 - various callsites of maildomain sub do not have to repeat the fallback
   default like your patch does.

> @@ -962,9 +1014,10 @@ X-Mailer: git-send-email $gitversion
>  		}
>  
>  		if (!$smtp) {
> -			die "Unable to initialize SMTP properly. Check config. ",
> +			die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",

This part is a good addition, but it needs to be in the earlier patch, no?

Re: [PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Jari Aalto]

Junio C Hamano <gitster@pobox.com> writes:

> So please explain why asking your local MTA (either mailhost or localhost)
> how it is configured to identify to other MTA is a good way to obtain the
> HELO domain you should give $smtp_server.  Your answer could be "I expect
> that most people use $smtp_server set to 'mailhost' or 'localhost' and
> have the MTA configured to talk with the outside world." (which by the way
> I do not think is true for most people).  Or it could be "Xsmtp MTA that
> is used by many people to send out mails through their ISP's mailserver
> uses the same trick to solve this issue".

The code is based on Perl library Test::Reporter

        http://search.cpan.org/~dagolden/Test-Reporter-1.55/lib/Test/Reporter.pm

        /usr/share/perl5/Test/Reporter/Mail/Util.pm ==> sub _maildomain ()

    Function _maildomain() in Test::Reporter uses much more intesive
    tests by scanning all kinds of files, like checking sendmail(1),
    smail(1) etc. configuration files before querying the MTA. I used
    only a portion of it, those parts that I was confortable with to
    handle.

Where to read the FQDN?

    git-send-email contacts MTA to see that it thinks the caller host is
    named, because -- to my knowledge -- the FQDN information is not
    readily available elsewhere in all systems. The MTA does reverse IP
    lookup (DNS) for git-send-email to read.

Why FQDN is passed to MTA?

    Tightly configured MTAs require that the caller gives a real DNS
    domain name that corresponds the IP address in the handshake. This
    prevents spammers from trying to hide their identity.

> Side note.  Apparently this seems to be a common issue.  For example,
> msmtp has this configurable via command line and configuration option:
>
>    domain argument
>           Use  this  command to set the argument of the SMTP EHLO (or LMTP
>           LHLO) command.  The default is localhost (stupid, but  working).
>           Possible  choices  are  the  domain  part  of  your mail address
>           (provider.example for joe@provider.example) or the fully  quali‐
>           fied domain name of your host (if available).
>
> Interestingly, one of the suggestions above is to derive it from the From
> address.
>
> In any case, I want to hear your justification.
>
>> +    --smtp-domain           <str>  * The domain name sent to HELO/EHLO handshake
>
> I think this addition makes sense. I suspect that we would also want to
> have sendemail.smtpdomain configuration variable.  They of course need to
> be documented.

Sure.

> +my $MAIL_DOMAIN;			# See maildomain()
> Why uppercase? 

It might be good to convert all global variables to de facto UPPERCASE.
What do you think? (a separate patch?).

> +        return $MAIL_DOMAIN if $MAIL_DOMAIN;
> +
> +	my $maildomain;
> Looks like a funny indent here...

SP vs. TAB in diff. Fixed.

> Please drop extra SP immediately after '('

Done.

> But as I already said, I tend to think the logic implemented by this part
> is of dubious validity.

Does the above explation address this? I concluded that if the Perl
Module developers use MTA to find out the FQDN, it must be a working
solution. Especially as the code is in the Test:: module libraries.

>> @@ -917,6 +962,8 @@ X-Mailer: git-send-email $gitversion
>>  		}
>>  	}
>>  
>> +	my $maildomain;
>> +
>>  	if ($dry_run) {
>>  		# We don't want to send the email.
>>  	} elsif ($smtp_server =~ m#^/#) {
>> @@ -936,13 +983,18 @@ X-Mailer: git-send-email $gitversion
>> ...
>> +			$maildomain = maildomain() || "localhost.localdomain";
>> +			$smtp ||= Net::SMTP::SSL->new($smtp_server,
>> +						      Hello => $maildomain,
>> +						      Port => $smtp_server_port);
>
> Why not use the same structure as how lifetime is defined for $smtp
> variable?  IOW,
>
> 			$mail_domain || = maildomain();
> 			$smtp ||= ...

This is now completely different. See new maildomain().

> without an extra local $maildomain variable?  If you do so, then
>
>  - You can lose the $maildomain variable local to this function;

It's needed in the error message (see variable's scope):

		if (!$smtp) {
			die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
			    "VALUES: server=$smtp_server ",
			    "encryption=$smtp_encryption ",
!!			    "maildomain=$maildomain",
			    defined $smtp_server_port ? "port=$smtp_server_port" : "";

>  - "return what the user configured" does not have to be in the maildomain
>    sub;

Refactored.

>  - the maildomain sub can return "localhost.localdomain" as a fallback
>    default; and

Refactored.
 
>  - various callsites of maildomain sub do not have to repeat the fallback
>    default like your patch does.
>
>> @@ -962,9 +1014,10 @@ X-Mailer: git-send-email $gitversion
>>  		}
>>  
>>  		if (!$smtp) {
>> -			die "Unable to initialize SMTP properly. Check config. ",
>> +			die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
>
> This part is a good addition, but it needs to be in the earlier patch, no?

The --smtp-debug was introduced in the current patch along with maildomain().

See next message for a reworked patch.
Jari

Re: [PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Junio C Hamano]

Jari Aalto <jari.aalto@cante.net> writes:

> The code is based on Perl library Test::Reporter

Thanks for the pointer.  Mention that somewhere in the code or commit,
perhaps?

> Why FQDN is passed to MTA?

That wasn't what I was asking---you know I was aware of the need when you
read the message from me you are responding to.

>> Side note.  Apparently this seems to be a common issue. ...

> It might be good to convert all global variables to de facto UPPERCASE.
> What do you think? (a separate patch?).

Look at the surrounding code in git-send-email.perl; no all-caps variable
except for the language defined globals like %ENV is used there.  In Perl
all-caps tend to give things magic meaning (think BEGIN, $ENV{}, @ARGV,...)
and I don't think it is a good idea to name your own stuff in all-caps
unless there is a good reason for it.  The variable being in global scope
is not a good enough reason.

> It's needed in the error message (see variable's scope):

A global is visible there, isn't it (see the other review message)?

>>> @@ -962,9 +1014,10 @@ X-Mailer: git-send-email $gitversion
>>>  		}
>>>  
>>>  		if (!$smtp) {
>>> -			die "Unable to initialize SMTP properly. Check config. ",
>>> +			die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
>>
>> This part is a good addition, but it needs to be in the earlier patch, no?
>
> The --smtp-debug was introduced in the current patch along with maildomain().

I don't think so, at least in the submitted form.  Perhaps you originally
wrote a single combined commit, and later split it into a few commits.

The patch under discussion adds --smtp-domain, and would apply on top of a
separate patch that adds --smtp-debug.  I already said that it is a good
thing to suggest using the option in die() message, but that should be
done _not_ in this patch, but in the previous one that added --smtp-debug.

Re: [PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Jari Aalto]

Junio C Hamano <gitster@pobox.com> writes:

>>>> +			die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
>>>
>>
>> The --smtp-debug was introduced in the current patch along with maildomain().
>
> I don't think so, at least in the submitted form.  Perhaps you originally
> wrote a single combined commit, and later split it into a few commits.

Correct. Fixed now.

Jari

Re: [PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Jari Aalto]

Junio C Hamano <gitster@pobox.com> writes:

>> Why FQDN is passed to MTA?
>
> That wasn't what I was asking---you know I was aware of the need when you
> read the message from me you are responding to.

I'm not sure I know what was asked exactly. Could you rephrase and we'll
try to see what is missing.

Jari

[PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Jari Aalto]

Add new functions maildomain_net(), maildomain_mta() and
maildomain(), which return FQDN where possible for use in
send_message(). The value is passed to Net::SMTP HELO/EHLO
handshake. The domain name can also be set via new --smtp-domain
option.

The default value in Net::SMTP may not get through:

  Net::SMTP=GLOB(0x267ec28)>>> EHLO localhost.localdomain
  Net::SMTP=GLOB(0x267ec28)<<< 550 EHLO argument does not match calling host

whereas using the FQDN that matches the IP, the result is:

  Net::SMTP=GLOB(0x15b8e80)>>> EHLO host.example.com
  Net::SMTP=GLOB(0x15b8e80)<<< 250-host.example.com Hello host.example.com [192.168.1.7]

Signed-off-by: Jari Aalto <jari.aalto@cante.net>
---
 git-send-email.perl |   83 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 81 insertions(+), 2 deletions(-)


    ==============================
    This is REVISION 3, reworked
    ==============================


diff --git a/git-send-email.perl b/git-send-email.perl
index 6af7bd3..80de092 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -64,6 +64,7 @@ git send-email [options] <file | directory | rev-list options >
     --smtp-pass             <str>  * Password for SMTP-AUTH; not necessary.
     --smtp-encryption       <str>  * tls or ssl; anything else disables.
     --smtp-ssl                     * Deprecated. Use '--smtp-encryption ssl'.
+    --smtp-domain           <str>  * The domain name sent to HELO/EHLO handshake
     --smtp-debug            <0|1>  * Disable, enable Net::SMTP debug.
 
   Automating:
@@ -131,6 +132,7 @@ my $have_email_valid = eval { require Email::Valid; 1 };
 my $have_mail_address = eval { require Mail::Address; 1 };
 my $smtp;
 my $auth;
+my $mail_domain;
 
 sub unique_email_list(@);
 sub cleanup_compose_files();
@@ -274,6 +276,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "smtp-ssl" => sub { $smtp_encryption = 'ssl' },
 		    "smtp-encryption=s" => \$smtp_encryption,
 		    "smtp-debug:i" => \$debug_net_smtp,
+		    "smtp-domain:s" => \$mail_domain,
 		    "identity=s" => \$identity,
 		    "annotate" => \$annotate,
 		    "compose" => \$compose,
@@ -834,6 +837,74 @@ sub sanitize_address
 
 }
 
+# Returns the local Fully Qualified Domain Name (FQDN) if available.
+#
+# Tightly configured MTAa require that a caller sends a real DNS
+# domain name that corresponds the IP address in the HELO/EHLO
+# handshake. This is used to verify the connection and prevent
+# spammers from trying to hide their identity. If the DNS and IP don't
+# match, the receiveing MTA may deny the connection.
+#
+# Here is a deny example of Net::SMTP with the default "localhost.localdomain"
+#
+# Net::SMTP=GLOB(0x267ec28)>>> EHLO localhost.localdomain
+# Net::SMTP=GLOB(0x267ec28)<<< 550 EHLO argument does not match calling host
+
+sub maildomain_net
+{
+	my $maildomain;
+	eval "use Net::Domain";
+
+	unless ($@) {
+		eval "use Net::Domain";
+		unless ($@) {
+		    my $domain = Net::Domain::domainname();
+		    $maildomain = $domain
+			    unless $^O eq 'darwin' && $domain =~ /\.local$/;
+		}
+	}
+
+	$maildomain;
+}
+
+sub maildomain_mta
+{
+	my $maildomain;
+	eval "use Net::SMTP";
+
+	unless ($@) {
+		for my $host (qw(mailhost localhost)) {
+			my $smtp = Net::SMTP->new($host);
+			if (defined $smtp) {
+				my $domain = $smtp->domain;
+				$smtp->quit;
+
+				$maildomain = $domain
+					unless $^O eq 'darwin' && $domain =~ /\.local$/;
+
+				last if $maildomain;
+			}
+		}
+	}
+
+	$maildomain;
+}
+
+{
+    my $mail_domain_system;		# Static variable
+sub maildomain
+{
+    if ($mail_domain) {
+	    $mail_domain		# Command line
+    } elsif ($mail_domain_system) {	# Saved system default
+	    $mail_domain_system;
+    } else {
+	    $mail_domain_system = maildomain_net() ||
+				 maildomain_mta() ||
+				 "localhost.localdomain";
+    }
+}}
+
 # Returns 1 if the message was sent, and 0 otherwise.
 # In actuality, the whole program dies when there
 # is an error sending a message.
@@ -917,6 +988,8 @@ X-Mailer: git-send-email $gitversion
 		}
 	}
 
+	my $maildomain;
+
 	if ($dry_run) {
 		# We don't want to send the email.
 	} elsif ($smtp_server =~ m#^/#) {
@@ -936,13 +1009,18 @@ X-Mailer: git-send-email $gitversion
 		if ($smtp_encryption eq 'ssl') {
 			$smtp_server_port ||= 465; # ssmtp
 			require Net::SMTP::SSL;
-			$smtp ||= Net::SMTP::SSL->new($smtp_server, Port => $smtp_server_port);
+			$maildomain = maildomain();
+			$smtp ||= Net::SMTP::SSL->new($smtp_server,
+						      Hello => $maildomain,
+						      Port => $smtp_server_port);
 		}
 		else {
 			require Net::SMTP;
+			$maildomain = maildomain();
 			$smtp ||= Net::SMTP->new((defined $smtp_server_port)
 						 ? "$smtp_server:$smtp_server_port"
 						 : $smtp_server,
+						 Hello => $maildomain,
 						 Debug => $debug_net_smtp);
 			if ($smtp_encryption eq 'tls' && $smtp) {
 				require Net::SMTP::SSL;
@@ -962,9 +1040,10 @@ X-Mailer: git-send-email $gitversion
 		}
 
 		if (!$smtp) {
-			die "Unable to initialize SMTP properly. Check config. ",
+			die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
 			    "VALUES: server=$smtp_server ",
 			    "encryption=$smtp_encryption ",
+			    "maildomain=$maildomain",
 			    defined $smtp_server_port ? "port=$smtp_server_port" : "";
 		}
 
-- 
1.7.0

Re: [PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Junio C Hamano]

Jari Aalto <jari.aalto@cante.net> writes:

> +sub maildomain_net
> +{
> +	my $maildomain;
> +	eval "use Net::Domain";
> +
> +	unless ($@) {
> +		eval "use Net::Domain";
> +		unless ($@) {

Sorry, but I don't understand the reason why you need to check the same
thing twice.  The original you borrowed from seems to be much cleanly
written; it essentially boils down to:

    if (eval "require Net::Domain") {
        my $domain = Net::Domain::Domainname();
        ...
    }

without need for separate "unless ($@)", nor doubly nested construct.

> +{
> +    my $mail_domain_system;		# Static variable

This, and ...


> @@ -917,6 +988,8 @@ X-Mailer: git-send-email $gitversion
>  		}
>  	}
>  
> +	my $maildomain;
> +

... this, I still don't see the point of them.

> @@ -962,9 +1040,10 @@ X-Mailer: git-send-email $gitversion
>  		}
>  
>  		if (!$smtp) {
> -			die "Unable to initialize SMTP properly. Check config. ",
> +			die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
>  			    "VALUES: server=$smtp_server ",
>  			    "encryption=$smtp_encryption ",
> +			    "maildomain=$maildomain",

You said you needed a separate local variable for reporting but that
doesn't explain why you need three redundant variables.  Why can't the
code look like this?

-- >8 -- snip -- >8 --
my $mail_domain; # toplevel global

GetOptions(...,
           "smtp-domain=s" => \$mail_domain,
           ...);

sub maildomain {
	maindomain_net() || maildomain_mta() || "localhost.localdomain";
}

sub send_message {
	...
        $mail_domain ||= maildomain();
        if (ssl) {
        	$smtp ||= Net::SMTP::SSL->new(...);
	} else {
        	$smtp || Net::SMTL->new(...);
	}
	...
        if (!$smtp) {
        	die "Unable to... your maildomain is set to $mail_domain";
	}
}
-- 8< -- snap -- 8< --

That is:

 - $mail_domain can be set from the command line;
 - once set, ||= ensures that it will we used without needing to
   call maildomain();
 - the value you used unsuccessfully to obtain $smtp is reported.

which seems to be more in line with the way how existing code avoids
resetting $smtp once it gets a working one.

Re: [PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Jari Aalto]

Junio C Hamano <gitster@pobox.com> writes:

> Jari Aalto <jari.aalto@cante.net> writes:
>
>> +sub maildomain_net
>> +{
>> +	my $maildomain;
>> +	eval "use Net::Domain";
>> +
>> +	unless ($@) {
>> +		eval "use Net::Domain";
>> +		unless ($@) {
>
> Sorry, but I don't understand the reason why you need to check the same
> thing twice.  The original you borrowed from seems to be much cleanly
> written; it essentially boils down to:
>
>     if (eval "require Net::Domain") {
>         my $domain = Net::Domain::Domainname();
>         ...
>     }
>
> without need for separate "unless ($@)", nor doubly nested construct.

Code written like that inside if-statements is pretty bad IMHO. It is
very debuggable, whereas using variables would be:

    >> +	eval "use Net::Domain";
    >> +
                Debug("use Net::Domain", $@);
    >> +	unless ($@) {


But that's my code to maintain, I'll do what is needed.
Jari

Re: [PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Jari Aalto]

Junio C Hamano <gitster@pobox.com> writes:
>     if (eval "require Net::Domain") {

Refactored.

>> +{
>> +    my $mail_domain_system;		# Static variable
>
> This, and ...

Refactored.
>
>> @@ -917,6 +988,8 @@ X-Mailer: git-send-email $gitversion
>>  		}
>>  	}
>>  
>> +	my $maildomain;
>> +

Refactored.

>
>> @@ -962,9 +1040,10 @@ X-Mailer: git-send-email $gitversion
>>  		}
>>  
>>  		if (!$smtp) {
>> -			die "Unable to initialize SMTP properly. Check config. ",
>> +			die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
>>  			    "VALUES: server=$smtp_server ",
>>  			    "encryption=$smtp_encryption ",
>> +			    "maildomain=$maildomain",
>
> You said you needed a separate local variable for reporting but that
> doesn't explain why you need three redundant variables.  Why can't the
> code look like this?

Refactored.

>
>  - once set, ||= ensures that it will we used without needing to

Refactored.

Jari

Re: [PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Jakub Narebski]

Jari Aalto <jari.aalto@cante.net> writes:

> +sub maildomain_net
> +{
> +	my $maildomain;
> +	eval "use Net::Domain";
> +
> +	unless ($@) {

I think the Perl idiom for this is

   	if (eval { require Net::Domain; 1 }) {

Note the block form of eval (which is more efficient, and is usual way
of trap exceptions in Perl), using return value of eval instead of
using $@ variable, and using 'require' rather than 'use'.

> +		eval "use Net::Domain";
> +		unless ($@) {

Why this duplication?  You have 'eval unless eval unless', with 
'eval unless' twice.

> +		    my $domain = Net::Domain::domainname();
> +		    $maildomain = $domain
> +			    unless $^O eq 'darwin' && $domain =~ /\.local$/;

I'd like to have a comment about the above line: why it is necessary?

> +		}
> +	}
> +
> +	$maildomain;

It is a matter of style, but unless function is a very simple one, the
preferred way is to use explicit return statement, i.e.

   	return $maildomain;

> +}

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Jari Aalto]

Jakub Narebski <jnareb@gmail.com> writes:

> Why this duplication?  You have 'eval unless eval unless', with 
> 'eval unless' twice.

A typo after rafactoring the function into 3. Thanks.

>
>> +		    my $domain = Net::Domain::domainname();
>> +		    $maildomain = $domain
>> +			    unless $^O eq 'darwin' && $domain =~ /\.local$/;
>
> I'd like to have a comment about the above line: why it is necessary?

I have no idea. It is used in Perl library Test::Reporter, so I trust it
is necessary for MAC OSX.

>> +	$maildomain;

> It is a matter of style, but unless function is a very simple one, the
> preferred way is to use explicit return statement, i.e.
>
>    	return $maildomain;

In Perl, the last statement's result is the returned value from a
function. Compare:

    $ perl -e 'print  if $_ = do{ 1; }'
    1

Yes, it's a style issue. I don't personally use "return" at the end of
simple functions in Perl.

I'll add the "return".
Jari

[PATCH 1/3] git-send-email.perl: improve error message in send_message()

[Jari Aalto]

Signed-off-by: Jari Aalto <jari.aalto@cante.net>
---
 git-send-email.perl |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

    ================================
    This is REVISION SET 4, reworked
    ================================

diff --git a/git-send-email.perl b/git-send-email.perl
index e05455f..221506c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -957,7 +957,10 @@ X-Mailer: git-send-email $gitversion
 		}
 
 		if (!$smtp) {
-			die "Unable to initialize SMTP properly.  Is there something wrong with your config?";
+			die "Unable to initialize SMTP properly. Check config. ",
+			    "VALUES: server=$smtp_server ",
+			    "encryption=$smtp_encryption ",
+			    defined $smtp_server_port ? "port=$smtp_server_port" : "";
 		}
 
 		if (defined $smtp_authuser) {
-- 
1.7.0

[PATCH 2/3] git-send-email.perl: add option --smtp-debug

[Jari Aalto]

Signed-off-by: Jari Aalto <jari.aalto@cante.net>
---
 git-send-email.perl |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)


    ================================
    This is REVISION SET 4, reworked
    ================================


diff --git a/git-send-email.perl b/git-send-email.perl
index 221506c..a8887ea 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -64,6 +64,7 @@ git send-email [options] <file | directory | rev-list options >
     --smtp-pass             <str>  * Password for SMTP-AUTH; not necessary.
     --smtp-encryption       <str>  * tls or ssl; anything else disables.
     --smtp-ssl                     * Deprecated. Use '--smtp-encryption ssl'.
+    --smtp-debug            <0|1>  * Disable, enable Net::SMTP debug.
 
   Automating:
     --identity              <str>  * Use the sendemail.<id> options.
@@ -187,6 +188,8 @@ my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts);
 my ($validate, $confirm);
 my (@suppress_cc);
 
+my ($debug_net_smtp) = 0;		# Net::SMTP, see send_message()
+
 my $not_set_by_user = "true but not set by the user";
 
 my %config_bool_settings = (
@@ -270,6 +273,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "smtp-pass:s" => \$smtp_authpass,
 		    "smtp-ssl" => sub { $smtp_encryption = 'ssl' },
 		    "smtp-encryption=s" => \$smtp_encryption,
+		    "smtp-debug:i" => \$debug_net_smtp,
 		    "identity=s" => \$identity,
 		    "annotate" => \$annotate,
 		    "compose" => \$compose,
@@ -938,7 +942,8 @@ X-Mailer: git-send-email $gitversion
 			require Net::SMTP;
 			$smtp ||= Net::SMTP->new((defined $smtp_server_port)
 						 ? "$smtp_server:$smtp_server_port"
-						 : $smtp_server);
+						 : $smtp_server,
+						 Debug => $debug_net_smtp);
 			if ($smtp_encryption eq 'tls' && $smtp) {
 				require Net::SMTP::SSL;
 				$smtp->command('STARTTLS');
@@ -957,7 +962,7 @@ X-Mailer: git-send-email $gitversion
 		}
 
 		if (!$smtp) {
-			die "Unable to initialize SMTP properly. Check config. ",
+			die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
 			    "VALUES: server=$smtp_server ",
 			    "encryption=$smtp_encryption ",
 			    defined $smtp_server_port ? "port=$smtp_server_port" : "";
-- 
1.7.0

[PATCH 3/3] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Jari Aalto]

Add new functions maildomain_net(), maildomain_mta() and
maildomain(), which return FQDN where possible for use in
send_message(). The value is passed to Net::SMTP HELO/EHLO
handshake. The domain name can also be set via new --smtp-domain
option.

The default value in Net::SMTP may not get through:

  Net::SMTP=GLOB(0x267ec28)>>> EHLO localhost.localdomain
  Net::SMTP=GLOB(0x267ec28)<<< 550 EHLO argument does not match calling host

whereas using the FQDN that matches the IP, the result is:

  Net::SMTP=GLOB(0x15b8e80)>>> EHLO host.example.com
  Net::SMTP=GLOB(0x15b8e80)<<< 250-host.example.com Hello host.example.com [192.168.1.7]

The maildomain*() code is based on ideas in Perl library
Test::Reporter by Graham Barr <gbarr@pobox.com> and Mark Overmeer
<mailtools@overmeer.net> released under 'the same terms as Perl
itself'.

Signed-off-by: Jari Aalto <jari.aalto@cante.net>
---
 git-send-email.perl |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 70 insertions(+), 1 deletions(-)



    ================================
    This is REVISION SET 4, reworked
    ================================



diff --git a/git-send-email.perl b/git-send-email.perl
index a8887ea..5fe8246 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -64,6 +64,7 @@ git send-email [options] <file | directory | rev-list options >
     --smtp-pass             <str>  * Password for SMTP-AUTH; not necessary.
     --smtp-encryption       <str>  * tls or ssl; anything else disables.
     --smtp-ssl                     * Deprecated. Use '--smtp-encryption ssl'.
+    --smtp-domain           <str>  * The domain name sent to HELO/EHLO handshake
     --smtp-debug            <0|1>  * Disable, enable Net::SMTP debug.
 
   Automating:
@@ -131,6 +132,8 @@ my $have_email_valid = eval { require Email::Valid; 1 };
 my $have_mail_address = eval { require Mail::Address; 1 };
 my $smtp;
 my $auth;
+my $mail_domain_default = "localhost.localdomain";
+my $mail_domain;
 
 sub unique_email_list(@);
 sub cleanup_compose_files();
@@ -274,6 +277,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "smtp-ssl" => sub { $smtp_encryption = 'ssl' },
 		    "smtp-encryption=s" => \$smtp_encryption,
 		    "smtp-debug:i" => \$debug_net_smtp,
+		    "smtp-domain:s" => \$mail_domain,
 		    "identity=s" => \$identity,
 		    "annotate" => \$annotate,
 		    "compose" => \$compose,
@@ -834,6 +838,65 @@ sub sanitize_address
 
 }
 
+# Returns the local Fully Qualified Domain Name (FQDN) if available.
+#
+# Tightly configured MTAa require that a caller sends a real DNS
+# domain name that corresponds the IP address in the HELO/EHLO
+# handshake. This is used to verify the connection and prevent
+# spammers from trying to hide their identity. If the DNS and IP don't
+# match, the receiveing MTA may deny the connection.
+#
+# Here is a deny example of Net::SMTP with the default "localhost.localdomain"
+#
+# Net::SMTP=GLOB(0x267ec28)>>> EHLO localhost.localdomain
+# Net::SMTP=GLOB(0x267ec28)<<< 550 EHLO argument does not match calling host
+#
+# This maildomain*() code is based on ideas in Perl library Test::Reporter
+# /usr/share/perl5/Test/Reporter/Mail/Util.pm ==> sub _maildomain ()
+
+sub maildomain_net
+{
+	my $maildomain;
+
+	if (eval { require Net::Domain; 1 }) {
+		eval "use Net::Domain";
+		unless ($@) {
+		    my $domain = Net::Domain::domainname();
+		    $maildomain = $domain
+			    unless $^O eq 'darwin' && $domain =~ /\.local$/;
+		}
+	}
+
+	return $maildomain;
+}
+
+sub maildomain_mta
+{
+	my $maildomain;
+
+	if (eval { require Net::SMTP; 1 }) {
+		for my $host (qw(mailhost localhost)) {
+			my $smtp = Net::SMTP->new($host);
+			if (defined $smtp) {
+				my $domain = $smtp->domain;
+				$smtp->quit;
+
+				$maildomain = $domain
+					unless $^O eq 'darwin' && $domain =~ /\.local$/;
+
+				last if $maildomain;
+			}
+		}
+	}
+
+	return $maildomain;
+}
+
+sub maildomain
+{
+	return maildomain_net() || maildomain_mta() || $mail_domain_default;
+}
+
 # Returns 1 if the message was sent, and 0 otherwise.
 # In actuality, the whole program dies when there
 # is an error sending a message.
@@ -936,13 +999,18 @@ X-Mailer: git-send-email $gitversion
 		if ($smtp_encryption eq 'ssl') {
 			$smtp_server_port ||= 465; # ssmtp
 			require Net::SMTP::SSL;
-			$smtp ||= Net::SMTP::SSL->new($smtp_server, Port => $smtp_server_port);
+			$mail_domain ||= maildomain();
+			$smtp ||= Net::SMTP::SSL->new($smtp_server,
+						      Hello => $mail_domain,
+						      Port => $smtp_server_port);
 		}
 		else {
 			require Net::SMTP;
+			$mail_domain ||= maildomain();
 			$smtp ||= Net::SMTP->new((defined $smtp_server_port)
 						 ? "$smtp_server:$smtp_server_port"
 						 : $smtp_server,
+						 Hello => $mail_domain,
 						 Debug => $debug_net_smtp);
 			if ($smtp_encryption eq 'tls' && $smtp) {
 				require Net::SMTP::SSL;
@@ -965,6 +1033,7 @@ X-Mailer: git-send-email $gitversion
 			die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
 			    "VALUES: server=$smtp_server ",
 			    "encryption=$smtp_encryption ",
+			    "maildomain=$mail_domain",
 			    defined $smtp_server_port ? "port=$smtp_server_port" : "";
 		}
 
-- 
1.7.0

Re: [PATCH 3/3] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Jakub Narebski]

Jari Aalto <jari.aalto@cante.net> writes:

> ---
>  git-send-email.perl |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 70 insertions(+), 1 deletions(-)
> 
> 
> 
>     ================================
>     This is REVISION SET 4, reworked
>     ================================
 
Here you should describe differences from v3 (from previous version),
for easier review.
 
> +# This maildomain*() code is based on ideas in Perl library Test::Reporter
> +# /usr/share/perl5/Test/Reporter/Mail/Util.pm ==> sub _maildomain ()

Nice... although it might be better to use Test::Reporter::Mail::Util
as canonical module name (the package can be installed somewhere else,
depending on operating system / distribution, and if one uses
local::lib for local / per-user installation).

> +sub maildomain_net
> +{
> +	my $maildomain;
> +
> +	if (eval { require Net::Domain; 1 }) {
> +		eval "use Net::Domain";
> +		unless ($@) {
> +		    my $domain = Net::Domain::domainname();
> +		    $maildomain = $domain
> +			    unless $^O eq 'darwin' && $domain =~ /\.local$/;


Here should be a comment 'following what Test::Reporter does' or
something like that.

> +		}
> +	}
> +
> +	return $maildomain;
> +}

You still have duplicated 'require' ('use' is 'require' + 'import')
and check if it succeeded.  It should read:

+sub maildomain_net {
+	my $maildomain;
+
+	if (eval { require Net::Domain; 1 }) {
+		my $domain = Net::Domain::domainname();
+		$maildomain = $domain
+			unless $^O eq 'darwin' && $domain =~ /\.local$/;
+	}
+
+	return $maildomain;
+}

In the subroutine below you do not duplicate check for require.

Sidenote: alternate soultion would be to write (with one less level of
indent):

+sub maildomain_net {
+	my $maildomain;
+
+	eval { require Net::Domain; };
+	return if $@;
+
+	$maildomain = Net::Domain::domainname();
+		unless $^O eq 'darwin' && $domain =~ /\.local$/;
+
+	return $maildomain;
+}


> +
> +sub maildomain_mta
> +{

Use the same Perl convention that used elsewhere in git-send-email.perl
(this is usually used Perl style).

+sub maildomain_mta {

> +	my $maildomain;
> +
> +	if (eval { require Net::SMTP; 1 }) {
> +		for my $host (qw(mailhost localhost)) {
> +			my $smtp = Net::SMTP->new($host);
> +			if (defined $smtp) {
> +				my $domain = $smtp->domain;
> +				$smtp->quit;
> +
> +				$maildomain = $domain
> +					unless $^O eq 'darwin' && $domain =~ /\.local$/;
> +
> +				last if $maildomain;
> +			}
> +		}
> +	}
> +
> +	return $maildomain;
> +}
> +
> +sub maildomain
> +{
> +	return maildomain_net() || maildomain_mta() || $mail_domain_default;
> +}

Nice.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 3/3] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Jari Aalto]

Jakub Narebski <jnareb@gmail.com> writes:

> Nice... although it might be better to use Test::Reporter::Mail::Util

Done.

>> +sub maildomain_net
> Here should be a comment 'following what Test::Reporter does' or
> something like that.

Done.

> Sidenote: alternate soultion would be to write (with one less level of
> indent):

Done all.

>> +sub maildomain_mta
>> +{
>
> Use the same Perl convention that used elsewhere in git-send-email.perl
> (this is usually used Perl style).
>
> +sub maildomain_mta {

The code uses variety of styles. The function below, where the
maildomain*() are related used:

    sub send_message
    {

Do I keep the "sub....\n{" ?

Jari

-----------------------------------------------------------------------

sub new {
        my ($class, $reason) = @_;
        return bless \$reason, shift;
--
sub readline {
        my $self = shift;
        die "Cannot use readline on FakeTerm: $$self";
--
sub usage {
        print <<EOT;
git send-email [options] <file | directory | rev-list options >
--
sub format_2822_time {
        my ($time) = @_;
        my @localtm = localtime($time);
--
sub unique_email_list(@);
sub cleanup_compose_files();

# Variables we fill in automatically, or via prompting:
--
sub do_edit {
        if (defined($multiedit) && !$multiedit) {
                map {
--
sub chain_reply_to {
        if (defined $chain_reply_to &&
            $chain_reply_to eq $not_set_by_user) {
--
sub signal_handler {

        # Make text normal
--
sub read_config {
        my ($prefix) = @_;

--
sub parse_address_line {
        if ($have_mail_address) {
                return map { $_->format } Mail::Address->parse($_[0]);
--
sub split_addrs {
        return quotewords('\s*,\s*', 1, @_);
}
--
sub check_file_rev_conflict($) {
        return unless $repo;
        my $f = shift;
--
sub get_patch_subject($) {
        my $fn = shift;
        open (my $fh, '<', $fn);
--
sub ask {
        my ($prompt, %arg) = @_;
        my $valid_re = $arg{valid_re};
--
sub expand_aliases {
        return map { expand_one_alias($_) } @_;
}
--
sub expand_one_alias {
        my $alias = shift;
        if ($EXPANDED_ALIASES{$alias}) {
--
sub extract_valid_address {
        my $address = shift;
        my $local_part_regexp = '[^<>"\s@]+';
--
sub make_message_id
{
        my $uniq;
--
sub unquote_rfc2047 {
        local ($_) = @_;
        my $encoding;
--
sub quote_rfc2047 {
        local $_ = shift;
        my $encoding = shift || 'UTF-8';
--
sub is_rfc2047_quoted {
        my $s = shift;
        my $token = '[^][()<>@,;:"\/?.= \000-\037\177-\377]+';
--
sub sanitize_address
{
        my ($recipient) = @_;
--
sub maildomain_net
{
        my $maildomain;
--
sub maildomain_mta
{
        my $maildomain;
--
sub maildomain
{
        return maildomain_net() || maildomain_mta() || $mail_domain_default;
--
sub send_message
{
        my @recipients = unique_email_list(@to);
--
sub cleanup_compose_files() {
        unlink($compose_filename, $compose_filename . ".final") if $compose;
}
--
sub unique_email_list(@) {
        my %seen;
        my @emails;
--
sub validate_patch {
        my $fn = shift;
        open(my $fh, '<', $fn)
--
sub file_has_nonascii {
        my $fn = shift;
        open(my $fh, '<', $fn)

[PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

[Jari Aalto]

Add new functions maildomain_net(), maildomain_mta() and
maildomain(), which return FQDN where possible for use in
send_message(). The value is passed to Net::SMTP HELO/EHLO
handshake. The domain name can also be set via new --smtp-domain
option.

The default value in Net::SMTP may not get through:

  Net::SMTP=GLOB(0x267ec28)>>> EHLO localhost.localdomain
  Net::SMTP=GLOB(0x267ec28)<<< 550 EHLO argument does not match calling host

whereas using the FQDN that matches the IP, the result is:

  Net::SMTP=GLOB(0x15b8e80)>>> EHLO host.example.com
  Net::SMTP=GLOB(0x15b8e80)<<< 250-host.example.com Hello host.example.com [192.168.1.7]

The maildomain*() code is based on ideas in Perl library
Test::Reporter by Graham Barr <gbarr@pobox.com> and Mark Overmeer
<mailtools@overmeer.net> released under 'the same terms as Perl
itself'.

Signed-off-by: Jari Aalto <jari.aalto@cante.net>
---
 git-send-email.perl |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 70 insertions(+), 1 deletions(-)



    ================================
    This is REVISION 5, reworked
    ================================

    Changes as suggested by Jakub Narebski in
    - http://permalink.gmane.org/gmane.comp.version-control.git/142149



diff --git a/git-send-email.perl b/git-send-email.perl
index a8887ea..f8cc222 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -64,6 +64,7 @@ git send-email [options] <file | directory | rev-list options >
     --smtp-pass             <str>  * Password for SMTP-AUTH; not necessary.
     --smtp-encryption       <str>  * tls or ssl; anything else disables.
     --smtp-ssl                     * Deprecated. Use '--smtp-encryption ssl'.
+    --smtp-domain           <str>  * The domain name sent to HELO/EHLO handshake
     --smtp-debug            <0|1>  * Disable, enable Net::SMTP debug.
 
   Automating:
@@ -131,6 +132,8 @@ my $have_email_valid = eval { require Email::Valid; 1 };
 my $have_mail_address = eval { require Mail::Address; 1 };
 my $smtp;
 my $auth;
+my $mail_domain_default = "localhost.localdomain";
+my $mail_domain;
 
 sub unique_email_list(@);
 sub cleanup_compose_files();
@@ -274,6 +277,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "smtp-ssl" => sub { $smtp_encryption = 'ssl' },
 		    "smtp-encryption=s" => \$smtp_encryption,
 		    "smtp-debug:i" => \$debug_net_smtp,
+		    "smtp-domain:s" => \$mail_domain,
 		    "identity=s" => \$identity,
 		    "annotate" => \$annotate,
 		    "compose" => \$compose,
@@ -834,6 +838,65 @@ sub sanitize_address
 
 }
 
+# Returns the local Fully Qualified Domain Name (FQDN) if available.
+#
+# Tightly configured MTAa require that a caller sends a real DNS
+# domain name that corresponds the IP address in the HELO/EHLO
+# handshake. This is used to verify the connection and prevent
+# spammers from trying to hide their identity. If the DNS and IP don't
+# match, the receiveing MTA may deny the connection.
+#
+# Here is a deny example of Net::SMTP with the default "localhost.localdomain"
+#
+# Net::SMTP=GLOB(0x267ec28)>>> EHLO localhost.localdomain
+# Net::SMTP=GLOB(0x267ec28)<<< 550 EHLO argument does not match calling host
+#
+# This maildomain*() code is based on ideas in Perl library
+# Test::Reporter::Util::_maildomain().
+
+sub maildomain_net
+{
+	eval { require Net::Domain; 1 };
+	return if $@;
+
+	my $domain = Net::Domain::domainname();
+
+	# handling as in Test::Reporter::Util::_maildomain()
+	my $maildomain = $domain
+		unless $^O eq 'darwin' && $domain =~ /\.local$/;
+
+	return $maildomain;
+}
+
+sub maildomain_mta
+{
+	eval { require Net::SMTP; 1 };
+	return if $@;
+
+	my $maildomain;
+
+	for my $host (qw(mailhost localhost)) {
+		my $smtp = Net::SMTP->new($host);
+		if (defined $smtp) {
+			my $domain = $smtp->domain;
+			$smtp->quit;
+
+			# handling as in Test::Reporter::Util::_maildomain()
+			$maildomain = $domain
+				unless $^O eq 'darwin' && $domain =~ /\.local$/;
+
+			last if $maildomain;
+		}
+	}
+
+	return $maildomain;
+}
+
+sub maildomain
+{
+	return maildomain_net() || maildomain_mta() || $mail_domain_default;
+}
+
 # Returns 1 if the message was sent, and 0 otherwise.
 # In actuality, the whole program dies when there
 # is an error sending a message.
@@ -936,13 +999,18 @@ X-Mailer: git-send-email $gitversion
 		if ($smtp_encryption eq 'ssl') {
 			$smtp_server_port ||= 465; # ssmtp
 			require Net::SMTP::SSL;
-			$smtp ||= Net::SMTP::SSL->new($smtp_server, Port => $smtp_server_port);
+			$mail_domain ||= maildomain();
+			$smtp ||= Net::SMTP::SSL->new($smtp_server,
+						      Hello => $mail_domain,
+						      Port => $smtp_server_port);
 		}
 		else {
 			require Net::SMTP;
+			$mail_domain ||= maildomain();
 			$smtp ||= Net::SMTP->new((defined $smtp_server_port)
 						 ? "$smtp_server:$smtp_server_port"
 						 : $smtp_server,
+						 Hello => $mail_domain,
 						 Debug => $debug_net_smtp);
 			if ($smtp_encryption eq 'tls' && $smtp) {
 				require Net::SMTP::SSL;
@@ -965,6 +1033,7 @@ X-Mailer: git-send-email $gitversion
 			die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
 			    "VALUES: server=$smtp_server ",
 			    "encryption=$smtp_encryption ",
+			    "maildomain=$mail_domain",
 			    defined $smtp_server_port ? "port=$smtp_server_port" : "";
 		}
 
-- 
1.7.0