launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



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 never have any particularly exotic escaping in envvars we give to it?

> +            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.