[PATCH v2 0/3] *** SUBJECT HERE ***

[PATCH v2 0/3] *** SUBJECT HERE ***

[Brian Gernhardt]

Changes from v1:
  - Moves valid FQDN conditions from line-ending conditions to a new
    function
  - Adds sendemail.smtpdomain to the list in config.txt

Patch 2/3 is unchanged.

Brian Gernhardt (3):
  send-email: Don't use FQDNs without a '.'
  Document send-email --smtp-domain
  send-email: Add sendemail.smtpdomain

 Documentation/config.txt         |    1 +
 Documentation/git-send-email.txt |    7 +++++++
 git-send-email.perl              |   32 ++++++++++++++++++--------------
 3 files changed, 26 insertions(+), 14 deletions(-)

[PATCH] Makefile: Simplify handling of python scripts

[Brian Gernhardt]

The sed script intended to add a standard opening to python scripts
was non-compatible and overly complex.  Simplifying it down to a set
of one-liners removes the compatibility issues of newlines.  Moving
the environment alterations from the Makefile to the python scripts
makes also makes the scripts easier to run in-place.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---
 Makefile              |    9 ++-------
 git-remote-testgit.py |    2 ++
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index f0fe351..87c90d6 100644
--- a/Makefile
+++ b/Makefile
@@ -1629,13 +1629,8 @@ $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C git_remote_helpers -s \
 		--no-print-directory prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' \
 		instlibdir` && \
-	sed -e '1{' \
-	    -e '	s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
-	    -e '}' \
-	    -e 's|^import sys.*|&; \\\
-	           import os; \\\
-	           sys.path.insert(0, os.getenv("GITPYTHONLIB",\
-	                                        "@@INSTLIBDIR@@"));|' \
+	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
+	    -e 's|\(os\.getenv("GITPYTHONLIB"\)[^)]*)|\1,"@@INSTLIBDIR@@")|' \
 	    -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
 	    $@.py >$@+ && \
 	chmod +x $@+ && \
diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index f61624e..9253922 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -2,6 +2,8 @@
 
 import hashlib
 import sys
+import os
+sys.path.insert(0, os.getenv("GITPYTHONLIB","."))
 
 from git_remote_helpers.util import die, debug, warn
 from git_remote_helpers.git.repo import GitRepo
-- 
1.7.1.rc0.243.g2ce66

Re: [PATCH] Makefile: Simplify handling of python scripts

[Sverre Rabbelier]

Heya,

On Fri, Apr 9, 2010 at 17:34, Brian Gernhardt
<brian@gernhardtsoftware.com> wrote:
> The sed script intended to add a standard opening to python scripts
> was non-compatible and overly complex.  Simplifying it down to a set
> of one-liners removes the compatibility issues of newlines.  Moving
> the environment alterations from the Makefile to the python scripts
> makes also makes the scripts easier to run in-place.

My sed foo is not that great, can you explain (in the commit message)
what exactly the new sed script does?

-- 
Cheers,

Sverre Rabbelier

[PATCH v2] Makefile: Simplify handling of python scripts

[Brian Gernhardt]

The sed script intended to add a standard opening to python scripts
was non-compatible and overly complex.  Simplifying it down to a set
of one-liners removes the compatibility issues of newlines.  Moving
the environment alterations from the Makefile to the python scripts
makes also makes the scripts easier to run in-place.

Specifically, the new sed script:

 - Alters the shebang line to use the configured Python.
 - Alters any os.getenv("GITPYTHONLIB") calls to use @@INSTLIBDIR@@ as the
   default.  This will replace any existing default or add a default if
   none is provided.
 - Replaces the @@INSTLIBDIR@@ placeholder with the directory git installs
   its python libraries to.

The last two steps could be combined into a single step, but is left
separate in case someone has another need for @@INSTLIBDIR@@ in their
script.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---

  On Apr 9, 2010, at 11:39 AM, Sverre Rabbelier wrote:

  > My sed foo is not that great, can you explain (in the commit message)
  > what exactly the new sed script does?
  
  Certainly.  (And the commit message is the only change from v1.)

 Makefile              |    9 ++-------
 git-remote-testgit.py |    2 ++
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index f0fe351..87c90d6 100644
--- a/Makefile
+++ b/Makefile
@@ -1629,13 +1629,8 @@ $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C git_remote_helpers -s \
 		--no-print-directory prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' \
 		instlibdir` && \
-	sed -e '1{' \
-	    -e '	s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
-	    -e '}' \
-	    -e 's|^import sys.*|&; \\\
-	           import os; \\\
-	           sys.path.insert(0, os.getenv("GITPYTHONLIB",\
-	                                        "@@INSTLIBDIR@@"));|' \
+	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
+	    -e 's|\(os\.getenv("GITPYTHONLIB"\)[^)]*)|\1,"@@INSTLIBDIR@@")|' \
 	    -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
 	    $@.py >$@+ && \
 	chmod +x $@+ && \
diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index f61624e..9253922 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -2,6 +2,8 @@
 
 import hashlib
 import sys
+import os
+sys.path.insert(0, os.getenv("GITPYTHONLIB","."))
 
 from git_remote_helpers.util import die, debug, warn
 from git_remote_helpers.git.repo import GitRepo
-- 
1.7.1.rc0.243.g2ce66

Re: [PATCH v2] Makefile: Simplify handling of python scripts

[Sverre Rabbelier]

 Heya,

On Fri, Apr 9, 2010 at 17:57, Brian Gernhardt
<brian@gernhardtsoftware.com> wrote:
>  Certainly.  (And the commit message is the only change from v1.)

Thanks, very clear!

Acked-by: Sverre Rabbelier <srabbelier@gmail.com>

-- 
Cheers,

Sverre Rabbelier

[PATCH v2 2/3] Document send-email --smtp-domain

[Brian Gernhardt]

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---
 Documentation/git-send-email.txt |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index ced35b2..f171471 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -119,6 +119,12 @@ Sending
 	value reverts to plain SMTP.  Default is the value of
 	'sendemail.smtpencryption'.
 
+--smtp-domain=<FQDN>::
+	Specifies the Fully Qualified Domain Name (FQDN) used in the
+	HELO/EHLO command to the SMTP server.  Some servers require the
+	FQDN to match your IP address.  If not set, git send-email attempts
+	to determine your FQDN automatically.
+
 --smtp-pass[=<password>]::
 	Password for SMTP-AUTH. The argument is optional: If no
 	argument is specified, then the empty string is used as
-- 
1.7.1.rc0.243.g2ce66

[PATCH v2 3/3] send-email: Add sendemail.smtpdomain

[Brian Gernhardt]

--smtp-domain is an option that if you need once, you probably will
need again.  To help with that, allow the user to set it in their
.gitconfig

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---
 Documentation/config.txt         |    1 +
 Documentation/git-send-email.txt |    3 ++-
 git-send-email.perl              |   20 ++++++++++----------
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 626b19a..b755beb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1651,6 +1651,7 @@ sendemail.smtppass::
 sendemail.suppresscc::
 sendemail.suppressfrom::
 sendemail.to::
+sendemail.smtpdomain::
 sendemail.smtpserver::
 sendemail.smtpserverport::
 sendemail.smtpuser::
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index f171471..288a4ec 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -123,7 +123,8 @@ Sending
 	Specifies the Fully Qualified Domain Name (FQDN) used in the
 	HELO/EHLO command to the SMTP server.  Some servers require the
 	FQDN to match your IP address.  If not set, git send-email attempts
-	to determine your FQDN automatically.
+	to determine your FQDN automatically.  Default is the value of
+	'sendemail.smtpdomain'.
 
 --smtp-pass[=<password>]::
 	Password for SMTP-AUTH. The argument is optional: If no
diff --git a/git-send-email.perl b/git-send-email.perl
index f491d44..bdfe3f2 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -132,8 +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_default = "localhost.localdomain";
-my $mail_domain;
+my $smtp_domain_default = "localhost.localdomain";
 
 sub unique_email_list(@);
 sub cleanup_compose_files();
@@ -190,7 +189,7 @@ sub do_edit {
 # Variables with corresponding config settings
 my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
 my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
-my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts);
+my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts, $smtp_domain);
 my ($validate, $confirm);
 my (@suppress_cc);
 
@@ -212,6 +211,7 @@ my %config_settings = (
     "smtpserverport" => \$smtp_server_port,
     "smtpuser" => \$smtp_authuser,
     "smtppass" => \$smtp_authpass,
+	"smtpdomain" => \$smtp_domain,
     "to" => \@to,
     "cc" => \@initial_cc,
     "cccmd" => \$cc_cmd,
@@ -283,7 +283,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,
+		    "smtp-domain:s" => \$smtp_domain,
 		    "identity=s" => \$identity,
 		    "annotate" => \$annotate,
 		    "compose" => \$compose,
@@ -904,7 +904,7 @@ sub maildomain_mta
 
 sub maildomain
 {
-	return maildomain_net() || maildomain_mta() || $mail_domain_default;
+	return maildomain_net() || maildomain_mta() || $smtp_domain_default;
 }
 
 # Returns 1 if the message was sent, and 0 otherwise.
@@ -1009,18 +1009,18 @@ X-Mailer: git-send-email $gitversion
 		if ($smtp_encryption eq 'ssl') {
 			$smtp_server_port ||= 465; # ssmtp
 			require Net::SMTP::SSL;
-			$mail_domain ||= maildomain();
+			$smtp_domain ||= maildomain();
 			$smtp ||= Net::SMTP::SSL->new($smtp_server,
-						      Hello => $mail_domain,
+						      Hello => $smtp_domain,
 						      Port => $smtp_server_port);
 		}
 		else {
 			require Net::SMTP;
-			$mail_domain ||= maildomain();
+			$smtp_domain ||= maildomain();
 			$smtp ||= Net::SMTP->new((defined $smtp_server_port)
 						 ? "$smtp_server:$smtp_server_port"
 						 : $smtp_server,
-						 Hello => $mail_domain,
+						 Hello => $smtp_domain,
 						 Debug => $debug_net_smtp);
 			if ($smtp_encryption eq 'tls' && $smtp) {
 				require Net::SMTP::SSL;
@@ -1043,7 +1043,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",
+			    "maildomain=$smtp_domain",
 			    defined $smtp_server_port ? "port=$smtp_server_port" : "";
 		}
 
-- 
1.7.1.rc0.243.g2ce66

Re: [PATCH v2 3/3] send-email: Add sendemail.smtpdomain

[Jakub Narebski]

On Fri, 9 Apr 2010, Brian Gernhardt wrote:

> diff --git a/git-send-email.perl b/git-send-email.perl
> index f491d44..bdfe3f2 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -132,8 +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_default = "localhost.localdomain";
> -my $mail_domain;
> +my $smtp_domain_default = "localhost.localdomain";

Why this change, this renaming of variables from $mail_domain_default
and $mail_domain to $smtp_domain_default and $smtp_domain?  Why you
have removed this forward declaration of $smtp_domain/$mail_domain?
  
>  sub unique_email_list(@);
>  sub cleanup_compose_files();
> @@ -190,7 +189,7 @@ sub do_edit {
>  # Variables with corresponding config settings
>  my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
>  my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
> -my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts);
> +my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts, $smtp_domain);
>  my ($validate, $confirm);
>  my (@suppress_cc);

Why have you moved $smtp_domain declaration (formerly $mail_domain)
here?  And why it is not described in commit message (at least "Cleanup.",
or something like this)?

-- 
Jakub Narebski
Poland

Re: [PATCH v2 3/3] send-email: Add sendemail.smtpdomain

[Brian Gernhardt]

On Apr 9, 2010, at 2:40 PM, Jakub Narebski wrote:

> On Fri, 9 Apr 2010, Brian Gernhardt wrote:
> 
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index f491d44..bdfe3f2 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -132,8 +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_default = "localhost.localdomain";
>> -my $mail_domain;
>> +my $smtp_domain_default = "localhost.localdomain";
> 
> Why this change, this renaming of variables from $mail_domain_default
> and $mail_domain to $smtp_domain_default and $smtp_domain?  Why you
> have removed this forward declaration of $smtp_domain/$mail_domain?

I changed it to better match many of the other variables in the script.  The variables that hold command line options are usually named after the option.  I left the default there as this seemed to be an area of the script that set "constant" variables (like $have_email_valid and $have_mail_address).

>> sub unique_email_list(@);
>> sub cleanup_compose_files();
>> @@ -190,7 +189,7 @@ sub do_edit {
>> # Variables with corresponding config settings
>> my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
>> my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
>> -my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts);
>> +my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts, $smtp_domain);
>> my ($validate, $confirm);
>> my (@suppress_cc);
> 
> Why have you moved $smtp_domain declaration (formerly $mail_domain)
> here?  And why it is not described in commit message (at least "Cleanup.",
> or something like this)?

$smtp_domain moved here to match comment just above "Variables with corresponding config settings".  $mail_domain was up near some unrelated globals and I wanted to move it near other similar variables.

Yes, all of this should have been in my commit message.  I blame the hour I was originally coding.  I had intended to fix my Python problem and send it in, but then I had to fix send-email because my MTA complained about "EHLO My-Computer".

I appreciate the review and help, although I wish I wasn't making a fool of myself in public.  ;-)

~~ Brian Gernhardt

[PATCH v2 1/3] send-email: Don't use FQDNs without a '.'

[Brian Gernhardt]

Although Net::Domain::domainname attempts to be very thorough, the
host's configuration can still refuse to give a FQDN.  Check to see if
what we receive contains a dot as a basic sanity check.

Since the same condition is used twice and getting complex, let's move
it to a new function.

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---
 git-send-email.perl |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index ce569a9..f491d44 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -863,14 +863,19 @@ sub sanitize_address
 # This maildomain*() code is based on ideas in Perl library Test::Reporter
 # /usr/share/perl5/Test/Reporter/Mail/Util.pm ==> sub _maildomain ()
 
+sub valid_fqdn
+{
+	my $domain = $_[0];
+	return !($^O eq 'darwin' && $domain =~ /\.local$/) && $domain =~ /\./;
+}
+
 sub maildomain_net
 {
 	my $maildomain;
 
 	if (eval { require Net::Domain; 1 }) {
 		my $domain = Net::Domain::domainname();
-		$maildomain = $domain
-			unless $^O eq 'darwin' && $domain =~ /\.local$/;
+		$maildomain = $domain if valid_fqdn( $domain );
 	}
 
 	return $maildomain;
@@ -887,8 +892,7 @@ sub maildomain_mta
 				my $domain = $smtp->domain;
 				$smtp->quit;
 
-				$maildomain = $domain
-					unless $^O eq 'darwin' && $domain =~ /\.local$/;
+				$maildomain = $domain if valid_fqdn( $domain );
 
 				last if $maildomain;
 			}
-- 
1.7.1.rc0.243.g2ce66

Re: [PATCH v2 1/3] send-email: Don't use FQDNs without a '.'

[Jakub Narebski]

On Fri, 9 Apr 2010, Brian Gernhardt wrote:
 
> Since the same condition is used twice and getting complex, let's move
> it to a new function.

Good idea.


Note that the comments below are just nitpicking about Perl style.
 
> @@ -863,14 +863,19 @@ sub sanitize_address
>  # This maildomain*() code is based on ideas in Perl library Test::Reporter
>  # /usr/share/perl5/Test/Reporter/Mail/Util.pm ==> sub _maildomain ()
>  
> +sub valid_fqdn
> +{
> +	my $domain = $_[0];
> +	return !($^O eq 'darwin' && $domain =~ /\.local$/) && $domain =~ /\./;
> +}

A matter of style: in Perl it more usual to use

  sub <name> {
  	...
  }

style rather than

  sub <name>
  {
  	...
  }

Unfortunately git-send-email.perl is a bit inconsistent in the style used;
23 subroutines use Perl style, 5 subroutines including previous one i.e.
sanitize_address use C-like style (one of).


Also, the usual way of unrolling @_; is to use either

  my ($par1, $par2, ...) = @_;

or use

  mu $par = shift;

The form $_[0] etc. is used very rarely.  I think it is even against 
Perl Best Practices (see http://www.perlcritic.org and Perl::Critic).


So in my opinion this fragment should be:

+sub valid_fqdn {
+	my $domain = shift;
+	return !($^O eq 'darwin' && $domain =~ /\.local$/) && $domain =~ /\./;
+}


> +
>  sub maildomain_net
>  {
>  	my $maildomain;
>  
>  	if (eval { require Net::Domain; 1 }) {
>  		my $domain = Net::Domain::domainname();
> -		$maildomain = $domain
> -			unless $^O eq 'darwin' && $domain =~ /\.local$/;
> +		$maildomain = $domain if valid_fqdn( $domain );
>  	}
>  
>  	return $maildomain;
> @@ -887,8 +892,7 @@ sub maildomain_mta
>  				my $domain = $smtp->domain;
>  				$smtp->quit;
>  
> -				$maildomain = $domain
> -					unless $^O eq 'darwin' && $domain =~ /\.local$/;
> +				$maildomain = $domain if valid_fqdn( $domain );
>  
>  				last if $maildomain;
>  			}

Style: usually there is no space around function arguments, so 
'valid_fqdn($domain);'.

-- 
Jakub Narebski
Poland

Re: [PATCH v2 1/3] send-email: Don't use FQDNs without a '.'

[Brian Gernhardt]

On Apr 9, 2010, at 12:31 PM, Jakub Narebski wrote:

> Note that the comments below are just nitpicking about Perl style.

Fair enough.  I've been using Ruby and Shell far more than Perl recently.  I've gotten a bit rusty.

> A matter of style: in Perl it more usual to use
> 
>  sub <name> {
>  	...
>  }
> 
> style rather than
> 
>  sub <name>
>  {
>  	...
>  }
> 
> Unfortunately git-send-email.perl is a bit inconsistent in the style used;
> 23 subroutines use Perl style, 5 subroutines including previous one i.e.
> sanitize_address use C-like style (one of).

I was copying style from the other functions I was working on.  I'll make my additions more "standard" and add a patch to clean up the rest.

> Also, the usual way of unrolling @_; is to use either
> 
>  my ($par1, $par2, ...) = @_;
> 
> or use
> 
>  mu $par = shift;
> 
> The form $_[0] etc. is used very rarely.  I think it is even against 
> Perl Best Practices (see http://www.perlcritic.org and Perl::Critic).

I knew that.  I really did.  But I started off trying to write

sub valid_fqdn( $domain )

Which would be valid Perl 6, but not Perl 5.  So then I tried using

my $domain = $1

Which, while valid, is wrong.  So I changed it to @_[1], @_[0], and finally $_[0].  My brain wasn't running at 100% yesterday, apparently.

> Style: usually there is no space around function arguments, so 
> 'valid_fqdn($domain);'.

University training is difficult to overcome.  They demanded spaces nearly everywhere, so I type them by something akin to reflex.

Thank you for all the review!
~~ Brian Gernhardt