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

However, I have discovered that the potential for confusion in using theis_<platform> aliases ingit.compat is not limited tois_win. Specifically,is_darwin is alwaysFalse, including on macOS systems.

This is becauseis_darwin is short foros.name == "darwin", but"darwin"is not currently andseems never to have been one of the possible values ofos.name. In Python 2, there were more values ofos.name than in Python 3, but in both languagesos.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 checksys.platform == "darwin", becausesys.platform is more granular thanos.name and does take on the value of"darwin" on macOS. GitPython does not currently need to do this, and no uses ofis_darwin appear ingit/ ortest/ currently. This appears to have been the case since4545762 (#1295).

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

Possible fixes

Although GitPython doesn't useis_darwin anymore, it is a public name ingit.compat (since that module does not define__all__ and the nameis_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 onis_darwin having the wrong value.

Because removing it would be a breaking change, it should probably be kept, but fixed to use the expressionsys.platform == "darwin". However, this does have the disadvantage that a developer who checks its implementation may assumeis_win andis_posix are analogous, examiningsys.platform. In the case ofis_win, which checksos.name = "nt", I believe it is currently equivalent to checkingsys.platform == "win32", though historically this may not always have been the case. In the case ofis_posix, however, it is not equivalent to any straightforward expression withsys.platform, because a few "POSIX" platforms are distinguished with custom values ofsys.platform.

Both for this reason and, more so, because of the confusion that turns out to have arisen from theis_<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 ifDeprecationWarning is emitted, that warning is hidden by default--I thinkis_darwin should probably be fixed tosys.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 asos.name and they are able to check code more precisely based on them. This helps with things like flags in thesubprocess 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 newDeprecationWarnings, though I'd be pleased to do so if requested.

New warnings?

Although I recommend theis_<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 fromtypes.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. SeeCustomizing module attribute access (though it doesn't discuss static typing implications).
• It seems very likely thatDeprecationWarnings on particular module-level attribute accesses will be of use elsewhere in the project. (One possible place is for__version__ as discussed inthis andthat 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 ingit.compat, which could perhaps beentirely 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 ofall of the facilities ingit.compat, though whether or not thatshould 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 ifgit.compat as a whole is deprecated, then emitting aDeprecationWarning won't require any special attribute handling, as it can just be done with a top-levelwarnings.warn call in the module.