Report actual attempted Git command when Git.refresh fails #1812

EliahKagan · 2024-01-25T09:38:56Z

This modifies Git.refresh to report the attempted Git command rather than always git even if a different command was used. This affects the command attribute of the GitCommandNotFound exception, as well as (and automatically) the text displayed as cmdline: since that is from that attribute.

It also expands and adds more tests of git.refesh (which calls Git.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.refresh that should make no behavioral change. This includes an improvement to the wording of the comment in Git.refresh discussed 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".

For when refreshing should succeed and when it should fail.

shutil.which does not always find the git that Popen would run, at least 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 the exception message shows the command the failed refresh used. This test fails due to a bug where "git" is always shown (gitpython-developers#1809).

See gitpython-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. See gitpython-developers#1811. This addresses it incompletely, because while it is probably not necessary while running the test suite to preserve an old False value of git.GIT_OK (most tests can't work if that happens anyway), the value of Git.GIT_PYTHON_GIT_EXECUTABLE is not the only other global state that effects the behavior of subsequently run tests and that may be changed as a result of the refresh 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 to gitpython-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 is called with no arguments, then the cleanup can just be a call to git.refresh(). Otherwise, sufficiently general cleanup may be more complicated.

For gitpython-developers#1811. This is the simple way to do it. This relies on git.refresh working when called when no arguments, so if that ever breaks, then it may be difficult or inefficient to investigate. But this is simpler than any alternatives that are equally or more robust. This is already better than the previous incomplete way that only restored Git.GIT_PYTHON_GIT_EXECUTABLE (and nothing in FetchInfo). Furthermore, because the tests already depend to a large extent on git.refreh working when called with no arguments, as otherwise the initial refresh may not have set Git.GIT_PYTHON_GIT_EXECUTABLE usably, 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 to briefly state the claims they verify.

Byron

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)

test/test_git.py

EliahKagan added 10 commits January 24, 2024 13:38

Split test_refresh into two tests

ba4cbae

For when refreshing should succeed and when it should fail.

Simplify test of refresh that should succeed

f241808

shutil.which does not always find the git that Popen would run, at least on Windows, but it is no worse than running a "command -v" or "where" command as was previously done here via os.popen.

Have test of refresh that should fail assert command

f98aadd

This extends test_refresh_bad_git_path so that it asserts that the exception message shows the command the failed refresh used. This test fails due to a bug where "git" is always shown (gitpython-developers#1809).

Fix wrong GitCommandNotFound command from refresh

24d6067

See gitpython-developers#1809. The test extended in the previous commit now passes.

Also test refresh with an already-absolute bad path

3a34dee

Refactor _rollback_refresh slightly for clarity

ae28c98

State what the refresh tests verify

9b7e15f

This adds short docstrings to each of the four refresh tests to briefly state the claims they verify.

EliahKagan marked this pull request as ready for review January 25, 2024 09:45

EliahKagan mentioned this pull request Jan 26, 2024

Don't suppress messages when logging is not configured #1813

Merged

Byron approved these changes Jan 26, 2024

View reviewed changes

test/test_git.py Show resolved Hide resolved

Byron merged commit 9a7cec1 into gitpython-developers:main Jan 26, 2024

EliahKagan deleted the refresh-cmdline branch January 26, 2024 10:11

EliahKagan mentioned this pull request Jan 28, 2024

Git.refresh warns to stdout and does not log #1808

Closed

EliahKagan mentioned this pull request Feb 6, 2024

Have initial refresh use a logger to warn #1815

Merged

EliahKagan mentioned this pull request Apr 1, 2024

Bad git executable #1882

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Report actual attempted Git command when Git.refresh fails #1812

Report actual attempted Git command when Git.refresh fails #1812

Uh oh!

EliahKagan commented Jan 25, 2024 •

edited

Loading

Byron left a comment

Uh oh!

Uh oh!

Report actual attempted Git command when Git.refresh fails #1812

Report actual attempted Git command when Git.refresh fails #1812

Uh oh!

Conversation

EliahKagan commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Byron left a comment

Choose a reason for hiding this comment

Uh oh!

EliahKagan commented Jan 25, 2024 •

edited

Loading