* [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
* [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] 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 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
* [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
* 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 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 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
* 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 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).