Repository: gitpython-developers/GitPython
Pull request #1816 (2024-02-06 08:49:12)

Omit warning prefix in "Bad git executable" message
This builds on the changes in #1815 by omitting the `WARNING: ` prefix from the message itself that is produced when `GIT_PYTHON_REFRESH` is set to `warn` or a synonym (such as `w` or `log`) and the initial refresh performed at import time fails. This is the change suggested in https://github.com/gitpython-developers/GitPython/pull/1815#issuecomment-1925244493.

When logging is not configured, this changes what we see when the initial refresh fails:

```diff
-WARNING: Bad git executable.
+Bad git executable.
```

This may be considered slightly worse than before, but the advantage is that it doesn't print something that looks like a severity (or a redundancy) when logging *is* configured. For example, with common logging configuration such as by `logging.basicConfig()`, we get this improvement:

```diff
-CRITICAL:git.cmd:WARNING: Bad git executable.
+CRITICAL:git.cmd:Bad git executable.
```

This also makes some improvements to the tests. Besides updating them for the changed message (3250313), it:

- Test that the `GitCommandNotFound` exception raised when attempting a `git` operation, when the command in `Git.GIT_PYTHON_GIT_EXECUTABLE` does not exist but was set directly (rather than through the environment variable or an explicit call to one of the `refresh` functions), has an expected `command` attribute value (85ef145). This case has been under test for a long time, but the test had not checked that attribute.
- Reorder switch the order of that and an adjacent test so the tests of behavior that directly involves `GIT_PYTHON_GIT_EXECUTABLE` are all together (also in 85ef145).
- Use `Git` consistently in `test_git.py`, instead of using `Git` in some places and `type(self.git)` in others. See db36174 for the rationale.
- Change a call like `_logger.warning("%s", X)` to `_logger.warning(X)` in `test_index.py`, because GitPython uses the latter in all the other places where they would be equivalent. See 4b86993 for details. (This is relevant to this PR because 5faf621 adds another such call.)


Patch:
diff --git a/git/cmd.py b/git/cmd.py
index 22b5ea99..f58e6df5 100644
--- a/git/cmd.py
+++ b/git/cmd.py
@@ -450,7 +450,7 @@ class Git(LazyMixin):
                     )
 
                     if mode in warn:
-                        _logger.critical("WARNING: %s", err)
+                        _logger.critical(err)
                     else:
                         raise ImportError(err)
                 else:


Test patch:
diff --git a/test/test_git.py b/test/test_git.py
index ea3d067e..f12428f9 100644
--- a/test/test_git.py
+++ b/test/test_git.py
@@ -321,17 +321,17 @@ class TestGit(TestBase):
             self.assertIsInstance(n, int)
         # END verify number types
 
-    def test_cmd_override(self):
-        with mock.patch.object(
-            type(self.git),
-            "GIT_PYTHON_GIT_EXECUTABLE",
-            osp.join("some", "path", "which", "doesn't", "exist", "gitbinary"),
-        ):
-            self.assertRaises(GitCommandNotFound, self.git.version)
-
     def test_git_exc_name_is_git(self):
         self.assertEqual(self.git.git_exec_name, "git")
 
+    def test_cmd_override(self):
+        """Directly set bad GIT_PYTHON_GIT_EXECUTABLE causes git operations to raise."""
+        bad_path = osp.join("some", "path", "which", "doesn't", "exist", "gitbinary")
+        with mock.patch.object(Git, "GIT_PYTHON_GIT_EXECUTABLE", bad_path):
+            with self.assertRaises(GitCommandNotFound) as ctx:
+                self.git.version()
+            self.assertEqual(ctx.exception.command, [bad_path, "version"])
+
     @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."""
@@ -341,7 +341,7 @@ class TestGit(TestBase):
             "GIT_PYTHON_REFRESH": mode,
         }
         with _rollback_refresh():
-            type(self.git).GIT_PYTHON_GIT_EXECUTABLE = None  # Simulate startup.
+            Git.GIT_PYTHON_GIT_EXECUTABLE = None  # Simulate startup.
 
             with mock.patch.dict(os.environ, set_vars):
                 refresh()
@@ -356,14 +356,14 @@ class TestGit(TestBase):
             "GIT_PYTHON_REFRESH": mode,
         }
         with _rollback_refresh():
-            type(self.git).GIT_PYTHON_GIT_EXECUTABLE = None  # Simulate startup.
+            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.assertRegex(message, r"\ABad git executable.\n")
                 self.assertEqual(self.git.GIT_PYTHON_GIT_EXECUTABLE, "git")
 
     @ddt.data(("2",), ("r",), ("raise",), ("e",), ("error",))
@@ -375,7 +375,7 @@ class TestGit(TestBase):
             "GIT_PYTHON_REFRESH": mode,
         }
         with _rollback_refresh():
-            type(self.git).GIT_PYTHON_GIT_EXECUTABLE = None  # Simulate startup.
+            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"):
@@ -386,7 +386,7 @@ class TestGit(TestBase):
         absolute_path = shutil.which("git")
 
         with _rollback_refresh():
-            type(self.git).GIT_PYTHON_GIT_EXECUTABLE = None  # Simulate startup.
+            Git.GIT_PYTHON_GIT_EXECUTABLE = None  # Simulate startup.
 
             with mock.patch.dict(os.environ, {"GIT_PYTHON_GIT_EXECUTABLE": absolute_path}):
                 refresh()
@@ -397,8 +397,8 @@ class TestGit(TestBase):
         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.
+            with mock.patch.object(Git, "git_exec_name", ""):
+                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"}):
@@ -442,7 +442,7 @@ class TestGit(TestBase):
         """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 = ""
+            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"}):
diff --git a/test/test_index.py b/test/test_index.py
index f456f8be..ffe71450 100644
--- a/test/test_index.py
+++ b/test/test_index.py
@@ -174,7 +174,7 @@ class WinBashStatus:
         except UnicodeDecodeError:
             pass
         except LookupError as error:
-            _logger.warning("%s", str(error))  # Message already says "Unknown encoding:".
+            _logger.warning(str(error))  # Message already says "Unknown encoding:".
 
         # Assume UTF-8. If invalid, substitute Unicode replacement characters.
         return stdout.decode("utf-8", errors="replace")