Tidy up exec.py, rename to subprocess.py

We never exec anyway so _KART_NO_EXEC is confusing.
But we do still need to do subprocess.run slightly differently during
tests, unless we figure out a better way to capture output.

This makes the names of things more in line with how they work,
and makes calls to kart/subprocess.py look and work identically to
calls to subprocess (until we turn on _KART_TEST).

(Ideally we would get rid of _KART_TEST and use fd-capturing,
but this will take a bit more expertise. However, this still gets
us a tiny bit closer to that goal).

kart/branch.py

@@ -3,10 +3,10 @@
 import click
 import pygit2
 
-from .exceptions import InvalidOperation
-from .exec import run_and_wait
-from .output_util import dump_json_output
-from kart.cli_util import KartCommand
+from kart import subprocess
+from kart.exceptions import InvalidOperation
+from kart.output_util import dump_json_output
+from kart.cli_util import KartCommand, tool_environment
 
 
 @click.command(cls=KartCommand, context_settings=dict(ignore_unknown_options=True))
@@ -42,7 +42,10 @@ 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))
+    p = subprocess.run(
+        ["git", "-C", repo.path, "branch", *args], env=tool_environment()
+    )
+    sys.exit(p.returncode)
 
 
 def list_branches_json(repo):

kart/cli.py

@@ -4,25 +4,23 @@
 import inspect
 import logging
 import os
-import io
 import pathlib
 import re
-import subprocess
 import sys
 import traceback
 
 import click
 import pygit2
 
 from . import core, is_darwin, is_linux, is_windows  # noqa
-from .cli_util import (
+from kart.cli_util import (
     add_help_subcommand,
     call_and_exit_flag,
     tool_environment,
     KartGroup,
 )
-from .context import Context
-from .exec import run_and_wait
+from kart.context import Context
+from kart import subprocess
 
 MODULE_COMMANDS = {
     "annotations.cli": {"build-annotations"},
@@ -186,7 +184,7 @@ def cli(ctx, repo_dir, verbose, post_mortem):
         logging.getLogger("sqlalchemy.engine").setLevel("INFO")
 
 
-# straight process-replace commands
+# Straight delegate-to-subprocess commands.
 
 
 @cli.command(context_settings=dict(ignore_unknown_options=True))
@@ -201,14 +199,7 @@ def cli(ctx, repo_dir, verbose, post_mortem):
 @click.argument("args", nargs=-1, type=click.UNPROCESSED)
 def push(ctx, do_progress, args):
     """Update remote refs along with associated objects"""
-    ctx.invoke(
-        git,
-        args=[
-            "push",
-            "--progress" if do_progress else "--quiet",
-            *args,
-        ],
-    )
+    _run_git(ctx, ["push", "--progress" if do_progress else "--quiet", *args])
 
 
 @cli.command(context_settings=dict(ignore_unknown_options=True))
@@ -223,67 +214,63 @@ def push(ctx, do_progress, args):
 @click.argument("args", nargs=-1, type=click.UNPROCESSED)
 def fetch(ctx, do_progress, args):
     """Download objects and refs from another repository"""
-    ctx.invoke(
-        git,
-        args=[
-            "fetch",
-            "--progress" if do_progress else "--quiet",
-            *args,
-        ],
-    )
+    _run_git(ctx, ["fetch", "--progress" if do_progress else "--quiet", *args])
 
 
 @cli.command(context_settings=dict(ignore_unknown_options=True))
 @click.pass_context
 @click.argument("args", nargs=-1, type=click.UNPROCESSED)
 def remote(ctx, args):
     """Manage set of tracked repositories"""
-    ctx.invoke(git, args=["remote", *args])
+    _run_git(ctx, ["remote", *args])
 
 
 @cli.command(context_settings=dict(ignore_unknown_options=True))
 @click.pass_context
 @click.argument("args", nargs=-1, type=click.UNPROCESSED)
 def tag(ctx, args):
     """Create, list, delete or verify a tag object signed with GPG"""
-    ctx.invoke(git, args=["tag", *args])
+    _run_git(ctx, ["tag", *args])
 
 
 @cli.command(context_settings=dict(ignore_unknown_options=True))
 @click.pass_context
 @click.argument("args", nargs=-1, type=click.UNPROCESSED)
 def reflog(ctx, args):
     """Manage reflog information"""
-    ctx.invoke(git, args=["reflog", *args])
+    _run_git(ctx, ["reflog", *args])
 
 
 @cli.command(context_settings=dict(ignore_unknown_options=True))
 @click.pass_context
 @click.argument("args", nargs=-1, type=click.UNPROCESSED)
 def config(ctx, args):
     """Get and set repository or global options"""
-    ctx.invoke(git, args=["config", *args])
+    _run_git(ctx, ["config", *args])
 
 
 @cli.command(context_settings=dict(ignore_unknown_options=True))
 @click.pass_context
 @click.argument("args", nargs=-1, type=click.UNPROCESSED)
 def gc(ctx, args):
     """Cleanup unnecessary files and optimize the local repository"""
-    ctx.invoke(git, args=["gc", *args])
+    _run_git(ctx, ["gc", *args])
 
 
 @cli.command(context_settings=dict(ignore_unknown_options=True), hidden=True)
 @click.pass_context
 @click.argument("args", nargs=-1, type=click.UNPROCESSED)
 def git(ctx, args):
-    """
-    Run an arbitrary Git command, using kart's packaged Git
-    """
+    """Run an arbitrary Git command, using kart's packaged Git"""
+    _run_git(ctx, args)
+
+
+def _run_git(ctx, args):
     params = ["git"]
     if ctx.obj.user_repo_path:
         params += ["-C", ctx.obj.user_repo_path]
-    run_and_wait("git", [*params, *args])
+    p = subprocess.run([*params, *args], env=tool_environment())
+    sys.exit(p.returncode)
 
 
 @cli.command(context_settings=dict(ignore_unknown_options=True), hidden=True)
@@ -294,11 +281,12 @@ 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"]
-    if ctx.obj.user_repo_path:
-        params += ["-C", ctx.obj.user_repo_path]
-    params += ["lfs"]
-    run_and_wait("git", [*params, *args])
+    from kart.repo import KartRepoState
+
+    repo = ctx.obj.get_repo(allowed_states=KartRepoState.ALL_STATES)
+    kwargs = {"cwd": repo.workdir_path} if repo else {}
+    p = subprocess.run(["git-lfs", *args], **kwargs, env=tool_environment)
+    sys.exit(p.returncode)
 
 
 @cli.command(

kart/log.py

@@ -1,19 +1,17 @@
-import subprocess
 import sys
 import logging
 from datetime import datetime, timedelta, timezone
 
 import click
 
-from kart import diff_estimation
+from kart import diff_estimation, subprocess
 from kart.cli_util import (
     OutputFormatType,
     parse_output_format,
     tool_environment,
 )
 from kart.completion_shared import path_completer
 from kart.exceptions import NotYetImplemented, SubprocessError
-from kart.exec import run_and_wait
 from kart.key_filters import RepoKeyFilter
 from kart.output_util import dump_json_output
 from kart.parse_args import PreserveDoubleDash, parse_commits_and_filters
@@ -259,8 +257,9 @@ 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)
+        cmd = ["git", "-C", repo.path, "log", *options, *commits, "--", *paths]
+        r = subprocess.run(cmd, env=tool_environment())
+        sys.exit(r.returncode)
 
     elif output_type in ("json", "json-lines"):
         try:

kart/subprocess.py

@@ -0,0 +1,31 @@
+import os
+import sys
+import subprocess as s
+from subprocess import *  # noqa
+
+
+# kart.subprocess works exactly like subprocess unless the _KART_TEST environment variable is set.
+# In that case, kart.subprocess.run is actually run_for_test which makes sure the subprocess output
+# passes through sys.stdout and sys.stderr and is captured by CliRunner.
+
+
+def run(*args, **kwargs):
+    if "_KART_TEST" in os.environ:
+        return run_for_test(*args, **kwargs)
+    else:
+        return s.run(*args, **kwargs)
+
+
+def run_for_test(*args, **kwargs):
+    # Don't do this if we're doing anything complicated:
+    if "stdout" in kwargs or "stderr" in kwargs or "capture_output" in kwargs:
+        return s.run(*args, **kwargs)
+
+    # Helps CliRunner capture subprocess output during testing. This is pretty hacky.
+    # TODO - find out if there's a better way to let CliRunner capture subprocess output.
+    p = s.run(*args, **kwargs, capture_output=True, encoding="utf-8")
+    sys.stdout.write(p.stdout)
+    sys.stdout.flush()
+    sys.stderr.write(p.stderr)
+    sys.stderr.flush()
+    return p

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 looks for this env var to capture subprocess output
+        env={"_KART_TEST": "1"},
         # workaround Click's environment isolation so debugging works.
         in_pdb=request.config.getoption("--pdb-trace"),
     )