Merge pull request #715 from koordinates/exec-comments

Exec refactor

.gitignore

@@ -76,6 +76,7 @@ pytest.xml
 docs/_build/
 *.code-workspace
 .vscode/
+.idea/
 
 *.p12
 kart/_env.py

kart/branch.py

@@ -4,7 +4,7 @@
 import pygit2
 
 from .exceptions import InvalidOperation
-from .exec import run_and_wait
+from .subprocess_util import run
 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_and_wait("git", ["git", "-C", repo.path, "branch"] + list(args))
+    run("git", ["-C", repo.path, "branch"] + list(args))
 
 
 def list_branches_json(repo):

kart/cli.py

@@ -22,7 +22,7 @@
     KartGroup,
 )
 from .context import Context
-from .exec import run_and_wait
+from kart.subprocess_util import run
 
 MODULE_COMMANDS = {
     "annotations.cli": {"build-annotations"},
@@ -275,10 +275,10 @@ def git(ctx, args):
     """
     Run an arbitrary Git command, using kart's packaged Git
     """
-    params = ["git"]
+    params = []
     if ctx.obj.user_repo_path:
         params += ["-C", ctx.obj.user_repo_path]
-    run_and_wait("git", [*params, *args])
+    run("git", [*params, *args])
 
 
 @cli.command(context_settings=dict(ignore_unknown_options=True), hidden=True)
@@ -289,11 +289,11 @@ 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 = ["git"]
+    params = []
     if ctx.obj.user_repo_path:
         params += ["-C", ctx.obj.user_repo_path]
     params += ["lfs"]
-    run_and_wait("git", [*params, *args])
+    run("git", [*params, *args])
 
 
 @cli.command(

kart/exec.py

@@ -1,30 +1,3 @@
-import os
-import subprocess
-import sys
 
-from . import is_windows
-from .cli_util import tool_environment
 
 
-def _kart_no_exec(cmd, args, env):
-    # used in testing. This is pretty hackzy
-    p = subprocess.run([cmd] + args[1:], capture_output=True, encoding="utf-8", env=env)
-    sys.stdout.write(p.stdout)
-    sys.stdout.flush()
-    sys.stderr.write(p.stderr)
-    sys.stderr.flush()
-    sys.exit(p.returncode)
-
-
-def run_and_wait(cmd, args):
-    """
-    run a process and wait for it to exit, this is required
-    when in helper mode as execvpe overwrites the process so
-    the caller can't be notified when the command is complete
-    """
-    env = tool_environment(os.environ)
-    if "_KART_NO_EXEC" in os.environ:
-        _kart_no_exec(cmd, args, env)
-    else:
-        p = subprocess.run([cmd] + args[1:], env=env, stdin=sys.stdin, stdout=sys.stdout, stderr=sys.stderr)
-        sys.exit(p.returncode)

kart/log.py

@@ -13,7 +13,7 @@
 )
 from kart.completion_shared import path_completer
 from kart.exceptions import NotYetImplemented, SubprocessError
-from kart.exec import run_and_wait
+from kart.subprocess_util import run
 from kart.key_filters import RepoKeyFilter
 from kart.output_util import dump_json_output
 from kart.parse_args import PreserveDoubleDash, parse_revisions_and_filters
@@ -259,8 +259,8 @@ def log(
     if output_type == "text":
         if fmt:
             options.append(f"--format={fmt}")
-        git_args = ["git", "-C", repo.path, "log", *options, *commits, "--", *paths]
-        run_and_wait("git", git_args)
+        git_args = ["-C", repo.path, "log", *options, *commits, "--", *paths]
+        run("git", git_args)
 
     elif output_type in ("json", "json-lines"):
         try:

kart/subprocess_util.py

@@ -1,10 +1,13 @@
 import asyncio
+import os
+import subprocess
 import sys
 from asyncio import IncompleteReadError, LimitOverrunError
 from asyncio.subprocess import PIPE
 from functools import partial
 
 from . import is_windows
+from .cli_util import tool_environment
 
 
 async def read_stream_and_display(stream, display):
@@ -138,3 +141,46 @@ def subprocess_tee(cmd, **kwargs):
         read_and_display(cmd, **kwargs)
     )
     return return_code, stdout, stderr
+
+
+def run_with_capture(cmd, args, env):
+    # 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.
+    # Capturing the output in a PIPE and then writing to sys.stdout is compatible
+    # with click.testing which sets sys.stdout and sys.stderr to a custom
+    # 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)
+    sys.stdout.write(p.stdout)
+    sys.stdout.flush()
+    sys.stderr.write(p.stderr)
+    sys.stderr.flush()
+    sys.exit(p.returncode)
+
+
+def run(cmd, args):
+    """
+    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.
+    """
+    env = tool_environment(os.environ)
+    if "_KART_RUN_WITH_CAPTURE" in os.environ:
+        run_with_capture(cmd, args, env)
+    else:
+        p = subprocess.run(
+            [cmd] + args,
+            encoding="utf-8",
+            env=env,
+            stdin=sys.stdin,
+            stdout=sys.stdout,
+            stderr=sys.stderr,
+        )
+        sys.exit(p.returncode)

tests/conftest.py

@@ -445,8 +445,8 @@ def isolation_pdb(self, env=None):
 def cli_runner(request):
     """A wrapper round Click's test CliRunner to improve usefulness"""
     return KartCliRunner(
-        # kart.cli._execvp() looks for this env var to prevent fork/exec in tests.
-        env={"_KART_NO_EXEC": "1"},
+        # kart.subprocess_util.run looks for this env var to ensure output is captured in tests.
+        env={"_KART_RUN_WITH_CAPTURE": "1"},
         # workaround Click's environment isolation so debugging works.
         in_pdb=request.config.getoption("--pdb-trace"),
     )