[update-checkout] refactor shell invocations

utils/swift_build_support/swift_build_support/shell.py

@@ -209,50 +209,3 @@ def remove(path, dry_run=None, echo=True):
     if dry_run:
         return
     os.remove(path)
-
-
-# Initialized later
-lock = None
-
-
-def run(*args, **kwargs):
-    repo_path = kwargs.pop('repo_path', os.getcwd())
-    echo_output = kwargs.pop('echo', False)
-    dry_run = kwargs.pop('dry_run', False)
-    env = kwargs.get('env', None)
-    prefix = kwargs.pop('prefix', '')
-    if dry_run:
-        _echo_command(dry_run, *args, env=env, prompt="{0}+ ".format(prefix))
-        return (None, 0, args)
-
-    my_pipe = subprocess.Popen(
-        *args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
-        universal_newlines=True,
-        encoding='utf-8',
-        **kwargs)
-    (output, _) = my_pipe.communicate()
-    output = output.encode(encoding='ascii', errors='replace')
-    ret = my_pipe.wait()
-
-    if lock:
-        lock.acquire()
-    if echo_output:
-        sys.stdout.flush()
-        sys.stderr.flush()
-        _echo_command(dry_run, *args, env=env, prompt="{0}+ ".format(prefix))
-        if output:
-            for line in output.splitlines():
-                print("{0}{1}".format(prefix, line.decode('utf-8', errors='replace')))
-        sys.stdout.flush()
-        sys.stderr.flush()
-    if lock:
-        lock.release()
-
-    if ret != 0:
-        eout = Exception(f"[{repo_path}] '{args}' failed with '{output.decode('utf-8')}'")
-        eout.ret = ret
-        eout.arguments = args
-        eout.repo_path = repo_path
-        eout.stderr = output
-        raise eout
-    return (output, 0, args)

utils/update_checkout/tests/test_clone.py

@@ -41,10 +41,7 @@ class CloneTestCase(scheme_mock.SchemeMockTestCase):
                             '--verbose'])
 
         # Test that we're actually checking out the 'extra' scheme based on the output
-        self.assertIn(
+        self.assertIn("git checkout refs/heads/main", output.decode("utf-8"))
-            f"git -C {os.path.join(self.source_root, 'repo1')} checkout refs/heads/main",
-            output.decode("utf-8"),
-        )
 
     def test_manager_not_called_on_long_socket(self):
         fake_tmpdir = '/tmp/very/' + '/long' * 20 + '/tmp'

utils/update_checkout/update_checkout/git_command.py

@@ -1,37 +1,90 @@
-from typing import List, Any, Optional, Union
+import shlex
-
+import subprocess
-from swift_build_support.swift_build_support import shell
+import sys
+from typing import List, Any, Optional, Dict
 
 
 class Git:
     @staticmethod
-    def run(repo_path: Optional[str], args: List[str], **kwargs):
+    def run(
-        cmd = ["git"]
-        if repo_path is not None:
-            cmd += ["-C", repo_path]
-        kwargs["repo_path"] = repo_path
-        # FIXME: The way we are passing args below is broken. shell.run takes 
-        # *args as list arguments and sometimes treats them as one positional 
-        # argument or as a list of arguments.
-        return shell.run(cmd+args, **kwargs)
-
-    @staticmethod
-    def capture(
         repo_path: str,
         args: List[str],
-        stderr=None,
+        echo: bool = False,
-        env=None,
+        env: Optional[Dict[str, Any]] = None,
-        dry_run=None,
+        prefix: str = "",
-        echo=True,
+        allow_non_zero_exit: bool = False,
-        optional=False,
+        fatal: bool = False,
-        allow_non_zero_exit=False,
+        **kwargs,
-    ) -> Union[str, Any, None]:
+    ):
-        return shell.capture(
+        command = Git._build_command(args)
-            ["git", "-C", repo_path] + args,
+
-            stderr=stderr,
+        try:
-            env=env,
+            result = subprocess.run(
-            dry_run=dry_run,
+                command,
-            echo=echo,
+                stdout=subprocess.PIPE,
-            optional=optional,
+                stderr=subprocess.STDOUT,
-            allow_non_zero_exit=allow_non_zero_exit,
+                universal_newlines=True,
-        )
+                encoding="utf-8",
+                env=env,
+                cwd=repo_path,
+                **kwargs,
+            )
+            output = result.stdout
+            if echo:
+                Git._echo_command(command, output, env, prefix)
+            if not allow_non_zero_exit:
+                result.check_returncode()
+        except subprocess.CalledProcessError as e:
+            if fatal:
+                sys.exit(
+                    f"command `{command}` terminated with a non-zero exit "
+                    f"status {str(e.returncode)}, aborting")
+            eout = Exception(
+                f"[{repo_path}] '{Git._quote_command(command)}' failed with '{output}'"
+            )
+            eout.ret = e.returncode
+            eout.arguments = command
+            eout.repo_path = repo_path
+            eout.stderr = output
+            raise eout
+        except OSError as e:
+            if fatal:
+                sys.exit(
+                    f"could not execute '{Git._quote_command(command)}': "
+                    f"{e.strerror}"
+                )
+        return (output.strip(), result.returncode, command)
+
+    @staticmethod
+    def _echo_command(
+        command: List[str],
+        output: Optional[str] = None,
+        env: Optional[Dict[str, Any]] = None,
+        prefix: str = "",
+    ):
+        sys.stdout.flush()
+        sys.stderr.flush()
+        command_str = []
+        if env is not None:
+            command_str += ["env"] + [
+                Git._quote(f"{k}={v}") for (k, v) in sorted(env.items())
+            ]
+        command_str.append(Git._quote_command(command))
+        print(f"{prefix}+ {' '.join(command_str)}", file=sys.stderr)
+        if output:
+            for line in output.splitlines():
+                print(prefix+line)
+        sys.stdout.flush()
+        sys.stderr.flush()
+
+    @staticmethod
+    def _build_command(args: List[str]) -> List[str]:
+        return ["git"] + args
+
+    @staticmethod
+    def _quote(arg: Any) -> str:
+        return shlex.quote(str(arg))
+
+    @staticmethod
+    def _quote_command(command: Any) -> str:
+        return " ".join(Git._quote(arg) for arg in command)

utils/update_checkout/update_checkout/parallel_runner.py

@@ -31,7 +31,7 @@ class TaskTracker:
         self._done_task_counter += 1
         self._lock.release()
 
-    def status(self) -> Tuple[List[str], int]:
+    def status(self) -> Tuple[str, int]:
         self._lock.acquire()
         running_tasks_str = ", ".join(self.running_tasks)
         done_tasks = self.done_task_counter

utils/update_checkout/update_checkout/update_checkout.py

@@ -42,13 +42,13 @@ def confirm_tag_in_repo(repo_path: str, tag: str, repo_name: str) -> Optional[st
         exist.
     """
 
-    tag_exists = Git.capture(
+    tag_exists, _, _ = Git.run(
-        repo_path, ["ls-remote", "--tags", "origin", tag], echo=False
+        repo_path, ["ls-remote", "--tags", "origin", tag], fatal=True
     )
     if not tag_exists:
         print("Tag '" + tag + "' does not exist for '" +
               repo_name + "', just updating regularly")
-        tag = None
+        return None
     return tag
 
 
@@ -61,7 +61,7 @@ def find_rev_by_timestamp(repo_path: str, timestamp: str, repo_name: str, refspe
     args = ["log", "-1", "--format=%H", "--first-parent", "--before=" + timestamp]
     if refspec_exists:
         args.append(refspec)
-    rev = Git.capture(repo_path, args).strip()
+    rev, _, _ = Git.run(repo_path, args, fatal=True)
     if rev:
         return rev
     else:
@@ -98,7 +98,7 @@ def get_branch_for_repo(repo_path, config, repo_name, scheme_name, scheme_map,
             pr_id = cross_repos_pr[remote_repo_id]
             repo_branch = "ci_pr_{0}".format(pr_id)
             Git.run(repo_path, ["checkout", scheme_branch], echo=True)
-            Git.capture(repo_path, ["branch", "-D", repo_branch], echo=True, allow_non_zero_exit=True)
+            Git.run(repo_path, ["branch", "-D", repo_branch], echo=True, allow_non_zero_exit=True, fatal=True)
             Git.run(repo_path, ["fetch", "origin", "pull/{0}/merge:{1}".format(pr_id, repo_branch), "--tags"], echo=True)
     return repo_branch, cross_repo
 
@@ -172,7 +172,7 @@ def update_single_repository(pool_args: UpdateArguments):
                 pass
 
         if checkout_target:
-            Git.run(repo_path, ['status', '--porcelain', '-uno'], echo=False)
+            Git.run(repo_path, ['status', '--porcelain', '-uno'])
 
             # Some of the projects switch branches/tags when they
             # are updated. Local checkout might not have that tag/branch
@@ -199,8 +199,7 @@ def update_single_repository(pool_args: UpdateArguments):
                 )
             except Exception:
                 try:
-                    result = Git.run(repo_path, ["rev-parse", checkout_target])
+                    revision, _, _ = Git.run(repo_path, ["rev-parse", checkout_target])
-                    revision = result[0].strip()
                     Git.run(
                         repo_path, ["checkout", revision], echo=verbose, prefix=prefix
                     )
@@ -233,7 +232,7 @@ def update_single_repository(pool_args: UpdateArguments):
             # This git command returns error code 1 if HEAD is detached.
             # Otherwise there was some other error, and we need to handle
             # it like other command errors.
-            Git.run(repo_path, ["symbolic-ref", "-q", "HEAD"], echo=False)
+            Git.run(repo_path, ["symbolic-ref", "-q", "HEAD"])
         except Exception as e:
             if e.ret == 1:
                 detached_head = True
@@ -286,9 +285,8 @@ def get_timestamp_to_match(match_timestamp, source_root):
     if not match_timestamp:
         return None
     swift_repo_path = os.path.join(source_root, "swift")
-    return Git.capture(
+    output, _, _ = Git.run(swift_repo_path, ["log", "-1", "--format=%cI"], fatal=True)
-        swift_repo_path, ["log", "-1", "--format=%cI"], echo=False
+    return output
-    ).strip()
 
 
 def get_scheme_map(config, scheme_name):
@@ -410,22 +408,19 @@ def obtain_additional_swift_sources(pool_args: AdditionalSwiftSourcesArguments):
         print("Cloning '" + pool_args.repo_name + "'")
 
     if pool_args.skip_history:
-        Git.run(None, ['clone', '--recursive', '--depth', '1',
+        Git.run(args.source_root, ['clone', '--recursive', '--depth', '1',
                     '--branch', repo_branch, remote, repo_name] +
                     (['--no-tags'] if skip_tags else []),
-                    cwd=args.source_root,
                     env=env,
                     echo=verbose)
     elif pool_args.use_submodules:
-        Git.run(None, ['submodule', 'add', remote, repo_name] +
+        Git.run(args.source_root, ['submodule', 'add', remote, repo_name] +
                     (['--no-tags'] if skip_tags else []),
-                    cwd=args.source_root,
                     env=env,
                     echo=verbose)
     else:
-        Git.run(None, ['clone', '--recursive', remote, repo_name] +
+        Git.run(args.source_root, ['clone', '--recursive', remote, repo_name] +
                     (['--no-tags'] if skip_tags else []),
-                    cwd=args.source_root,
                     env=env,
                     echo=verbose)
 
@@ -433,12 +428,11 @@ def obtain_additional_swift_sources(pool_args: AdditionalSwiftSourcesArguments):
     if pool_args.scheme_name:
         src_path = os.path.join(repo_path, ".git")
         Git.run(
-            None,
+            args.source_root,
             ["--git-dir", src_path, "--work-tree", repo_path, "checkout", repo_branch],
             env=env,
-            echo=False,
         )
-    Git.run(repo_path, ["submodule", "update", "--recursive"], env=env, echo=False)
+    Git.run(repo_path, ["submodule", "update", "--recursive"], env=env)
 
 
 def obtain_all_additional_swift_sources(args, config, with_ssh, scheme_name,
@@ -455,8 +449,7 @@ def obtain_all_additional_swift_sources(args, config, with_ssh, scheme_name,
 
         if use_submodules:
             repo_exists = False
-            submodules_status = Git.capture(repo_path, ['submodule', 'status'],
+            submodules_status, _, _ = Git.run(repo_path, ['submodule', 'status'], fatal=True)
-                                            echo=False)
             if submodules_status:
                 for line in submodules_status.splitlines():
                     if line[0].endswith(repo_name):
@@ -557,7 +550,7 @@ def repo_hashes(args, config):
     for repo_name, _ in sorted(config['repos'].items(), key=lambda x: x[0]):
         repo_path = os.path.join(args.source_root, repo_name)
         if os.path.exists(repo_path):
-            h = Git.capture(repo_path, ["rev-parse", "HEAD"], echo=False).strip()
+            h, _, _ = Git.run(repo_path, ["rev-parse", "HEAD"], fatal=True)
         else:
             h = "skip"
         repos[repo_name] = str(h)
@@ -633,11 +626,12 @@ def validate_config(config: Dict[str, Any]):
 
 
 def full_target_name(repo_path, repository, target):
-    tag = Git.capture(repo_path, ["tag", "-l", target], echo=False).strip()
+    tag, _, _ = Git.run(repo_path, ["tag", "-l", target], fatal=True)
     if tag == target:
         return tag
 
-    branch = Git.capture(repo_path, ["branch", "--list", target], echo=False).strip().replace("* ", "")
+    branch, _, _ = Git.run(repo_path, ["branch", "--list", target], fatal=True)
+    branch = branch.replace("* ", "")
     if branch == target:
         name = "%s/%s" % (repository, target)
         return name