test_git_work_tree_env renders os.environ inert in unpatching attempt

[EliahKagan]

TestRepo.test_git_work_tree_env contains this code, which is intended to patch and unpatch two environment variables:

GitPython/test/test_repo.py

Lines 1341 to 1350 in a5a6464

	oldenv = os.environ.copy()
	os.environ["GIT_DIR"] = new_git_dir
	os.environ["GIT_WORK_TREE"] = repo_dir
	try:
	r = Repo()
	self.assertEqual(r.working_tree_dir, repo_dir)
	self.assertEqual(r.working_dir, repo_dir)
	finally:
	os.environ = oldenv

However, that does not unpatch the variables, and more importantly, it prevents writes through os.environ from ever actually setting environment variables (to be inherited automatically by subprocesses) for the rest of the process's lifetime.

>>> import os
>>> type(os.environ)
<class 'os._Environ'>
>>> oldenv = os.environ.copy()
>>> type(oldenv)
<class 'dict'>
>>> os.system("printenv FOO")
256
>>> os.environ["FOO"] = "bar"
>>> os.system("printenv FOO")
bar
0
>>> os.environ = oldenv
>>> os.system("printenv FOO")
bar
0
>>> os.environ["BAZ"] = "quux"
>>> os.system("printenv BAZ")
256

This happens because, all types in Python being reference types, the environ attribute of os merely refers to the object of a special mutable mapping type that updates the process's environment variables when mutated. Calling its copy method constructs a dict object with the same items. Assigning that dict back to os.environ does not modify the original os.environ object in any way, but instead causes the environ attribute of os to point to the dict object from that point forward. From then on, writes to os.environ have no effect on the running process's environment variables.

Yet, this does not cause any other tests to fail, nor do any currently passing tests fail as a result of fixing it. I believe this is for three reasons, which I list in descending order of what I guess to be their significance:

1. When GitPython runs git, it doesn't rely on automatically passing its own environment variables to the git subprocess. Instead, it builds a modified environment based on its own and passes that explicitly. The way it finds out about its own environment is by consulting os.environ, which doesn't actually have to work for setting environment variables, because Popen is doing it:

   GitPython/git/cmd.py

   Lines 948 to 955 in a5a6464

   	env = os.environ.copy()
   	# Attempt to force all output to plain ascii english, which is what some parsing code
   	# may expect.
   	# According to stackoverflow (http://goo.gl/l74GC8), we are setting LANGUAGE as well
   	# just to be sure.
   	env["LANGUAGE"] = "C"
   	env["LC_ALL"] = "C"
   	env.update(self._environment)

   GitPython/git/cmd.py

   Lines 987 to 989 in a5a6464

   	proc = Popen(
   	command,
   	env=env,

2. When GitPython, or any library it uses if that library is written in Python, or the testing framework, accesses and changes environment variables for use within the process, that is also nearly always via os.environ, so again, it's okay if the real environment is not used.

3. The test is in test_submodules.py, and test modules are usually exercised in alphabetical order, with only test_tree.py and test_util.py coming after it.

This only affects the tests, and it can be solved in any way appropriate for tests. Specifically, it can be solved--and also that test code significantly simplified--by patching os.environ with unittest.mock.patch.dict. We cannot use this in the code of the git module, because it upcases environment variable names (#1646). But it is not a problem to use it in tests (besides the test in #1650 that #1646 is fixed), and it is already being used in a few places in the test suite.

---

The way I found out about this was that flake8 reported it once I removed test/ from its excluded directories:

test/test_repo.py:1350:13: B003 Assigning to `os.environ` doesn't clear the environment. Subprocesses are going to see outdated variables, in disagreement with the current process. Use `os.environ.clear()` or the `env=` argument to Popen.
            os.environ = oldenv
            ^

The specific suggestions aren't quite applicable here (we do need to mutate os.environ, and to do so less drastically than with clear), but that message nonetheless accurately identifies the problem, which seems previously to have gone undetected. I think it is a good idea to enable flake8 for test/ as well as git/ (though perhaps sometime soon we might manage to replace flake8 with ruff). I plan to include a fix for this, and also for less serious, mostly merely stylistic issues found while examining test/ with the help of flake8, in the same PR that fixes #1670.