is_darwin is always False (os.name is never "darwin")

[EliahKagan]

As discussed in #1679 and #1679 (review), code inside or outside of GitPython that uses is_win from git.compat is prone to bugs, because it is easy to assume wrongly that is_win would be true on Cygwin. Instead, is_win is short for os.name == "nt", and writing that out makes clearer what one is testing.

However, I have discovered that the potential for confusion in using the is_<platform> aliases in git.compat is not limited to is_win. Specifically, is_darwin is always False, including on macOS systems.

This is because is_darwin is short for os.name == "darwin", but "darwin" is not currently and seems never to have been one of the possible values of os.name. In Python 2, there were more values of os.name than in Python 3, but in both languages os.name is always "posix" on the operating system today called macOS (historically called Mac OS X, with Mac OS 9 and earlier not being POSIX systems).

In the infrequent case that one wishes to test for macOS, one can check sys.platform == "darwin", because sys.platform is more granular than os.name and does take on the value of "darwin" on macOS. GitPython does not currently need to do this, and no uses of is_darwin appear in git/ or test/ currently. This appears to have been the case since 4545762 (#1295).

The buggy definition of is_darwin seems to have had limited impact in the project, having been introduced in f495e94 (part of #519) and only used in a couple of places, then none since #1295. Before #519, the correct expression sys.platform == 'darwin' was used, but it seems to have been inadvertently changed to the always-false os.name == 'darwin' in that commit. (The is_<platform> attributes were soon after changed from functions to variables/constants in e61439b, which was also in #519.)

Possible fixes

Although GitPython doesn't use is_darwin anymore, it is a public name in git.compat (since that module does not define __all__ and the name is_darwin neither starts with a _ nor has been documented as nonpublic). So it may be in other projects that use GitPython. If so, those other projects suffer either from this bug, or if not then from another bug of their own where they depend on is_darwin having the wrong value.

Because removing it would be a breaking change, it should probably be kept, but fixed to use the expression sys.platform == "darwin". However, this does have the disadvantage that a developer who checks its implementation may assume is_win and is_posix are analogous, examining sys.platform. In the case of is_win, which checks os.name = "nt", I believe it is currently equivalent to checking sys.platform == "win32", though historically this may not always have been the case. In the case of is_posix, however, it is not equivalent to any straightforward expression with sys.platform, because a few "POSIX" platforms are distinguished with custom values of sys.platform.

Both for this reason and, more so, because of the confusion that turns out to have arisen from the is_<platform> aliases, I recommend all of them be replaced, and also that they be documented as deprecated. Because deprecation is often not readily discovered when one is not looking for it--even if DeprecationWarning is emitted, that warning is hidden by default--I think is_darwin should probably be fixed to sys.platform == "darwin" as well, in spite of the disadvantages of doing so.

A further benefit of replacing all uses is that static type checkers (and some editors) understand checks against standard-library attributes such as os.name and they are able to check code more precisely based on them. This helps with things like flags in the subprocess module that differ across platforms (though attribute accesses based on them have to be written across separate expressions, rather than for example in a ternary expression, to get the full benefit of these distinctions, at least with current type checkers). As far as I know, there's no fundamental reason this shouldn't also work with module attributes assigned that kind of expression (typing it automatically as a literal value), but that does not seem to happen.

I've opened #1732 for this. For the reasons given below, I haven't included any code to emit new DeprecationWarnings, though I'd be pleased to do so if requested.

New warnings?

Although I recommend the is_<platform> attributes be deprecated, I am unsure if it is worthwhile to have accesses to those attributes raise any warnings at this time. There are a few considerations:

• This can be done by defining a module-level __getattr__, but I think that slightly decreases the benefits of static typing. It can also be done by writing a class that inherits from types.ModuleType and defines properties, and assigning that class to the __class__ attribute of the module object. This may be preferable, if it turns out to play better with static typing. See Customizing module attribute access (though it doesn't discuss static typing implications).
• It seems very likely that DeprecationWarnings on particular module-level attribute accesses will be of use elsewhere in the project. (One possible place is for __version__ as discussed in this and that comment, but I think there may be a number of others, too.) But I'd be more comfortable introducing either of these things once static typing is in better shape, so their effects on that can be better judged.
• There is also a more specific and, in my view, important reason to be reluctant to add this functionality for those attributes right now: they are in git.compat, which could perhaps be entirely deprecated sometime soon. That module's original purpose was to help in supporting both Python 2 and Python 3. Since GitPython currently only supports Python 3, it might be possible to eliminate use of all of the facilities in git.compat, though whether or not that should be done is another question, since some uses of it might be considered to improve clarity for reasons not tied to the distinction between Python 2 and Python 3. But if git.compat as a whole is deprecated, then emitting a DeprecationWarning won't require any special attribute handling, as it can just be done with a top-level warnings.warn call in the module.