From 74d9c08bc4b8a61cd9f31e2294e1ff74d7b2be08 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 11 Sep 2023 23:11:43 -0400 Subject: [PATCH 1/4] Better explain the env_case test This expands and adds comments in test_it_avoids_upcasing_unrelated_environment_variable_names and its supporting fixture env_case.py so it is clear exactly what is being tested and how/why the test works to test it. --- test/fixtures/env_case.py | 7 ++++++- test/test_git.py | 14 ++++++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/test/fixtures/env_case.py b/test/fixtures/env_case.py index 120e59289..fe85ac41d 100644 --- a/test/fixtures/env_case.py +++ b/test/fixtures/env_case.py @@ -1,13 +1,18 @@ +# Steps 3 and 4 for test_it_avoids_upcasing_unrelated_environment_variable_names. + import subprocess import sys +# Step 3a: Import the module, in case that upcases the environment variable name. import git _, working_dir, env_var_name = sys.argv -# Importing git should be enough, but this really makes sure Git.execute is called. +# Step 3b: Use Git.execute explicitly, in case that upcases the environment variable. +# (Importing git should be enough, but this ensures Git.execute is called.) repo = git.Repo(working_dir) # Hold the reference. git.Git(repo.working_dir).execute(["git", "version"]) +# Step 4: Create the non-Python grandchild that accesses the variable case-sensitively. 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 f1d35a355..3b4a633b6 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -98,15 +98,25 @@ 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. + # Step 1: Set the environment variable in this parent process. Because os.putenv is a thin + # wrapper around a system API, os.environ never sees the variable in this parent + # process, so the name is not upcased even on Windows. + os.putenv(old_name, "1") + + # Step 2: Create the child process that inherits the environment variable. It will see it + # in os.environ with an upcased name, but if it is not mutated through os.environ + # then it will pass it on to its own child processes with the original name. The + # child process will use GitPython, and we are testing that it passes the variable + # with the exact original name to its own child processes. cmdline = [ sys.executable, fixture_path("env_case.py"), self.rorepo.working_dir, old_name, ] - pair_text = subprocess.check_output(cmdline, shell=False, text=True) + pair_text = subprocess.check_output(cmdline, shell=False, text=True) # Steps 3 and 4. + new_name = pair_text.split("=")[0] self.assertEqual(new_name, old_name) From a59aaead478213b6fd83dd39c607baa2ead97e09 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 11 Sep 2023 23:14:51 -0400 Subject: [PATCH 2/4] Condense an overly long comment To better focus on the key information. --- test/test_git.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index 3b4a633b6..60cd60bd9 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -104,11 +104,9 @@ def test_it_avoids_upcasing_unrelated_environment_variable_names(self): # process, so the name is not upcased even on Windows. os.putenv(old_name, "1") - # Step 2: Create the child process that inherits the environment variable. It will see it - # in os.environ with an upcased name, but if it is not mutated through os.environ - # then it will pass it on to its own child processes with the original name. The - # child process will use GitPython, and we are testing that it passes the variable - # with the exact original name to its own child processes. + # Step 2: Create the child process that inherits the environment variable. The child uses + # GitPython, and we are testing that it passes the variable with the exact original + # name to its own child process (the grandchild). cmdline = [ sys.executable, fixture_path("env_case.py"), From 22b8dba6b4c73d04f3dbfabbb858fb5316fec711 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 11 Sep 2023 23:28:14 -0400 Subject: [PATCH 3/4] Improve git.util.cwd docstring This frames the reentrancy claim in terms of what is similar to and different from contextlib.chdir (which we would just use, but it is only available on Python 3.11 and later). --- git/util.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/git/util.py b/git/util.py index dee467dd3..636e79806 100644 --- a/git/util.py +++ b/git/util.py @@ -150,7 +150,10 @@ 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.""" + """Context manager to temporarily change directory. + + This is similar to contextlib.chdir introduced in Python 3.11, but the context + manager object returned by a single call to this function is not reentrant.""" old_dir = os.getcwd() os.chdir(new_dir) try: From c547555026e099defd361e95e8c06acd5f2c0a2f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 12 Sep 2023 00:56:01 -0400 Subject: [PATCH 4/4] Clarify test relationship to env_case.py fixture --- test/test_git.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index 60cd60bd9..4d57a2d86 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -109,11 +109,11 @@ def test_it_avoids_upcasing_unrelated_environment_variable_names(self): # name to its own child process (the grandchild). cmdline = [ sys.executable, - fixture_path("env_case.py"), + fixture_path("env_case.py"), # Contains steps 3 and 4. self.rorepo.working_dir, old_name, ] - pair_text = subprocess.check_output(cmdline, shell=False, text=True) # Steps 3 and 4. + pair_text = subprocess.check_output(cmdline, shell=False, text=True) # Run steps 3 and 4. new_name = pair_text.split("=")[0] self.assertEqual(new_name, old_name)