HIDE_WINDOWS_*_ERRORS environment variables are interpreted strangely

[EliahKagan]

Summary

As noted in #1697:

The HIDE_WINDOWS_KNOWN_ERRORS and HIDE_WINDOWS_FREEZE_ERRORS variables are intended to be affected by the same-named environment variables, but this does not work properly. They are True by default, but the only way to use environment variables to set them to a false value is to define the environment variable with an empty value (since everything else, as a string, is truthy). In addition to being very unlikely to have been the intended behavior, this is also hard to do on Windows when using typical shells, because in both cmd.exe and PowerShell setting an environment variable as empty actually removes the variable altogether.

Details

The attributes have type bool outside native Windows systems (where they are False) and in native Windows systems where the environment variables don't exist (they are True):

GitPython/git/util.py

Lines 122 to 126 in 8107cbf

	#: We need an easy way to see if Appveyor TCs start failing,
	#: so the errors marked with this var are considered "acknowledged" ones, awaiting remedy,
	#: till then, we wish to hide them.
	HIDE_WINDOWS_KNOWN_ERRORS = is_win and os.environ.get("HIDE_WINDOWS_KNOWN_ERRORS", True)
	HIDE_WINDOWS_FREEZE_ERRORS = is_win and os.environ.get("HIDE_WINDOWS_FREEZE_ERRORS", True)

Otherwise they are the str values of the same-named environment variables. They are always used as conditions, and as such, the only ways to set them False on a native Windows system--besides editing that code, which in practice seems to be what is done--is to write to the attributes with Python code or to set the environment variables to the empty string.

In bash on Windows, such as Git Bash, this is easy to do, though not at all obvious that it should be done and also very unintuitive. In shells meant for Windows, cmd.exe and PowerShell, setting an environment variable to the empty string causes it to be automatically unset, and extra effort is required to actually set the empty string.

The tricky part

HIDE_WINDOWS_KNOWN_ERRORS is defined in git.util and listed in __all__. So it is a breaking change to remove or rename it. Because code that uses the GitPython library is explicitly allowed to assume the attribute will continue to exist, it's worth considering the subjective question of what, if anything, users can reasonably assume about it.

Although comments shown above explains its original purpose, its semantics are not really documented anywhere, and nothing makes any statements about how it is set. Furthermore, if people are setting nonempty values that intuitively feel like they express falsehood--such as 0--then they probably expect that this is setting it to False or some falsy value. Therefore, I think it's reasonable to regard the current behavior as a bug and fix it, even though actually removing or even officially deprecating the attribute should wait until later because it currently affects the behavior of git.util.rmtree, through the callback it passes to shutil.rmtree.

In contrast, HIDE_WINDOWS_FREEZE_ERRORS is not listed in __all__ and could be renamed, removed, or arbitrarily changed at any time. But as long as HIDE_WINDOWS_KNOWN_ERRORS remains in git.util, I don't think the presence HIDE_WINDOWS_FREEZE_ERRORS should be seen as any special problem.

Although I don't advocate formally deprecating the HIDE_WINDOWS_KNOWN_ERRORS attribute just yet, the environment variable is another story. In addition to being parsed differently, the presence of either environment variable could cause a warning to be printed when git.util is loaded (which happens when the git module is loaded).

Context

This relates to the deeper issue #790. Fixing this narrower issue won't solve the problem that raising SkipTest as a result of failing to delete files can produce unpredictable effects in production--because it signals a condition that would typically be signaled by PermissionError or some other exception derived from OSError, and would bypass exception handling meant for that--as well as masking bugs in applications or other libraries that use GitPython by causing their automated tests to be skipped. But solving that will carry its own challenge. The main problem is not how to provide the functionality to the test suite, but instead that there may be code using GitPython that has come to rely on exceptions from deleting files being wrapped in SkipTest.

Although I definitely think #790 should be solved, and there are various approaches to mitigating disruption from doing so, I think it is worth fixing this narrower issue of how the environment variables are interpreted now. I've included such a fix in #1700 (which also would fix #1699).