Merge pull request #856 from koordinates/connect-stdin-stdout-stderr

Explicitly connect stdin, stdout, stderr

CHANGELOG.md

@@ -16,6 +16,7 @@ _When adding new entries to the changelog, please include issue/PR numbers where
 - Point clouds (or rasters): require `--no-convert-to-dataset-format` to add new tiles to a dataset that can be committed as-is but only if the dataset's format is changed. Note `--convert-to-dataset-format` works as before. [#842](https://github.com/koordinates/kart/issues/842)
 - Allow kart to check out attached files into the file-based working copy.
 - Fixes a bug where tab-completion wasn't working with helper mode, affects macOS and Linux. [#851](https://github.com/koordinates/kart/pull/851)
+- Fixes a bug where git-lfs (and potentially other subprocesses) wasn't working reliably with helper mode, affects macOS and Linux. [#853](https://github.com/koordinates/kart/issues/853)
 
 ## 0.12.3
 

cli_helper/kart.c

@@ -191,30 +191,6 @@ int is_helper_enabled()
     return (env == NULL || *env != '0');
 }
 
-/**
- * @brief Check whether using the helper is allowed for this kart command.
- * the helper is disallowed for certain commands since they need to be able to launch an editor.
- * Launching an editor from the helper doesn't work since it's not properly attached to the terminal.
- * @return 0 no, 1 yes
- */
-int is_helper_allowed(char **argv) {
-    char **arg_ptr;
-    for (arg_ptr = argv; *arg_ptr != NULL; arg_ptr++) {
-        if (strncmp(*arg_ptr, "-", 1) == 0 || strcmp(*arg_ptr, "kart") == 0) {
-            // Skip options.
-            continue;
-        }
-        // kart commit, kart merge, and kart pull can all spawn an editor for the commit message.
-        if (strcmp(*arg_ptr, "commit") == 0 ||
-            strcmp(*arg_ptr, "merge") == 0 ||
-            strcmp(*arg_ptr, "pull") == 0) {
-            return 0;
-        }
-        return 1;
-    }
-    return 1;
-}
-
 /**
  * @brief Exit signal handler for SIGALRM
  */
@@ -241,7 +217,7 @@ int main(int argc, char **argv, char **environ)
         exit(1);
     }
 
-    if (is_helper_enabled() && is_helper_allowed(argv))
+    if (is_helper_enabled())
     {
         debug("enabled %s, pid=%d\n", cmd_path, getpid());
 

kart/branch.py

@@ -3,9 +3,9 @@
 import click
 import pygit2
 
-from .exceptions import InvalidOperation
-from .subprocess_util import run_then_exit
-from .output_util import dump_json_output
+from kart.exceptions import InvalidOperation
+from kart import subprocess_util as subprocess
+from kart.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_then_exit(["git", "-C", repo.path, "branch"] + list(args))
+    subprocess.run_then_exit(["git", "-C", repo.path, "branch"] + list(args))
 
 
 def list_branches_json(repo):

kart/checkout.py

@@ -1,22 +1,22 @@
 import functools
-import subprocess
 
 import click
 import pygit2
-from .completion_shared import ref_completer
 
-from .cli_util import tool_environment
-from .exceptions import (
+from kart.cli_util import KartCommand
+from kart.completion_shared import ref_completer
+from kart.exceptions import (
     NO_BRANCH,
     NO_COMMIT,
     InvalidOperation,
     NotFound,
 )
-from .key_filters import RepoKeyFilter
-from .promisor_utils import get_partial_clone_envelope
-from .spatial_filter import SpatialFilterString, spatial_filter_help_text
-from .structs import CommitWithReference
-from kart.cli_util import KartCommand
+from kart.key_filters import RepoKeyFilter
+from kart.promisor_utils import get_partial_clone_envelope
+from kart.spatial_filter import SpatialFilterString, spatial_filter_help_text
+from kart.structs import CommitWithReference
+from kart import subprocess_util as subprocess
+
 
 _DISCARD_CHANGES_HELP_MESSAGE = (
     "Commit these changes first (`kart commit`) or"
@@ -193,7 +193,6 @@ def checkout(
 def _git_fetch_supports_flag(repo, flag):
     r = subprocess.run(
         ["git", "fetch", "?", f"--{flag}"],
-        env=tool_environment(),
         cwd=repo.workdir_path,
         capture_output=True,
         text=True,

kart/cli.py

@@ -7,7 +7,6 @@
 from multiprocessing import freeze_support
 import pathlib
 import re
-import subprocess
 import sys
 import traceback
 from pathlib import Path
@@ -16,15 +15,14 @@
 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 kart.context import Context
 from kart.parse_args import PreserveDoubleDash
-from kart.subprocess_util import run_then_exit
+from kart import subprocess_util as subprocess
 
 MODULE_COMMANDS = {
     "annotations.cli": {"build-annotations"},
@@ -99,21 +97,19 @@ def print_version(ctx):
     click.echo(f"Kart v{get_version()}, Copyright (c) Kart Contributors")
 
     git_version = (
-        subprocess.check_output(["git", "--version"], env=tool_environment())
+        subprocess.check_output(["git", "--version"])
         .decode("ascii")
         .strip()
         .split()[-1]
     )
 
     gitlfs_version = re.match(
         r"git-lfs/([^ ]+) \(",
-        subprocess.check_output(
-            ["git-lfs", "version"], env=tool_environment(), text=True
-        ),
+        subprocess.check_output(["git-lfs", "version"], text=True),
     ).group(1)
 
     pdal_version = (
-        subprocess.check_output(["pdal", "--version"], env=tool_environment())
+        subprocess.check_output(["pdal", "--version"])
         .decode("ascii")
         .strip()
         .split()[2]
@@ -302,7 +298,7 @@ def git(ctx, args):
     repo_params = []
     if ctx.obj.user_repo_path:
         repo_params = ["-C", ctx.obj.user_repo_path]
-    run_then_exit(["git", *repo_params, *args])
+    subprocess.run_then_exit(["git", *repo_params, *args])
 
 
 @cli.command(context_settings=dict(ignore_unknown_options=True), hidden=True)
@@ -315,7 +311,7 @@ def lfs(ctx, args):
     repo_params = []
     if ctx.obj.user_repo_path:
         repo_params = ["-C", ctx.obj.user_repo_path]
-    run_then_exit(["git", *repo_params, "lfs", *args])
+    subprocess.run_then_exit(["git", *repo_params, "lfs", *args])
 
 
 @cli.command(

kart/cli_util.py

@@ -1,20 +1,16 @@
-import functools
 import io
 import json
-import os
 import logging
 import platform
-import warnings
 import shutil
-import subprocess
-import sys
 from pathlib import Path
 
 import click
-import pygit2
 from click.core import Argument
 from click.shell_completion import CompletionItem
 
+from kart import subprocess_util as subprocess
+
 L = logging.getLogger("kart.cli_util")
 
 
@@ -111,107 +107,6 @@ def render_windows(command_path: str) -> bytes:
     click.echo_via_pager(text_page.read_text())
 
 
-def _pygit2_configs():
-    """
-    Yields pygit2.Config objects in order of decreasing specificity.
-    """
-    try:
-        # ~/.gitconfig
-        yield pygit2.Config.get_global_config()
-    except OSError:
-        pass
-    try:
-        # ~/.config/git/config
-        yield pygit2.Config.get_xdg_config()
-    except OSError:
-        pass
-
-    if "GIT_CONFIG_NOSYSTEM" not in os.environ:
-        # /etc/gitconfig
-        try:
-            yield pygit2.Config.get_system_config()
-        except OSError:
-            pass
-
-
-# These are all the Kart defaults that differ from git's defaults.
-# (all of these can still be overridden by setting them in a git config file.)
-GIT_CONFIG_DEFAULT_OVERRIDES = {
-    # git will change to this branch sooner or later, but hasn't yet.
-    "init.defaultBranch": "main",
-    # Deltified objects seem to affect clone and diff performance really badly
-    # for Kart repos. So we disable them by default.
-    "pack.depth": 0,
-    "pack.window": 0,
-}
-if platform.system() == "Linux":
-    import certifi
-
-    GIT_CONFIG_DEFAULT_OVERRIDES["http.sslCAInfo"] = certifi.where()
-
-# These are the settings that Kart always *overrides* in git config.
-# i.e. regardless of your local git settings, kart will use these settings instead.
-GIT_CONFIG_FORCE_OVERRIDES = {
-    # We use base64 for feature paths.
-    # "kcya" and "kcyA" are *not* the same feature; that way lies data loss
-    "core.ignoreCase": "false",
-}
-
-
-# from https://github.com/git/git/blob/ebf3c04b262aa27fbb97f8a0156c2347fecafafb/quote.c#L12-L44
-def _git_sq_quote_buf(src):
-    dst = src.replace("'", r"'\''").replace("!", r"'\!'")
-    return f"'{dst}'"
-
-
-_ORIG_GIT_CONFIG_PARAMETERS = os.environ.get("GIT_CONFIG_PARAMETERS")
-
-
-@functools.lru_cache()
-def init_git_config():
-    """
-    Initialises default config values that differ from git's defaults.
-    """
-    configs = list(_pygit2_configs())
-    new_config_params = []
-    for k, v in GIT_CONFIG_DEFAULT_OVERRIDES.items():
-        for config in configs:
-            if k in config:
-                break
-        else:
-            new_config_params.append(_git_sq_quote_buf(f"{k}={v}"))
-    for k, v in GIT_CONFIG_FORCE_OVERRIDES.items():
-        new_config_params.append(_git_sq_quote_buf(f"{k}={v}"))
-
-    if new_config_params:
-        os.environ["GIT_CONFIG_PARAMETERS"] = " ".join(
-            filter(None, [*new_config_params, _ORIG_GIT_CONFIG_PARAMETERS])
-        )
-
-
-def tool_environment(env=None):
-    """
-    Returns a dict of environment for launching an external process
-    """
-    init_git_config()
-    env = (env or os.environ).copy()
-
-    # Add kart bin directory to the start of the path:
-    kart_bin_path = str(Path(sys.executable).parents[0])
-    if "PATH" in env:
-        env["PATH"] = kart_bin_path + os.pathsep + env["PATH"]
-    else:
-        env["PATH"] = kart_bin_path
-
-    if platform.system() == "Linux":
-        # https://pyinstaller.readthedocs.io/en/stable/runtime-information.html#ld-library-path-libpath-considerations
-        if "LD_LIBRARY_PATH_ORIG" in env:
-            env["LD_LIBRARY_PATH"] = env["LD_LIBRARY_PATH_ORIG"]
-        else:
-            env.pop("LD_LIBRARY_PATH", None)
-    return env
-
-
 def add_help_subcommand(group):
     @group.command(add_help_option=False, hidden=True)
     @click.argument("topic", default=None, required=False, nargs=1)

kart/commit.py

@@ -2,34 +2,33 @@
 import re
 import shlex
 import shutil
-import subprocess
 import sys
 from datetime import datetime, timedelta, timezone
 
 import click
 
+from kart import is_windows
+from kart.base_diff_writer import BaseDiffWriter
+from kart.cli_util import StringFromFile, KartCommand
+from kart.core import check_git_user
 from kart.diff_format import DiffFormat
-
-from . import is_windows
-from .base_diff_writer import BaseDiffWriter
-from .cli_util import StringFromFile, tool_environment, KartCommand
-from .core import check_git_user
-from .exceptions import (
+from kart.exceptions import (
     NO_CHANGES,
     SPATIAL_FILTER_CONFLICT,
     InvalidOperation,
     NotFound,
     SubprocessError,
 )
-from .key_filters import RepoKeyFilter
-from .output_util import dump_json_output
-from .repo import KartRepoFiles
-from .status import (
+from kart.key_filters import RepoKeyFilter
+from kart.output_util import dump_json_output
+from kart.repo import KartRepoFiles
+from kart.status import (
     diff_status_to_text,
     get_branch_status_message,
     get_diff_status_message,
 )
-from .timestamps import (
+from kart import subprocess_util as subprocess
+from kart.timestamps import (
     commit_time_to_text,
     datetime_to_iso8601_utc,
     timedelta_to_iso8601_tz,
@@ -269,7 +268,7 @@ def user_edit_file(path):
 
 
 def run_editor_cmd(editor_cmd):
-    subprocess.check_call(editor_cmd, shell=True, env=tool_environment())
+    subprocess.check_call(editor_cmd, shell=True)
 
 
 def commit_obj_to_json(commit, repo, wc_diff):

kart/completion.py

@@ -2,14 +2,16 @@
 import re
 import sys
 import platform
-import subprocess
 from enum import Enum
 from pathlib import Path
 from typing import Optional, Tuple, Optional, Any, Dict, List
 
 import click
 from click.shell_completion import _SOURCE_BASH, _SOURCE_ZSH, _SOURCE_FISH
 
+from kart import subprocess_util as subprocess
+
+
 try:
     import shellingham
 except ImportError:

kart/diff_estimation.py

@@ -1,10 +1,10 @@
-import subprocess
 import threading
 
 import pygit2
 
 from kart.diff_util import get_dataset_diff
 from kart.exceptions import SubprocessError
+from kart import subprocess_util as subprocess
 
 ACCURACY_SUBTREE_SAMPLES = {
     "veryfast": 2,