Merge pull request #1815 from EliahKagan/refresh-env

Have initial refresh use a logger to warn

Author: Byron

Diff for: 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 for: 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)