patch, 3 partssend-email: Don't use FQDNs without a '.'

The first patch keeps send-email from using a bogus FQDN for my laptop. Oddly enough, my MTA doesn't mind "localhost.localdomain" but complains about "My-Computer".

The second adds documentation for --smtp-domain to the git-send-email man page. The only documentation for it was in the code and the usage message.

The last is just an enhancement, but I was honestly surprised that --smtp-domain didn't have a configuration.

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

 Documentation/git-send-email.txt |    7 +++++++
 git-send-email.perl              |   26 ++++++++++++++------------
 2 files changed, 21 insertions(+), 12 deletions(-)

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.

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---

 My OS X machine doesn't add ".local" to it's hostname for some reason.
 Since a FQDN requires at least one . between the TLD and hostname,
 we can check for it to avoid nonsense results like "My-Computer".

 git-send-email.perl |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

Brian Gernhardt <brian@gernhardtsoftware.com> writes:

It would become *much* easier to read if we stop using the statement modifier, and write it in a more straightforward way:

	unless (($^O eq 'darwin' && $domain =~ /\.local$/)
        	|| $domain !~ /\./) {
                $maildomain = $domain;
	}

as the condition seems to have grown large enough to exceed "by the way don't do this under this narrow condition", which is what statement modifiers are designed to be used. Also mixing && and or that have different precedence taxes readers' brainpower unnecessarily.

I also think that this particular exception logic should probably be in a separate helper function that is called from the two places.

Jari, you are the last person who touched code around this area. What do you think?

Move duplicate maildomain checks to a single subroutine. Require that a FQDN contains at least one period.

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

    Here is improved version.

    -- Jari

    >Re: [PATCH 1/3] send-email: Don't use FQDNs without a '.'
    >Junio C Hamano <gitster@pobox.com> writes:
    >> Brian Gernhardt <brian@gernhardtsoftware.com> writes:
    >
    >>  		$maildomain = $domain
    >> -			unless $^O eq 'darwin' && $domain =~ /\.local$/;
    >> +			unless $^O eq 'darwin' && $domain =~ /\.local$/
    >> +				or $domain !~ /\./;
    >
    > [Junio] It would become *much* easier to read if we stop using the statement
    > modifier, and write it in a more straightforward way:
    >
    > 	unless (($^O eq 'darwin' && $domain =~ /\.local$/)
    >         	|| $domain !~ /\./) {
    >                 $maildomain = $domain;
    > 	}
    >
    > as the condition seems to have grown large enough to exceed "by the way
    > don't do this under this narrow condition", which is what statement
    > modifiers are designed to be used.  Also mixing && and or that have
    > different precedence taxes readers' brainpower unnecessarily.
    >
    > I also think that this particular exception logic should probably be in a
    > separate helper function that is called from the two places.
    >
    > Jari, you are the last person who touched code around this area.  What do
    > you think?

Move duplicate maildomain checks to a single subroutine. Require that a FQDN contains at least one period.

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

      Here is improved version 2 (use "tab" for indentation).
 
      -- Jari
 
      >Re: [PATCH 1/3] send-email: Don't use FQDNs without a '.'
      >Junio C Hamano <gitster@pobox.com> writes:
      >> Brian Gernhardt <brian@gernhardtsoftware.com> writes:
      >
      >>  		$maildomain = $domain
      >> -			unless $^O eq 'darwin' && $domain =~ /\.local$/;
      >> +			unless $^O eq 'darwin' && $domain =~ /\.local$/
      >> +				or $domain !~ /\./;
      >
      > [Junio] It would become *much* easier to read if we stop using the statement
      > modifier, and write it in a more straightforward way:
      >
      > 	unless (($^O eq 'darwin' && $domain =~ /\.local$/)
      >         	|| $domain !~ /\./) {
      >                 $maildomain = $domain;
      > 	}
      >
      > as the condition seems to have grown large enough to exceed "by the way
      > don't do this under this narrow condition", which is what statement
      > modifiers are designed to be used.  Also mixing && and or that have
      > different precedence taxes readers' brainpower unnecessarily.
      >
      > I also think that this particular exception logic should probably be in a
      > separate helper function that is called from the two places.
      >
      > Jari, you are the last person who touched code around this area.  What do
      > you think?

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

Good idea.

[...]

Style:

  +sub maildomain_sanitize {
  +	local $domain = shift;
  +
  +	#  On Mac, the the domain name is not necessarily in FQDN format
  +	#  Require a period in the string
  +
  +	if ($^O eq 'darwin'  &&  $domain =~ /\.local$/) {
  +		# Nope, pass this one.
  +	} elsif ($domain =~ /\./) {
  +		return $domain;
  +	}
  +}

(probably fixed in new version of this series).

Move duplicate maildomain checks to a single subroutine. Require that a FQDN contains at least one period.

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

    Jakub Narebski <jnareb@gmail.com> writes:
    >   +sub maildomain_sanitize {
    >   +	local $domain = shift;
    >   +
    >   +	#  On Mac, the the domain name is not necessarily in FQDN format
    >   +	#  Require a period in the string
    >   +
    >   +	if ($^O eq 'darwin'  &&  $domain =~ /\.local$/) {
    >   +		# Nope, pass this one.
    >   +	} elsif ($domain =~ /\./) {
    >   +		return $domain;
    >   +	}
    >   +}

    Thanks for 2nd eye. Changed these in above:

    * Starting brace placement at function start
    * Placement of else
    * Maybe: use or "return" (debatable).

    But not these. Motivation:

    * The "$_" simplifies ussage everywhere in the function. XP: less is more.
    * The "and" is more readable than "&&". The "and" is also safer
      due to its lower precedence.

    Jari

Jari Aalto wrote:

O.K.

Note however that while setting $_ ($ARG with English) simplifies regexp matching, you have to take care to use

   local $_ = shift;

and not

   my $_ = shift; 

And to use 'local'.

A bit of "difficulty conservation" at work.

[...]

Jakub Narebski <jnareb@gmail.com> writes:

Speaking of which...

Could we please move to using this everywhere:

    use English qw( -no_match_vars );

That would give all the benefits of readability without performance penalties. The load time seems to be neglible:

    $ perl --version | grep v5

    This is perl, v5.10.1 (*) built for x86_64-linux-gnu-thread-multi

    $ time perl -e 'print'

    real    0m0.005s
    user    0m0.000s
    sys     0m0.004s

    $ time perl -e 'use English qw( -no_match_vars ); print'

    real    0m0.017s
    user    0m0.008s
    sys     0m0.008s

As done.

The Perl 5.12 now finally allow my'ing of $ARG too. But maybe we don't switch to it just yet...

Jari

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---

 --smtp-debug is still undocumented.  It also appears to be a boolean that
 takes 0 or 1 instead of being of the form --[no-]smtp-debug.

 Documentation/git-send-email.txt |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

--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>
---

 Seemed like a natural thing to add.  Was surprised it wasn't there when I went
 poking around.

 Documentation/git-send-email.txt |    3 ++-
 git-send-email.perl              |   20 ++++++++++----------
 2 files changed, 12 insertions(+), 11 deletions(-)

Brian Gernhardt <brian@gernhardtsoftware.com> writes:

             via 'sendemail.smtpdomain' ?

You should also add one-liner to Documentation/config.txt, mentioning 'sendemail.smtpdomain' configuration variable (the part referring reader to git-send-email(1) documentation).

P.S. Some of git-send-email configuration variables: sendemail.identity, sendemail.smtpencryption, sendemail.smtpssl (deprecated), and the concept of sendemail.<identity>.* are described in Documentation/config.txt, which means that they are present in git-config(1) manpage. Some of them are described in "Configuration" section of Documentation/git-send-email.txt (which means git-send-email(1) manpage): sendemail.aliasesfile, sendemail.aliasfiletype, sendemail.multiedit, sendemail.confirm; I think those are config variables which do not have corresponding command line option. The rest: sendemail.bcc, sendemail.cc, sendemail.from, sendemail.to, sendemail.envelopesender, sendemail.smtpencryption (which is described in git-config(1)), sendemail.smtppass, sendemail.smtpserver, sendemail.smtpserverport, sendemail.smtpuser, sendemail.cccmd, sendemail.chainreplyto, sendemail.identity and 'sendemail.<identity>' subsection (described in git-config(1)), sendemail.signedoffbycc, sendemail.suppresscc, sendemail.suppressfrom, sendemail.thread, and sendemail.validate are described only with corresponding command-line option.

Huh...