Better document env_case test/fixture and cwd

[EliahKagan]

This improves how some of the changes in #1650 are documented, expanding comments and a docstring.

I ended up doing this in four commits, but really it would probably be just as clear with two. If you'd like me to rebase this to have just two commits, please let me know.

Identifying the steps in the "env_case" test

This is my main motivation for opening this PR, and most of the change.

I was thinking about how it was not immediately obvious why test_it_avoids_upcasing_unrelated_environment_variable_names (the "env_case" test) works and why it is done in this way.

To improve on this, I wrote comments that divide it into four steps. Two of the steps occur in the test case method, and the others occur in the supporting env_case.py fixture. The comments are in both files.

This only adds and changes comments, but just in case I have locally tested that it does not stop the test from passing, nor from failing when using the version of the code in git/ from just before the merge of #1650.

Non-reentrancy of some context manager objects

I had intended my brief mention of non-reentrancy to distinguish git.util.cwd from contextlib.chdir, which is fully reentrant by having each returned object maintain a stack of directories (but which is not available before Python 3.11). But this was not clear at all--I had nowhere mentioned contextlib.chdir!--and it risked giving the wrong impression that cwd couldn't be called multiple times in nested with-statements.

Furthermore, the newly introduced patch_env function is non-reentrant in exactly the same narrow sense, but because it did not mention this and cwd did, this gave the wrong impression that patch_env was reentrant in this way. (I think patch_env actually doesn't need to document this--really cwd's limitation compared to contextlib.chdir is the only thing important enough to mention.)

I've somewhat clarified this, but maybe further improvements can be made in the future. This can be make clearer with extended example, but I would be very reluctant to bloat the cwd docstring with fully worked examples. If anything, maybe there is a way to say it shorter. Or maybe the reentrancy information should be removed altogether.

[Byron]

Thanks again for your tremendous help!

I appreciate these improvements and confirm that they make clearer what's happening. It's a bit sad that we don't have a general-purpose bug-tracker yet that we can refer to in a self-contained fashion (maybe similar to git-appraise), as I also find myself to be hesitant to link to GitHub issues, as doing so might also be beneficial here.

I ended up doing this in four commits, but really it would probably be just as clear with two. If you'd like me to rebase this to have just two commits, please let me know.

I'd never ask that as I believe that any git-commit workflow has its purpose, with different trade-offs. One day I hope there is tooling that makes leveraging this information easier than git log or git blame, but we will be getting there.

This only adds and changes comments, but just in case I have locally tested that it does not stop the test from passing, nor from failing when using the version of the code in git/ from just before the merge of #1650.

That's extremely diligent of you, but even if there would have been a chance for test-failure, it's fine if CI detects it first. It's a tool and ideally, it saves some time as well.

[EliahKagan]

That's extremely diligent of you, but even if there would have been a chance for test-failure, it's fine if CI detects it first. It's a tool and ideally, it saves some time as well.

I'm a big fan of discovering test failures through CI only. The issue, here, is that the env_case test is for a regression that only affects native Windows, as elaborated below. So current CI, which uses a Cygwin build of python, would not catch a problem. Otherwise I would not have separately tested manually. However, given that the changes to the env_case test and fixture were just comments, it still probably wasn't necessary for me to test.

---

#1635 (CVE-2023-40590) and #1646 only affected GitPython on native Windows builds of python.

Although the reported case of #1646 mattered because of how it broke Cygwin make, I believe the python build used must have been native even there, because #1636 only patched os.environ when is_win was true, a behavior #1650 carries forward in its more limited patching. When I wrote the description of #1650, I was not sure about git.compat.is_win on Cygwin python builds. But I have since verified that it is False, which makes sense since it's based on os.name, which is posix rather than nt on Cygwin builds.

When I found that out, I was briefly worried CVE-2023-40590 might not be fully patched. In #1636, I had committed the fix and then the test, and mistakenly assumed--based on my local verification, which was with native Windows, not Cygwin--that CI would have shown a test failure if I had committed and pushed them in the other order. The regression test introduced in #1636 (which is now verifying the modified fix as of #1650) covers both true and false cases of is_win, but it would nonetheless probably not detect the bug with a Cygwin python build even if it were present. This is because, if the current directory were automatically included in a subprocess.Popen path search in Cygwin Python, it would be an inherited behavior from the Windows API and presumably would only be triggered by .exe files. (See python/cpython#91558 (comment).) But we only test with an .exe file when is_win is true.

So if GitPython on Cygwin python builds had suffered from CVE-2023-40590, it still would. Fortunately, I am pretty sure it does not. I have manually tested the subprocess module on a Cygwin build of python, both with and without shell=True (though with shell=True on a Cygwin build, the shell is Cygwin's /bin/sh, so I was pretty sure it wouldn't happen that way). Based on this, the Windows behavior that gave rise to CVE-2023-40590 does not happen in Cygwin python builds. I've also tested this in a MSYS2 build of python--MSYS2 being a major derivative of Cygwin--with the same result.

Note that CVE-2023-40590 did affect MinGW python builds installed by MSYS2 pacman (such as MINGW64 and UCRT64 builds), because unlike MSYS2 builds, those are actually native Windows builds. But because they are native builds, os.name is nt on those, so git.compat.is_win is True, so #1636 and #1650 patch NoDefaultCurrentDirectoryInExePath in as intended.