-
-
Notifications
You must be signed in to change notification settings - Fork 910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Commit_ish is much broader than commit-ish #1858
Comments
Thanks for bringing this to my attention. To me it seems that, no matter what, the Along with that, I agree that it would be good to further clarify that Lastly, In summary, I think |
I had at first feared that this might change the runtime behavior of reasonable code, but it looks like that may not be the case. In particular, due to the way it is written,
Even using values obtained from This is good news for removing the never-treeish alternatives because it suggests that only static typing could break--and depending on how people are using I think I'm going to look at bit more into whether there are runtime implications of this change, even though a cursory examination suggests there are not. There is also the question of how GitPython uses it. There are many occurrences of it in GitPython's type annotations. Some appear to intend a union of all four actual git object types, while other appear to intend only those types that can actually be commit-ish. If it turns out that this impression is wrong and that GitPython uses it in a consistent way--closer examination will tell--then changing it may not be justified. But so long as that is not so--which it seems it may not be--then I think changing its definition may be justifiable. A new union can be created for all four actual git object types. There is a question of how it should be named; I can let you know if I have trouble coming up with a good name, but if you have a particular name or names that should be preferred then you can let me know. Assuming these changes can be made, I think there are two reasonable approaches. One is for me to expand and retitle #1859 to include these changes, assuming I am able to make them. The other approach is that I could weaken or remove some of the unjustified wording in the |
Thanks for looking into and validating it!
I thought that maybe Thanks again for your help with this, I am sure you will find a good path forward. |
And Lit_commit_ish to Lit_old_commit_ish. This is temporary, and only for purposes of bookkeeping while this type is split into two types. Specifically, Old_commit_ish will go away soon, because each occurrence of it will be changed to one of: - A newly redefined Commit_ish union of types representing only git object types that are sometimes commit-ish. - A newly introduced union (using some name formerly not used in GitPython) of all four types representing git object types, the same types as the old Commit_ish, here renamed Old_commit_ish, had overbroadly covered. - Perhaps in some cases something else. For context, see gitpython-developers#1858 (including comments), and comments in gitpython-developers#1859.
I went with The reason I didn't go with The reason I didn't go with |
In git, if I understand correctly, a commit-ish is a git object from which a commit can be reached by dereferencing it zero or more times, which is to say that all commits are commit-ish, some tag objects are commit-ish--those that, through (possibly repeated) dereferencing, eventually reach a commit--and no other types of git objects are ever commit-ish.
As gitglossary(7) says:
Therefore, all instances of GitPython's
Commit
class, and some instances of GitPython'sTagObject
class, encapsulate git objects that are actually commit-ish.But GitPython has a
Commit_ish
union type in thegit.types
module, and thatCommit_ish
type is considerably broader:GitPython/git/types.py
Line 53 in b2c3d8b
These four classes are the GitPython classes whose instances encapsulate any of the four types of git objects (of which blobs and trees are never actually commit-ish):
GitPython uses its
Commit_ish
type in accordance with this much broader concept, at least some of the time and possibly always. For example,Commit_ish
is given as the return type ofObject.new
:GitPython/git/objects/base.py
Lines 77 to 78 in b2c3d8b
Commit_ish
cannot simply be replaced byObject
because GitPython'sObject
class is also, throughIndexObject
, a superclass ofSubmodule
(and theRootModule
subclass ofSubmodule
):GitPython/git/objects/submodule/base.py
Line 82 in b2c3d8b
GitPython/git/objects/submodule/base.py
Lines 87 to 88 in b2c3d8b
GitPython/git/objects/submodule/base.py
Lines 100 to 101 in b2c3d8b
However, elsewhere in GitPython,
Commit_ish
is used where it seems only a commit is intended to be allowed, though it is unclear if this is unintentional, intentional but only to allow type checkers to allow some code that can only reasonably be checked at runtime, or intentional for some other reason. For example, theRepo.commit
method, when called with one argument, looks up a commit in the repository it represents from aCommit_ish
or string, and returns the commit it finds as aCommit
:GitPython/git/repo/base.py
Line 698 in b2c3d8b
This leads to a situation where one can write code that type checkers allow and that may appear intended to work, but that always fails, and in a way that may be unclear to users less familiar with git concepts:
An argument that this specific situation with
Repo.commit
is not a typing bug is that this operation is fundamentally one that can only be checked at runtime in some cases. After all, an argument of typestr
is also allowed and it cannot known until runtime what object a string happens to name. Even so, the method docstring should possibly be expanded to clarify this issue. Or perhaps if the situation withCommit_ish
is improved, then the potential for confusion will go away.One way to improve this situation is to clearly document it in a docstring for the
Commit_ish
type. But if possible it seems to me that more should be done:Object
may benefit from greater clarity about what it ideally represents (git objects) versus the entirety of what it represents (that anObject
can also be aSubmodule
), and the way thatTree_ish
is narrower than all tree-ish git objects whileCommit_ish
is broader than all commit-ish git objects can be noted in one of their docstrings.Commit_ish
should be deprecated and one or more new types introduced, replacing all uses of it in GitPython.If I am making a fundamental mistake about git concepts here, and GitPython's
Commit_ish
has a closer and more intuitive relationship to commit-ish git objects than I think it does, then I apologize.I have not figured out very much from GitPython's revision history what the reason for defining
Commit_ish
as it is currently defined is, or alternatively why this union of all four actual git object types was introduced with the narrower-seeming nameCommit_ish
. However, theCommit_ish
type was introduced in 82b131c (#1282), where the annotations it was used to replace had listed all four typesCommit
,TagObject
,Tree
, andBlob
as explicit alternatives.The text was updated successfully, but these errors were encountered: