Tidy up subprocess_util calls ahead of subprocess bugfix

kart/branch.py

@@ -4,7 +4,7 @@
 import pygit2
 
 from .exceptions import InvalidOperation
-from .subprocess_util import run
+from .subprocess_util import run_then_exit
 from .output_util import dump_json_output
 from kart.cli_util import KartCommand
 
@@ -42,7 +42,7 @@ def branch(ctx, output_format, args):
                 f"Cannot delete the branch '{branch}' which you are currently on."
             )
 
-    run("git", ["-C", repo.path, "branch"] + list(args))
+    run_then_exit(["git", "-C", repo.path, "branch"] + list(args))
 
 
 def list_branches_json(repo):

kart/cli.py

@@ -24,7 +24,7 @@
 )
 from .context import Context
 from kart.parse_args import PreserveDoubleDash
-from kart.subprocess_util import run
+from kart.subprocess_util import run_then_exit
 
 MODULE_COMMANDS = {
     "annotations.cli": {"build-annotations"},
@@ -299,10 +299,10 @@ def git(ctx, args):
     """
     Run an arbitrary Git command, using kart's packaged Git
     """
-    params = []
+    repo_params = []
     if ctx.obj.user_repo_path:
-        params += ["-C", ctx.obj.user_repo_path]
-    run("git", [*params, *args])
+        repo_params = ["-C", ctx.obj.user_repo_path]
+    run_then_exit(["git", *repo_params, *args])
 
 
 @cli.command(context_settings=dict(ignore_unknown_options=True), hidden=True)
@@ -311,13 +311,11 @@ def git(ctx, args):
 def lfs(ctx, args):
     """
     Run an arbitrary Git LFS command, using Kart's packaged Git.
-    Git LFS is not yet packaged with Kart so this will not work unless your Kart environment has Git LFS installed.
     """
-    params = []
+    repo_params = []
     if ctx.obj.user_repo_path:
-        params += ["-C", ctx.obj.user_repo_path]
-    params += ["lfs"]
-    run("git", [*params, *args])
+        repo_params = ["-C", ctx.obj.user_repo_path]
+    run_then_exit(["git", *repo_params, "lfs", *args])
 
 
 @cli.command(

kart/log.py

@@ -9,7 +9,7 @@
 from kart.cli_util import OutputFormatType, tool_environment
 from kart.completion_shared import ref_or_repo_path_completer
 from kart.exceptions import NotYetImplemented, SubprocessError
-from kart.subprocess_util import run
+from kart.subprocess_util import run_then_exit
 from kart.key_filters import RepoKeyFilter
 from kart.output_util import dump_json_output
 from kart.parse_args import PreserveDoubleDash, parse_revisions_and_filters
@@ -266,8 +266,8 @@ def log(
     if output_type == "text":
         if fmt:
             options.append(f"--format={fmt}")
-        git_args = ["-C", repo.path, "log", *options, *commits, "--", *paths]
-        run("git", git_args)
+        cmd = ["git", "-C", repo.path, "log", *options, *commits, "--", *paths]
+        run_then_exit(cmd)
 
     elif output_type in ("json", "json-lines"):
         if kwargs.get("graph"):

kart/subprocess_util.py

@@ -6,7 +6,6 @@
 from asyncio.subprocess import PIPE
 from functools import partial
 
-from . import is_windows
 from .cli_util import tool_environment
 
 
@@ -135,7 +134,7 @@ def subprocess_tee(cmd, **kwargs):
     return return_code, stdout, stderr
 
 
-def run_with_capture(cmd, args, env):
+def run_with_capture_then_exit(cmd):
     # In testing, .run must be set to capture_output and so use PIPEs to communicate
     # with the process to run whereas in normal operation the standard streams of
     # this process are passed into subprocess.run.
@@ -144,33 +143,37 @@ def run_with_capture(cmd, args, env):
     # io wrapper.
     # This io wrapper is not compatible with the stdin= kwarg to .run - in that case
     # it gets treated as a file like object and fails.
-    p = subprocess.run([cmd] + args, capture_output=True, encoding="utf-8", env=env)
+    p = subprocess.run(
+        cmd,
+        capture_output=True,
+        encoding="utf-8",
+        env=tool_environment(os.environ),
+    )
     sys.stdout.write(p.stdout)
     sys.stdout.flush()
     sys.stderr.write(p.stderr)
     sys.stderr.flush()
     sys.exit(p.returncode)
 
 
-def run(cmd, args):
+def run_then_exit(cmd):
     """
-    Run a process and wait for it to exit, this is required
-    when in helper mode as using execvpe overwrites the process so
-    the caller can't be notified when the command is complete.
-
-    The subprocess uses this processes standard streams.
-
-    If called in test then use capture mode rather than passing in 'real' standard
-    streams.
+    Works like subprocess.run, but has the following three differences.
+    1. Simplified - unlike subprocess.run, you can't configure streams, env, encoding, etc.
+       The environment used is tool_environment(), which is generally the right one.
+    2. Kart exits as soon as the subprocess exits, with the same return code as the subprocess.
+    3. Changes behaviour during testing to buffer output using PIPEs instead of connecting stdout and
+       stderr directly. This means that the test harness can read the subprocess stdout and stderr exactly
+       as if Kart had written directly. The downside (the reason we don't run like this always) is that
+       it buffers all the output until the process has finished, so the user wouldn't see progress.
     """
-    env = tool_environment(os.environ)
     if "_KART_RUN_WITH_CAPTURE" in os.environ:
-        run_with_capture(cmd, args, env)
+        run_with_capture_then_exit(cmd)
     else:
         p = subprocess.run(
-            [cmd] + args,
+            cmd,
             encoding="utf-8",
-            env=env,
+            env=tool_environment(os.environ),
             stdin=sys.stdin,
             stdout=sys.stdout,
             stderr=sys.stderr,