launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Diff comments:

> === modified file 'utilities/shhh.py'
> --- utilities/shhh.py	2012-01-01 03:10:25 +0000
> +++ utilities/shhh.py	2017-09-04 11:58:53 +0000
> @@ -23,49 +24,62 @@
>      order may be messed up if the command attempts to control order by
>      flushing stdout at points or setting it to unbuffered.
>  
> -
>      To test, we invoke both this method and this script with some commands
> -    and examine the output and exitvalue
> +    and examine the output and exit status.
>  
>      >>> python = sys.executable
>  
>      >>> def shhh_script(cmd):
>      ...     from subprocess import Popen, PIPE
> -    ...     script = '%s %s' % (python, __file__)
> -    ...     cmd = "%s '%s'" % (script, cmd)
> -    ...     p = Popen(cmd, shell=True, stdout=PIPE, stderr=PIPE)
> +    ...     cmd = [python, __file__] + cmd
> +    ...     p = Popen(cmd, stdout=PIPE, stderr=PIPE)
>      ...     (out, err) = p.communicate()
>      ...     return (out, err, p.returncode)
>  
> -    >>> cmd = '''%s -c "import sys; sys.exit(%d)"''' % (python, 0)
> +    >>> cmd = [python, "-c", "import sys; sys.exit(0)"]
>      >>> shhh(cmd)
>      0
>      >>> shhh_script(cmd)
>      ('', '', 0)
>  
> -    >>> cmd = '''%s -c "import sys; sys.exit(%d)"''' % (python, 1)
> +    >>> cmd = [python, "-c", "import sys; sys.exit(1)"]
>      >>> shhh(cmd)
>      1
>      >>> shhh_script(cmd)
>      ('', '', 1)
>  
> -    >>> cmd = '''%s -c "import sys; print 666; sys.exit(%d)"''' % (
> -    ...     python, 42)
> +    >>> cmd = [python, "-c", "import sys; print 666; sys.exit(42)"]
>      >>> shhh(cmd)
>      666
>      42
>      >>> shhh_script(cmd)
>      ('666\n', '', 42)
>  
> -    >>> cmd = (
> -    ...     '''%s -c "import sys; print 666; '''
> -    ...     '''print >> sys.stderr, 667; sys.exit(42)"''' % python
> -    ...     )
> +    >>> cmd = [
> +    ...     python, "-c",
> +    ...     "import sys; print 666; print >> sys.stderr, 667; sys.exit(42)",
> +    ...     ]
>      >>> shhh_script(cmd)
>      ('666\n', '667\n', 42)
> +
> +    >>> cmd = ["TEST=sentinel value=0", "sh", "-c", 'echo "$TEST"; exit 1']
> +    >>> shhh(cmd)
> +    sentinel value=0
> +    1
> +    >>> shhh_script(cmd)
> +    ('sentinel value=0\n', '', 1)
>      """
>  
> -    process = Popen(cmd, stdout=PIPE, stderr=PIPE, shell=True)
> +    env = dict(os.environ)
> +    cmd = list(cmd)
> +    while cmd:
> +        if "=" in cmd[0]:
> +            name, value = cmd[0].split("=", 1)
> +            env[name] = value

We do not; the only uses of environment variables today are setting PYTHONPATH to the empty string and LPCONFIG to various identifier-like things.

In any case, the approach I've taken here avoids needing to deal with escaping.  Environment variable assignments are passed as single arguments, and we just split them on the first "=" and stuff them straight into the environment.  As long as shhh.py is invoked correctly to start with (i.e. just quoting anything that requires quoting), there are no further unescaping passes involved and so no need for escaping.  The "TEST=sentinel value=0" case above proves this - albeit only with a space, but I think that's all that's required.

> +            del cmd[0]
> +        else:
> +            break
> +    process = Popen(cmd, stdout=PIPE, stderr=PIPE, env=env)
>      (out, err) = process.communicate()
>      if process.returncode == 0:
>          return 0


-- 
https://code.launchpad.net/~cjwatson/launchpad/shhh-no-shell/+merge/330155
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.