From 8dc8eb9e76a3162636856fa31b71ba10b33705eb Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 2 Feb 2024 01:06:26 -0500 Subject: [PATCH 1/4] Test established zero-argument refresh() behavior This adds tests like the existing ones but for when git.refresh is called with no arguments and the path is provided as the value of the GIT_PYTHON_GIT_EXECUTABLE environment variable. The preexisting tests, which this retains unchanged except with slightly more specific names to avoid confusion with the new tests, are of git.refresh(path). One benefit of these tests is to make a subtle but important aspect of the established behavior clear: relative paths are immediately resolved when passed as a path argument, but they are kept relative when given as the value of the environment variable. --- test/test_git.py | 69 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 64 insertions(+), 5 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index a1c6211db..1b630fee0 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -46,9 +46,24 @@ def _patch_out_env(name): @contextlib.contextmanager def _rollback_refresh(): + old_git_executable = Git.GIT_PYTHON_GIT_EXECUTABLE + + if old_git_executable is None: + raise RuntimeError("no executable string (need initial refresh before test)") + try: - yield Git.GIT_PYTHON_GIT_EXECUTABLE # Provide the old value for convenience. + yield old_git_executable # Provide the old value for convenience. finally: + # The cleanup refresh should always raise an exception if it fails, since if it + # fails then previously discovered test results could be misleading and, more + # importantly, subsequent tests may be unable to run or give misleading results. + # So pre-set a non-None value, so that the cleanup will be a "second" refresh. + # This covers cases where a test has set it to None to test a "first" refresh. + Git.GIT_PYTHON_GIT_EXECUTABLE = Git.git_exec_name + + # Do the cleanup refresh. This sets Git.GIT_PYTHON_GIT_EXECUTABLE to old_value + # in most cases. The reason to call it is to achieve other associated state + # changes as well, which include updating attributes of the FetchInfo class. refresh() @@ -314,7 +329,51 @@ def test_cmd_override(self): ): self.assertRaises(GitCommandNotFound, self.git.version) - def test_refresh_bad_absolute_git_path(self): + def test_refresh_from_bad_absolute_git_path_env(self): + """Bad absolute path from environment is reported and not set.""" + absolute_path = str(Path("yada").absolute()) + expected_pattern = rf"\n[ \t]*cmdline: {re.escape(absolute_path)}\Z" + + with _rollback_refresh() as old_git_executable: + with mock.patch.dict(os.environ, {"GIT_PYTHON_GIT_EXECUTABLE": absolute_path}): + with self.assertRaisesRegex(GitCommandNotFound, expected_pattern): + refresh() + self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, old_git_executable) + + def test_refresh_from_bad_relative_git_path_env(self): + """Bad relative path from environment is kept relative and reported, not set.""" + # Relative paths are not resolved when refresh() is called with no arguments, so + # use a string that's very unlikely to be a command name found in a path lookup. + relative_path = "yada-e47e70c6-acbf-40f8-ad65-13af93c2195b" + expected_pattern = rf"\n[ \t]*cmdline: {re.escape(relative_path)}\Z" + + with _rollback_refresh() as old_git_executable: + with mock.patch.dict(os.environ, {"GIT_PYTHON_GIT_EXECUTABLE": relative_path}): + with self.assertRaisesRegex(GitCommandNotFound, expected_pattern): + refresh() + self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, old_git_executable) + + def test_refresh_from_good_absolute_git_path_env(self): + """Good absolute path from environment is set.""" + absolute_path = shutil.which("git") + + with _rollback_refresh(): + with mock.patch.dict(os.environ, {"GIT_PYTHON_GIT_EXECUTABLE": absolute_path}): + refresh() + self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, absolute_path) + + def test_refresh_from_good_relative_git_path_env(self): + """Good relative path from environment is kept relative and set.""" + with _rollback_refresh(): + # Set a string that wouldn't work and isn't "git". + type(self.git).GIT_PYTHON_GIT_EXECUTABLE = "" + + # Now observe if setting the environment variable to "git" takes effect. + with mock.patch.dict(os.environ, {"GIT_PYTHON_GIT_EXECUTABLE": "git"}): + refresh() + self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, "git") + + def test_refresh_with_bad_absolute_git_path_arg(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,7 +383,7 @@ def test_refresh_bad_absolute_git_path(self): refresh(absolute_path) self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, old_git_executable) - def test_refresh_bad_relative_git_path(self): + def test_refresh_with_bad_relative_git_path_arg(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" @@ -334,7 +393,7 @@ def test_refresh_bad_relative_git_path(self): refresh("yada") self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, old_git_executable) - def test_refresh_good_absolute_git_path(self): + def test_refresh_with_good_absolute_git_path_arg(self): """Good absolute path arg is set.""" absolute_path = shutil.which("git") @@ -342,7 +401,7 @@ def test_refresh_good_absolute_git_path(self): refresh(absolute_path) self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, absolute_path) - def test_refresh_good_relative_git_path(self): + def test_refresh_with_good_relative_git_path_arg(self): """Good relative path arg is resolved to absolute path and set.""" absolute_path = shutil.which("git") dirname, basename = osp.split(absolute_path) From 147e80b88999669d6e499556e7dc083bdf84c62e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 2 Feb 2024 05:31:50 -0500 Subject: [PATCH 2/4] Test current initial-refresh behavior This adds tests of the initial refresh that is attempted automatically on import. All the refresh tests prior to this point test subsequent refreshes. Those tests are kept, and new ones are added that simulate the condition of not having yet done the initial refresh by setting Git.GIT_PYTHON_GIT_EXECUTABLE to None. Some current behavior these tests assert may change for #1808. --- test/test_git.py | 77 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-) diff --git a/test/test_git.py b/test/test_git.py index 1b630fee0..ab068e5c2 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -6,6 +6,7 @@ import contextlib import gc import inspect +import io import logging import os import os.path as osp @@ -329,6 +330,80 @@ def test_cmd_override(self): ): self.assertRaises(GitCommandNotFound, self.git.version) + def test_git_exc_name_is_git(self): + self.assertEqual(self.git.git_exec_name, "git") + + @ddt.data(("0",), ("q",), ("quiet",), ("s",), ("silence",), ("n",), ("none",)) + def test_initial_refresh_from_bad_git_path_env_quiet(self, case): + """In "q" mode, bad initial path sets "git" and is quiet.""" + (mode,) = case + set_vars = { + "GIT_PYTHON_GIT_EXECUTABLE": str(Path("yada").absolute()), # Any bad path. + "GIT_PYTHON_REFRESH": mode, + } + with _rollback_refresh(): + type(self.git).GIT_PYTHON_GIT_EXECUTABLE = None # Simulate startup. + + with mock.patch.dict(os.environ, set_vars): + refresh() + self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, "git") + + @ddt.data(("1",), ("w",), ("warn",), ("warning",)) + def test_initial_refresh_from_bad_git_path_env_warn(self, case): + """In "w" mode, bad initial path sets "git" and warns.""" + (mode,) = case + env_vars = { + "GIT_PYTHON_GIT_EXECUTABLE": str(Path("yada").absolute()), # Any bad path. + "GIT_PYTHON_REFRESH": mode, + } + with _rollback_refresh(): + type(self.git).GIT_PYTHON_GIT_EXECUTABLE = None # Simulate startup. + + with mock.patch.dict(os.environ, env_vars): + with contextlib.redirect_stdout(io.StringIO()) as out: + refresh() + self.assertRegex(out.getvalue(), r"\AWARNING: Bad git executable.\n") + self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, "git") + + @ddt.data(("2",), ("r",), ("raise",), ("e",), ("error",)) + def test_initial_refresh_from_bad_git_path_env_error(self, case): + """In "e" mode, bad initial path raises an exception.""" + (mode,) = case + env_vars = { + "GIT_PYTHON_GIT_EXECUTABLE": str(Path("yada").absolute()), # Any bad path. + "GIT_PYTHON_REFRESH": mode, + } + with _rollback_refresh(): + type(self.git).GIT_PYTHON_GIT_EXECUTABLE = None # Simulate startup. + + with mock.patch.dict(os.environ, env_vars): + with self.assertRaisesRegex(ImportError, r"\ABad git executable.\n"): + refresh() + + def test_initial_refresh_from_good_absolute_git_path_env(self): + """Good initial absolute path from environment is set.""" + absolute_path = shutil.which("git") + + with _rollback_refresh(): + type(self.git).GIT_PYTHON_GIT_EXECUTABLE = None # Simulate startup. + + with mock.patch.dict(os.environ, {"GIT_PYTHON_GIT_EXECUTABLE": absolute_path}): + refresh() + self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, absolute_path) + + def test_initial_refresh_from_good_relative_git_path_env(self): + """Good initial relative path from environment is kept relative and set.""" + with _rollback_refresh(): + # Set the fallback to a string that wouldn't work and isn't "git", so we are + # more likely to detect if "git" is not set from the environment variable. + with mock.patch.object(type(self.git), "git_exec_name", ""): + type(self.git).GIT_PYTHON_GIT_EXECUTABLE = None # Simulate startup. + + # Now observe if setting the environment variable to "git" takes effect. + with mock.patch.dict(os.environ, {"GIT_PYTHON_GIT_EXECUTABLE": "git"}): + refresh() + self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, "git") + def test_refresh_from_bad_absolute_git_path_env(self): """Bad absolute path from environment is reported and not set.""" absolute_path = str(Path("yada").absolute()) @@ -365,7 +440,7 @@ def test_refresh_from_good_absolute_git_path_env(self): def test_refresh_from_good_relative_git_path_env(self): """Good relative path from environment is kept relative and set.""" with _rollback_refresh(): - # Set a string that wouldn't work and isn't "git". + # Set as the executable name a string that wouldn't work and isn't "git". type(self.git).GIT_PYTHON_GIT_EXECUTABLE = "" # Now observe if setting the environment variable to "git" takes effect. From ac50d8e40a9362073033f28f9a69bcd04c62f378 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 2 Feb 2024 05:48:15 -0500 Subject: [PATCH 3/4] Change warning refresh-mode tests to expect logging This is instead of the current behavior writing the message to stdout. This commit does not change the behavior of the code under test, but it changes tests to assert the following: - "Bad git executable" messages are logged, at level CRITICAL. - "log" (and "l") is recognized as another synonym of "warn". - "silent" is recognized as a synonym of "quiet" (as "silence" is). Although it is ambiguous whether this should be logged at level ERROR or CRITICAL, because git.refresh is still expected to be usable and can be called manually, not having a working git is a condition in which GitPython, and any program that really relies on its functionality, should be expected not work. That is the general rationale for using CRIICAL here. There are also two specific reasons: - Existing messages GitPython logs as ERROR typically represent errors in individual operations on repositories, which could fail without indicating that GitPython's overall functionality is in an unusable state. Using the higher CRITICAL level for this situation makes sense for contrast. - Prior to #1813, logging messsges emitted from GitPython modules, no matter the level, were suppressed when logging was not configured, but because this message was printed instead of being logged, it was still shown. Now that it is to be logged, there may be a benefit to have an easy way for someone to bring back a close approximation of the old behavior. Having this message be at a higher logging level makes that easier to do. (This is a less important reason, as that should rarely be done.) test_initial_refresh_from_bad_git_path_env_warn is the main changed test. All tests should pass again once code is changed for #1808. --- test/test_git.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index ab068e5c2..ea3d067ee 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -6,7 +6,6 @@ import contextlib import gc import inspect -import io import logging import os import os.path as osp @@ -333,7 +332,7 @@ def test_cmd_override(self): def test_git_exc_name_is_git(self): self.assertEqual(self.git.git_exec_name, "git") - @ddt.data(("0",), ("q",), ("quiet",), ("s",), ("silence",), ("n",), ("none",)) + @ddt.data(("0",), ("q",), ("quiet",), ("s",), ("silence",), ("silent",), ("n",), ("none",)) def test_initial_refresh_from_bad_git_path_env_quiet(self, case): """In "q" mode, bad initial path sets "git" and is quiet.""" (mode,) = case @@ -348,9 +347,9 @@ def test_initial_refresh_from_bad_git_path_env_quiet(self, case): refresh() self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, "git") - @ddt.data(("1",), ("w",), ("warn",), ("warning",)) + @ddt.data(("1",), ("w",), ("warn",), ("warning",), ("l",), ("log",)) def test_initial_refresh_from_bad_git_path_env_warn(self, case): - """In "w" mode, bad initial path sets "git" and warns.""" + """In "w" mode, bad initial path sets "git" and warns, by logging.""" (mode,) = case env_vars = { "GIT_PYTHON_GIT_EXECUTABLE": str(Path("yada").absolute()), # Any bad path. @@ -360,9 +359,11 @@ def test_initial_refresh_from_bad_git_path_env_warn(self, case): type(self.git).GIT_PYTHON_GIT_EXECUTABLE = None # Simulate startup. with mock.patch.dict(os.environ, env_vars): - with contextlib.redirect_stdout(io.StringIO()) as out: + with self.assertLogs(cmd.__name__, logging.CRITICAL) as ctx: refresh() - self.assertRegex(out.getvalue(), r"\AWARNING: Bad git executable.\n") + self.assertEqual(len(ctx.records), 1) + message = ctx.records[0].getMessage() + self.assertRegex(message, r"\AWARNING: Bad git executable.\n") self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, "git") @ddt.data(("2",), ("r",), ("raise",), ("e",), ("error",)) From 3a6e3efb3592748768314969998604e136384622 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 26 Jan 2024 04:02:18 -0500 Subject: [PATCH 4/4] Have initial refresh use a logger to warn For #1808. See ac50d8e for details on the changes. --- git/cmd.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index a75d822fa..22b5ea99e 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -409,15 +409,15 @@ def refresh(cls, path: Union[None, PathLike] = None) -> bool: # expect GIT_PYTHON_REFRESH to either be unset or be one of the # following values: # - # 0|q|quiet|s|silence|n|none - # 1|w|warn|warning - # 2|r|raise|e|error + # 0|q|quiet|s|silence|silent|n|none + # 1|w|warn|warning|l|log + # 2|r|raise|e|error|exception mode = os.environ.get(cls._refresh_env_var, "raise").lower() - quiet = ["quiet", "q", "silence", "s", "none", "n", "0"] - warn = ["warn", "w", "warning", "1"] - error = ["error", "e", "raise", "r", "2"] + quiet = ["quiet", "q", "silence", "s", "silent", "none", "n", "0"] + warn = ["warn", "w", "warning", "log", "l", "1"] + error = ["error", "e", "exception", "raise", "r", "2"] if mode in quiet: pass @@ -428,10 +428,10 @@ def refresh(cls, path: Union[None, PathLike] = None) -> bool: %s All git commands will error until this is rectified. - This initial warning can be silenced or aggravated in the future by setting the + This initial message can be silenced or aggravated in the future by setting the $%s environment variable. Use one of the following values: - - %s: for no warning or exception - - %s: for a printed warning + - %s: for no message or exception + - %s: for a warning message (logged at level CRITICAL, displayed by default) - %s: for a raised exception Example: @@ -450,7 +450,7 @@ def refresh(cls, path: Union[None, PathLike] = None) -> bool: ) if mode in warn: - print("WARNING: %s" % err) + _logger.critical("WARNING: %s", err) else: raise ImportError(err) else: @@ -460,8 +460,8 @@ def refresh(cls, path: Union[None, PathLike] = None) -> bool: %s environment variable has been set but it has been set with an invalid value. Use only the following values: - - %s: for no warning or exception - - %s: for a printed warning + - %s: for no message or exception + - %s: for a warning message (logged at level CRITICAL, displayed by default) - %s: for a raised exception """ )