Patchwork mail: don't complain about a multi-word email.method config value

login
register
mail settings
Submitter Julien Cristau
Date Nov. 29, 2019, 6:20 p.m.
Message ID <e962071e8bee30af8b34.1575051650@tomate.cristau.org>
Download mbox | patch
Permalink /patch/43536/
State New
Headers show

Comments

Julien Cristau - Nov. 29, 2019, 6:20 p.m.
# HG changeset patch
# User Julien Cristau <jcristau@debian.org>
# Date 1575051591 -3600
#      Fri Nov 29 19:19:51 2019 +0100
# Node ID e962071e8bee30af8b3432e61b7f04acabbde0db
# Parent  640bae94f2f31b1c1325ad2e5cf7f4a5a9dbebdd
mail: don't complain about a multi-word email.method config value

I want to be able to set email.method to "ssh relay /usr/sbin/sendmail"
without needing an extra trivial shell script.
This works fine since we pass the full command to a shell, except for
validateconfig trying to find it in $PATH.
Yuya Nishihara - Nov. 30, 2019, 4:40 a.m.
On Fri, 29 Nov 2019 19:20:50 +0100, Julien Cristau wrote:
> # HG changeset patch
> # User Julien Cristau <jcristau@debian.org>
> # Date 1575051591 -3600
> #      Fri Nov 29 19:19:51 2019 +0100
> # Node ID e962071e8bee30af8b3432e61b7f04acabbde0db
> # Parent  640bae94f2f31b1c1325ad2e5cf7f4a5a9dbebdd
> mail: don't complain about a multi-word email.method config value
> 
> I want to be able to set email.method to "ssh relay /usr/sbin/sendmail"
> without needing an extra trivial shell script.
> This works fine since we pass the full command to a shell, except for
> validateconfig trying to find it in $PATH.
> 
> diff --git a/mercurial/mail.py b/mercurial/mail.py
> --- a/mercurial/mail.py
> +++ b/mercurial/mail.py
> @@ -241,17 +241,18 @@ def validateconfig(ui):
>          if not ui.config(b'smtp', b'host'):
>              raise error.Abort(
>                  _(
>                      b'smtp specified as email transport, '
>                      b'but no smtp host configured'
>                  )
>              )
>      else:
> -        if not procutil.findexe(method):
> +        program = method.split(None, 1)[0]

b''.split() will return an empty list, and it's probably better to use
procutil.shellsplit().
Julien Cristau - Dec. 2, 2019, 10:52 a.m.
On Sat, Nov 30, 2019 at 01:40:53PM +0900, Yuya Nishihara wrote:
> On Fri, 29 Nov 2019 19:20:50 +0100, Julien Cristau wrote:
> > # HG changeset patch
> > # User Julien Cristau <jcristau@debian.org>
> > # Date 1575051591 -3600
> > #      Fri Nov 29 19:19:51 2019 +0100
> > # Node ID e962071e8bee30af8b3432e61b7f04acabbde0db
> > # Parent  640bae94f2f31b1c1325ad2e5cf7f4a5a9dbebdd
> > mail: don't complain about a multi-word email.method config value
> > 
> > I want to be able to set email.method to "ssh relay /usr/sbin/sendmail"
> > without needing an extra trivial shell script.
> > This works fine since we pass the full command to a shell, except for
> > validateconfig trying to find it in $PATH.
> > 
> > diff --git a/mercurial/mail.py b/mercurial/mail.py
> > --- a/mercurial/mail.py
> > +++ b/mercurial/mail.py
> > @@ -241,17 +241,18 @@ def validateconfig(ui):
> >          if not ui.config(b'smtp', b'host'):
> >              raise error.Abort(
> >                  _(
> >                      b'smtp specified as email transport, '
> >                      b'but no smtp host configured'
> >                  )
> >              )
> >      else:
> > -        if not procutil.findexe(method):
> > +        program = method.split(None, 1)[0]
> 
> b''.split() will return an empty list, and it's probably better to use
> procutil.shellsplit().
> 
I did consider that, OTOH _sendmail already does the .split(None, 1)[0],
so I followed that pattern.  I guess I can change that one too...

Cheers,
Julien
Yuya Nishihara - Dec. 2, 2019, 1:08 p.m.
On Mon, 2 Dec 2019 11:52:53 +0100, Julien Cristau wrote:
> On Sat, Nov 30, 2019 at 01:40:53PM +0900, Yuya Nishihara wrote:
> > On Fri, 29 Nov 2019 19:20:50 +0100, Julien Cristau wrote:
> > > # HG changeset patch
> > > # User Julien Cristau <jcristau@debian.org>
> > > # Date 1575051591 -3600
> > > #      Fri Nov 29 19:19:51 2019 +0100
> > > # Node ID e962071e8bee30af8b3432e61b7f04acabbde0db
> > > # Parent  640bae94f2f31b1c1325ad2e5cf7f4a5a9dbebdd
> > > mail: don't complain about a multi-word email.method config value
> > > 
> > > I want to be able to set email.method to "ssh relay /usr/sbin/sendmail"
> > > without needing an extra trivial shell script.
> > > This works fine since we pass the full command to a shell, except for
> > > validateconfig trying to find it in $PATH.
> > > 
> > > diff --git a/mercurial/mail.py b/mercurial/mail.py
> > > --- a/mercurial/mail.py
> > > +++ b/mercurial/mail.py
> > > @@ -241,17 +241,18 @@ def validateconfig(ui):
> > >          if not ui.config(b'smtp', b'host'):
> > >              raise error.Abort(
> > >                  _(
> > >                      b'smtp specified as email transport, '
> > >                      b'but no smtp host configured'
> > >                  )
> > >              )
> > >      else:
> > > -        if not procutil.findexe(method):
> > > +        program = method.split(None, 1)[0]
> > 
> > b''.split() will return an empty list, and it's probably better to use
> > procutil.shellsplit().
> > 
> I did consider that, OTOH _sendmail already does the .split(None, 1)[0],
> so I followed that pattern.  I guess I can change that one too...

Good catch. I prefer fixing it, too.
Julien Cristau - Dec. 2, 2019, 1:52 p.m.
On Mon, Dec 02, 2019 at 10:08:39PM +0900, Yuya Nishihara wrote:
> On Mon, 2 Dec 2019 11:52:53 +0100, Julien Cristau wrote:
> > On Sat, Nov 30, 2019 at 01:40:53PM +0900, Yuya Nishihara wrote:
> > > b''.split() will return an empty list, and it's probably better to use
> > > procutil.shellsplit().
> > > 
> > I did consider that, OTOH _sendmail already does the .split(None, 1)[0],
> > so I followed that pattern.  I guess I can change that one too...
> 
> Good catch. I prefer fixing it, too.
> 
Re-sent as:
D7541 - created - 3f567faa8930: mail: use procutil.shellsplit instead of bytes.split to parse command
D7542 - created - 0e11d6856fdd: mail: don't complain about a multi-word email.method

Thanks for the review!
Julien

Patch

diff --git a/mercurial/mail.py b/mercurial/mail.py
--- a/mercurial/mail.py
+++ b/mercurial/mail.py
@@ -241,17 +241,18 @@  def validateconfig(ui):
         if not ui.config(b'smtp', b'host'):
             raise error.Abort(
                 _(
                     b'smtp specified as email transport, '
                     b'but no smtp host configured'
                 )
             )
     else:
-        if not procutil.findexe(method):
+        program = method.split(None, 1)[0]
+        if not procutil.findexe(program):
             raise error.Abort(
                 _(b'%r specified as email transport, but not in PATH') % method
             )
 
 
 def codec2iana(cs):
     # type: (str) -> str
     ''''''