threads / patch / 35730

patchAdded get sendmail from .mailrc

Subject: [PATCH] Added get sendmail from .mailrc

## tl;dr

8 messages between Jan 25, 2014 and Jan 28, 2014. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Brilliantov Kirill Vladimirovich· Jan 25, 2014, 09:46 UTC · lore
Signed-off-by: Brilliantov Kirill Vladimirovich <brilliantov@inbox.ru>
---
 git-send-email.perl | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
Show changes to git-send-email.perl +18 −0
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
Eric Wong· Jan 25, 2014, 22:37 UTC · re: Brilliantov Kirill Vladimirovich · lore

Re: [PATCH] Added get sendmail from .mailrc

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.
Show 7 quoted lines
> --- 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...

Brilliantov Kirill Vladimirovich· Jan 26, 2014, 07:34 UTC · re: Eric Wong · lore

Re: [PATCH] Added get sendmail from .mailrc

On 2014-01-25 22:37:21, Eric Wong wrote:
Show 8 quoted lines
> 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.

Show 14 quoted lines
> > --- 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
Brilliantov Kirill Vladimirovich· Jan 26, 2014, 08:11 UTC · re: Brilliantov Kirill Vladimirovich · lore

Re: [PATCH] Added get sendmail from .mailrc

On 2014-01-26 11:34:38, Brilliantov Kirill Vladimirovich wrote:
Show 12 quoted lines
> 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.

Eric Wong· Jan 26, 2014, 09:17 UTC · re: Brilliantov Kirill Vladimirovich · lore

Re: [PATCH] Added get sendmail from .mailrc

Brilliantov Kirill Vladimirovich <brilliantov@inbox.ru> wrote:
Show 17 quoted lines
> 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).

Jeff King· Jan 28, 2014, 01:04 UTC · re: Eric Wong · lore

Re: [PATCH] Added get sendmail from .mailrc

On Sun, Jan 26, 2014 at 09:17:09AM +0000, Eric Wong wrote:
Show 13 quoted lines
> > > > +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
Jeff King· Jan 28, 2014, 01:15 UTC · re: Brilliantov Kirill Vladimirovich · lore

Re: [PATCH] Added get sendmail from .mailrc

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: $!";
Show 7 quoted lines
> +			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
Kyle J. McKay· Jan 28, 2014, 06:19 UTC · re: Jeff King · lore

Re: [PATCH] Added get sendmail from .mailrc

On Jan 27, 2014, at 17:15, Jeff King wrote:
Show 5 quoted lines
> 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.

Show 22 quoted lines
>> +			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

← back to recent threads