Merge pull request #1815 from EliahKagan/refresh-env

Byron · web-flow · commit db6fb902fa00 · 2024-02-02T22:08:52.000+01:00
Have initial refresh use a logger to warn
diff --git 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
 """
 )
diff --git 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,127 @@ def test_cmd_override(self):
 ):
 self.assertRaises(GitCommandNotFound, self.git.version)
 
- def test_refresh_bad_absolute_git_path(self):
+ def test_git_exc_name_is_git(self):
+ self.assertEqual(self.git.git_exec_name, "git")
+
+ @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
+ 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",), ("l",), ("log",))
+ def test_initial_refresh_from_bad_git_path_env_warn(self, case):
+ """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.
+ "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.assertLogs(cmd.__name__, logging.CRITICAL) as ctx:
+ refresh()
+ 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",))
+ 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())
+ 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 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.
+ 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 +459,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,15 +469,15 @@ 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")
 
 with _rollback_refresh():
 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)
