* [PATCH v2 0/3] *** SUBJECT HERE *** @ 2010-04-09 15:34 Brian Gernhardt 2010-04-09 15:34 ` [PATCH] Makefile: Simplify handling of python scripts Brian Gernhardt ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Brian Gernhardt @ 2010-04-09 15:34 UTC (permalink / raw) To: Git List; +Cc: Jakub Narebski 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(-) ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] Makefile: Simplify handling of python scripts 2010-04-09 15:34 [PATCH v2 0/3] *** SUBJECT HERE *** Brian Gernhardt @ 2010-04-09 15:34 ` Brian Gernhardt 2010-04-09 15:39 ` Sverre Rabbelier 2010-04-09 15:34 ` [PATCH v2 2/3] Document send-email --smtp-domain Brian Gernhardt ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Brian Gernhardt @ 2010-04-09 15:34 UTC (permalink / raw) To: Git List; +Cc: Jakub Narebski 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] Makefile: Simplify handling of python scripts 2010-04-09 15:34 ` [PATCH] Makefile: Simplify handling of python scripts Brian Gernhardt @ 2010-04-09 15:39 ` Sverre Rabbelier 2010-04-09 15:57 ` [PATCH v2] " Brian Gernhardt 0 siblings, 1 reply; 13+ messages in thread From: Sverre Rabbelier @ 2010-04-09 15:39 UTC (permalink / raw) To: Brian Gernhardt; +Cc: Git List, Jakub Narebski 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] Makefile: Simplify handling of python scripts 2010-04-09 15:39 ` Sverre Rabbelier @ 2010-04-09 15:57 ` Brian Gernhardt 2010-04-09 16:05 ` Sverre Rabbelier 0 siblings, 1 reply; 13+ messages in thread From: Brian Gernhardt @ 2010-04-09 15:57 UTC (permalink / raw) To: Git List; +Cc: Sverre Rabbelier 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] Makefile: Simplify handling of python scripts 2010-04-09 15:57 ` [PATCH v2] " Brian Gernhardt @ 2010-04-09 16:05 ` Sverre Rabbelier 0 siblings, 0 replies; 13+ messages in thread From: Sverre Rabbelier @ 2010-04-09 16:05 UTC (permalink / raw) To: Brian Gernhardt; +Cc: Git List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 2/3] Document send-email --smtp-domain 2010-04-09 15:34 [PATCH v2 0/3] *** SUBJECT HERE *** Brian Gernhardt 2010-04-09 15:34 ` [PATCH] Makefile: Simplify handling of python scripts Brian Gernhardt @ 2010-04-09 15:34 ` Brian Gernhardt 2010-04-09 15:34 ` [PATCH v2 3/3] send-email: Add sendemail.smtpdomain Brian Gernhardt 2010-04-09 15:42 ` [PATCH v2 1/3] send-email: Don't use FQDNs without a '.' Brian Gernhardt 3 siblings, 0 replies; 13+ messages in thread From: Brian Gernhardt @ 2010-04-09 15:34 UTC (permalink / raw) To: Git List; +Cc: Jakub Narebski 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] send-email: Add sendemail.smtpdomain 2010-04-09 15:34 [PATCH v2 0/3] *** SUBJECT HERE *** Brian Gernhardt 2010-04-09 15:34 ` [PATCH] Makefile: Simplify handling of python scripts Brian Gernhardt 2010-04-09 15:34 ` [PATCH v2 2/3] Document send-email --smtp-domain Brian Gernhardt @ 2010-04-09 15:34 ` Brian Gernhardt 2010-04-09 18:40 ` Jakub Narebski 2010-04-09 15:42 ` [PATCH v2 1/3] send-email: Don't use FQDNs without a '.' Brian Gernhardt 3 siblings, 1 reply; 13+ messages in thread From: Brian Gernhardt @ 2010-04-09 15:34 UTC (permalink / raw) To: Git List; +Cc: Jakub Narebski --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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] send-email: Add sendemail.smtpdomain 2010-04-09 15:34 ` [PATCH v2 3/3] send-email: Add sendemail.smtpdomain Brian Gernhardt @ 2010-04-09 18:40 ` Jakub Narebski 2010-04-10 13:51 ` Brian Gernhardt 0 siblings, 1 reply; 13+ messages in thread From: Jakub Narebski @ 2010-04-09 18:40 UTC (permalink / raw) To: Brian Gernhardt; +Cc: Git List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] send-email: Add sendemail.smtpdomain 2010-04-09 18:40 ` Jakub Narebski @ 2010-04-10 13:51 ` Brian Gernhardt 0 siblings, 0 replies; 13+ messages in thread From: Brian Gernhardt @ 2010-04-10 13:51 UTC (permalink / raw) To: Jakub Narebski; +Cc: Git List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/3] send-email: Don't use FQDNs without a '.' 2010-04-09 15:34 [PATCH v2 0/3] *** SUBJECT HERE *** Brian Gernhardt ` (2 preceding siblings ...) 2010-04-09 15:34 ` [PATCH v2 3/3] send-email: Add sendemail.smtpdomain Brian Gernhardt @ 2010-04-09 15:42 ` Brian Gernhardt 2010-04-09 16:31 ` Jakub Narebski 3 siblings, 1 reply; 13+ messages in thread From: Brian Gernhardt @ 2010-04-09 15:42 UTC (permalink / raw) To: Git List; +Cc: Jakub Narebski 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] send-email: Don't use FQDNs without a '.' 2010-04-09 15:42 ` [PATCH v2 1/3] send-email: Don't use FQDNs without a '.' Brian Gernhardt @ 2010-04-09 16:31 ` Jakub Narebski 2010-04-10 13:44 ` Brian Gernhardt 0 siblings, 1 reply; 13+ messages in thread From: Jakub Narebski @ 2010-04-09 16:31 UTC (permalink / raw) To: Brian Gernhardt; +Cc: Git List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] send-email: Don't use FQDNs without a '.' 2010-04-09 16:31 ` Jakub Narebski @ 2010-04-10 13:44 ` Brian Gernhardt 0 siblings, 0 replies; 13+ messages in thread From: Brian Gernhardt @ 2010-04-10 13:44 UTC (permalink / raw) To: Jakub Narebski; +Cc: Git List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 0/3] *** SUBJECT HERE *** @ 2010-08-03 10:36 Jon Seymour 0 siblings, 0 replies; 13+ messages in thread From: Jon Seymour @ 2010-08-03 10:36 UTC (permalink / raw) To: git; +Cc: gitster, ams, jon.seymour This series fixes git stash branch so that it is more tolerant of stash-like commits created with git stash create. It particular, it doesn't require there to be a stash stack if a stash-like argument is specified and it doesn't attempt to drop non-stash references after applying the stash. This series replaces my previous patch that just included a test that demonstrated the existance of the issue. stash: It looks like a stash, but doesn't quack like a stash... stash: Allow git stash branch to process commits that look like stashes but are not stash references. stash: modify tests to reflect stash branch fixes. git-stash.sh | 10 ++++++++-- t/t3903-stash.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) -- 1.7.2.1.111.g8fc90 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2010-08-03 10:37 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-04-09 15:34 [PATCH v2 0/3] *** SUBJECT HERE *** Brian Gernhardt 2010-04-09 15:34 ` [PATCH] Makefile: Simplify handling of python scripts Brian Gernhardt 2010-04-09 15:39 ` Sverre Rabbelier 2010-04-09 15:57 ` [PATCH v2] " Brian Gernhardt 2010-04-09 16:05 ` Sverre Rabbelier 2010-04-09 15:34 ` [PATCH v2 2/3] Document send-email --smtp-domain Brian Gernhardt 2010-04-09 15:34 ` [PATCH v2 3/3] send-email: Add sendemail.smtpdomain Brian Gernhardt 2010-04-09 18:40 ` Jakub Narebski 2010-04-10 13:51 ` Brian Gernhardt 2010-04-09 15:42 ` [PATCH v2 1/3] send-email: Don't use FQDNs without a '.' Brian Gernhardt 2010-04-09 16:31 ` Jakub Narebski 2010-04-10 13:44 ` Brian Gernhardt -- strict thread matches above, loose matches on Subject: below -- 2010-08-03 10:36 [PATCH v2 0/3] *** SUBJECT HERE *** Jon Seymour
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).