Re: [PATCH] Added get sendmail from .mailrc
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 () {
+ 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 < /tmp/shim
#!/bin/sh
exec cat
EOF
chmod a+x /tmp/shim
cat < /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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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: $!";
> + while () {
> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Added get sendmail from .mailrc
On Sun, Jan 26, 2014 at 09:17:09AM +, 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Added get sendmail from .mailrc
Brilliantov Kirill Vladimirovich wrote: > On 2014-01-25 22:37:21, Eric Wong wrote: > > Brilliantov Kirill Vladimirovich 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). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Added get sendmail from .mailrc
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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Added get sendmail from .mailrc
On 2014-01-25 22:37:21, Eric Wong wrote: > Brilliantov Kirill Vladimirovich wrote: > > Signed-off-by: Brilliantov Kirill Vladimirovich > > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Added get sendmail from .mailrc
Brilliantov Kirill Vladimirovich wrote: > Signed-off-by: Brilliantov Kirill Vladimirovich > --- > 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... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
