Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork960
__init__: don't run refresh on import#2073
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
d3662f2 to1b2d3f0CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for contributing!
My intuition here is that the problem isImportError, and that the change not only removes that but also therefresh() call which I'd think is important.
Besides that, I'd definitely be unwilling to to casually bump the major version for this alone, if it is a major bump. It could also be a fix, but I'd be very afraid it's breaking downstream in too many ways.
However, I can see that the issue to solve here is that GitPython might be imported somewhere in the dependency chain on a system that has no Git, without ever running into other issues as no Git specific features are used. Thus it's not needed to fail on import, but rather, it should fail on first use.
This is the direction I'd want to take this - can it fail later? After all, there is plenty of functionality that might work even without Git.
@EliahKagan might have a better feeling about what could be done to improve the underlying problem and avoid a breaking change.
Setting this back to draft as I'd not want it merged just yet until it's clear if there are no better solutions to the problem. |
1b2d3f0 to24c4a0eCompareHow does the behaviour of this variant look to you? The GitCommandNotFound exception is a bit awkward as is, but that should be easy enough to clean up. |
It looks like CI is failing, and I don't see my question about the lack of |
Sorry, I thought this was clear from the diff: I introduced a Will take a look at the tests. |
Consumers of gitpython may not need to use it in all use cases, and maywant to be able to run (without using gitpython) in environments wheregit is not available on PATH. While this can be worked around by settingthe GIT_PYTHON_REFRESH environment variable, adding special handling forgitpython means that it can't be imported just like everything else inan import block at the top of the module, and environment variables havepotentially undesired propagation behaviour.Previously, it was also nontrivial to distinguish gitpython failing toimport because of missing git or because e.g. gitpython isn't installedat all, because the exception that's raised is an ImportError withoutfurther qualification (except in the error message).Thus, we now no longer perform `refresh` at the module top level,instead performing it lazily when an invocation of git is attempted.This also allows some functionality that doesn't rely on the git commandto work without, e.g. ref listing.
24c4a0e to2a876ebCompareByron commentedSep 25, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for clarifying. |
Consumers of gitpython may not need to use it in all use cases, and may want to be able to run (without using gitpython) in environments where git is not available on PATH. While this can be worked around by setting the GIT_PYTHON_REFRESH environment variable, adding special handling for gitpython means that it can't be imported just like everything else in an import block at the top of the module, and environment variables have potentially undesired propagation behaviour.
Previously, it was also nontrivial to distinguish gitpython failing to import because of missing git or because e.g. gitpython isn't installed at all, because the exception that's raised is an ImportError without further qualification (except in the error message).
Thus, we now no longer perform
refreshat the module top level.This istechnically observable behaviour that could break assumptions made downstream, so I've added a changelog entry with a major version bump. I'm not sure this is "breaking enough" that you would consider it needing a major bump.
Feedback both on the breakingness and on the change in principle very welcome!