Upgrade and broaden flake8, fixing style problems and bugs

.flake8

@@ -26,7 +26,7 @@ ignore = E265,E266,E731,E704,
          D,
          RST, RST3
 
-exclude = .tox,.venv,build,dist,doc,git/ext/,test
+exclude = .tox,.venv,build,dist,doc,git/ext/
 
 rst-roles =  # for flake8-RST-docstrings
     attr,class,func,meth,mod,obj,ref,term,var  # used by sphinx

.pre-commit-config.yaml

@@ -1,15 +1,15 @@
 repos:
   - repo: https://github.com/PyCQA/flake8
-    rev: 6.0.0
+    rev: 6.1.0
     hooks:
       - id: flake8
         additional_dependencies:
           [
-            flake8-bugbear==22.12.6,
-            flake8-comprehensions==3.10.1,
+            flake8-bugbear==23.9.16,
+            flake8-comprehensions==3.14.0,
             flake8-typing-imports==1.14.0,
           ]
-        exclude: ^doc|^git/ext/|^test/
+        exclude: ^doc|^git/ext/
 
   - repo: https://github.com/pre-commit/pre-commit-hooks
     rev: v4.4.0

git/objects/submodule/base.py

@@ -1403,7 +1403,7 @@ def iter_items(
             # END handle critical error
 
             # Make sure we are looking at a submodule object
-            if type(sm) != git.objects.submodule.base.Submodule:
+            if type(sm) is not git.objects.submodule.base.Submodule:
                 continue
 
             # fill in remaining info - saves time as it doesn't have to be parsed again

git/refs/log.py

@@ -244,7 +244,7 @@ def entry_at(cls, filepath: PathLike, index: int) -> "RefLogEntry":
             for i in range(index + 1):
                 line = fp.readline()
                 if not line:
-                    raise IndexError(f"Index file ended at line {i+1}, before given index was reached")
+                    raise IndexError(f"Index file ended at line {i + 1}, before given index was reached")
                 # END abort on eof
             # END handle runup
 

git/repo/base.py

@@ -206,7 +206,8 @@ def __init__(
         if expand_vars and re.search(self.re_envvars, epath):
             warnings.warn(
                 "The use of environment variables in paths is deprecated"
-                + "\nfor security reasons and may be removed in the future!!"
+                + "\nfor security reasons and may be removed in the future!!",
+                stacklevel=1,
             )
         epath = expand_path(epath, expand_vars)
         if epath is not None:

git/util.py

@@ -1136,7 +1136,7 @@ class IterableClassWatcher(type):
 
     def __init__(cls, name: str, bases: Tuple, clsdict: Dict) -> None:
         for base in bases:
-            if type(base) == IterableClassWatcher:
+            if type(base) is IterableClassWatcher:
                 warnings.warn(
                     f"GitPython Iterable subclassed by {name}. "
                     "Iterable is deprecated due to naming clash since v3.1.18"

test/lib/helper.py

@@ -94,17 +94,16 @@ def wrapper(self):
         os.mkdir(path)
         keep = False
         try:
-            try:
-                return func(self, path)
-            except Exception:
-                log.info(
-                    "Test %s.%s failed, output is at %r\n",
-                    type(self).__name__,
-                    func.__name__,
-                    path,
-                )
-                keep = True
-                raise
+            return func(self, path)
+        except Exception:
+            log.info(
+                "Test %s.%s failed, output is at %r\n",
+                type(self).__name__,
+                func.__name__,
+                path,
+            )
+            keep = True
+            raise
         finally:
             # Need to collect here to be sure all handles have been closed. It appears
             # a windows-only issue. In fact things should be deleted, as well as
@@ -147,12 +146,11 @@ def repo_creator(self):
             prev_cwd = os.getcwd()
             os.chdir(rw_repo.working_dir)
             try:
-                try:
-                    return func(self, rw_repo)
-                except:  # noqa E722
-                    log.info("Keeping repo after failure: %s", repo_dir)
-                    repo_dir = None
-                    raise
+                return func(self, rw_repo)
+            except:  # noqa E722
+                log.info("Keeping repo after failure: %s", repo_dir)
+                repo_dir = None
+                raise
             finally:
                 os.chdir(prev_cwd)
                 rw_repo.git.clear_cache()

test/test_commit.py

@@ -277,7 +277,7 @@ def __init__(self, *args, **kwargs):
                 super(Child, self).__init__(*args, **kwargs)
 
         child_commits = list(Child.iter_items(self.rorepo, "master", paths=("CHANGES", "AUTHORS")))
-        assert type(child_commits[0]) == Child
+        assert type(child_commits[0]) is Child
 
     def test_iter_items(self):
         # pretty not allowed
@@ -525,12 +525,12 @@ def test_trailers(self):
 
         # check that trailer stays empty for multiple msg combinations
         msgs = [
-            f"Subject\n",
-            f"Subject\n\nBody with some\nText\n",
-            f"Subject\n\nBody with\nText\n\nContinuation but\n doesn't contain colon\n",
-            f"Subject\n\nBody with\nText\n\nContinuation but\n only contains one :\n",
-            f"Subject\n\nBody with\nText\n\nKey: Value\nLine without colon\n",
-            f"Subject\n\nBody with\nText\n\nLine without colon\nKey: Value\n",
+            "Subject\n",
+            "Subject\n\nBody with some\nText\n",
+            "Subject\n\nBody with\nText\n\nContinuation but\n doesn't contain colon\n",
+            "Subject\n\nBody with\nText\n\nContinuation but\n only contains one :\n",
+            "Subject\n\nBody with\nText\n\nKey: Value\nLine without colon\n",
+            "Subject\n\nBody with\nText\n\nLine without colon\nKey: Value\n",
         ]
 
         for msg in msgs:

test/test_diff.py

@@ -7,7 +7,6 @@
 import ddt
 import shutil
 import tempfile
-import unittest
 from git import (
     Repo,
     GitCommandError,

test/test_docs.py

@@ -263,9 +263,9 @@ def test_references_and_objects(self, rw_dir):
         # [8-test_references_and_objects]
         hc = repo.head.commit
         hct = hc.tree
-        hc != hct  # @NoEffect
-        hc != repo.tags[0]  # @NoEffect
-        hc == repo.head.reference.commit  # @NoEffect
+        hc != hct  # noqa: B015  # @NoEffect
+        hc != repo.tags[0]  # noqa: B015  # @NoEffect
+        hc == repo.head.reference.commit  # noqa: B015  # @NoEffect
         # ![8-test_references_and_objects]
 
         # [9-test_references_and_objects]
@@ -369,7 +369,7 @@ def test_references_and_objects(self, rw_dir):
         # The index contains all blobs in a flat list
         assert len(list(index.iter_blobs())) == len([o for o in repo.head.commit.tree.traverse() if o.type == "blob"])
         # Access blob objects
-        for (_path, _stage), entry in index.entries.items():
+        for (_path, _stage), _entry in index.entries.items():
             pass
         new_file_path = os.path.join(repo.working_tree_dir, "new-file-name")
         open(new_file_path, "w").close()

test/test_git.py

@@ -195,17 +195,12 @@ def test_version(self):
         # END verify number types
 
     def test_cmd_override(self):
-        prev_cmd = self.git.GIT_PYTHON_GIT_EXECUTABLE
-        exc = GitCommandNotFound
-        try:
-            # set it to something that doesn't exist, assure it raises
-            type(self.git).GIT_PYTHON_GIT_EXECUTABLE = osp.join(
-                "some", "path", "which", "doesn't", "exist", "gitbinary"
-            )
-            self.assertRaises(exc, self.git.version)
-        finally:
-            type(self.git).GIT_PYTHON_GIT_EXECUTABLE = prev_cmd
-        # END undo adjustment
+        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_refresh(self):
         # test a bad git path refresh
@@ -250,7 +245,7 @@ def test_insert_after_kwarg_raises(self):
 
     def test_env_vars_passed_to_git(self):
         editor = "non_existent_editor"
-        with mock.patch.dict("os.environ", {"GIT_EDITOR": editor}):  # @UndefinedVariable
+        with mock.patch.dict(os.environ, {"GIT_EDITOR": editor}):
             self.assertEqual(self.git.var("GIT_EDITOR"), editor)
 
     @with_rw_directory

test/test_quick_doc.py

@@ -1,6 +1,3 @@
-import pytest
-
-
 from test.lib import TestBase
 from test.lib.helper import with_rw_directory
 
@@ -25,10 +22,11 @@ def test_init_repo_object(self, path_to_dir):
         repo = Repo(path_to_dir)
         # ![2-test_init_repo_object]
 
+        del repo  # Avoids "assigned to but never used" warning. Doesn't go in the docs.
+
     @with_rw_directory
     def test_cloned_repo_object(self, local_dir):
         from git import Repo
-        import git
 
         # code to clone from url
         # [1-test_cloned_repo_object]
@@ -72,7 +70,7 @@ def test_cloned_repo_object(self, local_dir):
 
         # [6-test_cloned_repo_object]
         commits_for_file_generator = repo.iter_commits(all=True, max_count=10, paths=update_file)
-        commits_for_file = [c for c in commits_for_file_generator]
+        commits_for_file = list(commits_for_file_generator)
         commits_for_file
 
         # Outputs: [<git.Commit "SHA1-HEX_HASH-2">,
@@ -136,7 +134,7 @@ def test_cloned_repo_object(self, local_dir):
 
         # Compare commit to commit
         # [11.3-test_cloned_repo_object]
-        first_commit = [c for c in repo.iter_commits(all=True)][-1]
+        first_commit = list(repo.iter_commits(all=True))[-1]
         diffs = repo.head.commit.diff(first_commit)
         for d in diffs:
             print(d.a_path)
@@ -154,7 +152,7 @@ def test_cloned_repo_object(self, local_dir):
 
         # Previous commit tree
         # [13-test_cloned_repo_object]
-        prev_commits = [c for c in repo.iter_commits(all=True, max_count=10)]  # last 10 commits from all branches
+        prev_commits = list(repo.iter_commits(all=True, max_count=10))  # last 10 commits from all branches
         tree = prev_commits[0].tree
         # ![13-test_cloned_repo_object]
 
@@ -210,7 +208,7 @@ def print_files_from_git(root, level=0):
 
         # print previous tree
         # [18.1-test_cloned_repo_object]
-        commits_for_file = [c for c in repo.iter_commits(all=True, paths=print_file)]
+        commits_for_file = list(repo.iter_commits(all=True, paths=print_file))
         tree = commits_for_file[-1].tree  # gets the first commit tree
         blob = tree[print_file]
 

test/test_remote.py

@@ -160,7 +160,7 @@ def _do_test_push_result(self, results, remote):
             # END error checking
         # END for each info
 
-        if any([info.flags & info.ERROR for info in results]):
+        if any(info.flags & info.ERROR for info in results):
             self.assertRaises(GitCommandError, results.raise_if_error)
         else:
             # No errors, so this should do nothing

test/test_repo.py

@@ -252,7 +252,8 @@ def test_clone_from_with_path_contains_unicode(self):
 
     @with_rw_directory
     @skip(
-        "the referenced repository was removed, and one needs to setup a new password controlled repo under the orgs control"
+        """The referenced repository was removed, and one needs to set up a new
+        password controlled repo under the org's control."""
     )
     def test_leaking_password_in_clone_logs(self, rw_dir):
         password = "fakepassword1234"
@@ -758,9 +759,9 @@ def test_blame_complex_revision(self, git):
 
     @mock.patch.object(Git, "_call_process")
     def test_blame_accepts_rev_opts(self, git):
-        res = self.rorepo.blame("HEAD", "README.md", rev_opts=["-M", "-C", "-C"])
         expected_args = ["blame", "HEAD", "-M", "-C", "-C", "--", "README.md"]
         boilerplate_kwargs = {"p": True, "stdout_as_string": False}
+        self.rorepo.blame("HEAD", "README.md", rev_opts=["-M", "-C", "-C"])
         git.assert_called_once_with(*expected_args, **boilerplate_kwargs)
 
     @skipIf(
@@ -846,18 +847,13 @@ def test_comparison_and_hash(self):
 
     @with_rw_directory
     def test_tilde_and_env_vars_in_repo_path(self, rw_dir):
-        ph = os.environ.get("HOME")
-        try:
+        with mock.patch.dict(os.environ, {"HOME": rw_dir}):
             os.environ["HOME"] = rw_dir
             Repo.init(osp.join("~", "test.git"), bare=True)
 
+        with mock.patch.dict(os.environ, {"FOO": rw_dir}):
             os.environ["FOO"] = rw_dir
             Repo.init(osp.join("$FOO", "test.git"), bare=True)
-        finally:
-            if ph:
-                os.environ["HOME"] = ph
-                del os.environ["FOO"]
-        # end assure HOME gets reset to what it was
 
     def test_git_cmd(self):
         # test CatFileContentStream, just to be very sure we have no fencepost errors
@@ -971,7 +967,7 @@ def _assert_rev_parse(self, name):
         # history with number
         ni = 11
         history = [obj.parents[0]]
-        for pn in range(ni):
+        for _ in range(ni):
             history.append(history[-1].parents[0])
         # END get given amount of commits
 
@@ -1329,6 +1325,7 @@ def test_git_work_tree_env(self, rw_dir):
         # move .git directory to a subdirectory
         # set GIT_DIR and GIT_WORK_TREE appropriately
         # check that repo.working_tree_dir == rw_dir
+
         self.rorepo.clone(join_path_native(rw_dir, "master_repo"))
 
         repo_dir = join_path_native(rw_dir, "master_repo")
@@ -1338,16 +1335,12 @@ def test_git_work_tree_env(self, rw_dir):
         os.mkdir(new_subdir)
         os.rename(old_git_dir, new_git_dir)
 
-        oldenv = os.environ.copy()
-        os.environ["GIT_DIR"] = new_git_dir
-        os.environ["GIT_WORK_TREE"] = repo_dir
+        to_patch = {"GIT_DIR": new_git_dir, "GIT_WORK_TREE": repo_dir}
 
-        try:
+        with mock.patch.dict(os.environ, to_patch):
             r = Repo()
             self.assertEqual(r.working_tree_dir, repo_dir)
             self.assertEqual(r.working_dir, repo_dir)
-        finally:
-            os.environ = oldenv
 
     @with_rw_directory
     def test_rebasing(self, rw_dir):

test/test_submodule.py

@@ -111,7 +111,7 @@ def _do_base_tests(self, rwrepo):
 
         # force it to reread its information
         del smold._url
-        smold.url == sm.url  # @NoEffect
+        smold.url == sm.url  # noqa: B015  # @NoEffect
 
         # test config_reader/writer methods
         sm.config_reader()
@@ -248,7 +248,7 @@ def _do_base_tests(self, rwrepo):
             assert csm.module_exists()
 
             # tracking branch once again
-            csm.module().head.ref.tracking_branch() is not None  # @NoEffect
+            assert csm.module().head.ref.tracking_branch() is not None
 
             # this flushed in a sub-submodule
             assert len(list(rwrepo.iter_submodules())) == 2
@@ -480,8 +480,9 @@ def test_base_bare(self, rwrepo):
         File "C:\\projects\\gitpython\\git\\cmd.py", line 559, in execute
         raise GitCommandNotFound(command, err)
         git.exc.GitCommandNotFound: Cmd('git') not found due to: OSError('[WinError 6] The handle is invalid')
-        cmdline: git clone -n --shared -v C:\\projects\\gitpython\\.git Users\\appveyor\\AppData\\Local\\Temp\\1\\tmplyp6kr_rnon_bare_test_root_module""",
-    )  # noqa E501
+        cmdline: git clone -n --shared -v C:\\projects\\gitpython\\.git Users\\appveyor\\AppData\\Local\\Temp\\1\\tmplyp6kr_rnon_bare_test_root_module
+        """,  # noqa E501
+    )
     @with_rw_repo(k_subm_current, bare=False)
     def test_root_module(self, rwrepo):
         # Can query everything without problems