From dd42e38f5201ff6858c6b787fa8dab16c6cbc7a9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 15 Feb 2024 09:35:15 -0500 Subject: [PATCH 1/4] Keep temp files out of project dir and improve cleanup This fixes a test bug in TestTree.test_tree_modifier_ordering where: - A `tmp` subdirectory of the directory the tests are run from (almost always the root of the GitPython source tree) was created, instead of creating it in /tmp or elsewhere configured. - That directory was never deleted afterward, creating new untracked files in GitPython's own working tree, and also making it so running the tests twice would always fail the second time, unless a manual intervening deletion was performed. (This had not broken CI, since CI checks clone the repository anew each time.) - The directory was changed into to set up the test, but not deterministically changed back out of. It would typically be exited, but this was not guaranteed to occur if some subprocess commands were to fail unexpectedly. In addition to fixing all three aspects of that bug, this also: - Remains in the temporary directory only as long as necessary to execute the subprocesses in it that produce the expected value for the test (including not entering it until running them). - Deletes the temporary directory immediately after exiting it, since it is only used to produce a file list to compare to. - Avoids interleaving file creation and git subprocess commands, since doing so made it harder to understand what was being set up and since the difference is not relevant to the test. - Does the work in that temporary directory before performing any operations with the code under test, because it is conceptually an "arrange" step, and because doing so makes its limited purpose clearer on reading the tests. - Some other refactoring to support the fixes and accompanying changes, including extracting the temporary directory logic to a helper method. This deliberately does not change the order in which any files are created. It also keeps the approach of using subprocess functions directly to operate on the temporary git repository (and changing directory while doing so, keeping each of the commands the same), since there might be good reasons to do this, such as to make very clear that the file order being obtained to compare to is really coming from git itself. Even if this is to be changed, it seems outside the scope of this bugfix. The test still fails if the fix to git/objects/tree.py from 365d44f (#1799) is absent. This was verified by running: git revert --no-commit 365d44f50a3d72d7ebfa063b142d2abd4082cfaa pytest --no-cov -vv test/test_tree.py git revert --abort --- test/test_tree.py | 54 ++++++++++++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/test/test_tree.py b/test/test_tree.py index 695cb803e..b44392e15 100644 --- a/test/test_tree.py +++ b/test/test_tree.py @@ -4,14 +4,15 @@ # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ from io import BytesIO +import os.path as osp +from pathlib import Path +import subprocess +import tempfile from git.objects import Tree, Blob +from git.util import cwd from test.lib import TestBase -import os -import os.path as osp -import subprocess - class TestTree(TestBase): def test_serializable(self): @@ -42,29 +43,41 @@ def test_serializable(self): testtree._deserialize(stream) # END for each item in tree - def test_tree_modifier_ordering(self): - def setup_git_repository_and_get_ordered_files(): - os.mkdir("tmp") - os.chdir("tmp") - subprocess.run(["git", "init", "-q"], check=True) - os.mkdir("file") - for filename in [ + @staticmethod + def _get_git_ordered_files(): + """Get files as git orders them, to compare in test_tree_modifier_ordering.""" + with tempfile.TemporaryDirectory() as tdir: + # Create directory contents. + Path(tdir, "file").mkdir() + for filename in ( "bin", "bin.d", "file.to", "file.toml", "file.toml.bin", "file0", - "file/a", - ]: - open(filename, "a").close() - - subprocess.run(["git", "add", "."], check=True) - subprocess.run(["git", "commit", "-m", "c1"], check=True) - tree_hash = subprocess.check_output(["git", "rev-parse", "HEAD^{tree}"]).decode().strip() - cat_file_output = subprocess.check_output(["git", "cat-file", "-p", tree_hash]).decode() + ): + Path(tdir, filename).touch() + Path(tdir, "file", "a").touch() + + with cwd(tdir): + # Prepare the repository. + subprocess.run(["git", "init", "-q"], check=True) + subprocess.run(["git", "add", "."], check=True) + subprocess.run(["git", "commit", "-m", "c1"], check=True) + + # Get git output from which an ordered file list can be parsed. + rev_parse_command = ["git", "rev-parse", "HEAD^{tree}"] + tree_hash = subprocess.check_output(rev_parse_command).decode().strip() + cat_file_command = ["git", "cat-file", "-p", tree_hash] + cat_file_output = subprocess.check_output(cat_file_command).decode() + return [line.split()[-1] for line in cat_file_output.split("\n") if line] + def test_tree_modifier_ordering(self): + """TreeModifier.set_done() sorts files in the same order git does.""" + git_file_names_in_order = self._get_git_ordered_files() + hexsha = "6c1faef799095f3990e9970bc2cb10aa0221cf9c" roottree = self.rorepo.tree(hexsha) blob_mode = Tree.blob_id << 12 @@ -92,9 +105,6 @@ def names_in_mod_cache(): here = file_names_in_order() return [e for e in a if e in here] - git_file_names_in_order = setup_git_repository_and_get_ordered_files() - os.chdir("..") - mod.set_done() assert names_in_mod_cache() == git_file_names_in_order, "set_done() performs git-sorting" From 90cf4d75c5ebcbdedae24a6cd26cc1e7dd3e3c08 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 15 Feb 2024 10:36:26 -0500 Subject: [PATCH 2/4] Fix new PermissionError in Windows with Python 3.7 In Python 3.7, tempfile.TemporaryDirectory doesn't delete files on cleanup whose permissions have to be changed to be deleted. This uses `@with_rw_directory` instead, though that may be overkill here. --- test/test_tree.py | 60 +++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 31 deletions(-) diff --git a/test/test_tree.py b/test/test_tree.py index b44392e15..0c06b950c 100644 --- a/test/test_tree.py +++ b/test/test_tree.py @@ -7,11 +7,10 @@ import os.path as osp from pathlib import Path import subprocess -import tempfile from git.objects import Tree, Blob from git.util import cwd -from test.lib import TestBase +from test.lib import TestBase, with_rw_directory class TestTree(TestBase): @@ -43,36 +42,35 @@ def test_serializable(self): testtree._deserialize(stream) # END for each item in tree - @staticmethod - def _get_git_ordered_files(): + @with_rw_directory + def _get_git_ordered_files(self, rw_dir): """Get files as git orders them, to compare in test_tree_modifier_ordering.""" - with tempfile.TemporaryDirectory() as tdir: - # Create directory contents. - Path(tdir, "file").mkdir() - for filename in ( - "bin", - "bin.d", - "file.to", - "file.toml", - "file.toml.bin", - "file0", - ): - Path(tdir, filename).touch() - Path(tdir, "file", "a").touch() - - with cwd(tdir): - # Prepare the repository. - subprocess.run(["git", "init", "-q"], check=True) - subprocess.run(["git", "add", "."], check=True) - subprocess.run(["git", "commit", "-m", "c1"], check=True) - - # Get git output from which an ordered file list can be parsed. - rev_parse_command = ["git", "rev-parse", "HEAD^{tree}"] - tree_hash = subprocess.check_output(rev_parse_command).decode().strip() - cat_file_command = ["git", "cat-file", "-p", tree_hash] - cat_file_output = subprocess.check_output(cat_file_command).decode() - - return [line.split()[-1] for line in cat_file_output.split("\n") if line] + # Create directory contents. + Path(rw_dir, "file").mkdir() + for filename in ( + "bin", + "bin.d", + "file.to", + "file.toml", + "file.toml.bin", + "file0", + ): + Path(rw_dir, filename).touch() + Path(rw_dir, "file", "a").touch() + + with cwd(rw_dir): + # Prepare the repository. + subprocess.run(["git", "init", "-q"], check=True) + subprocess.run(["git", "add", "."], check=True) + subprocess.run(["git", "commit", "-m", "c1"], check=True) + + # Get git output from which an ordered file list can be parsed. + rev_parse_command = ["git", "rev-parse", "HEAD^{tree}"] + tree_hash = subprocess.check_output(rev_parse_command).decode().strip() + cat_file_command = ["git", "cat-file", "-p", tree_hash] + cat_file_output = subprocess.check_output(cat_file_command).decode() + + return [line.split()[-1] for line in cat_file_output.split("\n") if line] def test_tree_modifier_ordering(self): """TreeModifier.set_done() sorts files in the same order git does.""" From 0114a997bf56eec86f217f99553c859613f1c9a4 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 15 Feb 2024 11:22:24 -0500 Subject: [PATCH 3/4] Use more ligtweight approach to guarantee deletion This changes from `@with_rw_directory` back to TemporaryDirectory, but calls git.util.rmtree on the repsitory's .git directory where some read-only files otherwise cause TemporaryDirectory's cleanup to raise PermissionError on Windows in Python 3.7. This avoids the gc.collect that is known not to be necessary in this speciifc situation, as well as the problem that, if operating in the temporary directory did fail, then name of the helper would be logged as the name of the test where the failure occurred. But this has the disadvantage of making the helper more complex and harder to understand. So this may not be the best approach either. --- test/test_tree.py | 65 +++++++++++++++++++++++++---------------------- 1 file changed, 35 insertions(+), 30 deletions(-) diff --git a/test/test_tree.py b/test/test_tree.py index 0c06b950c..371e3c625 100644 --- a/test/test_tree.py +++ b/test/test_tree.py @@ -7,10 +7,11 @@ import os.path as osp from pathlib import Path import subprocess +import tempfile from git.objects import Tree, Blob -from git.util import cwd -from test.lib import TestBase, with_rw_directory +from git.util import cwd, rmtree +from test.lib import TestBase class TestTree(TestBase): @@ -42,35 +43,39 @@ def test_serializable(self): testtree._deserialize(stream) # END for each item in tree - @with_rw_directory - def _get_git_ordered_files(self, rw_dir): + @staticmethod + def _get_git_ordered_files(): """Get files as git orders them, to compare in test_tree_modifier_ordering.""" - # Create directory contents. - Path(rw_dir, "file").mkdir() - for filename in ( - "bin", - "bin.d", - "file.to", - "file.toml", - "file.toml.bin", - "file0", - ): - Path(rw_dir, filename).touch() - Path(rw_dir, "file", "a").touch() - - with cwd(rw_dir): - # Prepare the repository. - subprocess.run(["git", "init", "-q"], check=True) - subprocess.run(["git", "add", "."], check=True) - subprocess.run(["git", "commit", "-m", "c1"], check=True) - - # Get git output from which an ordered file list can be parsed. - rev_parse_command = ["git", "rev-parse", "HEAD^{tree}"] - tree_hash = subprocess.check_output(rev_parse_command).decode().strip() - cat_file_command = ["git", "cat-file", "-p", tree_hash] - cat_file_output = subprocess.check_output(cat_file_command).decode() - - return [line.split()[-1] for line in cat_file_output.split("\n") if line] + with tempfile.TemporaryDirectory() as tdir: + # Create directory contents. + Path(tdir, "file").mkdir() + for filename in ( + "bin", + "bin.d", + "file.to", + "file.toml", + "file.toml.bin", + "file0", + ): + Path(tdir, filename).touch() + Path(tdir, "file", "a").touch() + + try: + with cwd(tdir): + # Prepare the repository. + subprocess.run(["git", "init", "-q"], check=True) + subprocess.run(["git", "add", "."], check=True) + subprocess.run(["git", "commit", "-m", "c1"], check=True) + + # Get git output from which an ordered file list can be parsed. + rev_parse_command = ["git", "rev-parse", "HEAD^{tree}"] + tree_hash = subprocess.check_output(rev_parse_command).decode().strip() + cat_file_command = ["git", "cat-file", "-p", tree_hash] + cat_file_output = subprocess.check_output(cat_file_command).decode() + finally: + rmtree(Path(tdir, ".git")) + + return [line.split()[-1] for line in cat_file_output.split("\n") if line] def test_tree_modifier_ordering(self): """TreeModifier.set_done() sorts files in the same order git does.""" From b780a8c3a119c94b4c41f3c6f922ced686be212f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 15 Feb 2024 11:47:08 -0500 Subject: [PATCH 4/4] Tweak `@with_rw_directory` and go back to using it The situation with test_tree_modifier_ordering's helper, where it makes sense to wrap the helper itself with `@with_rw_directory` while still deleting the temporary directory as soon as the helper itself finishes, in order to make clear that the test itself does not use the temporary directory that the helper used, only occurs in one place in GitPython's tests as far as I know. In particular, all other places where `@with_rw_directory` was actually in use are test methods, not helper methods, and the way the decorator would have its wrapper log on failure documented the decorated method as a test. Mainly for that reason, I had backed away from using `@with_rw_directory` here. But the test code seems much easier to understand when it is used compared to other approaches. While it also has the disadvantage of doing a gc.collect that is unnecessary here, this is not the only place what that is done. This commit brings back the test_tree_modifier_ordering helper implementation that uses `@with_rw_directory`, while also modifying the logging logic in `@with_rw_directory` slightly, so that when the decorated method is not named as a test, it is referred to as a "Helper" rather than a "Test" in the failure log message. --- test/lib/helper.py | 3 ++- test/test_tree.py | 65 +++++++++++++++++++++------------------------- 2 files changed, 32 insertions(+), 36 deletions(-) diff --git a/test/lib/helper.py b/test/lib/helper.py index e6784e51c..f951a6a12 100644 --- a/test/lib/helper.py +++ b/test/lib/helper.py @@ -97,7 +97,8 @@ def wrapper(self, *args, **kwargs): return func(self, path, *args, **kwargs) except Exception: _logger.info( - "Test %s.%s failed, output is at %r\n", + "%s %s.%s failed, output is at %r\n", + "Test" if func.__name__.startswith("test_") else "Helper", type(self).__name__, func.__name__, path, diff --git a/test/test_tree.py b/test/test_tree.py index 371e3c625..0c06b950c 100644 --- a/test/test_tree.py +++ b/test/test_tree.py @@ -7,11 +7,10 @@ import os.path as osp from pathlib import Path import subprocess -import tempfile from git.objects import Tree, Blob -from git.util import cwd, rmtree -from test.lib import TestBase +from git.util import cwd +from test.lib import TestBase, with_rw_directory class TestTree(TestBase): @@ -43,39 +42,35 @@ def test_serializable(self): testtree._deserialize(stream) # END for each item in tree - @staticmethod - def _get_git_ordered_files(): + @with_rw_directory + def _get_git_ordered_files(self, rw_dir): """Get files as git orders them, to compare in test_tree_modifier_ordering.""" - with tempfile.TemporaryDirectory() as tdir: - # Create directory contents. - Path(tdir, "file").mkdir() - for filename in ( - "bin", - "bin.d", - "file.to", - "file.toml", - "file.toml.bin", - "file0", - ): - Path(tdir, filename).touch() - Path(tdir, "file", "a").touch() - - try: - with cwd(tdir): - # Prepare the repository. - subprocess.run(["git", "init", "-q"], check=True) - subprocess.run(["git", "add", "."], check=True) - subprocess.run(["git", "commit", "-m", "c1"], check=True) - - # Get git output from which an ordered file list can be parsed. - rev_parse_command = ["git", "rev-parse", "HEAD^{tree}"] - tree_hash = subprocess.check_output(rev_parse_command).decode().strip() - cat_file_command = ["git", "cat-file", "-p", tree_hash] - cat_file_output = subprocess.check_output(cat_file_command).decode() - finally: - rmtree(Path(tdir, ".git")) - - return [line.split()[-1] for line in cat_file_output.split("\n") if line] + # Create directory contents. + Path(rw_dir, "file").mkdir() + for filename in ( + "bin", + "bin.d", + "file.to", + "file.toml", + "file.toml.bin", + "file0", + ): + Path(rw_dir, filename).touch() + Path(rw_dir, "file", "a").touch() + + with cwd(rw_dir): + # Prepare the repository. + subprocess.run(["git", "init", "-q"], check=True) + subprocess.run(["git", "add", "."], check=True) + subprocess.run(["git", "commit", "-m", "c1"], check=True) + + # Get git output from which an ordered file list can be parsed. + rev_parse_command = ["git", "rev-parse", "HEAD^{tree}"] + tree_hash = subprocess.check_output(rev_parse_command).decode().strip() + cat_file_command = ["git", "cat-file", "-p", tree_hash] + cat_file_output = subprocess.check_output(cat_file_command).decode() + + return [line.split()[-1] for line in cat_file_output.split("\n") if line] def test_tree_modifier_ordering(self): """TreeModifier.set_done() sorts files in the same order git does."""