[PATCH] Added get sendmail from .mailrc

[PATCH] Added get sendmail from .mailrc

[Brilliantov Kirill Vladimirovich]

Signed-off-by: Brilliantov Kirill Vladimirovich <brilliantov@inbox.ru>
---
 git-send-email.perl | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index 2016d9c..5345fdb 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -28,6 +28,7 @@ use File::Temp qw/ tempdir tempfile /;
 use File::Spec::Functions qw(catfile);
 use Error qw(:try);
 use Git;
+use File::HomeDir;
 
 Getopt::Long::Configure qw/ pass_through /;
 
@@ -804,6 +805,23 @@ if (!defined $smtp_server) {
 			last;
 		}
 	}
+
+	if (!defined $smtp_server) {
+		my $mailrc = File::HomeDir->my_home . "/.mailrc";
+		if (-e $mailrc) {
+			open FILE, $mailrc or die "Failed open $mailrc: $!";
+			while (<FILE>) {
+			    chomp;
+			    if (/set sendmail=.*/) {
+				my @data = split '"';
+				$smtp_server = $data[1];
+				last;
+			    }
+			}
+			close FILE;
+		}
+	}
+
 	$smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
 }
 
-- 
1.8.5.2

Re: [PATCH] Added get sendmail from .mailrc

[Eric Wong]

Brilliantov Kirill Vladimirovich <brilliantov@inbox.ru> wrote:
> Signed-off-by: Brilliantov Kirill Vladimirovich <brilliantov@inbox.ru>
> ---
>  git-send-email.perl | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

Some documentation references to .mailrc and its format would be nice.

> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -28,6 +28,7 @@ use File::Temp qw/ tempdir tempfile /;
>  use File::Spec::Functions qw(catfile);
>  use Error qw(:try);
>  use Git;
> +use File::HomeDir;

We should probably avoid a new dependency and also remain consistent
with the rest of git handles home directories.

Unfortunately, expand_user_path()/git_config_pathname() isn't currently
exposed to scripters right now...

Re: [PATCH] Added get sendmail from .mailrc

[Brilliantov Kirill Vladimirovich]

On 2014-01-25 22:37:21, Eric Wong wrote:
> Brilliantov Kirill Vladimirovich <brilliantov@inbox.ru> wrote:
> > Signed-off-by: Brilliantov Kirill Vladimirovich <brilliantov@inbox.ru>
> > ---
> >  git-send-email.perl | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> 
> Some documentation references to .mailrc and its format would be nice.
> 

Unfortunally I can't found official documentation on this option:
http://linux.die.net/man/1/mailx
http://publib.boulder.ibm.com/infocenter/aix/v6r1/topic/com.ibm.aix.files/doc/aixfiles/mailrc.htm

On my system (Debian GNU/Linux 7.3) documentation on mailx not conteins
description senmail options.

> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -28,6 +28,7 @@ use File::Temp qw/ tempdir tempfile /;
> >  use File::Spec::Functions qw(catfile);
> >  use Error qw(:try);
> >  use Git;
> > +use File::HomeDir;
> 
> We should probably avoid a new dependency and also remain consistent
> with the rest of git handles home directories.
> 
> Unfortunately, expand_user_path()/git_config_pathname() isn't currently
> exposed to scripters right now...
> 

Ok, if new dependency is not allowed I see next ways:
- add new argument
- add new configuration parameters

Re: [PATCH] Added get sendmail from .mailrc

[Brilliantov Kirill Vladimirovich]

On 2014-01-26 11:34:38, Brilliantov Kirill Vladimirovich wrote:
> On 2014-01-25 22:37:21, Eric Wong wrote:
> > 
> > We should probably avoid a new dependency and also remain consistent
> > with the rest of git handles home directories.
> > 
> > Unfortunately, expand_user_path()/git_config_pathname() isn't currently
> > exposed to scripters right now...
> > 
> 
> Ok, if new dependency is not allowed I see next ways:
> - add new argument
> - add new configuration parameters

Ok, git support setting path to sendmail-like program via sendemail.smtpserver
configuration option.
It is not very convenient because I need have separated configuration for mail
and git.

Re: [PATCH] Added get sendmail from .mailrc

[Eric Wong]

Brilliantov Kirill Vladimirovich <brilliantov@inbox.ru> wrote:
> On 2014-01-25 22:37:21, Eric Wong wrote:
> > Brilliantov Kirill Vladimirovich <brilliantov@inbox.ru> wrote:
> > > --- a/git-send-email.perl
> > > +++ b/git-send-email.perl
> > > @@ -28,6 +28,7 @@ use File::Temp qw/ tempdir tempfile /;
> > >  use File::Spec::Functions qw(catfile);
> > >  use Error qw(:try);
> > >  use Git;
> > > +use File::HomeDir;
> > 
> > We should probably avoid a new dependency and also remain consistent
> > with the rest of git handles home directories.
> > 
> > Unfortunately, expand_user_path()/git_config_pathname() isn't currently
> > exposed to scripters right now...
> 
> Ok, if new dependency is not allowed I see next ways:

Not saying it's not allowed.  I meant we should probably expose
expand_user_path()/git_config_pathname() C functions to script helpers
(so git-config or git-rev-parse can provide them to sh or perl scripts).

Re: [PATCH] Added get sendmail from .mailrc

[Jeff King]

On Sun, Jan 26, 2014 at 09:17:09AM +0000, Eric Wong wrote:

> > > > +use File::HomeDir;
> > > 
> > > We should probably avoid a new dependency and also remain consistent
> > > with the rest of git handles home directories.
> > > 
> > > Unfortunately, expand_user_path()/git_config_pathname() isn't currently
> > > exposed to scripters right now...
> > 
> > Ok, if new dependency is not allowed I see next ways:
> 
> Not saying it's not allowed.  I meant we should probably expose
> expand_user_path()/git_config_pathname() C functions to script helpers
> (so git-config or git-rev-parse can provide them to sh or perl scripts).

I do not think we need anything so complex. Most of the logic in
expand_user_path is about handling "~" and "~user". But here we _just_
want to know the current user's home directory, and for that
expand_user_path always just looks in $HOME.

So I think $ENV{HOME} would be fine to match what git does. My
understanding is that File::HomeDir does some magic that may work better
on non-Unix platforms. I do not know if we even care for this feature,
since .mailrc is presumably a Unix thing. But if we do, I think our
usual strategy with such things is to optionally use the dependency if
available, and fall back to something sane. Like:

  sub homedir {
        if (eval { require File::HomeDir; 1 }) {
                return File::HomeDir->my_home;
        }
        return $ENV{HOME};
  }

Whichever code path is followed, you should probably also check the
result for "undef", which the original patch did not do.

-Peff

Re: [PATCH] Added get sendmail from .mailrc

[Jeff King]

On Sat, Jan 25, 2014 at 01:46:50PM +0400, Brilliantov Kirill Vladimirovich wrote:

> +	if (!defined $smtp_server) {
> +		my $mailrc = File::HomeDir->my_home . "/.mailrc";

The new module dependency has been discussed elsewhere in the thread.

> +		if (-e $mailrc) {
> +			open FILE, $mailrc or die "Failed open $mailrc: $!";

Please use the three-argument of 'open', and use a regular scalar
instead of a glob. Both are safer, and we assume a modern enough perl to
support both. I.e.:

  open(my $file, '<', $mailrc)
          or die "failed to open $mailrc: $!";

> +			while (<FILE>) {
> +			    chomp;
> +			    if (/set sendmail=.*/) {
> +				my @data = split '"';
> +				$smtp_server = $data[1];
> +				last;
> +			    }

Your split is a rather unusual way to do the parsing, and it took me a
minute to figure it out. It might be more obvious as:

  if (/set sendmail="(.*)"/) {
          $smtp_server = $1;
          last;
  }

I do not know anything about the mailrc format, nor does it seem to be
well documented. Are the double-quotes required? If not, then the above
regex can easily make them optional. I also wonder if any whitespace is
allowed. E.g., this might be more forgiving:

  /set sendmail\s*=\s*"?(.*?)"?/

but I am just guessing at what the format allows.

-Peff

Re: [PATCH] Added get sendmail from .mailrc

[Kyle J. McKay]

On Jan 27, 2014, at 17:15, Jeff King wrote:
> On Sat, Jan 25, 2014 at 01:46:50PM +0400, Brilliantov Kirill  
> Vladimirovich wrote:
>
>> +	if (!defined $smtp_server) {
>> +		my $mailrc = File::HomeDir->my_home . "/.mailrc";

Actually, based on the output of "man mail", this should probably be  
something more like

   my $mailrc = $ENV{'MAILRC'} || "$ENV{'HOME'}/.mailrc";

which takes into account any MAILRC setting and also avoids the use of  
File::HomeDir.

>> +			while (<FILE>) {
>> +			    chomp;
>> +			    if (/set sendmail=.*/) {
>> +				my @data = split '"';
>> +				$smtp_server = $data[1];
>> +				last;
>> +			    }
>
> Your split is a rather unusual way to do the parsing, and it took me a
> minute to figure it out. It might be more obvious as:
>
>  if (/set sendmail="(.*)"/) {
>          $smtp_server = $1;
>          last;
>  }
>
> I do not know anything about the mailrc format, nor does it seem to be
> well documented. Are the double-quotes required? If not, then the  
> above
> regex can easily make them optional. I also wonder if any whitespace  
> is
> allowed.

 From "man mail":

    set   (se) With no arguments, prints all variable values.   
Otherwise,
          sets option.  Arguments are of the form option=value (no space
          before or after `=') or option.  Quotation marks may be placed
          around any part of the assignment statement to quote blanks or
          tabs, i.e. ``set indentprefix="->"''

My version of "man mail" does not list all the variables that can be  
set but it refers to "The Mail Reference Manual" document which  
presumably does.  I did find this [1] that documents many of the  
available variables including the sendmail one.  I then tried this:

cat <<EOF > /tmp/shim
#!/bin/sh
exec cat
EOF
chmod a+x /tmp/shim
cat <<EOF > /tmp/testrc
   se   send"mail"=/tm"p/"shim
EOF
echo 'testing' | MAILRC=/tmp/testrc mail -s test nobody

And to my surprise the contents of the new message were cat'd out to  
the terminal rather than being sent.  So clearly there's some room for  
improvement with the "set", white space and quote checking.

[1] http://www.cs.fsu.edu/sysinfo/mail/mailrc.html

--Kyle