From ad570de497a4512a336eed141726af5b7b7e126b Mon Sep 17 00:00:00 2001 From: Travis Runner Date: Thu, 7 Dec 2023 17:10:17 -0500 Subject: [PATCH 1/3] Avoid unsafe assumptions about tempdir content in tests test_new_should_raise_on_invalid_repo_location had previously used tempfile.gettempdir() directly to get a path assumed not to be a valid repository, assuming more than necessary about the directory in which temporary files and directories are created. A newly created temporary directory is now used for that check, instead. test_new_should_raise_on_non_existent_path had assumed no repos/foobar existed relative to the current directory. Typically there would be no such directory, but is unnecessary for the test to rely on this. A newly created temporary directory known to be empty is now used in place of the "repos" component, for that test. --- test/test_repo.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/test_repo.py b/test/test_repo.py index 033deca1e..c68dd074c 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -71,15 +71,19 @@ def tearDown(self): for lfp in glob.glob(_tc_lock_fpaths): if osp.isfile(lfp): raise AssertionError("Previous TC left hanging git-lock file: {}".format(lfp)) + import gc gc.collect() def test_new_should_raise_on_invalid_repo_location(self): - self.assertRaises(InvalidGitRepositoryError, Repo, tempfile.gettempdir()) + with tempfile.TemporaryDirectory() as tdir: + self.assertRaises(InvalidGitRepositoryError, Repo, tdir) def test_new_should_raise_on_non_existent_path(self): - self.assertRaises(NoSuchPathError, Repo, "repos/foobar") + with tempfile.TemporaryDirectory() as tdir: + nonexistent = osp.join(tdir, "foobar") + self.assertRaises(NoSuchPathError, Repo, nonexistent) @with_rw_repo("0.3.2.1") def test_repo_creation_from_different_paths(self, rw_repo): From 9277ff561bcc37c70b9e6e015f623af20216b9e0 Mon Sep 17 00:00:00 2001 From: Travis Runner Date: Thu, 7 Dec 2023 19:06:05 -0500 Subject: [PATCH 2/3] Avoid another tempdir content assumption in test test_init was using tempfile.gettempdir() directly to get the location where the hard-coded path repos/foo/bar.git would be used to test repository creation with relative and absolute paths. That reused the same location each time, and also assumed the directory would be usable, which could fail due to previous runs or due to the path being used separately from GitPython's tests. This commit fixes that by using that path inside a temporary directory, known at the start of the test to be empty. Reorganizing the acquision and cleanup logic also has had the effect of causing the test no longer to be skipped due to the logic in git.util.rmtree due to the final cleanup attempt (after all assertions). The directory is now successfully removed on Windows, and the test passes on all platforms. --- test/test_repo.py | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/test/test_repo.py b/test/test_repo.py index c68dd074c..845fabf15 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -41,7 +41,7 @@ UnsafeProtocolError, ) from git.repo.fun import touch -from git.util import bin_to_hex, cygpath, join_path_native, rmfile, rmtree +from git.util import bin_to_hex, cwd, cygpath, join_path_native, rmfile, rmtree from test.lib import TestBase, fixture, with_rw_directory, with_rw_repo @@ -511,13 +511,11 @@ def write(self, b): repo.git.log(n=100, output_stream=TestOutputStream(io.DEFAULT_BUFFER_SIZE)) def test_init(self): - prev_cwd = os.getcwd() - os.chdir(tempfile.gettempdir()) - git_dir_rela = "repos/foo/bar.git" - del_dir_abs = osp.abspath("repos") - git_dir_abs = osp.abspath(git_dir_rela) - try: - # with specific path + with tempfile.TemporaryDirectory() as tdir, cwd(tdir): + git_dir_rela = "repos/foo/bar.git" + git_dir_abs = osp.abspath(git_dir_rela) + + # With specific path for path in (git_dir_rela, git_dir_abs): r = Repo.init(path=path, bare=True) self.assertIsInstance(r, Repo) @@ -527,7 +525,7 @@ def test_init(self): self._assert_empty_repo(r) - # test clone + # Test clone clone_path = path + "_clone" rc = r.clone(clone_path) self._assert_empty_repo(rc) @@ -562,13 +560,6 @@ def test_init(self): assert not r.has_separate_working_tree() self._assert_empty_repo(r) - finally: - try: - rmtree(del_dir_abs) - except OSError: - pass - os.chdir(prev_cwd) - # END restore previous state def test_bare_property(self): self.rorepo.bare From c09ac1ab2c86a87080dcfb2fc0af64cbcc2bc9eb Mon Sep 17 00:00:00 2001 From: Travis Runner Date: Fri, 8 Dec 2023 01:18:54 -0500 Subject: [PATCH 3/3] Test InvalidGitRepositoryError in repo subdir A Repo object can of course be constructed from a path to a directory that is the root of an existing repository, and raises InvalidGitRepositoryError on a directory that is outside of any repository. Tests intended to show both conditions already exist. This adds a test to verify that InvalidGitRepositoryError is also raised when an attempt is made to construct a Repo object from a path to a directory that is not the root of a repository but that is known to be located inside an existing git repository. --- test/test_repo.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/test/test_repo.py b/test/test_repo.py index 845fabf15..836862b0c 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -77,9 +77,20 @@ def tearDown(self): gc.collect() def test_new_should_raise_on_invalid_repo_location(self): + # Ideally this tests a directory that is outside of any repository. In the rare + # case tempfile.gettempdir() is inside a repo, this still passes, but tests the + # same scenario as test_new_should_raise_on_invalid_repo_location_within_repo. with tempfile.TemporaryDirectory() as tdir: self.assertRaises(InvalidGitRepositoryError, Repo, tdir) + @with_rw_directory + def test_new_should_raise_on_invalid_repo_location_within_repo(self, rw_dir): + repo_dir = osp.join(rw_dir, "repo") + Repo.init(repo_dir) + subdir = osp.join(repo_dir, "subdir") + os.mkdir(subdir) + self.assertRaises(InvalidGitRepositoryError, Repo, subdir) + def test_new_should_raise_on_non_existent_path(self): with tempfile.TemporaryDirectory() as tdir: nonexistent = osp.join(tdir, "foobar") @@ -122,7 +133,7 @@ def test_tree_from_revision(self): self.assertEqual(tree.type, "tree") self.assertEqual(self.rorepo.tree(tree), tree) - # try from invalid revision that does not exist + # Try from an invalid revision that does not exist. self.assertRaises(BadName, self.rorepo.tree, "hello world") def test_pickleable(self):