Lint test/ (not just git/), fix warnings and a bug

This expands flake8 linting to include the test suite, and fixes
the resulting warnings. Four code changes are especially notable:

- Unit tests from which documentation is automatically generated
  contain a few occurrences of "# @NoEffect". These suppressions
  are extended to "# noqa: B015  # @NoEffect" so the corresponding
  suppression is also applied for flake8. This is significant
  because it actually changes what appears in the code examples in
  the generated documentation. However, since the "@NoEffect"
  annotation was considered acceptable, this may be okay too. The
  resulting examples did not become excessively long.

- Expressions like "[c for c in commits_for_file_generator]" appear
  in some unit tests from which documentation is automatically
  generated. The simpler form "list(commits_for_file_generator)"
  can replace them. This changes the examples in the documentation,
  but for the better, since that form is simpler (and also a better
  practice in general, thus a better thing to show readers). So I
  made those substitutions.

- In test_repo.TestRepo.test_git_work_tree_env, the code intended
  to unpatch environment variables after the test was ineffective,
  instead causing os.environ to refer to an ordinary dict object
  that does not affect environment variables when written to. This
  uses unittest.mock.patch.dict instead, so the variables are
  unpatched and subsequent writes to environment variables in the
  test process are effective.

- In test_submodule.TestSubmodule._do_base_tests, the expression
  statement "csm.module().head.ref.tracking_branch() is not None"
  appeared to be intended as an assertion, in spite of having been
  annoated @NoEffect. This is in light of the context and because,
  if the goal were only to exercise the function call, the
  "is not None" part would be superfluous. So I made it an
  assertion.

Author: EliahKagan

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

Diff for: .pre-commit-config.yaml

@@ -9,7 +9,7 @@ repos:
             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

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

Diff for: test/test_diff.py

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

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

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

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

Diff for: 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(
@@ -971,7 +972,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 +1330,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 +1340,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):

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