Theother parameter ofDiffable.diff is annotated this way:

Diffable.diff accepts a few special values:

• None for a working tree diff.
• Diffable.Index, atype object that is meant to be used opaquely rather than instantiated, to compare to the index.
• NULL_TREE, a direct instance ofobject (not to be confused withObject), for a diff against an empty tree.

The type ofNULL_TREE should be expressed better thanobject

The main problem here, which exists in the documentary effect of the annotations even if a type checker is never used, is the presence ofobject in the union. Sinceobject is the root of the type hierarchy in Python, a union of anything withobject is equivalent to justobject. But the intent of the annotation is not to express that it is correct to pass arbitrary objects asother. Instead, the idea is that it is okay to passNULL_TREE.

This is also the cause of the incompatible override type error on theIndexFile.diff override ingit.index.base, which does not include theobject alternative:

There are several places in GitPython's source code where an overridden method has an incompatible signature, violating substitutability. The reason this place is of interest is that it is hard to tell efficiently by examining the code or type errorswhat conceptually this means. It turns out it means the override does not supportNULL_TREE:

>>> import git>>> repo = git.Repo()>>> repo.index.diff(git.NULL_TREE)Traceback (most recent call last):  File "<stdin>", line 1, in <module>  File "C:\Users\ek\source\repos\GitPython\git\index\base.py", line 1521, in diff    raise ValueError("other must be None, Diffable.Index, a Tree or Commit, was %r" % other)ValueError: other must be None, Diffable.Index, a Tree or Commit, was <object object at 0x00000265862B8A30>

Butobject does not express (to humans or tools) that what is really meant is the literal valueNULL_TREE.

We cannot currently writeLiteral[NULL_TREE], but ifNULL_TREE is made into an enumeration constant, then this can be done, though cumbersomely it seems it can only be written--in annotations--asET.NULL_TREE whereET is the declared enumeration type. Or, ifET hasNULL_TREE as its only value, then writingET should be sufficient, though this may be less intuitive due to not involvingLiteral in any way while expressing a literal.

To be clear, this will not cause the mypy error about an incompatible override to go away. But it will make the error make sense, as well as making thesuppression comment on it make sense in terms of what it is expressing.And it will allow type checkers to catch errors about incorrect values ofother.

Diffable.Index should ideally also be improved

Diffable.Index is used as an opaque constant, as isNULL_TREE. ButDiffable.Index is a class. This is confusing because it should not be instantiated. Its static type can be expressed pretty specifically, asType[Diffable.Index] (as is done; I'm omitting the quotes here for simplicity). However, this does not allowif-else logic to be type-checked exhaustively, because there is no guarantee that theDiffable.Index class is the only value of the static typeType[Diffable.Index]:

classDerived(Diffable.Index):pass

Hopefully no one is doing that--just as hopefully no one is instantiating it--butmypy and other tools, including editors, cannot tell thatDiffable.Index is the only value ofType[Diffable.Index]. Humans who notice that it is intended to be used as an opaque constantwill recognize this, however. Also, itshould be possible to get type checkers to recognize it as the only value, and type-checkif-else logic exhaustively, by decorating the definition ofIndex with@final. This works for some type checkers, at leastpyright andpylance. But it does not work formypy.

The bigger issue is that it is easy to confuse and, for example, instantiate it.

Diffable.Index can also be made an enumeration constant, and all reasonable usage will continue to work.At runtime. However, if someone wants to statically annotate their own method to accept or return it, then they would need to change the annotations, since expressing it withType[Diffable.Index] would no longer be able to work. (Two separate special objects cannot both be used, because existing subclasses ofDiffable outside of GitPython would only be covering one. But if it is redefined, existing code can continue to cover it at runtime, since it should only ever have been, or be, used as an opaque constant.)

It is also possible for unreasonable usage to break at runtime, such as attempting to check ifx isDiffable.Index by writingissubclass(x, Diffable.Index). I am less worried about this.

It seems to me that this change is worth making forDiffable.Index as well, but this is a judgment call, and the new interface should perhaps be given special attention in review. When I searched for existing code outside GitPython, its forks, vendored copies of it, etc., that used these annotations in a way where they would cause a new static type error, I didn't find anything, but I only used GitHub code search, and I may not have found everything even on GitHub. My argument for making this change is to achieve greater correctness without breaking anything at runtime, andnot that I am at all confident nobody will have to change their code to keep it passing static checks.

Connection to#1859

I've included fixes for both of these things in#1859. See65863a2 especially, whose docstring has some more information about the specific design choices there.

If you decide you don't want the change toDiffable.Index there, that can be undone without sacrificing the rest. Although I am in favor of this aspect of the change, it should not be accepted on the basis of the sunk-cost fallacy. Furthermore, the sunk cost would be quite low, because it was already helpful in enabling me to figure out what needed to be done in other related parts of the code.

The purpose of this issue is twofold:

• To allow the bug to be tracked if#1859 is changed in such a way as not to fully solve it.
• To described it separately from that PR so it's easier to understand. I considered describing it in comments there, but thought opening an issue would be better.