From d88372a11ac145d92013dcc64b7d21a5a6ad3a91 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 7 Sep 2023 06:12:08 -0400 Subject: [PATCH 1/4] Add test for Windows env var upcasing regression --- test/test_git.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/test/test_git.py b/test/test_git.py index 540ea9f41..8ed9b64fe 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -10,7 +10,7 @@ import subprocess import sys from tempfile import TemporaryDirectory, TemporaryFile -from unittest import mock +from unittest import mock, skipUnless from git import Git, refresh, GitCommandError, GitCommandNotFound, Repo, cmd from test.lib import TestBase, fixture_path @@ -105,6 +105,27 @@ def test_it_executes_git_not_from_cwd(self): with _chdir(tmpdir): self.assertRegex(self.git.execute(["git", "version"]), r"^git version\b") + @skipUnless(is_win, "The regression only affected Windows, and this test logic is OS-specific.") + def test_it_avoids_upcasing_unrelated_environment_variable_names(self): + old_name = "28f425ca_d5d8_4257_b013_8d63166c8158" + if old_name == old_name.upper(): + raise RuntimeError("test bug or strange locale: old_name invariant under upcasing") + os.putenv(old_name, "1") # It has to be done this lower-level way to set it lower-case. + + script_lines = [ + "import subprocess, git", + + # Importing git should be enough, but this really makes sure Git.execute is called. + f"repo = git.Repo({self.rorepo.working_dir!r})", + "git.Git(repo.working_dir).execute(['git', 'version'])", + + f"print(subprocess.check_output(['set', {old_name!r}], shell=True, text=True))", + ] + cmdline = [sys.executable, "-c", "\n".join(script_lines)] + pair_text = subprocess.check_output(cmdline, shell=False, text=True) + new_name = pair_text.split("=")[0] + self.assertEqual(new_name, old_name) + def test_it_accepts_stdin(self): filename = fixture_path("cat_file_blob") with open(filename, "r") as fh: From 7296e5c021450743e5fe824e94b830a73eebc4c8 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 7 Sep 2023 06:36:34 -0400 Subject: [PATCH 2/4] Make test helper script a file, for readability --- test/fixtures/env_case.py | 13 +++++++++++++ test/test_git.py | 14 +++++--------- 2 files changed, 18 insertions(+), 9 deletions(-) create mode 100644 test/fixtures/env_case.py diff --git a/test/fixtures/env_case.py b/test/fixtures/env_case.py new file mode 100644 index 000000000..120e59289 --- /dev/null +++ b/test/fixtures/env_case.py @@ -0,0 +1,13 @@ +import subprocess +import sys + +import git + + +_, working_dir, env_var_name = sys.argv + +# Importing git should be enough, but this really makes sure Git.execute is called. +repo = git.Repo(working_dir) # Hold the reference. +git.Git(repo.working_dir).execute(["git", "version"]) + +print(subprocess.check_output(["set", env_var_name], shell=True, text=True)) diff --git a/test/test_git.py b/test/test_git.py index 8ed9b64fe..804cd22e4 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -112,16 +112,12 @@ def test_it_avoids_upcasing_unrelated_environment_variable_names(self): raise RuntimeError("test bug or strange locale: old_name invariant under upcasing") os.putenv(old_name, "1") # It has to be done this lower-level way to set it lower-case. - script_lines = [ - "import subprocess, git", - - # Importing git should be enough, but this really makes sure Git.execute is called. - f"repo = git.Repo({self.rorepo.working_dir!r})", - "git.Git(repo.working_dir).execute(['git', 'version'])", - - f"print(subprocess.check_output(['set', {old_name!r}], shell=True, text=True))", + cmdline = [ + sys.executable, + fixture_path("env_case.py"), + self.rorepo.working_dir, + old_name, ] - cmdline = [sys.executable, "-c", "\n".join(script_lines)] pair_text = subprocess.check_output(cmdline, shell=False, text=True) new_name = pair_text.split("=")[0] self.assertEqual(new_name, old_name) From c7fad20be5df0a86636459bf673ff9242a82e1fc Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 7 Sep 2023 04:32:36 -0400 Subject: [PATCH 3/4] Fix Windows env var upcasing regression This uses a simple hand-rolled context manager to patch the NoDefaultCurrentDirectoryInExePath variable, instead of unittest.mock.patch.dict. The latter set unrelated environment variables to the original (same) values via os.environ, and as a result, their names were all converted to upper-case on Windows. Because only environment variables that are actually set through os.environ have their names upcased, the only variable whose name should be upcased now is NoDefaultCurrentDirectoryInExePath, which should be fine (it has a single established use/meaning in Windows, where it's treated case-insensitively as environment variables in Windows *usually* are). --- git/cmd.py | 9 ++++----- git/util.py | 16 +++++++++++++++- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 3665eb029..d6f8f946a 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -14,7 +14,6 @@ import subprocess import threading from textwrap import dedent -import unittest.mock from git.compat import ( defenc, @@ -24,7 +23,7 @@ is_win, ) from git.exc import CommandError -from git.util import is_cygwin_git, cygpath, expand_path, remove_password_if_present +from git.util import is_cygwin_git, cygpath, expand_path, remove_password_if_present, patch_env from .exc import GitCommandError, GitCommandNotFound, UnsafeOptionError, UnsafeProtocolError from .util import ( @@ -965,10 +964,10 @@ def execute( '"kill_after_timeout" feature is not supported on Windows.', ) # Only search PATH, not CWD. This must be in the *caller* environment. The "1" can be any value. - patch_caller_env = unittest.mock.patch.dict(os.environ, {"NoDefaultCurrentDirectoryInExePath": "1"}) + maybe_patch_caller_env = patch_env("NoDefaultCurrentDirectoryInExePath", "1") else: cmd_not_found_exception = FileNotFoundError # NOQA # exists, flake8 unknown @UndefinedVariable - patch_caller_env = contextlib.nullcontext() + maybe_patch_caller_env = contextlib.nullcontext() # end handle stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb") @@ -984,7 +983,7 @@ def execute( istream_ok, ) try: - with patch_caller_env: + with maybe_patch_caller_env: proc = Popen( command, env=env, diff --git a/git/util.py b/git/util.py index f6dedf0f2..f80580cff 100644 --- a/git/util.py +++ b/git/util.py @@ -158,6 +158,20 @@ def cwd(new_dir: PathLike) -> Generator[PathLike, None, None]: os.chdir(old_dir) +@contextlib.contextmanager +def patch_env(name: str, value: str) -> Generator[None, None, None]: + """Context manager to temporarily patch an environment variable.""" + old_value = os.getenv(name) + os.environ[name] = value + try: + yield + finally: + if old_value is None: + del os.environ[name] + else: + os.environ[name] = old_value + + def rmtree(path: PathLike) -> None: """Remove the given recursively. @@ -935,7 +949,7 @@ def _obtain_lock_or_raise(self) -> None: ) try: - with open(lock_file, mode='w'): + with open(lock_file, mode="w"): pass except OSError as e: raise IOError(str(e)) from e From eebdb25ee6e88d8fce83ea0970bd08f5e5301f65 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 7 Sep 2023 06:59:02 -0400 Subject: [PATCH 4/4] Eliminate duplication of git.util.cwd logic --- git/util.py | 1 + test/test_git.py | 16 ++-------------- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/git/util.py b/git/util.py index f80580cff..dee467dd3 100644 --- a/git/util.py +++ b/git/util.py @@ -150,6 +150,7 @@ def wrapper(self: "Remote", *args: Any, **kwargs: Any) -> T: @contextlib.contextmanager def cwd(new_dir: PathLike) -> Generator[PathLike, None, None]: + """Context manager to temporarily change directory. Not reentrant.""" old_dir = os.getcwd() os.chdir(new_dir) try: diff --git a/test/test_git.py b/test/test_git.py index 804cd22e4..f1d35a355 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -4,7 +4,6 @@ # # This module is part of GitPython and is released under # the BSD License: http://www.opensource.org/licenses/bsd-license.php -import contextlib import os import shutil import subprocess @@ -15,24 +14,13 @@ from git import Git, refresh, GitCommandError, GitCommandNotFound, Repo, cmd from test.lib import TestBase, fixture_path from test.lib import with_rw_directory -from git.util import finalize_process +from git.util import cwd, finalize_process import os.path as osp from git.compat import is_win -@contextlib.contextmanager -def _chdir(new_dir): - """Context manager to temporarily change directory. Not reentrant.""" - old_dir = os.getcwd() - os.chdir(new_dir) - try: - yield - finally: - os.chdir(old_dir) - - class TestGit(TestBase): @classmethod def setUpClass(cls): @@ -102,7 +90,7 @@ def test_it_executes_git_not_from_cwd(self): print("#!/bin/sh", file=file) os.chmod(impostor_path, 0o755) - with _chdir(tmpdir): + with cwd(tmpdir): self.assertRegex(self.git.execute(["git", "version"]), r"^git version\b") @skipUnless(is_win, "The regression only affected Windows, and this test logic is OS-specific.")