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

However, that does not unpatch the variables, and more importantly, it prevents writes throughos.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")bar0>>> os.environ = oldenv>>> os.system("printenv FOO")bar0>>> os.environ["BAZ"] = "quux">>> os.system("printenv BAZ")256

This happens because, all types in Python being reference types, theenviron attribute ofos merelyrefers to the object of a special mutable mapping type that updates the process's environment variables when mutated. Calling itscopy method constructs adict object with the same items. Assigning thatdict back toos.environ does not modify the originalos.environ object in any way, but instead causes theenviron attribute ofos to point to thedict object from that point forward. From then on, writes toos.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 Iguess to be their significance:

1. When GitPython runsgit, it doesn't rely on automatically passing its own environment variables to thegit 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 consultingos.environ, which doesn't actually have to work for setting environment variables, becausePopen is doing it:

   GitPython/git/cmd.py

   Lines 948 to 955 ina5a6464

   	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 ina5a6464

   	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 viaos.environ, so again, it's okay if the real environment is not used.

3. The test is intest_submodules.py, and test modules are usually exercised in alphabetical order, with onlytest_tree.py andtest_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 patchingos.environ withunittest.mock.patch.dict. We cannot use this in the code of thegit 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 thatflake8 reported it once I removedtest/ 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 mutateos.environ, and to do so less drastically than withclear), but that message nonetheless accurately identifies the problem, which seems previously to have gone undetected. I think it is a good idea to enableflake8 fortest/ as well asgit/ (though perhaps sometime soon we might manage to replaceflake8 withruff). I plan to include a fix for this, and also for less serious, mostly merely stylistic issues found while examiningtest/ with the help offlake8, in the same PR that fixes#1670.