From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
This patch series introduces some very basic refactorizations and
improvements to send-email. My goal was to keep each patch relatively
small, but it may be better to look at the results of the following
patches:
[PATCH RFC3.5 06/12] send-email: Cleanup and streamline the SMTP code in send_message
[PATCH RFC3.5 07/12] send-email: Cleanup send_message 'log' code
[PATCH RFC3.5 09/12] Docs: send-email: Reorganize the CONFIGURATION section
The overall diffstat:
Documentation/git-send-email.txt | 201 ++++++++++++++++++++----
git-send-email.perl | 317 +++++++++++++++++++++++++-------------
2 files changed, 376 insertions(+), 142 deletions(-)
The over all patch series:
[PATCH RFC3.5 01/12] send-email: Cleanup the usage text and docs a bit
[PATCH RFC3.5 02/12] send-email: No longer repeatedly test if $smtp_server is a command
[PATCH RFC3.5 03/12] send-email: Interpret --smtp-server "" as "use a default".
[PATCH RFC3.5 04/12] send-email: Verification for --smtp-server and --smpt-server-port
[PATCH RFC3.5 05/12] send-email: Improve redability and error-handling in send_message's sendmail code
[PATCH RFC3.5 06/12] send-email: Cleanup and streamline the SMTP code in send_message
[PATCH RFC3.5 07/12] send-email: Cleanup send_message 'log' code
[PATCH RFC3.5 08/12] send-email: Move Subject sanitization from --compose code to send_message
[PATCH RFC3.5 09/12] Docs: send-email: Reorganize the CONFIGURATION section
[PATCH RFC3.5 10/12] Docs: Embolden the CONFIGURATION references
[PATCH RFC3.5 11/12] Docs: send-email: Clarification of sendemail.<identity>
[PATCH RFC3.5 12/12] Docs: send-email: git send-email -> 'send-email'
Sincerely,
Michael Witten
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
This is a minor change, but it's cleaner, and it sets up the
$smtp_server initialization code for future improvements.
Signed-off-by: Michael Witten <redacted>
---
git-send-email.perl | 19 ++++++++++++++-----
1 files changed, 14 insertions(+), 5 deletions(-)
@@ -186,7 +186,8 @@ sub do_edit {# Variables with corresponding config settingsmy($thread,$chain_reply_to,$suppress_from,$signed_off_by_cc,$cc_cmd);-my($smtp_server,$smtp_server_port,$smtp_authuser,$smtp_encryption);+my($smtp_server,$smtp_server_is_a_command);+my($smtp_server_port,$smtp_authuser,$smtp_encryption);my($identity,$aliasfiletype,@alias_files,@smtp_host_parts);my($validate,$confirm);my(@suppress_cc);
@@ -674,14 +675,22 @@ if (defined $initial_reply_to) {$initial_reply_to="<$initial_reply_to>"if$initial_reply_tone'';}-if(!defined$smtp_server){+if(defined$smtp_server){++$smtp_server_is_a_command=($smtp_server=~m{^/});++}else{# use a default:+foreach(qw( /usr/sbin/sendmail /usr/lib/sendmail )){if(-x$_){$smtp_server=$_;+$smtp_server_is_a_command=1;last;}}-$smtp_server||='localhost';# could be 127.0.0.1, too... *shrug*++$smtp_server='localhost';# 127.0.0.1 is not compatible with IPv6+unless$smtp_server_is_a_command;}if($compose&&$compose>0){
@@ -882,7 +891,7 @@ X-Mailer: git-send-email $gitversionif($dry_run){# We don't want to send the email.-}elsif($smtp_server=~m#^/#) {+}elsif($smtp_server_is_a_command){my$pid=openmy$sm,'|-';defined$pidordie$!;if(!$pid){
@@ -132,7 +132,9 @@ user is prompted for a password while the input is masked for privacy. be specified by the 'sendemail.smtpserver' configuration variable; the built-in default is `/usr/sbin/sendmail` or `/usr/lib/sendmail` if such a program is available, or- `localhost` otherwise.+ `localhost` otherwise. Also, a built-in default is used if+ `<host>` or 'sendemail.smtpserver' is the empty string (for+ example, if '--smtp-server ""' is specified on the command line). --smtp-server-port=<port>:: Specifies a port different from the default port (SMTP
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
--chain-reply-to doesn't take an argument.
The here-document quotation that defines the usage
text is now a single-quote form, so that no interpolation
takes place.
All usage text lines should be < 80 characters.
The usage text's option arguments match those of the docs.
The 'host:port' form of argument for --smtp-server was only
working for SSL connections, because the SSL connection code
was relying on undocumented behavior of Net::SMTP::SSL (really,
undocumented behavior of Net::SMTP's new method). Because the
main documentation for send-email doesn't even list it as valid,
<str:int> has been replaced with just <server> and <host | command>
in the usage text; this is only temporary and for purity.
Signed-off-by: Michael Witten <redacted>
---
Documentation/git-send-email.txt | 21 ++++++++-------
git-send-email.perl | 49 +++++++++++++++++++++-----------------
2 files changed, 38 insertions(+), 32 deletions(-)
@@ -74,8 +74,9 @@ See the CONFIGURATION section for 'sendemail.multiedit'. the value GIT_COMMITTER_IDENT, as returned by "git var -l". The user will still be prompted to confirm this entry.---in-reply-to=<identifier>::- Specify the contents of the first In-Reply-To header.+--in-reply-to=<message-id>::+ Specify the contents of the first In-Reply-To header;+ include the angle brackets `<` and `>`. Subsequent emails will refer to the previous email instead of this if --chain-reply-to is set (the default) Only necessary if --compose is also set. If --compose
@@ -106,7 +107,7 @@ Sending the 'sendemail.envelopesender' configuration variable; if that is unspecified, choosing the envelope sender is left to your MTA.---smtp-encryption=<encryption>::+--smtp-encryption=<type>:: Specify the encryption to use, either 'ssl' or 'tls'. Any other value reverts to plain SMTP. Default is the value of 'sendemail.smtpencryption'.
@@ -123,20 +124,20 @@ or on the command line. If a username has been specified (with specified (with '--smtp-pass' or 'sendemail.smtppass'), then the user is prompted for a password while the input is masked for privacy.---smtp-server=<host>::+--smtp-server=<server>:: If set, specifies the outgoing SMTP server to use (e.g. `smtp.example.com` or a raw IP address). Alternatively it can specify a full pathname of a sendmail-like program instead; the program must support the `-i` option. Default value can be specified by the 'sendemail.smtpserver' configuration- option; the built-in default is `/usr/sbin/sendmail` or- `/usr/lib/sendmail` if such program is available, or+ variable; the built-in default is `/usr/sbin/sendmail` or+ `/usr/lib/sendmail` if such a program is available, or `localhost` otherwise. --smtp-server-port=<port>:: Specifies a port different from the default port (SMTP servers typically listen to smtp port 25 and ssmtp port- 465); symbolic port names (e.g. "submission" instead of 465)+ 465); symbolic service names (e.g. "submission" instead of 587) are also accepted. The port can also be set with the 'sendemail.smtpserverport' configuration variable.
@@ -158,7 +159,7 @@ Automating Output of this command must be single email address per line. Default is the value of 'sendemail.cccmd' configuration value.---[no-]chain-reply-to=<identifier>::+--[no-]chain-reply-to:: If this is set, each email will be sent as a reply to the previous email sent. If disabled with "--no-chain-reply-to", all emails after the first will be sent as replies to the first email sent. When using
@@ -170,7 +171,7 @@ Automating A configuration identity. When given, causes values in the 'sendemail.<identity>' subsection to take precedence over values in the 'sendemail' section. The default identity is- the value of 'sendemail.identity'.+ the value of the 'sendemail.identity' configuration variable. --[no-]signed-off-by-cc:: If this is set, add emails found in Signed-off-by: or Cc: lines to the
@@ -214,7 +215,7 @@ specified, as well as 'body' if --no-signed-off-cc is specified. Administering ~~~~~~~~~~~~~---confirm=<mode>::+--confirm=<when>:: Confirm just before sending: + --
@@ -42,46 +42,51 @@ package main;subusage{-print<<EOT;++# All printed lines should be less than 80 characters.++print<<'EOT';gitsend-email[options]<file|directory|rev-listoptions>Composing:---from<str>*EmailFrom:---to<str>*EmailTo:---cc<str>*EmailCc:---bcc<str>*EmailBcc:---subject<str>*Email"Subject:"---in-reply-to<str>*Email"In-Reply-To:"---annotate*Revieweachpatchthatwillbesentinaneditor.+--from<address>*EmailFrom:+--to<address>*EmailTo:+--cc<address>*EmailCc:+--bcc<address>*EmailBcc:+--subject<string>*Email"Subject:"+--in-reply-to<message-id>*Email"In-Reply-To:";include'<'and'>'.+--annotate*Revieweachpatchthatwillbesentin+aneditor.--compose*Openaneditorforintroduction.Sending:---envelope-sender<str>*Emailenvelopesender.---smtp-server<str:int>*OutgoingSMTPservertouse.Theport-isoptional.Default'localhost'.---smtp-server-port<int>*OutgoingSMTPserverport.---smtp-user<str>*UsernameforSMTP-AUTH.---smtp-pass<str>*PasswordforSMTP-AUTH;notnecessary.---smtp-encryption<str>*tlsorssl;anythingelsedisables.+--envelope-sender<address>*Emailenvelopesender.+--smtp-server<server>*OutgoingSMTPserver.<host|command>+--smtp-server-port<port>*OutgoingSMTPserverport;symbolictoo.+--smtp-user<username>*UsernameforSMTP-AUTH.+--smtp-pass[<password>]*PasswordforSMTP-AUTH;notnecessary.+--smtp-encryption<type>*tlsorssl;anythingelsedisables.--smtp-ssl*Deprecated.Use'--smtp-encryption ssl'.Automating:---identity<str>*Usethesendemail.<id>options.---cc-cmd<str>*EmailCc:via`<str> \$patch_path`---suppress-cc<str>*author,self,sob,cc,cccmd,body,bodycc,all.---[no-]signed-off-by-cc*SendtoSigned-off-by:addresses.Defaulton.+--identity<identity>*Usethesendemail.<identity>options.+--cc-cmd<command>*EmailCc:via`<command> $patch_path`+--suppress-cc<category>*author,self,sob,cc,cccmd,body,+bodycc,all.+--[no-]signed-off-by-cc*SendtoSigned-off-by:addresses.+Defaulton.--[no-]suppress-from*Sendtoself.Defaultoff.--[no-]chain-reply-to*ChainIn-Reply-To:fields.Defaulton.--[no-]thread*UseIn-Reply-To:field.Defaulton.Administering:---confirm<str>*Confirmrecipientsbeforesending;+--confirm<when>*Confirmrecipientsbeforesending;auto,cc,compose,always,ornever.--quiet*Outputonelineofinfoperemail.--dry-run*Don'tactuallysendtheemails.--[no-]validate*Performpatchsanitychecks.Defaulton.---[no-]format-patch*understandanynonoptionalargumentsas-`git format-patch`ones.+--[no-]format-patch*Understandanynon-optionalargumentsas+`gitformat-patch'arguments.EOTexit(1);
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
The server URI is verified according to RFCs (including IPv6 support).
The $smtp_server and $smtp_server_port setup code has been moved
higher in the file, so that send-email fails fast if they are bad.
Now, the 'host:port' server URI form is handled regardless of the
documentation deficiencies of Net::SMTP{,::SSL}.
Signed-off-by: Michael Witten <redacted>
---
Documentation/git-send-email.txt | 50 ++++++++++++-----
git-send-email.perl | 115 +++++++++++++++++++++++++++++++------
2 files changed, 131 insertions(+), 34 deletions(-)
@@ -125,23 +125,43 @@ specified (with '--smtp-pass' or 'sendemail.smtppass'), then the user is prompted for a password while the input is masked for privacy. --smtp-server=<server>::- If set, specifies the outgoing SMTP server to use (e.g.- `smtp.example.com` or a raw IP address). Alternatively it can- specify a full pathname of a sendmail-like program instead;- the program must support the `-i` option. Default value can- be specified by the 'sendemail.smtpserver' configuration- variable; the built-in default is `/usr/sbin/sendmail` or- `/usr/lib/sendmail` if such a program is available, or- `localhost` otherwise. Also, a built-in default is used if- `<host>` or 'sendemail.smtpserver' is the empty string (for- example, if '--smtp-server ""' is specified on the command line).+ Specifies the outgoing SMTP server to use. The server may be+ given as a domain name (e.g. `smtp.example.com:587`), raw IP+ address (e.g. `192.168.0.1`), or absolute path to a command+ (e.g. `/usr/sbin/sendmail`).+++Usually a server is specified with its URI form:+++ host[:port]+++The optional port, which identifies a particular service at the+given host, is normally provided as a non-negative integer that+is representable in 16-bits; however, it is possible to use any+string composed of some combination of alphanumeric characters, the+underscore, the hyphen, and punctuation (special) characters, so+that symbolic service names can be employed (as defined by, say,+`/etc/services` on Unix systems).+++Alternatively the server can be specified as an absolute path to+a sendmail-like program; in particular, the program must support+`sendmail's` `-i` option.+++Default value can be specified by the 'sendemail.smtpserver'+configuration variable; the built-in default is `/usr/sbin/sendmail`+or `/usr/lib/sendmail` if such a program is available, or `localhost`+otherwise. Also, a built-in default is used if `<server>` or+'sendemail.smtpserver' is the empty string (for example, if+'--smtp-server ""' is specified on the command line). --smtp-server-port=<port>::- Specifies a port different from the default port (SMTP- servers typically listen to smtp port 25 and ssmtp port- 465); symbolic service names (e.g. "submission" instead of 587)- are also accepted. The port can also be set with the- 'sendemail.smtpserverport' configuration variable.+ Specifies a port different from the default port (SMTP servers+ typically listen to port 25, 587, or even non-standard 465); symbolic+ service names (e.g. "submission" instead of 587) are also accepted,+ provided the underlying system handles mappings in something like+ `/etc/services`; such service names may only be composed of some+ combination of alphanumeric characters, the underscore, the hyphen,+ and punctuation (special) characters. The port can also be set with+ the 'sendemail.smtpserverport' configuration variable. --smtp-ssl:: Legacy alias for '--smtp-encryption ssl'.
@@ -328,6 +328,101 @@ foreach my $setting (values %config_bool_settings) {# 'default' encryption is none -- this only prevents a warning$smtp_encryption=''unless(defined$smtp_encryption);+# Define a function that verifies a server URI and returns+# its host and port parts:++subparse_server_URI($){++# These regular experssions were derived from:+# * RFC 2373 (Appendix B) : IP Version 6 Addressing Architecture+# * RFC 2732 (Section 3) : Format for Literal IPv6 Addresses in URL's+# * RFC 2396 (Sec. 3.2.2) : Uniform Resource Identifiers (URI): Generic Syntax+# Also, ports are allowed to be symbolic, so that /etc/services mappings+# can be used.++# Define a port (RFC 2396 and extension):++my$port=qr/[\w[:punct:]]+/;++# Define an IPv4 address (RFC 2373):++my$dig3=qr/\d{1,3}/;+my$IPv4address=qr/$dig3(?:\.$dig3){3}/;++# Define an IPv6 address (RFC 2373):++my$hex4=qr/[[:xdigit:]]{1,4}/;+my$hexseq=qr/$hex4(?::$hex4)*/;# RFC 2373 is really that loose.+my$hexpart=qr/$hexseq|$hexseq?::$hexseq?/;+my$IPv6address=qr/$hexpart(?::$IPv4address)?/;++# Define an IPv6 literal (RFC 2732):++my$IPv6reference=qr/\[$IPv6address\]/;++# Define a server URI (RFC 2396 and RFC 2732):++my$toplabel=qr/[[:alpha:]](?:(?:[[:alnum:]]|-)*[[:alpha:]])?/;+my$domainlabel=qr/[[:alnum:]](?:(?:[[:alnum:]]|-)*[[:alnum:]])?/;+my$hostname=qr/(?:$domainlabel\.)*$toplabel\.?/;+my$host=qr/$hostname|$IPv4address|$IPv6reference/;+my$hostport=qr/^($host)(?::($port))?$/;++# Phew! Now parse++returnshift=~$hostport;+}++# Figure out how to contact the SMTP server.+# After this code, $smtp_server_port is either+# valid or undef:++if(defined$smtp_server&&$smtp_serverne''){++if($smtp_server_is_a_command=($smtp_server=~m{^/})){++die"--smtp-server: The SMTP server command does not exist: $smtp_server\n"+unless-x$smtp_server;++printSTDERR"--smtp-server: Using command '$smtp_server'; ignoring --smtp-server-port='$smtp_server_port'\n"+ifdefined$smtp_server_port;++}else{++($smtp_server,my$port)=parse_server_URI$smtp_server+ordie"--smtp-server: Not a valid server URI: '$smtp_server'\n";++if(defined$smtp_server_port){++$smtp_server_port=~ /[\w[:punct:]]+/ordie"--smtp-server-port: Invalid port: '$smtp_server_port'\n";++printSTDERR"--smtp-server-port: Using port '$smtp_server_port'; ignoring --smtp-server's port '$port'\n"+ifdefined$port;++}else{++$smtp_server_port=$port;+}+}++}else{# use a default:++foreach(qw( /usr/sbin/sendmail /usr/lib/sendmail )){+if(-x$_){+$smtp_server=$_;+$smtp_server_is_a_command=1;++printSTDERR"--smtp-server: Using command '$smtp_server'; ignoring --smtp-server-port='$smtp_server_port'\n"+ifdefined$smtp_server_port;++last;+}+}++$smtp_server='localhost';# 127.0.0.1 is not compatible with IPv6+unless$smtp_server_is_a_command;+}+# Set CC suppressionsmy(%suppress_cc);if(@suppress_cc){
@@ -675,24 +770,6 @@ if (defined $initial_reply_to) {$initial_reply_to="<$initial_reply_to>"if$initial_reply_tone'';}-if(defined$smtp_server&&$smtp_serverne''){--$smtp_server_is_a_command=($smtp_server=~m{^/});--}else{# use a default:--foreach(qw( /usr/sbin/sendmail /usr/lib/sendmail )){-if(-x$_){-$smtp_server=$_;-$smtp_server_is_a_command=1;-last;-}-}--$smtp_server='localhost';# 127.0.0.1 is not compatible with IPv6-unless$smtp_server_is_a_command;-}-if($compose&&$compose>0){@files=($compose_filename.".final",@files);}
@@ -967,15 +967,22 @@ X-Mailer: git-send-email $gitversion}if($dry_run){+# We don't want to send the email.+}elsif($smtp_server_is_a_command){-my$pid=openmy$sm,'|-';-defined$pidordie$!;-if(!$pid){-exec($smtp_server,@sendmail_parameters)ordie$!;-}-print$sm"$header\n$message";-close$smordie$?;++(my$pid=openmy$pipe,'|-')+//die"Could not fork to run '$smtp_server': $!\n";++$pidorexec($smtp_server,@sendmail_parameters)+ordie"Could not run '$smtp_server': $!\n";++local$SIG{PIPE}='IGNORE';++print$pipe"$header\n$message";+close$pipeordie"'$smtp_server' exited with status $?: $!\n";+}else{if(!defined$smtp_server){
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
Some of the code was never used or not necessary; it should
be easier to read now.
The code could even be simplified further, because Net::SMTP{,::SSL}
both take the PORT variable in their new methods (which, as of this
commit, are actually the same method). Moreover, both take a server
URI of the form 'host:port' that trumps any value passed to PORT.
Unfortunately, none of this is documented publicly, so it isn't
exploited out of purity.
Signed-off-by: Michael Witten <redacted>
---
git-send-email.perl | 93 +++++++++++++++++++++++++++-----------------------
1 files changed, 50 insertions(+), 43 deletions(-)
@@ -985,67 +985,74 @@ X-Mailer: git-send-email $gitversion}else{-if(!defined$smtp_server){-die"The required SMTP server is not properly defined."-}+gotoSEND_MAILif$smtp;++if($smtp_encryption=~ /ssl/i){++useNet::SMTP::SSL;+$smtp=Net::SMTP::SSL->new($smtp_server,Port=>$smtp_server_port//465)+ordie"Could not connect to SSL SMTP server '$smtp_server:$smtp_server_port'\n";++}else{++useNet::SMTP;++my$server_URI=(defined$smtp_server_port)+?"$smtp_server:$smtp_server_port"+:$smtp_server;++$smtp=Net::SMTP->new($server_URI)+ordie"Could not connect to SMTP server: '$server_URI'\n";++if($smtp_encryption=~ /tls/i){-if($smtp_encryptioneq'ssl'){-$smtp_server_port||=465;# ssmtp-requireNet::SMTP::SSL;-$smtp||=Net::SMTP::SSL->new($smtp_server,Port=>$smtp_server_port);-}-else{-requireNet::SMTP;-$smtp||=Net::SMTP->new((defined$smtp_server_port)-?"$smtp_server:$smtp_server_port"-:$smtp_server);-if($smtp_encryptioneq'tls'){-requireNet::SMTP::SSL;$smtp->command('STARTTLS');-$smtp->response();-if($smtp->code==220){-$smtp=Net::SMTP::SSL->start_SSL($smtp)-ordie"STARTTLS failed! ".$smtp->message;-$smtp_encryption='';-# Send EHLO again to receive fresh-# supported commands-$smtp->hello();-}else{-die"Server does not support STARTTLS! ".$smtp->message;-}-}-}+$smtp->response();# so $smtp->code works.++die"Server does not support STARTTLS: ".$smtp->message."\n"+unless$smtp->code==220;++useNet::SMTP::SSL;+Net::SMTP::SSL->start_SSL($smtp)+ordie"STARTTLS failed! ".$smtp->message."\n";++# Send EHLO again to receive fresh+# supported commands:-if(!$smtp){-die"Unable to initialize SMTP properly. Is there something wrong with your config?";+$smtp->hello();+}}if(defined$smtp_authuser){-if(!defined$smtp_authpass){+unless(defined$smtp_authpass){system"stty -echo";-do{+{print"Password: ";-$_=<STDIN>;+$smtp_authpass=<STDIN>;print"\n";-}while(!defined$_);--chomp($smtp_authpass=$_);+redounlessdefined$smtp_authpass;+chomp($smtp_authpass);+}system"stty echo";}-$auth||=$smtp->auth($smtp_authuser,$smtp_authpass)ordie$smtp->message;+$smtp->auth($smtp_authuser,$smtp_authpass)+ordie"Could not authenticate '$smtp_authuser': ".$smtp->message."\n";}-$smtp->mail($raw_from)ordie$smtp->message;-$smtp->to(@recipients)ordie$smtp->message;-$smtp->dataordie$smtp->message;-$smtp->datasend("$header\n$message")ordie$smtp->message;-$smtp->dataend()ordie$smtp->message;-$smtp->code=~ /250|200/ordie"Failed to send $subject\n".$smtp->message;+SEND_MAIL:++$smtp->mail($raw_from)and+$smtp->to(@recipients)and+$smtp->dataand+$smtp->datasend("$header\n$message")and+$smtp->dataendor++die"Failed to send '$subject': ".$smtp->message."\n";}if($quiet){printf(($dry_run?"Dry-":"")."Sent %s\n",$subject);
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
For each configuration variable, the reader is either prompted
to seek out the description of the corresponding command line option,
or a description is given if there is no corresponding command line
option.
The CONFIGURATION section has also been recast into the Composing,
Sending, Automating, and Administering sections, and configuration
variables are listed in alphabetical order within each section.
Signed-off-by: Michael Witten <redacted>
---
Documentation/git-send-email.txt | 83 ++++++++++++++++++++++++++++++++++----
1 files changed, 75 insertions(+), 8 deletions(-)
@@ -282,6 +282,28 @@ default to '--validate'. CONFIGURATION -------------+Composing+~~~~~~~~~++sendemail.bcc::+ See '--bcc'++sendemail.cc::+ See '--cc'++sendemail.multiedit::+ If true (default), a single editor instance will be spawned to edit+ files you have to edit (patches when '--annotate' is used, and the+ summary when '--compose' is used). If false, files will be edited one+ after the other, spawning a new editor each time.++sendemail.to::+ See '--from'+++Sending+~~~~~~~+ sendemail.aliasesfile:: To avoid typing long email addresses, point this to one or more email aliases files. You must also supply 'sendemail.aliasfiletype'.
@@ -290,16 +312,61 @@ sendemail.aliasfiletype:: Format of the file(s) specified in sendemail.aliasesfile. Must be one of 'mutt', 'mailrc', 'pine', or 'gnus'.-sendemail.multiedit::- If true (default), a single editor instance will be spawned to edit- files you have to edit (patches when '--annotate' is used, and the- summary when '--compose' is used). If false, files will be edited one- after the other, spawning a new editor each time.+sendemail.envelopesender::+ See '--envelope-sender'++sendemail.smtpencryption::+ See '--smtp-encryption'++sendemail.smtppass::+ See '--smtp-pass'++sendemail.smtpserver::+ See '--smtp-server'++sendemail.smtpserverport::+ See '--smtp-server-port'++sendemail.smtpuser::+ See '--smtp-ssl'+++Automating+~~~~~~~~~~++sendemail.cccmd::+ See '--cc-cmd'++sendemail.chainreplyto::+ See '--[no-]chain-reply-to'++sendemail.identity::+ See '--identity'++sendemail.signedoffbycc::+ See '--[no-]signed-off-by-cc'++sendemail.sleep::+ See '--sleep'++sendemail.suppresscc::+ See '--suppress-cc'++sendemail.suppressfrom::+ See '--[no-]suppress-from'++sendemail.thread::+ See '--[no-]thread'+++Administering+~~~~~~~~~~~~~ sendemail.confirm::- Sets the default for whether to confirm before sending. Must be- one of 'always', 'never', 'cc', 'compose', or 'auto'. See '--confirm'- in the previous section for the meaning of these values.+ See '--confirm'++sendemail.validate::+ See '--dry-run' Author
@@ -41,7 +41,7 @@ Composing --annotate:: Review and edit each patch you're about to send. See the- CONFIGURATION section for 'sendemail.multiedit'.+ *CONFIGURATION* section for 'sendemail.multiedit'. --bcc=<address>:: Specify a "Bcc:" value for each email. Default is the value of
@@ -67,7 +67,7 @@ and In-Reply-To headers will be used unless they are removed. + Missing From or In-Reply-To headers will be prompted for. +-See the CONFIGURATION section for 'sendemail.multiedit'.+See the *CONFIGURATION* section for 'sendemail.multiedit'. --from=<address>:: Specify the sender of the emails. This will default to
@@ -194,6 +194,7 @@ Automating 'sendemail.<identity>' subsection to take precedence over values in the 'sendemail' section. The default identity is the value of the 'sendemail.identity' configuration variable.+ See the *CONFIGURATION* section for more details. --[no-]signed-off-by-cc:: If this is set, add emails found in Signed-off-by: or Cc: lines to the
@@ -282,6 +283,48 @@ default to '--validate'. CONFIGURATION -------------+Configuration subsections are very useful in combination with '--identity'.+For instance, consider a configuration file that contains the following:++ ...++ [sendemail]+ smtpencryption = tls+ smtpserver = smtp.example.com+ smtpuser = user@example.com+ suppresscc = self+ confirm = always+ identity = test++ [sendemail "test"]+ to = user@example.com++ [sendemail "git-rfc"]+ to = git@vger.kernel.org++ [sendemail "git-submit"]+ to = gitster@pobox.com+ cc = git@vger.kernel.org+ confirm = never++ ...++With this configuration, 'send-email' processes the variables in `[sendemail]`.+Then, 'sendemail.identity' tells 'send-email' to process the variables in the+`[sendemail "test"]` subsection. Thus:++ git send-email <patch>++would send `<patch>` to `user@example.com` as a "test". When the user+decides that `<patch>` is ready to be sent for real, the user could+email the 'git' mailing list to make a request for comments (RFC):++ git send-email <patch> --identity git-rfc++Then, for final submission:++ git send-email <patch> --identity git-submit+ Composing ~~~~~~~~~
@@ -59,7 +59,7 @@ The --cc option must be repeated for each user you want on the cc list. Use $GIT_EDITOR, core.editor, $VISUAL, or $EDITOR to edit an introductory message for the patch series. +-When '--compose' is used, git send-email will use the From, Subject, and+When '--compose' is used, 'send-email' will use the From, Subject, and In-Reply-To headers specified in the message. If the body of the message (what you type after the headers and a blank line) only contains blank (or GIT: prefixed) lines the summary won't be sent, but From, Subject,
@@ -244,7 +244,7 @@ Administering -- - 'always' will always confirm before sending - 'never' will never confirm before sending-- 'cc' will confirm before sending when send-email has automatically+- 'cc' will confirm before sending when 'send-email' has automatically added addresses from the patch to the Cc list - 'compose' will confirm before sending the first message when using --compose. - 'auto' is equivalent to 'cc' + 'compose'
@@ -261,10 +261,10 @@ have been specified, in which case default to 'compose'. When an argument may be understood either as a reference or as a file name, choose to understand it as a format-patch argument ('--format-patch') or as a file name ('--no-format-patch'). By default, when such a conflict- occurs, git send-email will fail.+ occurs, 'send-email' will fail. --quiet::- Make git-send-email less verbose. One line per email should be+ Make 'send-email' less verbose. One line per email should be all that is output. --[no-]validate::
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
On Sat, Apr 18, 2009 at 18:35, Wesley J. Landaker [off-list ref] wrote:
On Saturday 18 April 2009 11:01:59 Michael Witten wrote:
quoted
`/usr/lib/sendmail` if such a program is available, or
- `localhost` otherwise.
+ `localhost` otherwise. Also, a built-in default is used if
I think you may have meant:
+ `localhost` otherwise. Also, the built-in default is used if
Good eye! However, I deliberately chose 'a built-in', because there
are multiple built-in values. However, 'the built-in' is used in the
previous sentence, now that I take a closer look myself.
}
if ($dry_run) {
+
# We don't want to send the email.
+
} elsif ($smtp_server_is_a_command) {
- my $pid = open my $sm, '|-';
- defined $pid or die $!;
- if (!$pid) {
- exec($smtp_server, @sendmail_parameters) or die $!;
- }
- print $sm "$header\n$message";
- close $sm or die $?;
+
+ (my $pid = open my $pipe, '|-')
+ // die "Could not fork to run '$smtp_server': $!\n";
Although it has no direct equivalent in C, Perl's // operator is related to its C-style or. In fact, it's exactly the same as ||, except that it tests the left hand side's definedness instead of its truth. Thus, $a // $b is similar to defined($a) || $b (except that it returns the value of $a rather than the value of defined($a)) and is exactly equivalent to defined($a) ? $a : $b . This is very useful for providing default values for variables. If you actually want to test if at least one of $a and $b is defined, use defined($a // $b) ...
However, I wonder if your comment is a veiled quip at my "Improve
redability" claim (which is also ironically unreadable). :-)
From: Thomas Adam <hidden> Date: 2016-06-15 22:46:37
2009/4/19 Michael Witten [off-list ref]:
However, I wonder if your comment is a veiled quip at my "Improve
redability" claim (which is also ironically unreadable). :-)
More concerning is that it's a perl 5.10ism -- you cannot assume that
perl 5.10 is installed on all platforms. I really wouldn't use this
construct.
-- Thomas Adam
I wonder if it would be clearer to always call quote_rfc2047, then
have that function just return its input unaltered if quoting is not
needed.
It actually ALWAYS changes the input. This code:
sub quote_rfc2047 {
local $_ = shift;
my $encoding = shift || 'utf-8';
s/([^-a-zA-Z0-9!*+\/])/sprintf("=%02X", ord($1))/eg;
s/(.*)/=\?$encoding\?q\?$1\?=/;
return $_;
}
print quote_rfc2047("Yiarg #&@$! This output is messy!") . "\n"
gives this output:
=?utf-8?q?Yiarg=20=23=26!=20This=20output=20is=20messy!?=
Therfore the /[^[:ascii:]]/ check actually saves us from corrupting
already encoded subjects or from encoding ones that shouldn't be. In
fact, I'm not entirely sure the original code is correct to make that
check, because some of the characters that are replaced are ascii
characters. This is all rather strange.
Thanks! I should have tested it more; I'm constantly amazed by my
inability to see the problems I introduce ;-)
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
On Sat, Apr 18, 2009 at 21:17, Thomas Adam [off-list ref] wrote:
More concerning is that it's a perl 5.10ism -- you cannot assume that
perl 5.10 is installed on all platforms. I really wouldn't use this
construct.
See that's the thing: How am I supposed to know it's a perl 5.10ism?
The Perl docs give absolutely no clue; Perl[5] is based way too much
on practice rather than theory, because only people that have been
using it since day 1 know what's going on. A couple of weeks ago, I
went to the perldoc website and just read each website one after the
other---that is my total knowledge of Perl, and already I've caught
flack a number of times for having used 'new-fangled technologies'; I
really wish the docs would specify when a feature became available.
... add that to the list of perldoc shortcomings.
I wonder if it would be clearer to always call quote_rfc2047, then
have that function just return its input unaltered if quoting is not
needed.
It actually ALWAYS changes the input. This code:
I think I was not clear. My suggestion was to move the /[^[:ascii:]]/
check to the inside of quote_rfc2047 exactly so that it doesn't always
change its input. i.e.
From: Jay Soffian <hidden> Date: 2016-06-15 22:46:37
On Sat, Apr 18, 2009 at 10:17 PM, Thomas Adam [off-list ref] wrote:
2009/4/19 Michael Witten [off-list ref]:
quoted
However, I wonder if your comment is a veiled quip at my "Improve
redability" claim (which is also ironically unreadable). :-)
More concerning is that it's a perl 5.10ism -- you cannot assume that
perl 5.10 is installed on all platforms. I really wouldn't use this
construct.
Exactly. I started with perl4 (no really...), then moved away from
hard-core perl coding around 2000 and have been just a perl dabbler
since then. So I didn't recognize //, though I suspected it was a
newer construct. :-)
j.
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
Why not? It's at least useful for testing.
Signed-off-by: Michael Witten <redacted>
---
NOTE: This also resulted in a similar change to:
[PATCH RFC3.5 04/12] send-email: Verification...
Documentation/git-send-email.txt | 4 +++-
git-send-email.perl | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
@@ -132,7 +132,9 @@ user is prompted for a password while the input is masked for privacy. be specified by the 'sendemail.smtpserver' configuration variable; the built-in default is `/usr/sbin/sendmail` or `/usr/lib/sendmail` if such a program is available, or- `localhost` otherwise.+ `localhost` otherwise. Also, the built-in default is used if+ `<host>` or 'sendemail.smtpserver' is the empty string (for+ example, if '--smtp-server ""' is specified on the command line). --smtp-server-port=<port>:: Specifies a port different from the default port (SMTP
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
The server URI is verified according to RFCs (including IPv6 support).
The $smtp_server and $smtp_server_port setup code has been moved
higher in the file, so that send-email fails fast if they are bad.
Now, the 'host:port' server URI form is handled regardless of the
documentation deficiencies of Net::SMTP{,::SSL}.
Signed-off-by: Michael Witten <redacted>
---
NOTE: This changed due to the change in:
[PATCH RFC3.5.1 03/12] send-email: Interpret...
Basically, only "a built-in" was changed to
"the built-in".
Is it ever useful to submit patches for patches
rather than sending the whole patch gain? Or would
that be even more trouble?
Documentation/git-send-email.txt | 50 ++++++++++++-----
git-send-email.perl | 115 +++++++++++++++++++++++++++++++------
2 files changed, 131 insertions(+), 34 deletions(-)
@@ -125,23 +125,43 @@ specified (with '--smtp-pass' or 'sendemail.smtppass'), then the user is prompted for a password while the input is masked for privacy. --smtp-server=<server>::- If set, specifies the outgoing SMTP server to use (e.g.- `smtp.example.com` or a raw IP address). Alternatively it can- specify a full pathname of a sendmail-like program instead;- the program must support the `-i` option. Default value can- be specified by the 'sendemail.smtpserver' configuration- variable; the built-in default is `/usr/sbin/sendmail` or- `/usr/lib/sendmail` if such a program is available, or- `localhost` otherwise. Also, the built-in default is used if- `<host>` or 'sendemail.smtpserver' is the empty string (for- example, if '--smtp-server ""' is specified on the command line).+ Specifies the outgoing SMTP server to use. The server may be+ given as a domain name (e.g. `smtp.example.com:587`), raw IP+ address (e.g. `192.168.0.1`), or absolute path to a command+ (e.g. `/usr/sbin/sendmail`).+++Usually a server is specified with its URI form:+++ host[:port]+++The optional port, which identifies a particular service at the+given host, is normally provided as a non-negative integer that+is representable in 16-bits; however, it is possible to use any+string composed of some combination of alphanumeric characters, the+underscore, the hyphen, and punctuation (special) characters, so+that symbolic service names can be employed (as defined by, say,+`/etc/services` on Unix systems).+++Alternatively the server can be specified as an absolute path to+a sendmail-like program; in particular, the program must support+`sendmail's` `-i` option.+++Default value can be specified by the 'sendemail.smtpserver'+configuration variable; the built-in default is `/usr/sbin/sendmail`+or `/usr/lib/sendmail` if such a program is available, or `localhost`+otherwise. Also, the built-in default is used if `<server>` or+'sendemail.smtpserver' is the empty string (for example, if+'--smtp-server ""' is specified on the command line). --smtp-server-port=<port>::- Specifies a port different from the default port (SMTP- servers typically listen to smtp port 25 and ssmtp port- 465); symbolic service names (e.g. "submission" instead of 587)- are also accepted. The port can also be set with the- 'sendemail.smtpserverport' configuration variable.+ Specifies a port different from the default port (SMTP servers+ typically listen to port 25, 587, or even non-standard 465); symbolic+ service names (e.g. "submission" instead of 587) are also accepted,+ provided the underlying system handles mappings in something like+ `/etc/services`; such service names may only be composed of some+ combination of alphanumeric characters, the underscore, the hyphen,+ and punctuation (special) characters. The port can also be set with+ the 'sendemail.smtpserverport' configuration variable. --smtp-ssl:: Legacy alias for '--smtp-encryption ssl'.
@@ -328,6 +328,101 @@ foreach my $setting (values %config_bool_settings) {# 'default' encryption is none -- this only prevents a warning$smtp_encryption=''unless(defined$smtp_encryption);+# Define a function that verifies a server URI and returns+# its host and port parts:++subparse_server_URI($){++# These regular experssions were derived from:+# * RFC 2373 (Appendix B) : IP Version 6 Addressing Architecture+# * RFC 2732 (Section 3) : Format for Literal IPv6 Addresses in URL's+# * RFC 2396 (Sec. 3.2.2) : Uniform Resource Identifiers (URI): Generic Syntax+# Also, ports are allowed to be symbolic, so that /etc/services mappings+# can be used.++# Define a port (RFC 2396 and extension):++my$port=qr/[\w[:punct:]]+/;++# Define an IPv4 address (RFC 2373):++my$dig3=qr/\d{1,3}/;+my$IPv4address=qr/$dig3(?:\.$dig3){3}/;++# Define an IPv6 address (RFC 2373):++my$hex4=qr/[[:xdigit:]]{1,4}/;+my$hexseq=qr/$hex4(?::$hex4)*/;# RFC 2373 is really that loose.+my$hexpart=qr/$hexseq|$hexseq?::$hexseq?/;+my$IPv6address=qr/$hexpart(?::$IPv4address)?/;++# Define an IPv6 literal (RFC 2732):++my$IPv6reference=qr/\[$IPv6address\]/;++# Define a server URI (RFC 2396 and RFC 2732):++my$toplabel=qr/[[:alpha:]](?:(?:[[:alnum:]]|-)*[[:alpha:]])?/;+my$domainlabel=qr/[[:alnum:]](?:(?:[[:alnum:]]|-)*[[:alnum:]])?/;+my$hostname=qr/(?:$domainlabel\.)*$toplabel\.?/;+my$host=qr/$hostname|$IPv4address|$IPv6reference/;+my$hostport=qr/^($host)(?::($port))?$/;++# Phew! Now parse++returnshift=~$hostport;+}++# Figure out how to contact the SMTP server.+# After this code, $smtp_server_port is either+# valid or undef:++if(defined$smtp_server&&$smtp_serverne''){++if($smtp_server_is_a_command=($smtp_server=~m{^/})){++die"--smtp-server: The SMTP server command does not exist: $smtp_server\n"+unless-x$smtp_server;++printSTDERR"--smtp-server: Using command '$smtp_server'; ignoring --smtp-server-port='$smtp_server_port'\n"+ifdefined$smtp_server_port;++}else{++($smtp_server,my$port)=parse_server_URI$smtp_server+ordie"--smtp-server: Not a valid server URI: '$smtp_server'\n";++if(defined$smtp_server_port){++$smtp_server_port=~ /[\w[:punct:]]+/ordie"--smtp-server-port: Invalid port: '$smtp_server_port'\n";++printSTDERR"--smtp-server-port: Using port '$smtp_server_port'; ignoring --smtp-server's port '$port'\n"+ifdefined$port;++}else{++$smtp_server_port=$port;+}+}++}else{# use a default:++foreach(qw( /usr/sbin/sendmail /usr/lib/sendmail )){+if(-x$_){+$smtp_server=$_;+$smtp_server_is_a_command=1;++printSTDERR"--smtp-server: Using command '$smtp_server'; ignoring --smtp-server-port='$smtp_server_port'\n"+ifdefined$smtp_server_port;++last;+}+}++$smtp_server='localhost'# 127.0.0.1 is not compatible with IPv6+unless$smtp_server_is_a_command;+}+# Set CC suppressionsmy(%suppress_cc);if(@suppress_cc){
@@ -675,24 +770,6 @@ if (defined $initial_reply_to) {$initial_reply_to="<$initial_reply_to>"if$initial_reply_tone'';}-if(defined$smtp_server&&$smtp_serverne''){--$smtp_server_is_a_command=($smtp_server=~m{^/});--}else{# use a default:--foreach(qw( /usr/sbin/sendmail /usr/lib/sendmail )){-if(-x$_){-$smtp_server=$_;-$smtp_server_is_a_command=1;-last;-}-}--$smtp_server='localhost'# 127.0.0.1 is not compatible with IPv6-unless$smtp_server_is_a_command;-}-if($compose&&$compose>0){@files=($compose_filename.".final",@files);}
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
On Sun, Apr 19, 2009 at 09:13, Jay Soffian [off-list ref] wrote:
I think I was not clear. My suggestion was to move the /[^[:ascii:]]/
check to the inside of quote_rfc2047 exactly so that it doesn't always
change its input. i.e.
Ah. However, there is still the question of whether the actual email
headers are present to declare the right encoding. I don't know enough
to comment on this, though; before this patch, this quoting was
performed by code that new to right the correct "Content-Type" and
"Content-Transfer-Encoding" headers. I suppose I'll have to read the
RFC.
This simplifies things for the function caller I think.
I'm morally opposed to this kind of thing. The caller should be
required to test whether quote_rfc2047() is required, as it's not the
job of quote_rfc2047 to validate. Suppose that quote_rfc2047 were
actually part of a library of useful functions that my program
imports. Perhaps my program knows that it must always quote some piece
of text. Why, then, should my program be forced to waste the cycles to
perform a useless test?
IMnsHO, verification should always be done by the caller with one
exception: Interactive (human) input should always be verified,
because humans represent an unreliable component in the system (in
terms of digital systems, their asynchronous input must be
synchronized with the clocked system). WIth this model, there's are
fewer wasted cycles, because you can reuse verification across similar
functions, and the code (particularly library code) is easier to
understand.
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
On Sun, Apr 19, 2009 at 09:39, Michael Witten [off-list ref] wrote:
I'm morally opposed to this kind of thing. The caller should be
required to test whether quote_rfc2047() is required, as it's not the
job of quote_rfc2047 to validate. Suppose that quote_rfc2047 were
actually part of a library of useful functions that my program
imports. Perhaps my program knows that it must always quote some piece
of text. Why, then, should my program be forced to waste the cycles to
perform a useless test?
IMnsHO, verification should always be done by the caller with one
exception: Interactive (human) input should always be verified,
because humans represent an unreliable component in the system (in
terms of digital systems, their asynchronous input must be
synchronized with the clocked system). WIth this model, there's are
fewer wasted cycles, because you can reuse verification across similar
functions, and the code (particularly library code) is easier to
understand.
I should add, though, that making the logic of the program clear is a
good idea. In that sense, your approach makes sense. Since we 'own'
quote_rfc2047(), I'd say we could take your approach, but rename the
function to something like quote_rfc2047_if_necessary(). If
quote_rfc2047() were part of a library, I think the only moral
solution would be to insist that callers wrap it in another function
named quote_rfc2047_if_necessary().
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
We might as well use the global variables while they exist; there's
no reason to print the result to a file and then read it back in.
Also, the entire file is already read and checked for:
/[^[:ascii:]]/
by:
my $need_8bit_cte = file_has_nonascii($compose_filename);
so we might as well use $need_8bit_cte until something less egregiously
inefficient is implemented.
Signed-off-by: Michael Witten <redacted>
---
git-send-email.perl | 9 ++-------
1 files changed, 2 insertions(+), 7 deletions(-)
From: Junio C Hamano <hidden> Date: 2016-06-15 22:46:37
Michael Witten [off-list ref] writes:
This is a minor change, but it's cleaner, and it sets up the
$smtp_server initialization code for future improvements.
...
-if (!defined $smtp_server) {
+if (defined $smtp_server) {
+
+ $smtp_server_is_a_command = ($smtp_server =~ m{^/});
+
+} else { # use a default:
+
foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) {
if (-x $_) {
$smtp_server = $_;
+ $smtp_server_is_a_command = 1;
last;
}
}
- $smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
+
+ $smtp_server = 'localhost'; # 127.0.0.1 is not compatible with IPv6
+ unless $smtp_server_is_a_command;
Nobody suggests to use 127.0.0.1 anymore with this change, so why not just
get rid of that comment?
Also the new statement looks wrong.
(1) you have ';' after assignment before the statement modifier "unless";
I do not think you meant it. I generally *dis*like statement
modifiers, but if you use it, at least please use it correctly.
(2) earlier, when $smtp_server is defined (say, the name of your smtp
host) but is not a command, we did not set smtp_server to
'localhost', but kept the value given by the user. Now you seem to
kill the user's wish with this change.
I think a genuine improvement would be something like:
if (!defined $smtp_server) {
$smtp_server = 'localhost';
}
Of course if you are writing for a project that is "5.8.1 or later only",
you could say:
$smtp_server //= 'localhost';
From: Junio C Hamano <hidden> Date: 2016-06-15 22:46:37
Michael Witten [off-list ref] writes:
Why not? It's at least useful for testing.
Why so? "Use a default" as opposed to using what?
It is unclear what "a default" is in this context. Do configured values
count as "a default"? I suspect not.
I think you meant "allow overriding the configured values and use the
default", but then you should spell what the defaults are (an available
local MTA binary, or SMTP port on localhost, I think). That is much more
informative than your "Why not?..."
In any case, I find it counterintuitive to trigger "use the default" with
an option. Something like "--ignore-config=smtp-server,smtp-port" might
make sense, though.
From: Junio C Hamano <hidden> Date: 2016-06-15 22:46:37
Michael Witten [off-list ref] writes:
The server URI is verified according to RFCs (including IPv6 support).
The $smtp_server and $smtp_server_port setup code has been moved
higher in the file, so that send-email fails fast if they are bad.
Now, the 'host:port' server URI form is handled regardless of the
documentation deficiencies of Net::SMTP{,::SSL}.
You said that in 01/12, too but I do not think there is any problem with
Perl documentation.
My installed copy of /usr/share/perl/5.10.0/Net/SMTP.pm has this:
B<Host> - SMTP host to connect to. It may be a single scalar, as defined for
the C<PeerAddr> option in L<IO::Socket::INET>, or a reference to
an array with hosts to try in turn. The L</host> method will return the value
which was used to connect to the host.
and of course PeerAddr allows host:port.
Please stop placing a false blame on others. I think it is you who lack
ability to read the documentation correctly in this particular case.
From: Junio C Hamano <hidden> Date: 2016-06-15 22:46:37
Michael Witten [off-list ref] writes:
+ die "Server does not support STARTTLS: " . $smtp->message . "\n"
+ unless $smtp->code == 220;
Statement modifiers merely make things even less readable, especially when
the conditional is the unlikely case. Please do not add more of them.
do this;
do that;
do something
if some condition that holds true most of the time;
do some other thing;
is already hard to follow, but it is probably excusable in some cases,
because your thought can flow "ah, Ok, these four things are done in
sequence" when you are quickly scanning the code to understand the overall
structure, letting your eyes ignore the "true most of the time" part.
But the following, which is equivalent to what you did, is inexcuable.
do this;
do that;
do something unusual
if some condition that rarely holds true;
do some other thing;
When your eyes and brain are coasting over this segment of code, your
thought process needs to stumble and hiccup at the statment that does
something unusual, and then need to realize that it is qualified with a
statement modifier that says "this is only for rare case".
Written without statement modifier:
do this;
do that;
if (some consition that rarely holds true) {
do something unusual
}
do some other thing;
it is much easier to coast over; you can tell "Ah, after doing this and
that, in the normal case we do some other thing" and do not have to even
look at the details of "something unusual" part.
- $smtp->mail( $raw_from ) or die $smtp->message;
- $smtp->to( @recipients ) or die $smtp->message;
- $smtp->data or die $smtp->message;
- $smtp->datasend("$header\n$message") or die $smtp->message;
- $smtp->dataend() or die $smtp->message;
- $smtp->code =~ /250|200/ or die "Failed to send $subject\n".$smtp->message;
+ SEND_MAIL:
+
+ $smtp->mail($raw_from) and
+ $smtp->to(@recipients) and
+ $smtp->data and
+ $smtp->datasend("$header\n$message") and
+ $smtp->dataend or
+
+ die "Failed to send '$subject': " . $smtp->message . "\n";
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
On Sun, Apr 19, 2009 at 20:41, Junio C Hamano [off-list ref] wrote:
Michael Witten [off-list ref] writes:
quoted
This is a minor change, but it's cleaner, and it sets up the
$smtp_server initialization code for future improvements.
...
-if (!defined $smtp_server) {
+if (defined $smtp_server) {
+
+ $smtp_server_is_a_command = ($smtp_server =~ m{^/});
+
+} else { # use a default:
+
foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) {
if (-x $_) {
$smtp_server = $_;
+ $smtp_server_is_a_command = 1;
last;
}
}
- $smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
+
+ $smtp_server = 'localhost'; # 127.0.0.1 is not compatible with IPv6
+ unless $smtp_server_is_a_command;
Nobody suggests to use 127.0.0.1 anymore with this change, so why not just
get rid of that comment?
Fine with me.
Also the new statement looks wrong.
(1) you have ';' after assignment before the statement modifier "unless";
I do not think you meant it. I generally *dis*like statement
modifiers, but if you use it, at least please use it correctly.
How embarassing. That's actually been fixed on my end since I sent
that patch; for some reason, I forget to send the update; sorry for
wasting your time.
(2) earlier, when $smtp_server is defined (say, the name of your smtp
host) but is not a command, we did not set smtp_server to
'localhost', but kept the value given by the user. Now you seem to
kill the user's wish with this change.
I think you misread the code (with the exception of the error on my
part). The code could be read:
if $smtp_server is already defined {
determine whether it is a command;
} else {
find a suitable default for it;
}
I think a genuine improvement would be something like:
if (!defined $smtp_server) {
$smtp_server = 'localhost';
}
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
On Sun, Apr 19, 2009 at 20:41, Junio C Hamano [off-list ref] wrote:
Michael Witten [off-list ref] writes:
quoted
Why not? It's at least useful for testing.
Why so? "Use a default" as opposed to using what?
Use a default as opposed to using the nonsensical empty string.
It is unclear what "a default" is in this context. Do configured values
count as "a default"? I suspect not.
I think you meant "allow overriding the configured values and use the
default", but then you should spell what the defaults are (an available
local MTA binary, or SMTP port on localhost, I think). That is much more
informative than your "Why not?..."
I see your irritation is derived from my carefree (careless?) commit
message. However, the patch doesn't do anything but trigger the
already present default-selecting code; there are already docs that
specify what those defaults are.
In any case, I find it counterintuitive to trigger "use the default" with
an option. Something like "--ignore-config=smtp-server,smtp-port" might
make sense, though.
I can get behind that kind of solution; however, I see no problem with
the shorthand --smtp-server "" either (other than the fact that it
requires an extra test in order to work).
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
On Sun, Apr 19, 2009 at 20:42, Junio C Hamano [off-list ref] wrote:
Michael Witten [off-list ref] writes:
quoted
+ die "Server does not support STARTTLS: " . $smtp->message . "\n"
+ unless $smtp->code == 220;
...
But the following, which is equivalent to what you did, is inexcuable.
do this;
do that;
do something unusual
if some condition that rarely holds true;
do some other thing;
When your eyes and brain are coasting over this segment of code, your
thought process needs to stumble and hiccup at the statment that does
something unusual, and then need to realize that it is qualified with a
statement modifier that says "this is only for rare case".
I mostly agree, and I frequently consider[ed] exactly those points.
However, there are 2 things that played a role in my decision:
* For most conditional cases, I personally
loathe curly braces around one statement.
* The flow is actually:
do this;
do that;
DIE "whisper some curses with the last breath"
UNLESS some condition that holds mostly true;
do some other thing;
The "die" and thoughtful spacing should be pretty good clues.
However, the "unless" can be strange to think with (at first);
I figured Perlers would be happy with it.
In any case, I also like:
condition and/or (do something);
or:
condition and/or do something;
The only thing keeping me from using that more often is that I assume
other people would be less comfortable with it and that it may
introduce an unnecessary comparison of the return value of "do
something"; also, it might make the line a little long, which some
people get really angry about.
Written without statement modifier:
do this;
do that;
if (some consition that rarely holds true) {
do something unusual
}
do some other thing;
I just have a hard time stomaching those curly braces. I really wish
perl didn't enforce them when there's only one statement. Also, I
would use some whitespace:
do this;
do that;
if (some consition that rarely holds true) {
do something unusual
}
do some other thing;
quoted
+ $smtp->mail($raw_from) and
+ $smtp->to(@recipients) and
+ $smtp->data and
+ $smtp->datasend("$header\n$message") and
+ $smtp->dataend or
+
+ die "Failed to send '$subject': " . $smtp->message . "\n";
These do make things more pleasant to read.
Thanks!
P.S.
Sorry if the formatting of this email is bad; I'm in the middle of a
large move between systems, and currently I'm stuck with gmail's
webmail, which insists on reformatting my text and refusing to render
in fixed-width font (though I bet I could hack firefox's css to get
that one working.... hmmm.....), and firefox doesn't make it easy to
input tabs.
So, I've actually been writing and sending some emails with a combination of:
* vim
* date +'%a, %e %b %Y %T %z'
* uuidgen (though I've found gmail makes a Message-ID for me)
* cat path/to/email.txt | perl -pe 's/\n/\r\n/; END {print
"\r\n"}' | msmtp -t
This email was written in the webmail in firefox; I actually counted
spaces for indentation in the hope that things line up. ;-)
From: Michael Witten <hidden> Date: 2016-06-15 22:46:37
On Sun, Apr 19, 2009 at 09:19, Michael Witten [off-list ref] wrote:
+ ($smtp_server, my $port) = parse_server_URI $smtp_server
+ or die "--smtp-server: Not a valid server URI: '$smtp_server'\n";
Whoops! I used to have:
my ($host, $port) = ...
but I tried to be clever and replaced it with the above.
Unfortunately, that means that an incorrect server 'URI' sets
$smtp_server to undef, which perl complains about when it interpolates
the die string.
I don't think this is a good idea. need_8bit_cte is about the _whole_
message, including all headers, and this is just about the subject.
Which means that we end up rfc2047-encoding the subject unnecessarily
quite a bit (since at least in git itself, most of the time the
non-ascii bits are in people's names).
This makes the subject unnecessarily ugly for readers which don't do
rfc2047 decoding. And while I expect that most real MUAs these days
handle the decoding, it also makes life harder for people looking
directly at message, or doing "grep -i ^subject: foo.mbox". Yes, I know
that doesn't even remotely follow the standards (e.g., it won't handle
line-wrapped headers), but I don't see any need to make it worse.
All of that being said, even if we decided that it _is_ OK to quote
even when it wasn't unnecessary, your patch still isn't right.
need_8bit_cte is not "does this message need an 8-bit cte at all?" but
rather "does _we_ need to add an 8-bit cte?". A few lines above the ones
you changed, notice that when we see the message already has a
MIME-Version header, we turn set $need_8bit_cte to 0. But in that case,
we still may need to encode the subject if it has non-ascii characters.
-Peff
From: Michael Witten <hidden> Date: 2016-06-15 22:46:38
On Mon, Apr 20, 2009 at 21:34, Jeff King [off-list ref] wrote:
A few lines above the ones
you changed, notice that when we see the message already has a
MIME-Version header, we turn set $need_8bit_cte to 0. But in that case,
we still may need to encode the subject if it has non-ascii characters.
I think this is the only problem with the patch, and I'm glad you
caught it. However, I can't make further comment until I read the RFC,
so I'm going to withdraw his patch from consideration. Thanks!