From ba4cbae2f5dcc417b5c09054b539d30b2ea1b33d Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 24 Jan 2024 13:38:24 -0500 Subject: [PATCH 01/10] Split test_refresh into two tests For when refreshing should succeed and when it should fail. --- test/test_git.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index 3b9abc712..fc15b7842 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -306,11 +306,10 @@ def test_cmd_override(self): ): self.assertRaises(GitCommandNotFound, self.git.version) - def test_refresh(self): - # Test a bad git path refresh. + def test_refresh_bad_git_path(self): self.assertRaises(GitCommandNotFound, refresh, "yada") - # Test a good path refresh. + def test_refresh_good_git_path(self): which_cmd = "where" if os.name == "nt" else "command -v" path = os.popen("{0} git".format(which_cmd)).read().strip().split("\n")[0] refresh(path) From f24180873b4fb7e8790e3f36e1d570f456565f42 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 24 Jan 2024 14:05:01 -0500 Subject: [PATCH 02/10] Simplify test of refresh that should succeed 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. --- test/test_git.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index fc15b7842..6200896d9 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -310,8 +310,7 @@ def test_refresh_bad_git_path(self): self.assertRaises(GitCommandNotFound, refresh, "yada") def test_refresh_good_git_path(self): - which_cmd = "where" if os.name == "nt" else "command -v" - path = os.popen("{0} git".format(which_cmd)).read().strip().split("\n")[0] + path = shutil.which("git") refresh(path) def test_options_are_passed_to_git(self): From f98aadddfe390348c2858690fec9577301ea9ba9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 24 Jan 2024 17:14:26 -0500 Subject: [PATCH 03/10] Have test of refresh that should fail assert command 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 (#1809). --- test/test_git.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/test_git.py b/test/test_git.py index 6200896d9..ea47abfae 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -307,7 +307,11 @@ def test_cmd_override(self): self.assertRaises(GitCommandNotFound, self.git.version) def test_refresh_bad_git_path(self): - self.assertRaises(GitCommandNotFound, refresh, "yada") + path = "yada" + escaped_abspath = re.escape(str(Path(path).absolute())) + expected_pattern = rf"\n[ \t]*cmdline: {escaped_abspath}\Z" + with self.assertRaisesRegex(GitCommandNotFound, expected_pattern): + refresh(path) def test_refresh_good_git_path(self): path = shutil.which("git") From 24d6067d9f2a3666d5812e98d6f6bb2bc7cce27f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 24 Jan 2024 17:29:40 -0500 Subject: [PATCH 04/10] Fix wrong GitCommandNotFound command from refresh See #1809. The test extended in the previous commit now passes. --- git/cmd.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git/cmd.py b/git/cmd.py index 24ba71b5f..1893c6b73 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -483,7 +483,7 @@ def refresh(cls, path: Union[None, PathLike] = None) -> bool: else: # After the first refresh (when GIT_PYTHON_GIT_EXECUTABLE is no longer # None) we raise an exception. - raise GitCommandNotFound("git", err) + raise GitCommandNotFound(new_git, err) return has_git From 3a34dee196c2cfb4873f161d17388847f9f1c1cc Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 24 Jan 2024 18:15:08 -0500 Subject: [PATCH 05/10] Also test refresh with an already-absolute bad path --- test/test_git.py | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index ea47abfae..45f32c85c 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -306,16 +306,23 @@ def test_cmd_override(self): ): self.assertRaises(GitCommandNotFound, self.git.version) - def test_refresh_bad_git_path(self): - path = "yada" - escaped_abspath = re.escape(str(Path(path).absolute())) - expected_pattern = rf"\n[ \t]*cmdline: {escaped_abspath}\Z" + def test_refresh_bad_absolute_git_path(self): + absolute_path = str(Path("yada").absolute()) + expected_pattern = rf"\n[ \t]*cmdline: {re.escape(absolute_path)}\Z" with self.assertRaisesRegex(GitCommandNotFound, expected_pattern): - refresh(path) + refresh(absolute_path) - def test_refresh_good_git_path(self): - path = shutil.which("git") - refresh(path) + def test_refresh_bad_relative_git_path(self): + relative_path = "yada" + absolute_path = str(Path(relative_path).absolute()) + expected_pattern = rf"\n[ \t]*cmdline: {re.escape(absolute_path)}\Z" + with self.assertRaisesRegex(GitCommandNotFound, expected_pattern): + refresh(relative_path) + + def test_refresh_good_absolute_git_path(self): + absolute_path = shutil.which("git") + refresh(absolute_path) + self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, absolute_path) def test_options_are_passed_to_git(self): # This works because any command after git --version is ignored. From 5ad7cb2f4eeff030b27e0ece3a308464cad415b8 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 24 Jan 2024 18:46:48 -0500 Subject: [PATCH 06/10] Start work on making refresh tests restore state + Test a successful refresh with a relative path, which will be safer to do once the refresh tests restore changed state. See #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 #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. --- test/test_git.py | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index 45f32c85c..9b6059098 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -44,6 +44,15 @@ def _patch_out_env(name): os.environ[name] = old_value +@contextlib.contextmanager +def _restore_git_executable(): + old_git_executable = Git.GIT_PYTHON_GIT_EXECUTABLE + try: + yield old_git_executable # Let test code run that may rebind the attribute. + finally: + Git.GIT_PYTHON_GIT_EXECUTABLE = old_git_executable + + @ddt.ddt class TestGit(TestBase): @classmethod @@ -309,20 +318,36 @@ def test_cmd_override(self): def test_refresh_bad_absolute_git_path(self): absolute_path = str(Path("yada").absolute()) expected_pattern = rf"\n[ \t]*cmdline: {re.escape(absolute_path)}\Z" - with self.assertRaisesRegex(GitCommandNotFound, expected_pattern): - refresh(absolute_path) + + with _restore_git_executable() as old_git_executable: + with self.assertRaisesRegex(GitCommandNotFound, expected_pattern): + refresh(absolute_path) + self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, old_git_executable) def test_refresh_bad_relative_git_path(self): - relative_path = "yada" - absolute_path = str(Path(relative_path).absolute()) + absolute_path = str(Path("yada").absolute()) expected_pattern = rf"\n[ \t]*cmdline: {re.escape(absolute_path)}\Z" - with self.assertRaisesRegex(GitCommandNotFound, expected_pattern): - refresh(relative_path) + + with _restore_git_executable() as old_git_executable: + with self.assertRaisesRegex(GitCommandNotFound, expected_pattern): + refresh("yada") + self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, old_git_executable) def test_refresh_good_absolute_git_path(self): absolute_path = shutil.which("git") - refresh(absolute_path) - self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, absolute_path) + + with _restore_git_executable(): + refresh(absolute_path) + self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, absolute_path) + + def test_refresh_good_relative_git_path(self): + absolute_path = shutil.which("git") + dirname, basename = osp.split(absolute_path) + + with cwd(dirname): + with _restore_git_executable(): + refresh(basename) + self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, absolute_path) def test_options_are_passed_to_git(self): # This works because any command after git --version is ignored. From b3fc30eeb476e6d76a292b5a58788efa576fb9a1 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 24 Jan 2024 19:40:31 -0500 Subject: [PATCH 07/10] Re-refresh to restore state after refresh tests For #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. --- test/test_git.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index 9b6059098..080486a55 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -45,12 +45,12 @@ def _patch_out_env(name): @contextlib.contextmanager -def _restore_git_executable(): +def _rollback_refresh(): old_git_executable = Git.GIT_PYTHON_GIT_EXECUTABLE try: - yield old_git_executable # Let test code run that may rebind the attribute. + yield old_git_executable # Let test code run that may mutate class state. finally: - Git.GIT_PYTHON_GIT_EXECUTABLE = old_git_executable + refresh() @ddt.ddt @@ -319,7 +319,7 @@ def test_refresh_bad_absolute_git_path(self): absolute_path = str(Path("yada").absolute()) expected_pattern = rf"\n[ \t]*cmdline: {re.escape(absolute_path)}\Z" - with _restore_git_executable() as old_git_executable: + with _rollback_refresh() as old_git_executable: with self.assertRaisesRegex(GitCommandNotFound, expected_pattern): refresh(absolute_path) self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, old_git_executable) @@ -328,7 +328,7 @@ def test_refresh_bad_relative_git_path(self): absolute_path = str(Path("yada").absolute()) expected_pattern = rf"\n[ \t]*cmdline: {re.escape(absolute_path)}\Z" - with _restore_git_executable() as old_git_executable: + with _rollback_refresh() as old_git_executable: with self.assertRaisesRegex(GitCommandNotFound, expected_pattern): refresh("yada") self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, old_git_executable) @@ -336,7 +336,7 @@ def test_refresh_bad_relative_git_path(self): def test_refresh_good_absolute_git_path(self): absolute_path = shutil.which("git") - with _restore_git_executable(): + with _rollback_refresh(): refresh(absolute_path) self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, absolute_path) @@ -345,7 +345,7 @@ def test_refresh_good_relative_git_path(self): dirname, basename = osp.split(absolute_path) with cwd(dirname): - with _restore_git_executable(): + with _rollback_refresh(): refresh(basename) self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, absolute_path) From ae28c986310703a83d0e2495e0e50530231040ee Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 24 Jan 2024 20:55:51 -0500 Subject: [PATCH 08/10] Refactor _rollback_refresh slightly for clarity --- test/test_git.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index 080486a55..96730a7a5 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -46,9 +46,8 @@ def _patch_out_env(name): @contextlib.contextmanager def _rollback_refresh(): - old_git_executable = Git.GIT_PYTHON_GIT_EXECUTABLE try: - yield old_git_executable # Let test code run that may mutate class state. + yield Git.GIT_PYTHON_GIT_EXECUTABLE # Provide the old value for convenience. finally: refresh() From 3bb63f3766605a597fa1dbca2367cb6ffda4a3ba Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 24 Jan 2024 23:27:39 -0500 Subject: [PATCH 09/10] Condense FetchInfo.refresh using contextlib.suppress 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: https://github.com/gitpython-developers/GitPython/pull/1810#discussion_r1464468620 --- git/cmd.py | 2 +- git/remote.py | 33 +++++++++++---------------------- 2 files changed, 12 insertions(+), 23 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 1893c6b73..b3bd48b81 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -478,7 +478,7 @@ def refresh(cls, path: Union[None, PathLike] = None) -> bool: # We get here if this was the initial refresh and the refresh mode was # not error. Go ahead and set the GIT_PYTHON_GIT_EXECUTABLE such that we # discern the difference between the first refresh at import time - # and subsequent calls to refresh(). + # and subsequent calls to git.refresh or this refresh method. cls.GIT_PYTHON_GIT_EXECUTABLE = cls.git_exec_name else: # After the first refresh (when GIT_PYTHON_GIT_EXECUTABLE is no longer diff --git a/git/remote.py b/git/remote.py index 98a421b3a..7ebe566b3 100644 --- a/git/remote.py +++ b/git/remote.py @@ -5,30 +5,24 @@ """Module implementing a remote object allowing easy access to git remotes.""" +import contextlib import logging import re -from git.cmd import handle_process_output, Git +from git.cmd import Git, handle_process_output from git.compat import defenc, force_text +from git.config import GitConfigParser, SectionConstraint, cp from git.exc import GitCommandError +from git.refs import Head, Reference, RemoteReference, SymbolicReference, TagReference from git.util import ( - LazyMixin, - IterableObj, + CallableRemoteProgress, IterableList, + IterableObj, + LazyMixin, RemoteProgress, - CallableRemoteProgress, -) -from git.util import ( join_path, ) -from git.config import ( - GitConfigParser, - SectionConstraint, - cp, -) -from git.refs import Head, Reference, RemoteReference, SymbolicReference, TagReference - # typing------------------------------------------------------- from typing import ( @@ -345,18 +339,13 @@ class FetchInfo(IterableObj): @classmethod def refresh(cls) -> Literal[True]: """This gets called by the refresh function (see the top level __init__).""" - # clear the old values in _flag_map - try: + # Clear the old values in _flag_map. + with contextlib.suppress(KeyError): del cls._flag_map["t"] - except KeyError: - pass - - try: + with contextlib.suppress(KeyError): del cls._flag_map["-"] - except KeyError: - pass - # set the value given the git version + # Set the value given the git version. if Git().version_info[:2] >= (2, 10): cls._flag_map["t"] = cls.TAG_UPDATE else: From 9b7e15f69186a61bc6413a6da0a1ea13776522bc Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 25 Jan 2024 00:38:40 -0500 Subject: [PATCH 10/10] State what the refresh tests verify This adds short docstrings to each of the four refresh tests to briefly state the claims they verify. --- test/test_git.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/test_git.py b/test/test_git.py index 96730a7a5..a9af0b04d 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -315,6 +315,7 @@ def test_cmd_override(self): self.assertRaises(GitCommandNotFound, self.git.version) def test_refresh_bad_absolute_git_path(self): + """Bad absolute path arg is reported and not set.""" absolute_path = str(Path("yada").absolute()) expected_pattern = rf"\n[ \t]*cmdline: {re.escape(absolute_path)}\Z" @@ -324,6 +325,7 @@ def test_refresh_bad_absolute_git_path(self): self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, old_git_executable) def test_refresh_bad_relative_git_path(self): + """Bad relative path arg is resolved to absolute path and reported, not set.""" absolute_path = str(Path("yada").absolute()) expected_pattern = rf"\n[ \t]*cmdline: {re.escape(absolute_path)}\Z" @@ -333,6 +335,7 @@ def test_refresh_bad_relative_git_path(self): self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, old_git_executable) def test_refresh_good_absolute_git_path(self): + """Good absolute path arg is set.""" absolute_path = shutil.which("git") with _rollback_refresh(): @@ -340,6 +343,7 @@ def test_refresh_good_absolute_git_path(self): self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, absolute_path) def test_refresh_good_relative_git_path(self): + """Good relative path arg is resolved to absolute path and set.""" absolute_path = shutil.which("git") dirname, basename = osp.split(absolute_path)