Merge pull request #859 from koordinates/fix-git-config

Don't modify os.environ when calling subprocesses...

kart/subprocess_util.py

@@ -295,22 +295,14 @@ def tool_environment(*, base_env=None, env_overrides=None):
     Sets the PATH, GIT_CONFIG_PARAMETERS, etc appropriately.
 
     base_env - the environment to start from, defaults to os.environ if not set.
-    env_overrides - any extra environment variables to add after the tool-environment
+    env_overrides - any caller-provided extra environment variables to add after the tool-environment
+        is configured.
     """
-    # Adds our GIT_CONFIG_PARAMETERS to os.environ, so that if we launch git as a subprocess, it will have the config we want.
-    # TODO - a bit strange that this function modifies both the actual os.environ and the tool_environment that generally inherits
-    # from it - we should stick to modifying one or the other.
-    init_git_config()
-    # TODO - tool_environment would be easier to use if you could supply it with a dict of extra environment variables that
-    # you need, instead of having to supply the entire env.
     env = (base_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
+    tool_env_overrides = get_tool_environment_overrides()
+    _merge_env_variable(env, "PATH", tool_env_overrides, env, os.pathsep)
+    _merge_env_variable(env, "GIT_CONFIG_PARAMETERS", tool_env_overrides, env, " ")
 
     if platform.system() == "Linux":
         # https://pyinstaller.readthedocs.io/en/stable/runtime-information.html#ld-library-path-libpath-considerations
@@ -324,7 +316,29 @@ def tool_environment(*, base_env=None, env_overrides=None):
     return env
 
 
-_ORIG_GIT_CONFIG_PARAMETERS = os.environ.get("GIT_CONFIG_PARAMETERS")
+def _merge_env_variable(output_dict, key, lhs_dict, rhs_dict, separator):
+    lhs_value = lhs_dict.get(key)
+    rhs_value = rhs_dict.get(key)
+    if lhs_value or rhs_value:
+        output_dict[key] = _merge_env_variable_value(lhs_value, rhs_value, separator)
+
+
+def _merge_env_variable_value(lhs_value, rhs_value, separator):
+    if not lhs_value:
+        return rhs_value
+    if not rhs_value:
+        return lhs_value
+    return f"{lhs_value}{separator}{rhs_value}"
+
+
+@functools.lru_cache(maxsize=1)
+def get_tool_environment_overrides():
+    return {
+        # Add kart bin directory to the start of the path:
+        "PATH": str(Path(sys.executable).parents[0]),
+        # Modify git config:
+        "GIT_CONFIG_PARAMETERS": get_git_config_parameters(),
+    }
 
 
 # These are all the Kart defaults that differ from git's defaults.
@@ -351,14 +365,7 @@ def tool_environment(*, base_env=None, env_overrides=None):
 }
 
 
-# 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}'"
-
-
-@functools.lru_cache()
-def init_git_config():
+def get_git_config_parameters():
     """
     Initialises default config values that differ from git's defaults.
     """
@@ -370,13 +377,17 @@ def init_git_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])
-        )
+    return " ".join(new_config_params)
+
+
+# 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}'"
 
 
 def _pygit2_configs():

tests/conftest.py

@@ -421,9 +421,6 @@ def __init__(self, *args, in_pdb=False, mix_stderr=False, **kwargs):
 
     def invoke(self, args=None, **kwargs):
         from kart.cli import load_commands_from_args, cli
-        from kart.subprocess_util import init_git_config
-
-        init_git_config.cache_clear()
 
         if args:
             # force everything to strings (eg. PathLike objects, numbers)

tests/test_cli.py

@@ -1,12 +1,9 @@
 import contextlib
 import json
-import os
-import platform
 import re
 import sys
 from pathlib import Path
 
-import pygit2
 import pytest
 
 from kart import cli
@@ -38,20 +35,6 @@ def test_help_page_render(cli_runner, command):
     assert r.exit_code == 0, r.stderr
 
 
-@pytest.fixture
-def empty_gitconfig(monkeypatch, tmpdir):
-    old = os.environ["HOME"]
-    (tmpdir / ".gitconfig").write_text("", encoding="utf8")
-    monkeypatch.setenv("HOME", str(tmpdir))
-    pygit2.option(
-        pygit2.GIT_OPT_SET_SEARCH_PATH, pygit2.GIT_CONFIG_LEVEL_GLOBAL, str(tmpdir)
-    )
-    yield
-    pygit2.option(
-        pygit2.GIT_OPT_SET_SEARCH_PATH, pygit2.GIT_CONFIG_LEVEL_GLOBAL, str(old)
-    )
-
-
 @pytest.fixture
 def sys_path_reset(monkeypatch):
     """A context manager to save & reset after code that changes sys.path"""
@@ -65,20 +48,6 @@ def _sys_path_reset():
     return _sys_path_reset
 
 
-def test_config(empty_gitconfig, cli_runner):
-    # don't load the ~/.gitconfig file from conftest.py
-    # (because it sets init.defaultBranch and we're trying to test what
-    # happens without that set)
-    # note: merely changing os.environ['HOME'] doesn't help here;
-    # once libgit has seen one HOME it never notices if we change it.
-
-    # The default init.defaultBranch in git is still 'master' as of 2.30.0
-    # but we override it to 'main'. Let's check that works properly
-    r = cli_runner.invoke(["config", "init.defaultBranch"])
-    assert r.exit_code == 0, r.stderr
-    assert r.stdout == "main\n"
-
-
 def test_ext_run(tmp_path, cli_runner, sys_path_reset):
     # missing script
     with sys_path_reset():

tests/test_subprocess_util.py

@@ -1,7 +1,10 @@
 import os
 import platform
+import pytest
 import sys
 
+import pygit2
+
 from kart import subprocess_util
 
 
@@ -30,4 +33,35 @@ def test_subprocess_tool_environment():
         env_exec = subprocess_util.tool_environment(base_env=base_env)
         assert env_exec is not base_env
         env_exec.pop("PATH", None)
+        env_exec.pop("GIT_CONFIG_PARAMETERS", None)
         assert env_exec == base_env
+
+
+@pytest.fixture
+def empty_gitconfig(monkeypatch, tmpdir):
+    old = os.environ["HOME"]
+    (tmpdir / ".gitconfig").write_text("", encoding="utf8")
+    monkeypatch.setenv("HOME", str(tmpdir))
+    pygit2.option(
+        pygit2.GIT_OPT_SET_SEARCH_PATH, pygit2.GIT_CONFIG_LEVEL_GLOBAL, str(tmpdir)
+    )
+    subprocess_util.get_tool_environment_overrides.cache_clear()
+    yield
+    subprocess_util.get_tool_environment_overrides.cache_clear()
+    pygit2.option(
+        pygit2.GIT_OPT_SET_SEARCH_PATH, pygit2.GIT_CONFIG_LEVEL_GLOBAL, str(old)
+    )
+
+
+def test_git_config(empty_gitconfig, cli_runner):
+    # don't load the ~/.gitconfig file from conftest.py
+    # (because it sets init.defaultBranch and we're trying to test what
+    # happens without that set)
+    # note: merely changing os.environ['HOME'] doesn't help here;
+    # once libgit has seen one HOME it never notices if we change it.
+
+    # The default init.defaultBranch in git is still 'master' as of 2.30.0
+    # but we override it to 'main'. Let's check that works properly
+    r = cli_runner.invoke(["config", "init.defaultBranch"])
+    assert r.exit_code == 0, r.stderr
+    assert r.stdout == "main\n"