Merge "Rework run_shell_command"

2 files changed, 48 insertions, 65 deletions

diff --git a/pbr/packaging.py b/pbr/packaging.py
index fea9d10..8233dec 100644
--- a/pbr/packaging.py
+++ b/pbr/packaging.py
@@ -23,7 +23,6 @@ Utilities with minimum-depends for use in setup.py
 import email
 import os
 import re
-import shlex
 import subprocess
 import sys
 
@@ -82,33 +81,25 @@ def _parse_mailmap(mailmap_info):
     return mapping
 
 
-def _wrap_in_quotes(values):
-    return ["'%s'" % value for value in values]
-
-
-def _make_links_args(links):
-    return ["-f '%s'" % link for link in links]
-
-
 def _pip_install(links, requires, root=None, option_dict=dict()):
     if get_boolean_option(
             option_dict, 'skip_pip_install', 'SKIP_PIP_INSTALL'):
         return
-    root_cmd = ""
+    cmd = [sys.executable, '-m', 'pip.__init__', 'install']
     if root:
-        root_cmd = "--root=\"%s\"" % root
+        cmd.append("--root=%s" % root)
+    for link in links:
+        cmd.append("-f")
+        cmd.append(link)
     _run_shell_command(
-        "\"%s\" -m pip.__init__ install %s %s %s" % (
-            sys.executable,
-            root_cmd,
-            " ".join(links),
-            " ".join(_wrap_in_quotes(requires))),
+        cmd + requires,
         throw_on_error=True, buffer=False)
 
 
 def read_git_mailmap(root_dir=None, mailmap='.mailmap'):
     if not root_dir:
-        root_dir = _run_shell_command('git rev-parse --show-toplevel')
+        root_dir = _run_shell_command(
+            ['git', 'rev-parse', '--show-toplevel'])
 
     mailmap = os.path.join(root_dir, mailmap)
     if os.path.exists(mailmap):
@@ -203,6 +194,13 @@ def parse_dependency_links(requirements_files=None):
     return dependency_links
 
 
+def _run_git_command(cmd, git_dir, **kwargs):
+    if not isinstance(cmd, (list, tuple)):
+        cmd = [cmd]
+    return _run_shell_command(
+        ['git', '--git-dir=%s' % git_dir] + cmd, **kwargs)
+
+
 def _run_shell_command(cmd, throw_on_error=False, buffer=True):
     if buffer:
         out_location = subprocess.PIPE
@@ -211,11 +209,7 @@ def _run_shell_command(cmd, throw_on_error=False, buffer=True):
         out_location = None
         err_location = None
 
-    # shlex has issues with unicode on older versions
-    if sys.version_info < (2, 7):
-        cmd = cmd.encode('ascii')
-
-    output = subprocess.Popen(shlex.split(cmd),
+    output = subprocess.Popen(cmd,
                               stdout=out_location,
                               stderr=err_location)
     out = output.communicate()
@@ -228,7 +222,7 @@ def _run_shell_command(cmd, throw_on_error=False, buffer=True):
 
 
 def _get_git_directory():
-    return _run_shell_command("git rev-parse --git-dir", None)
+    return _run_shell_command(['git', 'rev-parse', '--git-dir'])
 
 
 def get_boolean_option(option_dict, option_name, env_name):
@@ -252,8 +246,7 @@ def write_git_changelog(git_dir=None, dest_dir=os.path.curdir,
         if git_dir is None:
             git_dir = _get_git_directory()
         if git_dir:
-            git_log_cmd = 'git --git-dir=\"%s\" log' % git_dir
-            changelog = _run_shell_command(git_log_cmd)
+            changelog = _run_git_command('log', git_dir)
             mailmap = read_git_mailmap()
             with open(new_changelog, "wb") as changelog_file:
                 changelog_file.write(canonicalize_emails(
@@ -279,14 +272,12 @@ def generate_authors(git_dir=None, dest_dir='.', option_dict=dict()):
             authors = []
 
             # don't include jenkins email address in AUTHORS file
-            git_log_cmd = ("git --git-dir=\"" + git_dir +
-                           "\" log --format='%aN <%aE>'")
-            authors += _run_shell_command(git_log_cmd).split('\n')
+            git_log_cmd = ['log', '--format=%aN <%aE>']
+            authors += _run_git_command(git_log_cmd, git_dir).split('\n')
             authors = [a for a in authors if not re.search(ignore_emails, a)]
 
             # get all co-authors from commit messages
-            co_authors_cmd = ("git log --git-dir=\"" + git_dir + "\"")
-            co_authors_out = _run_shell_command(co_authors_cmd)
+            co_authors_out = _run_git_command('log', git_dir)
             co_authors = re.findall('Co-authored-by:.+', co_authors_out,
                                     re.MULTILINE)
             co_authors = [signed.split(":", 1)[1].strip()
@@ -319,8 +310,7 @@ def _find_git_files(dirname='', git_dir=None):
         git_dir = _get_git_directory()
     if git_dir:
         log.info("[pbr] In git context, generating filelist from git")
-        git_ls_cmd = "git --git-dir=\"%s\" ls-files -z" % git_dir
-        file_list = _run_shell_command(git_ls_cmd)
+        file_list = _run_git_command(['ls-files', '-z'], git_dir)
         file_list = file_list.split(b'\x00'.decode('utf-8'))
     return [f for f in file_list if f]
 
@@ -364,9 +354,9 @@ class LocalInstall(install.install):
         option_dict = self.distribution.get_option_dict('pbr')
         if (not self.single_version_externally_managed
                 and self.distribution.install_requires):
-            links = _make_links_args(self.distribution.dependency_links)
             _pip_install(
-                links, self.distribution.install_requires, self.root,
+                self.distribution.dependency_links,
+                self.distribution.install_requires, self.root,
                 option_dict=option_dict)
 
         return du_install.install.run(self)
@@ -397,8 +387,7 @@ class _PipInstallTestRequires(object):
 
     def install_test_requirements(self):
 
-        links = _make_links_args(
-            parse_dependency_links(TEST_REQUIREMENTS_FILES))
+        links = parse_dependency_links(TEST_REQUIREMENTS_FILES)
         if self.distribution.tests_require:
             option_dict = self.distribution.get_option_dict('pbr')
             _pip_install(
@@ -742,14 +731,13 @@ def _get_revno(git_dir):
     tags then we fall back to counting commits since the beginning
     of time.
     """
-    describe = _run_shell_command(
-        "git --git-dir=\"%s\" describe --always" % git_dir)
+    describe = _run_git_command(['describe', '--always'], git_dir)
     if "-" in describe:
         return describe.rsplit("-", 2)[-2]
 
     # no tags found
-    revlist = _run_shell_command(
-        "git --git-dir=\"%s\" rev-list --abbrev-commit HEAD" % git_dir)
+    revlist = _run_git_command(
+        ['rev-list', '--abbrev-commit', 'HEAD'], git_dir)
     return len(revlist.splitlines())
 
 
@@ -763,18 +751,16 @@ def _get_version_from_git(pre_version):
     if git_dir:
         if pre_version:
             try:
-                return _run_shell_command(
-                    "git --git-dir=\"" + git_dir + "\" describe --exact-match",
+                return _run_git_command(
+                    ['describe', '--exact-match'], git_dir,
                     throw_on_error=True).replace('-', '.')
             except Exception:
-                sha = _run_shell_command(
-                    "git --git-dir=\"" + git_dir +
-                    "\" log -n1 --pretty=format:%h")
+                sha = _run_git_command(
+                    ['log', '-n1', '--pretty=format:%h'], git_dir)
                 return "%s.a%s.g%s" % (pre_version, _get_revno(git_dir), sha)
         else:
-            return _run_shell_command(
-                "git --git-dir=\"" + git_dir + "\" describe --always").replace(
-                    '-', '.')
+            return _run_git_command(
+                ['describe', '--always'], git_dir).replace('-', '.')
     return None
 
 
diff --git a/pbr/tests/test_setup.py b/pbr/tests/test_setup.py
index d54f4fd..189bd51 100644
--- a/pbr/tests/test_setup.py
+++ b/pbr/tests/test_setup.py
@@ -170,20 +170,15 @@ class GitLogsTest(tests.BaseTestCase):
         with open(os.path.join(self.temp_path, "ChangeLog"), "r") as ch_fh:
             self.assertTrue("email@foo.com" in ch_fh.read())
 
-    def _fake_log_output(self, cmd, mapping):
-        for (k, v) in mapping.items():
-            if cmd.startswith(k):
-                return v.encode('utf-8')
-        return b""
-
     def test_generate_authors(self):
-        author_old = "Foo Foo <email@foo.com>"
-        author_new = "Bar Bar <email@bar.com>"
-        co_author = "Foo Bar <foo@bar.com>"
-        co_author_by = "Co-authored-by: " + co_author
-
-        git_log_cmd = ("git --git-dir=%s log --format" % self.git_dir)
-        git_co_log_cmd = ("git log --git-dir=%s" % self.git_dir)
+        author_old = u"Foo Foo <email@foo.com>"
+        author_new = u"Bar Bar <email@bar.com>"
+        co_author = u"Foo Bar <foo@bar.com>"
+        co_author_by = u"Co-authored-by: " + co_author
+
+        git_log_cmd = (
+            "git --git-dir=%s log --format=%%aN <%%aE>" % self.git_dir)
+        git_co_log_cmd = ("git --git-dir=%s log" % self.git_dir)
         git_top_level = "git rev-parse --show-toplevel"
         cmd_map = {
             git_log_cmd: author_new,
@@ -197,10 +192,12 @@ class GitLogsTest(tests.BaseTestCase):
             "os.path.exists",
             lambda path: os.path.abspath(path) in exist_files))
 
-        self.useFixture(fixtures.FakePopen(lambda proc_args: {
-            "stdout": BytesIO(
-                self._fake_log_output(' '.join(proc_args["args"]), cmd_map))
-        }))
+        def _fake_run_shell_command(cmd, **kwargs):
+            return cmd_map[" ".join(cmd)]
+
+        self.useFixture(fixtures.MonkeyPatch(
+            "pbr.packaging._run_shell_command",
+            _fake_run_shell_command))
 
         with open(os.path.join(self.temp_path, "AUTHORS.in"), "w") as auth_fh:
             auth_fh.write("%s\n" % author_old)