Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork938
Report actual attempted Git command when Git.refresh fails#1812
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
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
For when refreshing should succeed and when it should fail.
shutil.which does not always find the git that Popen would run, atleast on Windows, but it is no worse than running a "command -v" or"where" command as was previously done here via os.popen.
This extends test_refresh_bad_git_path so that it asserts that theexception message shows the command the failed refresh used.This test fails due to a bug where "git" is always shown (gitpython-developers#1809).
Seegitpython-developers#1809. The test extended in the previous commit now passes.
+ Test a successful refresh with a relative path, which will be safer to do once the refresh tests restore changed state.Seegitpython-developers#1811. This addresses it incompletely, because while it isprobably not necessary while running the test suite to preserve anold False value of git.GIT_OK (most tests can't work if thathappens anyway), the value of Git.GIT_PYTHON_GIT_EXECUTABLE is notthe only other global state that effects the behavior ofsubsequently run tests and that may be changed as a result of therefresh tests.1. After the git.refresh function calls git.cmd.Git.refresh, it calls git.remote.FetchInfo.refresh, which rebinds the git.remote.FetchInfo._flag_map attribute.2. Future changes to git.cmd.Git.refresh may mutate other state in the Git class, and ideally the coupling would be loose enough that the refresh tests wouldn't have to be updated for that if the behavior being tested does not change.3. Future changes to git.refresh may perform other refreshing actions, and ideally it would be easy (and obvious) what has to be done to patch it back. In particular, it will likely call another Git method that mutates class-wide state due togitpython-developers#1791, and for such state that is also of the Git class, ideally no further changes would have to be made to the code that restores state after the refresh tests.If we assume git.refresh is working at least in the case that it iscalled with no arguments, then the cleanup can just be a call togit.refresh(). Otherwise, sufficiently general cleanup may be morecomplicated.
Forgitpython-developers#1811. This is the simple way to do it. This relies ongit.refresh working when called when no arguments, so if that everbreaks, then it may be difficult or inefficient to investigate. Butthis is simpler than any alternatives that are equally or morerobust.This is already better than the previous incomplete way that onlyrestored Git.GIT_PYTHON_GIT_EXECUTABLE (and nothing in FetchInfo).Furthermore, because the tests already depend to a large extent ongit.refreh working when called with no arguments, as otherwise theinitial refresh may not have set Git.GIT_PYTHON_GIT_EXECUTABLEusably, it might not be worthwhile to explore other approaches.
I think this refactoring slightly improves readability.+ Condense/sort/group imports (while adding contextlib import).+ Minor style tweaks in FetchInfo.refresh comments.+ Make a comment in Git.refresh clearer, building on revisions in:gitpython-developers#1810 (comment)
This adds short docstrings to each of the four refresh tests tobriefly state the claims they verify.
Byron approved these changesJan 26, 2024
There 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 a lot for tackling this, it's much appreciated!
(On a personal note, your contributions are so numerous, I have to think about ways to express my gratitude in a non-repetitive fashion 😅 - it's a great problem to have though, please keep 'em coming)
Uh oh!
There was an error while loading.Please reload this page.
lettuce-botbot referenced this pull request in lettuce-financial/github-bot-signed-commitFeb 15, 2024
[](https://renovatebot.com)This PR contains the following updates:| Package | Change | Age | Adoption | Passing | Confidence ||---|---|---|---|---|---|| [GitPython](https://togithub.com/gitpython-developers/GitPython) |`==3.1.41` -> `==3.1.42` |[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|---### Release Notes<details><summary>gitpython-developers/GitPython (GitPython)</summary>###[`v3.1.42`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.42)[CompareSource](https://togithub.com/gitpython-developers/GitPython/compare/3.1.41...3.1.42)#### What's Changed- Fix release link in changelog by[@​PeterJCLaw](https://togithub.com/PeterJCLaw) in[https://github.com/gitpython-developers/GitPython/pull/1795](https://togithub.com/gitpython-developers/GitPython/pull/1795)- Remove test dependency on sumtypes library by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1798](https://togithub.com/gitpython-developers/GitPython/pull/1798)- Pin Sphinx plugins to compatible versions by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1803](https://togithub.com/gitpython-developers/GitPython/pull/1803)- fix: treeNotSorted issue by[@​et-repositories](https://togithub.com/et-repositories) in[https://github.com/gitpython-developers/GitPython/pull/1799](https://togithub.com/gitpython-developers/GitPython/pull/1799)- Remove git.util.NullHandler by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1807](https://togithub.com/gitpython-developers/GitPython/pull/1807)- Clarify why GIT_PYTHON_GIT_EXECUTABLE may be set on failure by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1810](https://togithub.com/gitpython-developers/GitPython/pull/1810)- Report actual attempted Git command when Git.refresh fails by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1812](https://togithub.com/gitpython-developers/GitPython/pull/1812)- Don't suppress messages when logging is not configured by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1813](https://togithub.com/gitpython-developers/GitPython/pull/1813)- Pin Python 3.9.16 on Cygwin CI by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1814](https://togithub.com/gitpython-developers/GitPython/pull/1814)- Have initial refresh use a logger to warn by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1815](https://togithub.com/gitpython-developers/GitPython/pull/1815)- Omit warning prefix in "Bad git executable" message by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1816](https://togithub.com/gitpython-developers/GitPython/pull/1816)- Test with M1 macOS CI runner by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1817](https://togithub.com/gitpython-developers/GitPython/pull/1817)- Bump pre-commit/action from 3.0.0 to 3.0.1 by[@​dependabot](https://togithub.com/dependabot) in[https://github.com/gitpython-developers/GitPython/pull/1818](https://togithub.com/gitpython-developers/GitPython/pull/1818)- Bump Vampire/setup-wsl from 2.0.2 to 3.0.0 by[@​dependabot](https://togithub.com/dependabot) in[https://github.com/gitpython-developers/GitPython/pull/1819](https://togithub.com/gitpython-developers/GitPython/pull/1819)- Remove deprecated section in README.md by[@​marcm-ml](https://togithub.com/marcm-ml) in[https://github.com/gitpython-developers/GitPython/pull/1823](https://togithub.com/gitpython-developers/GitPython/pull/1823)- Keep temp files out of project dir and improve cleanup by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1825](https://togithub.com/gitpython-developers/GitPython/pull/1825)#### New Contributors- [@​PeterJCLaw](https://togithub.com/PeterJCLaw) made their firstcontribution in[https://github.com/gitpython-developers/GitPython/pull/1795](https://togithub.com/gitpython-developers/GitPython/pull/1795)- [@​et-repositories](https://togithub.com/et-repositories) madetheir first contribution in[https://github.com/gitpython-developers/GitPython/pull/1799](https://togithub.com/gitpython-developers/GitPython/pull/1799)- [@​marcm-ml](https://togithub.com/marcm-ml) made their firstcontribution in[https://github.com/gitpython-developers/GitPython/pull/1823](https://togithub.com/gitpython-developers/GitPython/pull/1823)**Full Changelog**:gitpython-developers/GitPython@3.1.41...3.1.42</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined),Automerge - At any time (no schedule defined).🚦 **Automerge**: Disabled by config. Please merge this manually once youare satisfied.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about theseupdates again.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR has been generated by [MendRenovate](https://www.mend.io/free-developer-tools/renovate/). Viewrepository job log[here](https://developer.mend.io/github/lettuce-financial/github-bot-signed-commit).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xNzMuMCIsInVwZGF0ZWRJblZlciI6IjM3LjE3My4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
%5d(https%3a%2f%2frenovatebot.com){kind=link}
renovatebot referenced this pull request in allenporter/flux-localFeb 16, 2024
[](https://renovatebot.com)This PR contains the following updates:| Package | Change | Age | Adoption | Passing | Confidence ||---|---|---|---|---|---|| [GitPython](https://togithub.com/gitpython-developers/GitPython) |`==3.1.41` -> `==3.1.42` |[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|[](https://docs.renovatebot.com/merge-confidence/)|---### Release Notes<details><summary>gitpython-developers/GitPython (GitPython)</summary>###[`v3.1.42`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.42)[CompareSource](https://togithub.com/gitpython-developers/GitPython/compare/3.1.41...3.1.42)#### What's Changed- Fix release link in changelog by[@​PeterJCLaw](https://togithub.com/PeterJCLaw) in[https://github.com/gitpython-developers/GitPython/pull/1795](https://togithub.com/gitpython-developers/GitPython/pull/1795)- Remove test dependency on sumtypes library by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1798](https://togithub.com/gitpython-developers/GitPython/pull/1798)- Pin Sphinx plugins to compatible versions by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1803](https://togithub.com/gitpython-developers/GitPython/pull/1803)- fix: treeNotSorted issue by[@​et-repositories](https://togithub.com/et-repositories) in[https://github.com/gitpython-developers/GitPython/pull/1799](https://togithub.com/gitpython-developers/GitPython/pull/1799)- Remove git.util.NullHandler by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1807](https://togithub.com/gitpython-developers/GitPython/pull/1807)- Clarify why GIT_PYTHON_GIT_EXECUTABLE may be set on failure by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1810](https://togithub.com/gitpython-developers/GitPython/pull/1810)- Report actual attempted Git command when Git.refresh fails by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1812](https://togithub.com/gitpython-developers/GitPython/pull/1812)- Don't suppress messages when logging is not configured by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1813](https://togithub.com/gitpython-developers/GitPython/pull/1813)- Pin Python 3.9.16 on Cygwin CI by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1814](https://togithub.com/gitpython-developers/GitPython/pull/1814)- Have initial refresh use a logger to warn by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1815](https://togithub.com/gitpython-developers/GitPython/pull/1815)- Omit warning prefix in "Bad git executable" message by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1816](https://togithub.com/gitpython-developers/GitPython/pull/1816)- Test with M1 macOS CI runner by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1817](https://togithub.com/gitpython-developers/GitPython/pull/1817)- Bump pre-commit/action from 3.0.0 to 3.0.1 by[@​dependabot](https://togithub.com/dependabot) in[https://github.com/gitpython-developers/GitPython/pull/1818](https://togithub.com/gitpython-developers/GitPython/pull/1818)- Bump Vampire/setup-wsl from 2.0.2 to 3.0.0 by[@​dependabot](https://togithub.com/dependabot) in[https://github.com/gitpython-developers/GitPython/pull/1819](https://togithub.com/gitpython-developers/GitPython/pull/1819)- Remove deprecated section in README.md by[@​marcm-ml](https://togithub.com/marcm-ml) in[https://github.com/gitpython-developers/GitPython/pull/1823](https://togithub.com/gitpython-developers/GitPython/pull/1823)- Keep temp files out of project dir and improve cleanup by[@​EliahKagan](https://togithub.com/EliahKagan) in[https://github.com/gitpython-developers/GitPython/pull/1825](https://togithub.com/gitpython-developers/GitPython/pull/1825)#### New Contributors- [@​PeterJCLaw](https://togithub.com/PeterJCLaw) made their firstcontribution in[https://github.com/gitpython-developers/GitPython/pull/1795](https://togithub.com/gitpython-developers/GitPython/pull/1795)- [@​et-repositories](https://togithub.com/et-repositories) madetheir first contribution in[https://github.com/gitpython-developers/GitPython/pull/1799](https://togithub.com/gitpython-developers/GitPython/pull/1799)- [@​marcm-ml](https://togithub.com/marcm-ml) made their firstcontribution in[https://github.com/gitpython-developers/GitPython/pull/1823](https://togithub.com/gitpython-developers/GitPython/pull/1823)**Full Changelog**:gitpython-developers/GitPython@3.1.41...3.1.42</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined),Automerge - At any time (no schedule defined).🚦 **Automerge**: Enabled.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick therebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about this updateagain.---- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, checkthis box---This PR has been generated by [MendRenovate](https://www.mend.io/free-developer-tools/renovate/). Viewrepository job log[here](https://developer.mend.io/github/allenporter/flux-local).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xNzMuMCIsInVwZGF0ZWRJblZlciI6IjM3LjE3My4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
None yet
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading.Please reload this page.
Fixes#1809
Fixes#1811
This modifies
Git.refreshto report the attempted Git command rather than alwaysgiteven if a different command was used. This affects thecommandattribute of theGitCommandNotFoundexception, as well as (and automatically) the text displayed ascmdline:since that is from that attribute.It also expands and adds more tests of
git.refesh(which callsGit.refresh) to encompass this behavior, verifying that they fail before#1809 is fixed and pass afterwards, as well as verify some behavior that was already established but not yet under test.Since#1811 would otherwise be exacerbated by these changes, I've also made it so those tests roll back the global changes they make that may affect other test by calling
git.refresh()with no arguments. I believe this can be considered to fix that issue.There is a bit more information in the commit messages, including about why I've taken that approach to#1811, at least so far, rather than specifically capturing and restoring the pieces of state that are currently affected.
This PR also includes some minor refactoring and comment revision for readability in some code called by
git.refreshthat should make no behavioral change. This includes an improvement to the wording of the comment inGit.refreshdiscussed in review comments on#1810; the opportunity for this came closer than I'd predicted in#1810 (comment). (It might be possible to improve it further.)Edit: Corrected "some behavior that was not established" to "some behavior that was already established but not yet under test".