From 2382891377f60c1467c1965b25e3ecf293f39b80 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 19:41:55 -0400 Subject: [PATCH 01/89] Test that deprecated Diff.renamed property warns This starts on a test.deprecation subpackage for deprecation tests. Having these tests in a separate directory inside the test suite may or may not be how they will ultimately be orgnaized, but it has two advantages: - Once all the tests are written, it should be easy to see what in GitPython is deprecated. - Some deprecation warnings -- those on module or class attribute access -- will require the introduction of new dynamic behavior, and thus run the risk of breaking static type checking. So that should be checked for, where applicable. But currently the test suite has no type annotations and is not checked by mypy. Having deprecation-related tests under the same path will make it easier to enable mypy for just this part of the test suite (for now). It is also for this latter reason that the one test so far is written without using the GitPython test suite's existing fixtures whose uses are harder to annotate. This may be changed if warranted, though some of the more complex deprecation-related tests may benefit from being written as pure pytest tests. Although a number of deprecated features in GitPython do already issue warnings, Diff.renamed is one of the features that does not yet do so. So the newly introduced test will fail until that is fixed in the next commit. --- test/deprecation/__init__.py | 2 ++ test/deprecation/test_various.py | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+) create mode 100644 test/deprecation/__init__.py create mode 100644 test/deprecation/test_various.py diff --git a/test/deprecation/__init__.py b/test/deprecation/__init__.py new file mode 100644 index 000000000..56b5d89db --- /dev/null +++ b/test/deprecation/__init__.py @@ -0,0 +1,2 @@ +# This module is part of GitPython and is released under the +# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ diff --git a/test/deprecation/test_various.py b/test/deprecation/test_various.py new file mode 100644 index 000000000..9f948bc48 --- /dev/null +++ b/test/deprecation/test_various.py @@ -0,0 +1,20 @@ +# This module is part of GitPython and is released under the +# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ + +"""Tests of assorted deprecation warnings with no extra subtleties to check.""" + +from git.diff import NULL_TREE +from git.repo import Repo + +import pytest + + +def test_diff_renamed_warns(tmp_path): + (tmp_path / "a.txt").write_text("hello\n", encoding="utf-8") + repo = Repo.init(tmp_path) + repo.index.add(["a.txt"]) + commit = repo.index.commit("Initial commit") + (diff,) = commit.diff(NULL_TREE) # Exactly one file in the diff. + + with pytest.deprecated_call(): + diff.renamed From e7dec7d0eecc362f02418820a146735a68430fbd Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 19:52:17 -0400 Subject: [PATCH 02/89] Have the deprecated Diff.renamed property issue a warning --- git/diff.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/git/diff.py b/git/diff.py index 0e39fe7a8..f89b12d98 100644 --- a/git/diff.py +++ b/git/diff.py @@ -7,6 +7,7 @@ import enum import re +import warnings from git.cmd import handle_process_output from git.compat import defenc @@ -554,6 +555,11 @@ def renamed(self) -> bool: This property is deprecated. Please use the :attr:`renamed_file` property instead. """ + warnings.warn( + "Diff.renamed is deprecated, use Diff.renamed_file instead", + DeprecationWarning, + stacklevel=2, + ) return self.renamed_file @property From a8f109ca1704827b017349d6d97f8a0a3229d9a8 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 20:04:25 -0400 Subject: [PATCH 03/89] Fix exception in Popen.__del__ in test on Windows The newly introduced test would pass, but show: Exception ignored in: Traceback (most recent call last): File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.752.0_x64__qbz5n2kfra8p0\Lib\subprocess.py", line 1130, in __del__ self._internal_poll(_deadstate=_maxsize) File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.12_3.12.752.0_x64__qbz5n2kfra8p0\Lib\subprocess.py", line 1575, in _internal_poll if _WaitForSingleObject(self._handle, 0) == _WAIT_OBJECT_0: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ OSError: [WinError 6] The handle is invalid This was due to how, at least for now, the deprecation warning tests are not using GitPython's normal fixtures, which help clean things up on Windows. This adds a small amount of ad-hoc cleanup. It also moves the acquisition of the Diff object into a pytest fixture, which can be reused for the immediately forthcoming test that the preferred property does not warn. --- test/deprecation/test_various.py | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/test/deprecation/test_various.py b/test/deprecation/test_various.py index 9f948bc48..056e0ac0d 100644 --- a/test/deprecation/test_various.py +++ b/test/deprecation/test_various.py @@ -3,18 +3,27 @@ """Tests of assorted deprecation warnings with no extra subtleties to check.""" -from git.diff import NULL_TREE -from git.repo import Repo +import gc import pytest +from git.diff import NULL_TREE +from git.repo import Repo + -def test_diff_renamed_warns(tmp_path): +@pytest.fixture +def single_diff(tmp_path): + """Fixture to supply a single-file diff.""" (tmp_path / "a.txt").write_text("hello\n", encoding="utf-8") repo = Repo.init(tmp_path) repo.index.add(["a.txt"]) commit = repo.index.commit("Initial commit") (diff,) = commit.diff(NULL_TREE) # Exactly one file in the diff. + yield diff + del repo, commit, diff + gc.collect() + +def test_diff_renamed_warns(single_diff): with pytest.deprecated_call(): - diff.renamed + single_diff.renamed From fffa6cea663b179c0e5d53cf5eb93159e2bbc3b0 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 20:31:10 -0400 Subject: [PATCH 04/89] Test that the preferred renamed_file property does not warn --- test/deprecation/test_various.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/deprecation/test_various.py b/test/deprecation/test_various.py index 056e0ac0d..9dd95f723 100644 --- a/test/deprecation/test_various.py +++ b/test/deprecation/test_various.py @@ -4,6 +4,7 @@ """Tests of assorted deprecation warnings with no extra subtleties to check.""" import gc +import warnings import pytest @@ -25,5 +26,14 @@ def single_diff(tmp_path): def test_diff_renamed_warns(single_diff): + """The deprecated Diff.renamed property issues a deprecation warning.""" with pytest.deprecated_call(): single_diff.renamed + + +def test_diff_renamed_file_does_not_warn(single_diff): + """The preferred Diff.renamed_file property issues no deprecation warning.""" + with warnings.catch_warnings(): + # FIXME: Refine this to filter for deprecation warnings from GitPython. + warnings.simplefilter("error", DeprecationWarning) + single_diff.renamed_file From bc111b799b06990fd7dde6385265d0e6d3a6289d Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 20:31:30 -0400 Subject: [PATCH 05/89] Add a TODO for simplifying the single_diff fixture --- test/deprecation/test_various.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/deprecation/test_various.py b/test/deprecation/test_various.py index 9dd95f723..efdb0a57c 100644 --- a/test/deprecation/test_various.py +++ b/test/deprecation/test_various.py @@ -15,6 +15,7 @@ @pytest.fixture def single_diff(tmp_path): """Fixture to supply a single-file diff.""" + # TODO: Consider making a fake diff rather than using a real repo and commit. (tmp_path / "a.txt").write_text("hello\n", encoding="utf-8") repo = Repo.init(tmp_path) repo.index.add(["a.txt"]) From e3728c3dca9cac2b49f827dbf5d1a100602f33d9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 20:54:50 -0400 Subject: [PATCH 06/89] Decompose new fixture logic better This will be even more helpful when testing a deprecated member of the Commit class (not yet done). --- test/deprecation/test_various.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/test/deprecation/test_various.py b/test/deprecation/test_various.py index efdb0a57c..398367b61 100644 --- a/test/deprecation/test_various.py +++ b/test/deprecation/test_various.py @@ -13,28 +13,33 @@ @pytest.fixture -def single_diff(tmp_path): - """Fixture to supply a single-file diff.""" - # TODO: Consider making a fake diff rather than using a real repo and commit. +def commit(tmp_path): + """Fixture to supply a one-commit repo's commit, enough for deprecation tests.""" (tmp_path / "a.txt").write_text("hello\n", encoding="utf-8") repo = Repo.init(tmp_path) repo.index.add(["a.txt"]) - commit = repo.index.commit("Initial commit") + yield repo.index.commit("Initial commit") + del repo + gc.collect() + + +@pytest.fixture +def diff(commit): + """Fixture to supply a single-file diff.""" + # TODO: Consider making a fake diff rather than using a real repo and commit. (diff,) = commit.diff(NULL_TREE) # Exactly one file in the diff. yield diff - del repo, commit, diff - gc.collect() -def test_diff_renamed_warns(single_diff): +def test_diff_renamed_warns(diff): """The deprecated Diff.renamed property issues a deprecation warning.""" with pytest.deprecated_call(): - single_diff.renamed + diff.renamed -def test_diff_renamed_file_does_not_warn(single_diff): +def test_diff_renamed_file_does_not_warn(diff): """The preferred Diff.renamed_file property issues no deprecation warning.""" with warnings.catch_warnings(): # FIXME: Refine this to filter for deprecation warnings from GitPython. warnings.simplefilter("error", DeprecationWarning) - single_diff.renamed_file + diff.renamed_file From ff4b58dd56d382b26d83f2f41f7d11d15e9db8dc Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 20:57:40 -0400 Subject: [PATCH 07/89] Extract no-deprecation-warning asserter as a context manager + Remove the TODO suggesting the diff not be computed from a real commit, since we're going to have the logic for that in use in forthcoming commit-specific deprecation warning tests anyway. --- test/deprecation/test_various.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/test/deprecation/test_various.py b/test/deprecation/test_various.py index 398367b61..60f94311f 100644 --- a/test/deprecation/test_various.py +++ b/test/deprecation/test_various.py @@ -3,6 +3,7 @@ """Tests of assorted deprecation warnings with no extra subtleties to check.""" +import contextlib import gc import warnings @@ -12,6 +13,15 @@ from git.repo import Repo +@contextlib.contextmanager +def _assert_no_deprecation_warning(): + """Context manager to assert that code does not issue any deprecation warnings.""" + with warnings.catch_warnings(): + # FIXME: Refine this to filter for deprecation warnings from GitPython. + warnings.simplefilter("error", DeprecationWarning) + yield + + @pytest.fixture def commit(tmp_path): """Fixture to supply a one-commit repo's commit, enough for deprecation tests.""" @@ -26,7 +36,6 @@ def commit(tmp_path): @pytest.fixture def diff(commit): """Fixture to supply a single-file diff.""" - # TODO: Consider making a fake diff rather than using a real repo and commit. (diff,) = commit.diff(NULL_TREE) # Exactly one file in the diff. yield diff @@ -39,7 +48,5 @@ def test_diff_renamed_warns(diff): def test_diff_renamed_file_does_not_warn(diff): """The preferred Diff.renamed_file property issues no deprecation warning.""" - with warnings.catch_warnings(): - # FIXME: Refine this to filter for deprecation warnings from GitPython. - warnings.simplefilter("error", DeprecationWarning) + with _assert_no_deprecation_warning(): diff.renamed_file From 2c526962ebf0a5d4d31a725577db112f2ce60447 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 21:04:06 -0400 Subject: [PATCH 08/89] Test that the deprecated Commit.trailers property warns And that the non-deprecated recommended alternative trailers_list and trailers_dict properties do not warn. The test that trailers warns does not yet pass yet, because it has not yet been made to warn. --- test/deprecation/test_various.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/deprecation/test_various.py b/test/deprecation/test_various.py index 60f94311f..99ce882b0 100644 --- a/test/deprecation/test_various.py +++ b/test/deprecation/test_various.py @@ -50,3 +50,21 @@ def test_diff_renamed_file_does_not_warn(diff): """The preferred Diff.renamed_file property issues no deprecation warning.""" with _assert_no_deprecation_warning(): diff.renamed_file + + +def test_commit_trailers_warns(commit): + """The deprecated Commit.trailers property issues a deprecation warning.""" + with pytest.deprecated_call(): + commit.trailers + + +def test_commit_trailers_list_does_not_warn(commit): + """The nondeprecated Commit.trailers_list property issues no deprecation warning.""" + with _assert_no_deprecation_warning(): + commit.trailers_list + + +def test_commit_trailers_dict_does_not_warn(commit): + """The nondeprecated Commit.trailers_dict property issues no deprecation warning.""" + with _assert_no_deprecation_warning(): + commit.trailers_dict From 03464d90a66caf6a5d8dbbd37318758331c85168 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 21:12:41 -0400 Subject: [PATCH 09/89] Have the deprecated Commit.trailers property issue a warning --- git/objects/commit.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/git/objects/commit.py b/git/objects/commit.py index 8de52980c..d957c9051 100644 --- a/git/objects/commit.py +++ b/git/objects/commit.py @@ -14,6 +14,7 @@ from subprocess import Popen, PIPE import sys from time import altzone, daylight, localtime, time, timezone +import warnings from gitdb import IStream @@ -399,6 +400,11 @@ def trailers(self) -> Dict[str, str]: Dictionary containing whitespace stripped trailer information. Only contains the latest instance of each trailer key. """ + warnings.warn( + "Commit.trailers is deprecated, use Commit.trailers_list or Commit.trailers_dict instead", + DeprecationWarning, + stacklevel=2, + ) return {k: v[0] for k, v in self.trailers_dict.items()} @property From 9d096e08d7645bc5206c4728b45efcf26486b635 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 21:51:11 -0400 Subject: [PATCH 10/89] Test that Traversable.{list_,}traverse, but not overrides, warn --- test/deprecation/test_various.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/deprecation/test_various.py b/test/deprecation/test_various.py index 99ce882b0..be3195a5e 100644 --- a/test/deprecation/test_various.py +++ b/test/deprecation/test_various.py @@ -10,6 +10,7 @@ import pytest from git.diff import NULL_TREE +from git.objects.util import Traversable from git.repo import Repo @@ -68,3 +69,27 @@ def test_commit_trailers_dict_does_not_warn(commit): """The nondeprecated Commit.trailers_dict property issues no deprecation warning.""" with _assert_no_deprecation_warning(): commit.trailers_dict + + +def test_traverse_list_traverse_in_base_class_warns(commit): + """Traversable.list_traverse's base implementation issues a deprecation warning.""" + with pytest.deprecated_call(): + Traversable.list_traverse(commit) + + +def test_traversable_list_traverse_override_does_not_warn(commit): + """Calling list_traverse on concrete subclasses is not deprecated, does not warn.""" + with _assert_no_deprecation_warning(): + commit.list_traverse() + + +def test_traverse_traverse_in_base_class_warns(commit): + """Traversable.traverse's base implementation issues a deprecation warning.""" + with pytest.deprecated_call(): + Traversable.traverse(commit) + + +def test_traverse_traverse_override_does_not_warn(commit): + """Calling traverse on concrete subclasses is not deprecated, does not warn.""" + with _assert_no_deprecation_warning(): + commit.traverse() From 21c2b72b019627dba81b35085117b79660567abd Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Mar 2024 01:51:54 -0400 Subject: [PATCH 11/89] Use the :exc: Sphinx role for DeprecationWarning Python warnings are exceptions, to facilitate converting them to errors by raising them. Thus the more specific :exc: role applies and is a better fit than the more general :class: role. --- git/util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git/util.py b/git/util.py index 5839f9720..8c1c26012 100644 --- a/git/util.py +++ b/git/util.py @@ -1285,7 +1285,7 @@ def list_items(cls, repo: "Repo", *args: Any, **kwargs: Any) -> IterableList[T_I class IterableClassWatcher(type): - """Metaclass that issues :class:`DeprecationWarning` when :class:`git.util.Iterable` + """Metaclass that issues :exc:`DeprecationWarning` when :class:`git.util.Iterable` is subclassed.""" def __init__(cls, name: str, bases: Tuple, clsdict: Dict) -> None: From ca385a59439006355de02ddab9012031e8577e41 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Mar 2024 02:00:43 -0400 Subject: [PATCH 12/89] Test that subclassing deprecated git.util.Iterable warns And that subclassing the strongly preferred git.util.Iterable class does not warn. --- test/deprecation/test_various.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/deprecation/test_various.py b/test/deprecation/test_various.py index be3195a5e..71f9cf940 100644 --- a/test/deprecation/test_various.py +++ b/test/deprecation/test_various.py @@ -12,6 +12,7 @@ from git.diff import NULL_TREE from git.objects.util import Traversable from git.repo import Repo +from git.util import Iterable as _Iterable, IterableObj @contextlib.contextmanager @@ -93,3 +94,19 @@ def test_traverse_traverse_override_does_not_warn(commit): """Calling traverse on concrete subclasses is not deprecated, does not warn.""" with _assert_no_deprecation_warning(): commit.traverse() + + +def test_iterable_inheriting_warns(): + """Subclassing the deprecated git.util.Iterable issues a deprecation warning.""" + with pytest.deprecated_call(): + + class Derived(_Iterable): + pass + + +def test_iterable_obj_inheriting_does_not_warn(): + """Subclassing git.util.IterableObj is not deprecated, does not warn.""" + with _assert_no_deprecation_warning(): + + class Derived(IterableObj): + pass From 8bbcb26ea6e798a10969570d24cd5e9c401feed7 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Mar 2024 11:43:13 -0400 Subject: [PATCH 13/89] Call repo.close() instead of manually collecting The code this replaces in the `commit` pytest fixture seems not to be needed anymore, and could arguably be removed now with no further related changes. But whether it is needed depends on subtle and sometimes nondeterministic factors, and may also vary across Python versions. To be safe, this replaces it with a call to the Repo instance's close method, which is in effect more robust than what was being done before, as it calls clear_cache on the the Git object that the Repo object uses, does a gitdb/smmap collection, and on Windows calls gc.collect both before and after that collection. This may happen immediately anyway if the Repo object is not reachable from any cycle, since the reference count should go to zero after each of the deprecation warning tests (the fixture's lifetime is that of the test case), and Repo.close is called in Repo.__del__. But this makes it happen immediately even if there is a cycle. --- test/deprecation/test_various.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/deprecation/test_various.py b/test/deprecation/test_various.py index 71f9cf940..a82b989b7 100644 --- a/test/deprecation/test_various.py +++ b/test/deprecation/test_various.py @@ -4,7 +4,6 @@ """Tests of assorted deprecation warnings with no extra subtleties to check.""" import contextlib -import gc import warnings import pytest @@ -31,8 +30,7 @@ def commit(tmp_path): repo = Repo.init(tmp_path) repo.index.add(["a.txt"]) yield repo.index.commit("Initial commit") - del repo - gc.collect() + repo.close() @pytest.fixture From b8ce99031d3183a895f15e49fb7149a56a653065 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Mar 2024 19:39:00 -0400 Subject: [PATCH 14/89] Better name and document the basic deprecation test module The other deprecation test modules this refers to don't exist yet but will be introduced soon. --- .../deprecation/{test_various.py => test_basic.py} | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) rename test/deprecation/{test_various.py => test_basic.py} (80%) diff --git a/test/deprecation/test_various.py b/test/deprecation/test_basic.py similarity index 80% rename from test/deprecation/test_various.py rename to test/deprecation/test_basic.py index a82b989b7..459d79268 100644 --- a/test/deprecation/test_various.py +++ b/test/deprecation/test_basic.py @@ -1,7 +1,19 @@ # This module is part of GitPython and is released under the # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ -"""Tests of assorted deprecation warnings with no extra subtleties to check.""" +"""Tests of assorted deprecation warnings when there are no extra subtleties to check. + +This tests deprecation warnings where all that needs be verified is that a deprecated +property, function, or class issues a DeprecationWarning when used and, if applicable, +that recommended alternatives do not issue the warning. + +This is in contrast to other modules within test.deprecation, which test warnings where +there is a risk of breaking other runtime behavior, or of breaking static type checking +or making it less useful, by introducing the warning or in plausible future changes to +how the warning is implemented. That happens when it is necessary to customize attribute +access on a module or class, in a way it was not customized before, to issue a warning. +It is inapplicable to the deprecations whose warnings are tested in this module. +""" import contextlib import warnings From 61273aa2084ada8c48d0f9e511556d3d72eec32c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Mar 2024 20:49:34 -0400 Subject: [PATCH 15/89] Annotate basic deprecation tests; have mypy scan it - Add type hints to test/deprecation/basic.py. As its module docstring (already) notes, that test module does not contain code where it is specifically important that it be type checked to verify important properties of the code under test. However, other test.deprecation.* modules will, and it is much more convenient to be able to scan the whole directory than the directory except for one file. (Less importantly, for the deprecation tests to be easily readable as a coherent whole, it makes sense that all, not just most, would have annotations.) - Configure mypy in pyproject.toml so it can be run without arguments (mypy errors when run that way unless configured), where the effect is to scan the git/ directory, as was commonly done before, as well as the test/deprecation/ directory. - Change the CI test workflow, as well as tox.ini, to run mypy with no arguments instead of passing `-p git`, so that it will scan both the git package as it had before and the test.deprecation package (due to the new pyproject.toml configuration). - Change the readme to recommend running it that way, too. --- .github/workflows/pythonpackage.yml | 2 +- README.md | 2 +- pyproject.toml | 1 + test/deprecation/test_basic.py | 40 +++++++++++++++++++---------- tox.ini | 2 +- 5 files changed, 30 insertions(+), 17 deletions(-) diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 7cee0cd64..4c918a92d 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -88,7 +88,7 @@ jobs: - name: Check types with mypy run: | - mypy --python-version=${{ matrix.python-version }} -p git + mypy --python-version=${{ matrix.python-version }} env: MYPY_FORCE_COLOR: "1" TERM: "xterm-256color" # For color: https://github.com/python/mypy/issues/13817 diff --git a/README.md b/README.md index 30af532db..9bedaaae7 100644 --- a/README.md +++ b/README.md @@ -167,7 +167,7 @@ This includes the linting and autoformatting done by Ruff, as well as some other To typecheck, run: ```sh -mypy -p git +mypy ``` #### CI (and tox) diff --git a/pyproject.toml b/pyproject.toml index 1dc1e6aed..5eac2be09 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -21,6 +21,7 @@ testpaths = "test" # Space separated list of paths from root e.g test tests doc [tool.mypy] python_version = "3.8" +files = ["git/", "test/deprecation/"] disallow_untyped_defs = true no_implicit_optional = true warn_redundant_casts = true diff --git a/test/deprecation/test_basic.py b/test/deprecation/test_basic.py index 459d79268..8ee7e72b1 100644 --- a/test/deprecation/test_basic.py +++ b/test/deprecation/test_basic.py @@ -25,9 +25,21 @@ from git.repo import Repo from git.util import Iterable as _Iterable, IterableObj +# typing ----------------------------------------------------------------- + +from typing import Generator, TYPE_CHECKING + +if TYPE_CHECKING: + from pathlib import Path + + from git.diff import Diff + from git.objects.commit import Commit + +# ------------------------------------------------------------------------ + @contextlib.contextmanager -def _assert_no_deprecation_warning(): +def _assert_no_deprecation_warning() -> Generator[None, None, None]: """Context manager to assert that code does not issue any deprecation warnings.""" with warnings.catch_warnings(): # FIXME: Refine this to filter for deprecation warnings from GitPython. @@ -36,7 +48,7 @@ def _assert_no_deprecation_warning(): @pytest.fixture -def commit(tmp_path): +def commit(tmp_path: "Path") -> Generator["Commit", None, None]: """Fixture to supply a one-commit repo's commit, enough for deprecation tests.""" (tmp_path / "a.txt").write_text("hello\n", encoding="utf-8") repo = Repo.init(tmp_path) @@ -46,67 +58,67 @@ def commit(tmp_path): @pytest.fixture -def diff(commit): +def diff(commit: "Commit") -> Generator["Diff", None, None]: """Fixture to supply a single-file diff.""" (diff,) = commit.diff(NULL_TREE) # Exactly one file in the diff. yield diff -def test_diff_renamed_warns(diff): +def test_diff_renamed_warns(diff: "Diff") -> None: """The deprecated Diff.renamed property issues a deprecation warning.""" with pytest.deprecated_call(): diff.renamed -def test_diff_renamed_file_does_not_warn(diff): +def test_diff_renamed_file_does_not_warn(diff: "Diff") -> None: """The preferred Diff.renamed_file property issues no deprecation warning.""" with _assert_no_deprecation_warning(): diff.renamed_file -def test_commit_trailers_warns(commit): +def test_commit_trailers_warns(commit: "Commit") -> None: """The deprecated Commit.trailers property issues a deprecation warning.""" with pytest.deprecated_call(): commit.trailers -def test_commit_trailers_list_does_not_warn(commit): +def test_commit_trailers_list_does_not_warn(commit: "Commit") -> None: """The nondeprecated Commit.trailers_list property issues no deprecation warning.""" with _assert_no_deprecation_warning(): commit.trailers_list -def test_commit_trailers_dict_does_not_warn(commit): +def test_commit_trailers_dict_does_not_warn(commit: "Commit") -> None: """The nondeprecated Commit.trailers_dict property issues no deprecation warning.""" with _assert_no_deprecation_warning(): commit.trailers_dict -def test_traverse_list_traverse_in_base_class_warns(commit): +def test_traverse_list_traverse_in_base_class_warns(commit: "Commit") -> None: """Traversable.list_traverse's base implementation issues a deprecation warning.""" with pytest.deprecated_call(): Traversable.list_traverse(commit) -def test_traversable_list_traverse_override_does_not_warn(commit): +def test_traversable_list_traverse_override_does_not_warn(commit: "Commit") -> None: """Calling list_traverse on concrete subclasses is not deprecated, does not warn.""" with _assert_no_deprecation_warning(): commit.list_traverse() -def test_traverse_traverse_in_base_class_warns(commit): +def test_traverse_traverse_in_base_class_warns(commit: "Commit") -> None: """Traversable.traverse's base implementation issues a deprecation warning.""" with pytest.deprecated_call(): Traversable.traverse(commit) -def test_traverse_traverse_override_does_not_warn(commit): +def test_traverse_traverse_override_does_not_warn(commit: "Commit") -> None: """Calling traverse on concrete subclasses is not deprecated, does not warn.""" with _assert_no_deprecation_warning(): commit.traverse() -def test_iterable_inheriting_warns(): +def test_iterable_inheriting_warns() -> None: """Subclassing the deprecated git.util.Iterable issues a deprecation warning.""" with pytest.deprecated_call(): @@ -114,7 +126,7 @@ class Derived(_Iterable): pass -def test_iterable_obj_inheriting_does_not_warn(): +def test_iterable_obj_inheriting_does_not_warn() -> None: """Subclassing git.util.IterableObj is not deprecated, does not warn.""" with _assert_no_deprecation_warning(): diff --git a/tox.ini b/tox.ini index 33074a78a..fc62fa587 100644 --- a/tox.ini +++ b/tox.ini @@ -30,7 +30,7 @@ description = Typecheck with mypy base_python = py{39,310,311,312,38,37} set_env = MYPY_FORCE_COLOR = 1 -commands = mypy -p git +commands = mypy ignore_outcome = true [testenv:html] From b7a3d8c08537d00aac065b7dcbe1a4896919ee07 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 19 Mar 2024 17:05:06 -0400 Subject: [PATCH 16/89] Start on top-level module attribute access regression tests --- test/deprecation/test_attributes.py | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 test/deprecation/test_attributes.py diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py new file mode 100644 index 000000000..2673cc172 --- /dev/null +++ b/test/deprecation/test_attributes.py @@ -0,0 +1,10 @@ +"""Tests for dynamic and static attribute errors.""" + +import pytest + +import git + + +def test_no_attribute() -> None: + with pytest.raises(AttributeError): + git.foo From 105f50056126a73e8cbfeb0d1157695fe6b296b1 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 19 Mar 2024 17:15:02 -0400 Subject: [PATCH 17/89] Test attribute access and importing separately Rather than only testing attribute access. --- test/deprecation/test_attributes.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 2673cc172..aea3278f8 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -2,9 +2,14 @@ import pytest -import git +def test_cannot_get_undefined() -> None: + import git -def test_no_attribute() -> None: with pytest.raises(AttributeError): git.foo + + +def test_cannot_import_undefined() -> None: + with pytest.raises(ImportError): + from git import foo From 859e38cfb395e6a937c30fedd36a9b3896747188 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 20 Mar 2024 20:14:22 -0400 Subject: [PATCH 18/89] Expand to test top-level deprecated names --- test/deprecation/test_attributes.py | 89 ++++++++++++++++++++++++++++- 1 file changed, 88 insertions(+), 1 deletion(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index aea3278f8..428dab236 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -1,5 +1,7 @@ """Tests for dynamic and static attribute errors.""" +import importlib + import pytest @@ -12,4 +14,89 @@ def test_cannot_get_undefined() -> None: def test_cannot_import_undefined() -> None: with pytest.raises(ImportError): - from git import foo + from git import foo # noqa: F401 + + +def test_util_alias_access_resolves() -> None: + """These resolve for now, though they're private we do not guarantee this.""" + import git + + assert git.util is git.index.util + + +def test_util_alias_import_resolves() -> None: + from git import util + import git + + util is git.index.util + + +def test_util_alias_access_warns() -> None: + import git + + with pytest.deprecated_call() as ctx: + git.util + + assert len(ctx) == 1 + message = ctx[0].message.args[0] + assert "git.util" in message + assert "git.index.util" in message + assert "should not be relied on" in message + + +def test_util_alias_import_warns() -> None: + with pytest.deprecated_call() as ctx: + from git import util # noqa: F401 + + message = ctx[0].message.args[0] + assert "git.util" in message + assert "git.index.util" in message + assert "should not be relied on" in message + + +_parametrize_by_private_alias = pytest.mark.parametrize( + "name, fullname", + [ + ("head", "git.refs.head"), + ("log", "git.refs.log"), + ("reference", "git.refs.reference"), + ("symbolic", "git.refs.symbolic"), + ("tag", "git.refs.tag"), + ("base", "git.index.base"), + ("fun", "git.index.fun"), + ("typ", "git.index.typ"), + ], +) + + +@_parametrize_by_private_alias +def test_private_module_alias_access_resolves(name: str, fullname: str) -> None: + """These resolve for now, though they're private we do not guarantee this.""" + import git + + assert getattr(git, name) is importlib.import_module(fullname) + + +@_parametrize_by_private_alias +def test_private_module_alias_import_resolves(name: str, fullname: str) -> None: + exec(f"from git import {name}") + locals()[name] is importlib.import_module(fullname) + + +@_parametrize_by_private_alias +def test_private_module_alias_access_warns(name: str, fullname: str) -> None: + import git + + with pytest.deprecated_call() as ctx: + getattr(git, name) + + assert len(ctx) == 1 + assert ctx[0].message.args[0].endswith(f"Use {fullname} instead.") + + +@_parametrize_by_private_alias +def test_private_module_alias_import_warns(name: str, fullname: str) -> None: + with pytest.deprecated_call() as ctx: + exec(f"from git import {name}") + + assert ctx[0].message.args[0].endswith(f"Use {fullname} instead.") From 46a739da24331849323a7c583ffd61a51583d3c1 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 20 Mar 2024 20:20:04 -0400 Subject: [PATCH 19/89] Hoist `import git` to module level in test module Because it's going to be necessary to express things in terms of it in parametrization markings, in order for mypy to show the expected errors for names that are available dynamically but deliberately static type errors. --- test/deprecation/test_attributes.py | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 428dab236..85aa7a579 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -4,10 +4,10 @@ import pytest +import git -def test_cannot_get_undefined() -> None: - import git +def test_cannot_get_undefined() -> None: with pytest.raises(AttributeError): git.foo @@ -19,21 +19,16 @@ def test_cannot_import_undefined() -> None: def test_util_alias_access_resolves() -> None: """These resolve for now, though they're private we do not guarantee this.""" - import git - assert git.util is git.index.util def test_util_alias_import_resolves() -> None: from git import util - import git util is git.index.util def test_util_alias_access_warns() -> None: - import git - with pytest.deprecated_call() as ctx: git.util @@ -72,8 +67,6 @@ def test_util_alias_import_warns() -> None: @_parametrize_by_private_alias def test_private_module_alias_access_resolves(name: str, fullname: str) -> None: """These resolve for now, though they're private we do not guarantee this.""" - import git - assert getattr(git, name) is importlib.import_module(fullname) @@ -85,8 +78,6 @@ def test_private_module_alias_import_resolves(name: str, fullname: str) -> None: @_parametrize_by_private_alias def test_private_module_alias_access_warns(name: str, fullname: str) -> None: - import git - with pytest.deprecated_call() as ctx: getattr(git, name) From a2df3a8283274dda9236d0d41cf44a38317560cb Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 20 Mar 2024 20:40:17 -0400 Subject: [PATCH 20/89] Test static typing of private module aliases This tests that mypy considers them not to be present. That mypy is configured with `warn_unused_ignores = true` is key, since that is what verifies that the type errors really do occur, based on the suppressions written for them. --- test/deprecation/test_attributes.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 85aa7a579..53612bde2 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -9,12 +9,12 @@ def test_cannot_get_undefined() -> None: with pytest.raises(AttributeError): - git.foo + git.foo # type: ignore[attr-defined] def test_cannot_import_undefined() -> None: with pytest.raises(ImportError): - from git import foo # noqa: F401 + from git import foo # type: ignore[attr-defined] # noqa: F401 def test_util_alias_access_resolves() -> None: @@ -49,6 +49,21 @@ def test_util_alias_import_warns() -> None: assert "should not be relied on" in message +def test_private_module_aliases() -> None: + """These exist dynamically but mypy will show them as absent (intentionally). + + More detailed dynamic behavior is examined in the subsequent test cases. + """ + git.head # type: ignore[attr-defined] + git.log # type: ignore[attr-defined] + git.reference # type: ignore[attr-defined] + git.symbolic # type: ignore[attr-defined] + git.tag # type: ignore[attr-defined] + git.base # type: ignore[attr-defined] + git.fun # type: ignore[attr-defined] + git.typ # type: ignore[attr-defined] + + _parametrize_by_private_alias = pytest.mark.parametrize( "name, fullname", [ From a15a830d47089ba625374fa3fd65e8568b7e0372 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 20 Mar 2024 20:52:56 -0400 Subject: [PATCH 21/89] Improve a couple test case docstrings --- test/deprecation/test_attributes.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 53612bde2..6df1359f5 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -18,7 +18,7 @@ def test_cannot_import_undefined() -> None: def test_util_alias_access_resolves() -> None: - """These resolve for now, though they're private we do not guarantee this.""" + """These resolve for now, though they're private and we do not guarantee this.""" assert git.util is git.index.util @@ -50,7 +50,7 @@ def test_util_alias_import_warns() -> None: def test_private_module_aliases() -> None: - """These exist dynamically but mypy will show them as absent (intentionally). + """These exist dynamically (for now) but mypy treats them as absent (intentionally). More detailed dynamic behavior is examined in the subsequent test cases. """ From dbaa535e47b61db0f9ce29c65b2a6616f6454196 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 20 Mar 2024 20:57:05 -0400 Subject: [PATCH 22/89] Add a couple missing assert keywords --- test/deprecation/test_attributes.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 6df1359f5..b9ca1d7c2 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -25,7 +25,7 @@ def test_util_alias_access_resolves() -> None: def test_util_alias_import_resolves() -> None: from git import util - util is git.index.util + assert util is git.index.util def test_util_alias_access_warns() -> None: @@ -88,7 +88,7 @@ def test_private_module_alias_access_resolves(name: str, fullname: str) -> None: @_parametrize_by_private_alias def test_private_module_alias_import_resolves(name: str, fullname: str) -> None: exec(f"from git import {name}") - locals()[name] is importlib.import_module(fullname) + assert locals()[name] is importlib.import_module(fullname) @_parametrize_by_private_alias From d00c843434806389bfb0bf7992505f358e97513f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 20 Mar 2024 21:00:05 -0400 Subject: [PATCH 23/89] Clarify how test_private_module_aliases is statically checkable --- test/deprecation/test_attributes.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index b9ca1d7c2..386ae1838 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -52,6 +52,9 @@ def test_util_alias_import_warns() -> None: def test_private_module_aliases() -> None: """These exist dynamically (for now) but mypy treats them as absent (intentionally). + This code verifies the effect of static type checking when analyzed by mypy, if mypy + is configured with ``warn_unused_ignores = true``. + More detailed dynamic behavior is examined in the subsequent test cases. """ git.head # type: ignore[attr-defined] From 983fda774a6eedda3b62ac2fa94ce54675a5f662 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 21 Mar 2024 02:04:21 -0400 Subject: [PATCH 24/89] Move mark-sharing tests into a class --- test/deprecation/test_attributes.py | 46 +++++++++++++---------------- 1 file changed, 20 insertions(+), 26 deletions(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 386ae1838..7af77000f 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -49,8 +49,8 @@ def test_util_alias_import_warns() -> None: assert "should not be relied on" in message -def test_private_module_aliases() -> None: - """These exist dynamically (for now) but mypy treats them as absent (intentionally). +def test_private_module_aliases_exist_dynamically() -> None: + """These exist at runtime (for now) but mypy treats them as absent (intentionally). This code verifies the effect of static type checking when analyzed by mypy, if mypy is configured with ``warn_unused_ignores = true``. @@ -67,7 +67,7 @@ def test_private_module_aliases() -> None: git.typ # type: ignore[attr-defined] -_parametrize_by_private_alias = pytest.mark.parametrize( +@pytest.mark.parametrize( "name, fullname", [ ("head", "git.refs.head"), @@ -80,32 +80,26 @@ def test_private_module_aliases() -> None: ("typ", "git.index.typ"), ], ) +class TestPrivateModuleAliases: + """Tests of the private module aliases' shared specific runtime behaviors.""" + def test_private_module_alias_access_resolves(self, name: str, fullname: str) -> None: + """These resolve for now, though they're private we do not guarantee this.""" + assert getattr(git, name) is importlib.import_module(fullname) -@_parametrize_by_private_alias -def test_private_module_alias_access_resolves(name: str, fullname: str) -> None: - """These resolve for now, though they're private we do not guarantee this.""" - assert getattr(git, name) is importlib.import_module(fullname) - - -@_parametrize_by_private_alias -def test_private_module_alias_import_resolves(name: str, fullname: str) -> None: - exec(f"from git import {name}") - assert locals()[name] is importlib.import_module(fullname) - - -@_parametrize_by_private_alias -def test_private_module_alias_access_warns(name: str, fullname: str) -> None: - with pytest.deprecated_call() as ctx: - getattr(git, name) + def test_private_module_alias_import_resolves(self, name: str, fullname: str) -> None: + exec(f"from git import {name}") + assert locals()[name] is importlib.import_module(fullname) - assert len(ctx) == 1 - assert ctx[0].message.args[0].endswith(f"Use {fullname} instead.") + def test_private_module_alias_access_warns(self, name: str, fullname: str) -> None: + with pytest.deprecated_call() as ctx: + getattr(git, name) + assert len(ctx) == 1 + assert ctx[0].message.args[0].endswith(f"Use {fullname} instead.") -@_parametrize_by_private_alias -def test_private_module_alias_import_warns(name: str, fullname: str) -> None: - with pytest.deprecated_call() as ctx: - exec(f"from git import {name}") + def test_private_module_alias_import_warns(self, name: str, fullname: str) -> None: + with pytest.deprecated_call() as ctx: + exec(f"from git import {name}") - assert ctx[0].message.args[0].endswith(f"Use {fullname} instead.") + assert ctx[0].message.args[0].endswith(f"Use {fullname} instead.") From 19acd4cf551dfd6c7774e1ca7794ef83b321c8b6 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 22 Mar 2024 01:33:00 -0400 Subject: [PATCH 25/89] Add FIXME for what to do next --- test/deprecation/test_attributes.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 7af77000f..f249ae871 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -103,3 +103,14 @@ def test_private_module_alias_import_warns(self, name: str, fullname: str) -> No exec(f"from git import {name}") assert ctx[0].message.args[0].endswith(f"Use {fullname} instead.") + + +reveal_type(git.util.git_working_dir) + +# FIXME: Add one or more test cases that access something like git.util.git_working_dir +# to verify that it is available, and also use assert_type on it to ensure mypy knows +# that accesses to expressions of the form git.util.XYZ resolve to git.index.util.XYZ. +# +# It may be necessary for GitPython, in git/__init__.py, to import util from git.index +# explicitly before (still) deleting the util global, in order for mypy to know what is +# going on. Also check pyright. From f39bbb5172987b0462b5d1845c0cb0cf3824b3d5 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 22 Mar 2024 10:52:32 -0400 Subject: [PATCH 26/89] Fix a test docstring --- test/deprecation/test_attributes.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index f249ae871..69a1aa1f3 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -84,7 +84,7 @@ class TestPrivateModuleAliases: """Tests of the private module aliases' shared specific runtime behaviors.""" def test_private_module_alias_access_resolves(self, name: str, fullname: str) -> None: - """These resolve for now, though they're private we do not guarantee this.""" + """These resolve for now, though they're private and we do not guarantee this.""" assert getattr(git, name) is importlib.import_module(fullname) def test_private_module_alias_import_resolves(self, name: str, fullname: str) -> None: From aee7078e28b5de9bb5cd605ff23f37d7328dccc9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 22 Mar 2024 18:56:02 -0400 Subject: [PATCH 27/89] Test resolution into git.index.util using git.util --- test/deprecation/test_attributes.py | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 69a1aa1f3..74f51a09e 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -1,6 +1,7 @@ """Tests for dynamic and static attribute errors.""" import importlib +from typing import Type import pytest @@ -28,6 +29,23 @@ def test_util_alias_import_resolves() -> None: assert util is git.index.util +def test_util_alias_members_resolve() -> None: + """git.index.util members can be accessed via git.util, and mypy recognizes it.""" + # TODO: When typing_extensions is made a test dependency, use assert_type for this. + gu_tfs = git.util.TemporaryFileSwap + from git.index.util import TemporaryFileSwap + + def accepts_tfs_type(t: Type[TemporaryFileSwap]) -> None: + pass + + def rejects_tfs_type(t: Type[git.Git]) -> None: + pass + + accepts_tfs_type(gu_tfs) + rejects_tfs_type(gu_tfs) # type: ignore[arg-type] + assert gu_tfs is TemporaryFileSwap + + def test_util_alias_access_warns() -> None: with pytest.deprecated_call() as ctx: git.util @@ -103,14 +121,3 @@ def test_private_module_alias_import_warns(self, name: str, fullname: str) -> No exec(f"from git import {name}") assert ctx[0].message.args[0].endswith(f"Use {fullname} instead.") - - -reveal_type(git.util.git_working_dir) - -# FIXME: Add one or more test cases that access something like git.util.git_working_dir -# to verify that it is available, and also use assert_type on it to ensure mypy knows -# that accesses to expressions of the form git.util.XYZ resolve to git.index.util.XYZ. -# -# It may be necessary for GitPython, in git/__init__.py, to import util from git.index -# explicitly before (still) deleting the util global, in order for mypy to know what is -# going on. Also check pyright. From 7f4a19135c755065538d13ed4faeb9c10cc203c8 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 22 Mar 2024 19:01:11 -0400 Subject: [PATCH 28/89] Fix brittle way of checking warning messages Which was causing a type error. --- test/deprecation/test_attributes.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 74f51a09e..0f142fbe7 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -51,7 +51,7 @@ def test_util_alias_access_warns() -> None: git.util assert len(ctx) == 1 - message = ctx[0].message.args[0] + message = str(ctx[0].message) assert "git.util" in message assert "git.index.util" in message assert "should not be relied on" in message @@ -61,7 +61,7 @@ def test_util_alias_import_warns() -> None: with pytest.deprecated_call() as ctx: from git import util # noqa: F401 - message = ctx[0].message.args[0] + message = str(ctx[0].message) assert "git.util" in message assert "git.index.util" in message assert "should not be relied on" in message @@ -114,10 +114,12 @@ def test_private_module_alias_access_warns(self, name: str, fullname: str) -> No getattr(git, name) assert len(ctx) == 1 - assert ctx[0].message.args[0].endswith(f"Use {fullname} instead.") + message = str(ctx[0].message) + assert message.endswith(f"Use {fullname} instead.") def test_private_module_alias_import_warns(self, name: str, fullname: str) -> None: with pytest.deprecated_call() as ctx: exec(f"from git import {name}") - assert ctx[0].message.args[0].endswith(f"Use {fullname} instead.") + message = str(ctx[0].message) + assert message.endswith(f"Use {fullname} instead.") From d08a5768f6e8aa78de5f10c7e7a0777d2e4dfec3 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 22 Mar 2024 19:09:44 -0400 Subject: [PATCH 29/89] Clarify todo --- test/deprecation/test_attributes.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 0f142fbe7..829ff29d2 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -31,7 +31,6 @@ def test_util_alias_import_resolves() -> None: def test_util_alias_members_resolve() -> None: """git.index.util members can be accessed via git.util, and mypy recognizes it.""" - # TODO: When typing_extensions is made a test dependency, use assert_type for this. gu_tfs = git.util.TemporaryFileSwap from git.index.util import TemporaryFileSwap @@ -41,8 +40,10 @@ def accepts_tfs_type(t: Type[TemporaryFileSwap]) -> None: def rejects_tfs_type(t: Type[git.Git]) -> None: pass + # TODO: When typing_extensions is made a test dependency, use assert_type for this. accepts_tfs_type(gu_tfs) rejects_tfs_type(gu_tfs) # type: ignore[arg-type] + assert gu_tfs is TemporaryFileSwap From 9d58e6d367dc4651213e674b9f586e0a18d787bc Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 14:35:36 -0400 Subject: [PATCH 30/89] Start reorganizing new tests more in the GitPython style --- test/deprecation/test_attributes.py | 142 +++++++++++++++------------- 1 file changed, 77 insertions(+), 65 deletions(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 829ff29d2..97aa9bc50 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -1,4 +1,8 @@ -"""Tests for dynamic and static attribute errors.""" +"""Tests for dynamic and static attribute errors in GitPython's top-level git module. + +Provided mypy has ``warn_unused_ignores = true`` set, running mypy on these test cases +checks static typing of the code under test. (Running pytest checks dynamic behavior.) +""" import importlib from typing import Type @@ -18,17 +22,6 @@ def test_cannot_import_undefined() -> None: from git import foo # type: ignore[attr-defined] # noqa: F401 -def test_util_alias_access_resolves() -> None: - """These resolve for now, though they're private and we do not guarantee this.""" - assert git.util is git.index.util - - -def test_util_alias_import_resolves() -> None: - from git import util - - assert util is git.index.util - - def test_util_alias_members_resolve() -> None: """git.index.util members can be accessed via git.util, and mypy recognizes it.""" gu_tfs = git.util.TemporaryFileSwap @@ -68,59 +61,78 @@ def test_util_alias_import_warns() -> None: assert "should not be relied on" in message -def test_private_module_aliases_exist_dynamically() -> None: - """These exist at runtime (for now) but mypy treats them as absent (intentionally). - - This code verifies the effect of static type checking when analyzed by mypy, if mypy - is configured with ``warn_unused_ignores = true``. - - More detailed dynamic behavior is examined in the subsequent test cases. - """ - git.head # type: ignore[attr-defined] - git.log # type: ignore[attr-defined] - git.reference # type: ignore[attr-defined] - git.symbolic # type: ignore[attr-defined] - git.tag # type: ignore[attr-defined] - git.base # type: ignore[attr-defined] - git.fun # type: ignore[attr-defined] - git.typ # type: ignore[attr-defined] - - -@pytest.mark.parametrize( - "name, fullname", - [ - ("head", "git.refs.head"), - ("log", "git.refs.log"), - ("reference", "git.refs.reference"), - ("symbolic", "git.refs.symbolic"), - ("tag", "git.refs.tag"), - ("base", "git.index.base"), - ("fun", "git.index.fun"), - ("typ", "git.index.typ"), - ], +# Split out util and have all its tests be separate, above. +_MODULE_ALIAS_TARGETS = ( + git.refs.head, + git.refs.log, + git.refs.reference, + git.refs.symbolic, + git.refs.tag, + git.index.base, + git.index.fun, + git.index.typ, + git.index.util, ) -class TestPrivateModuleAliases: - """Tests of the private module aliases' shared specific runtime behaviors.""" - def test_private_module_alias_access_resolves(self, name: str, fullname: str) -> None: - """These resolve for now, though they're private and we do not guarantee this.""" - assert getattr(git, name) is importlib.import_module(fullname) - def test_private_module_alias_import_resolves(self, name: str, fullname: str) -> None: - exec(f"from git import {name}") - assert locals()[name] is importlib.import_module(fullname) - - def test_private_module_alias_access_warns(self, name: str, fullname: str) -> None: - with pytest.deprecated_call() as ctx: - getattr(git, name) - - assert len(ctx) == 1 - message = str(ctx[0].message) - assert message.endswith(f"Use {fullname} instead.") - - def test_private_module_alias_import_warns(self, name: str, fullname: str) -> None: - with pytest.deprecated_call() as ctx: - exec(f"from git import {name}") - - message = str(ctx[0].message) - assert message.endswith(f"Use {fullname} instead.") +def test_private_module_alias_access_on_git_module() -> None: + """Private alias access works, warns, and except for util is a mypy error.""" + with pytest.deprecated_call() as ctx: + assert ( + git.head, # type: ignore[attr-defined] + git.log, # type: ignore[attr-defined] + git.reference, # type: ignore[attr-defined] + git.symbolic, # type: ignore[attr-defined] + git.tag, # type: ignore[attr-defined] + git.base, # type: ignore[attr-defined] + git.fun, # type: ignore[attr-defined] + git.typ, # type: ignore[attr-defined] + git.util, + ) == _MODULE_ALIAS_TARGETS + + messages = [str(w.message) for w in ctx] + for target, message in zip(_MODULE_ALIAS_TARGETS[:-1], messages[:-1], strict=True): + assert message.endswith(f"Use {target.__name__} instead.") + + util_message = messages[-1] + assert "git.util" in util_message + assert "git.index.util" in util_message + assert "should not be relied on" in util_message + + +def test_private_module_alias_import_from_git_module() -> None: + """Private alias import works, warns, and except for util is a mypy error.""" + with pytest.deprecated_call() as ctx: + from git import head # type: ignore[attr-defined] + from git import log # type: ignore[attr-defined] + from git import reference # type: ignore[attr-defined] + from git import symbolic # type: ignore[attr-defined] + from git import tag # type: ignore[attr-defined] + from git import base # type: ignore[attr-defined] + from git import fun # type: ignore[attr-defined] + from git import typ # type: ignore[attr-defined] + from git import util + + assert ( + head, + log, + reference, + symbolic, + tag, + base, + fun, + typ, + util, + ) == _MODULE_ALIAS_TARGETS + + # FIXME: This fails because, with imports, multiple consecutive accesses may occur. + # In practice, with CPython, it is always exactly two accesses, the first from the + # equivalent of a hasattr, and the second to fetch the attribute intentionally. + messages = [str(w.message) for w in ctx] + for target, message in zip(_MODULE_ALIAS_TARGETS[:-1], messages[:-1], strict=True): + assert message.endswith(f"Use {target.__name__} instead.") + + util_message = messages[-1] + assert "git.util" in util_message + assert "git.index.util" in util_message + assert "should not be relied on" in util_message From 45c128bcd82cd278d7926425179503c22ce1271c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 15:50:39 -0400 Subject: [PATCH 31/89] Finish reorganizing; fix assertion for duplicated messages To support the changes, this adds typing-extensions as a test dependency, since we are now importing from it in a test module. But this should probably be required and used conditionally based on whether the Python version has assert_type in its typing module. --- test-requirements.txt | 1 + test/deprecation/test_attributes.py | 98 ++++++++++++++--------------- 2 files changed, 50 insertions(+), 49 deletions(-) diff --git a/test-requirements.txt b/test-requirements.txt index e1f5e2ed4..106b46aee 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -8,3 +8,4 @@ pytest-cov pytest-instafail pytest-mock pytest-sugar +typing-extensions diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 97aa9bc50..35e2e48bb 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -4,61 +4,72 @@ checks static typing of the code under test. (Running pytest checks dynamic behavior.) """ -import importlib +from itertools import groupby from typing import Type import pytest +from typing_extensions import assert_type import git -def test_cannot_get_undefined() -> None: +def test_cannot_access_undefined() -> None: + """Accessing a bogus attribute in git remains both a dynamic and static error.""" with pytest.raises(AttributeError): git.foo # type: ignore[attr-defined] def test_cannot_import_undefined() -> None: + """Importing a bogus attribute from git remains both a dynamic and static error.""" with pytest.raises(ImportError): from git import foo # type: ignore[attr-defined] # noqa: F401 -def test_util_alias_members_resolve() -> None: - """git.index.util members can be accessed via git.util, and mypy recognizes it.""" - gu_tfs = git.util.TemporaryFileSwap - from git.index.util import TemporaryFileSwap +def test_util_alias_access() -> None: + """Accessing util in git works, warns, and mypy verifies it and its attributes.""" + # The attribute access should succeed. + with pytest.deprecated_call() as ctx: + util = git.util - def accepts_tfs_type(t: Type[TemporaryFileSwap]) -> None: - pass + # There should be exactly one warning and it should have our util-specific message. + (message,) = [str(entry.message) for entry in ctx] + assert "git.util" in message + assert "git.index.util" in message + assert "should not be relied on" in message - def rejects_tfs_type(t: Type[git.Git]) -> None: - pass + # We check access through the util alias to the TemporaryFileSwap member, since it + # is slightly simpler to validate and reason about than the other public members, + # which are functions (specifically, higher-order functions for use as decorators). + from git.index.util import TemporaryFileSwap - # TODO: When typing_extensions is made a test dependency, use assert_type for this. - accepts_tfs_type(gu_tfs) - rejects_tfs_type(gu_tfs) # type: ignore[arg-type] + assert_type(util.TemporaryFileSwap, Type[TemporaryFileSwap]) - assert gu_tfs is TemporaryFileSwap + # This comes after the static assertion, just in case it would affect the inference. + assert util.TemporaryFileSwap is TemporaryFileSwap -def test_util_alias_access_warns() -> None: +def test_util_alias_import() -> None: + """Importing util from git works, warns, and mypy verifies it and its attributes.""" + # The import should succeed. with pytest.deprecated_call() as ctx: - git.util + from git import util - assert len(ctx) == 1 - message = str(ctx[0].message) + # There may be multiple warnings. In CPython there will be currently always be + # exactly two, possibly due to the equivalent of calling hasattr to do a pre-check + # prior to retrieving the attribute for actual use. However, all warnings should + # have the same message, and it should be our util-specific message. + (message,) = {str(entry.message) for entry in ctx} assert "git.util" in message assert "git.index.util" in message assert "should not be relied on" in message + # As above, we check access through the util alias to the TemporaryFileSwap member. + from git.index.util import TemporaryFileSwap -def test_util_alias_import_warns() -> None: - with pytest.deprecated_call() as ctx: - from git import util # noqa: F401 + assert_type(util.TemporaryFileSwap, Type[TemporaryFileSwap]) - message = str(ctx[0].message) - assert "git.util" in message - assert "git.index.util" in message - assert "should not be relied on" in message + # This comes after the static assertion, just in case it would affect the inference. + assert util.TemporaryFileSwap is TemporaryFileSwap # Split out util and have all its tests be separate, above. @@ -71,12 +82,11 @@ def test_util_alias_import_warns() -> None: git.index.base, git.index.fun, git.index.typ, - git.index.util, ) -def test_private_module_alias_access_on_git_module() -> None: - """Private alias access works, warns, and except for util is a mypy error.""" +def test_private_module_alias_access() -> None: + """Non-util private alias access works, warns, but is a deliberate mypy error.""" with pytest.deprecated_call() as ctx: assert ( git.head, # type: ignore[attr-defined] @@ -87,21 +97,16 @@ def test_private_module_alias_access_on_git_module() -> None: git.base, # type: ignore[attr-defined] git.fun, # type: ignore[attr-defined] git.typ, # type: ignore[attr-defined] - git.util, ) == _MODULE_ALIAS_TARGETS + # Each should have warned exactly once, and note what to use instead. messages = [str(w.message) for w in ctx] - for target, message in zip(_MODULE_ALIAS_TARGETS[:-1], messages[:-1], strict=True): + for target, message in zip(_MODULE_ALIAS_TARGETS, messages, strict=True): assert message.endswith(f"Use {target.__name__} instead.") - util_message = messages[-1] - assert "git.util" in util_message - assert "git.index.util" in util_message - assert "should not be relied on" in util_message - -def test_private_module_alias_import_from_git_module() -> None: - """Private alias import works, warns, and except for util is a mypy error.""" +def test_private_module_alias_import() -> None: + """Non-util private alias access works, warns, but is a deliberate mypy error.""" with pytest.deprecated_call() as ctx: from git import head # type: ignore[attr-defined] from git import log # type: ignore[attr-defined] @@ -111,7 +116,6 @@ def test_private_module_alias_import_from_git_module() -> None: from git import base # type: ignore[attr-defined] from git import fun # type: ignore[attr-defined] from git import typ # type: ignore[attr-defined] - from git import util assert ( head, @@ -122,17 +126,13 @@ def test_private_module_alias_import_from_git_module() -> None: base, fun, typ, - util, ) == _MODULE_ALIAS_TARGETS - # FIXME: This fails because, with imports, multiple consecutive accesses may occur. - # In practice, with CPython, it is always exactly two accesses, the first from the - # equivalent of a hasattr, and the second to fetch the attribute intentionally. - messages = [str(w.message) for w in ctx] - for target, message in zip(_MODULE_ALIAS_TARGETS[:-1], messages[:-1], strict=True): + # Each import may warn multiple times. In CPython there will be currently always be + # exactly two warnings per import, possibly due to the equivalent of calling hasattr + # to do a pre-check prior to retrieving the attribute for actual use. However, for + # each import, all messages should be the same and should note what to use instead. + messages_with_duplicates = [str(w.message) for w in ctx] + messages = [message for message, _ in groupby(messages_with_duplicates)] + for target, message in zip(_MODULE_ALIAS_TARGETS, messages, strict=True): assert message.endswith(f"Use {target.__name__} instead.") - - util_message = messages[-1] - assert "git.util" in util_message - assert "git.index.util" in util_message - assert "should not be relied on" in util_message From 247dc15fd81ecc806be732d7f1ef0c12e26920d8 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 15:55:57 -0400 Subject: [PATCH 32/89] Add imports so pyright recognizes refs and index pyright still reports git.util as private, as it should. (mypy does not, or does not by default, report private member access. GitPython does not generally use pyright as part of development at this time, but I am checking some code with it during the process of writing the new tests.) --- test/deprecation/test_attributes.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 35e2e48bb..95feaaf8e 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -11,6 +11,14 @@ from typing_extensions import assert_type import git +import git.index.base +import git.index.fun +import git.index.typ +import git.refs.head +import git.refs.log +import git.refs.reference +import git.refs.symbolic +import git.refs.tag def test_cannot_access_undefined() -> None: From b05963c3749494f633117316a493ab2b179e0069 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 16:05:53 -0400 Subject: [PATCH 33/89] Expand and clarify test module docstring About why there are so many separate mypy suppressions even when they could be consolidated into a smaller number in some places. --- test/deprecation/test_attributes.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 95feaaf8e..218b9dd13 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -1,7 +1,11 @@ """Tests for dynamic and static attribute errors in GitPython's top-level git module. Provided mypy has ``warn_unused_ignores = true`` set, running mypy on these test cases -checks static typing of the code under test. (Running pytest checks dynamic behavior.) +checks static typing of the code under test. This is the reason for the many separate +single-line attr-defined suppressions, so those should not be replaced with a smaller +number of more broadly scoped suppressions, even where it is feasible to do so. + +Running pytest checks dynamic behavior as usual. """ from itertools import groupby From 074bbc72a84db7605f3136dc9f4b4ad822a3b481 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 16:08:37 -0400 Subject: [PATCH 34/89] Tiny import tweak --- test/deprecation/test_attributes.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 218b9dd13..1bcca44d5 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -8,7 +8,7 @@ Running pytest checks dynamic behavior as usual. """ -from itertools import groupby +import itertools from typing import Type import pytest @@ -145,6 +145,6 @@ def test_private_module_alias_import() -> None: # to do a pre-check prior to retrieving the attribute for actual use. However, for # each import, all messages should be the same and should note what to use instead. messages_with_duplicates = [str(w.message) for w in ctx] - messages = [message for message, _ in groupby(messages_with_duplicates)] + messages = [message for message, _ in itertools.groupby(messages_with_duplicates)] for target, message in zip(_MODULE_ALIAS_TARGETS, messages, strict=True): assert message.endswith(f"Use {target.__name__} instead.") From 18608e472535149e542cc30ff95755ed962c9156 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 16:12:59 -0400 Subject: [PATCH 35/89] Pick a better name for _MODULE_ALIAS_TARGETS And add a docstring to document it, mainly to clarify that util is intentionally omitted from that constant. --- test/deprecation/test_attributes.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 1bcca44d5..6e98a5e09 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -85,7 +85,7 @@ def test_util_alias_import() -> None: # Split out util and have all its tests be separate, above. -_MODULE_ALIAS_TARGETS = ( +_PRIVATE_MODULE_ALIAS_TARGETS = ( git.refs.head, git.refs.log, git.refs.reference, @@ -95,6 +95,7 @@ def test_util_alias_import() -> None: git.index.fun, git.index.typ, ) +"""Targets of private aliases in the git module to some modules, not including util.""" def test_private_module_alias_access() -> None: @@ -109,11 +110,11 @@ def test_private_module_alias_access() -> None: git.base, # type: ignore[attr-defined] git.fun, # type: ignore[attr-defined] git.typ, # type: ignore[attr-defined] - ) == _MODULE_ALIAS_TARGETS + ) == _PRIVATE_MODULE_ALIAS_TARGETS # Each should have warned exactly once, and note what to use instead. messages = [str(w.message) for w in ctx] - for target, message in zip(_MODULE_ALIAS_TARGETS, messages, strict=True): + for target, message in zip(_PRIVATE_MODULE_ALIAS_TARGETS, messages, strict=True): assert message.endswith(f"Use {target.__name__} instead.") @@ -138,7 +139,7 @@ def test_private_module_alias_import() -> None: base, fun, typ, - ) == _MODULE_ALIAS_TARGETS + ) == _PRIVATE_MODULE_ALIAS_TARGETS # Each import may warn multiple times. In CPython there will be currently always be # exactly two warnings per import, possibly due to the equivalent of calling hasattr @@ -146,5 +147,5 @@ def test_private_module_alias_import() -> None: # each import, all messages should be the same and should note what to use instead. messages_with_duplicates = [str(w.message) for w in ctx] messages = [message for message, _ in itertools.groupby(messages_with_duplicates)] - for target, message in zip(_MODULE_ALIAS_TARGETS, messages, strict=True): + for target, message in zip(_PRIVATE_MODULE_ALIAS_TARGETS, messages, strict=True): assert message.endswith(f"Use {target.__name__} instead.") From 1f290f17943be338bb79a54ce1bef21e90da4402 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 16:36:22 -0400 Subject: [PATCH 36/89] Use typing_extensions only if needed This makes the typing-extensions test dependency < 3.11 only, and conditionally imports assert_type from typing or typing_extensions depending on the Python version. --- test-requirements.txt | 2 +- test/deprecation/test_attributes.py | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/test-requirements.txt b/test-requirements.txt index 106b46aee..75e9e81fa 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -8,4 +8,4 @@ pytest-cov pytest-instafail pytest-mock pytest-sugar -typing-extensions +typing-extensions ; python_version < "3.11" diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 6e98a5e09..2150186f9 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -9,8 +9,14 @@ """ import itertools +import sys from typing import Type +if sys.version_info >= (3, 11): + from typing import assert_type +else: + from typing_extensions import assert_type + import pytest from typing_extensions import assert_type From 7a4f7eb092fbf68b058de38ee2df193c86d4e34e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 17:07:32 -0400 Subject: [PATCH 37/89] Fix zip calls This omits strict=True, which is only supported in Python 3.10 and later, and instead explicitly asserts that the arguments are the same length (which is arguably better for its explicitness anyway). --- test/deprecation/test_attributes.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index 2150186f9..cd26f602b 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -120,7 +120,10 @@ def test_private_module_alias_access() -> None: # Each should have warned exactly once, and note what to use instead. messages = [str(w.message) for w in ctx] - for target, message in zip(_PRIVATE_MODULE_ALIAS_TARGETS, messages, strict=True): + + assert len(messages) == len(_PRIVATE_MODULE_ALIAS_TARGETS) + + for target, message in zip(_PRIVATE_MODULE_ALIAS_TARGETS, messages): assert message.endswith(f"Use {target.__name__} instead.") @@ -153,5 +156,8 @@ def test_private_module_alias_import() -> None: # each import, all messages should be the same and should note what to use instead. messages_with_duplicates = [str(w.message) for w in ctx] messages = [message for message, _ in itertools.groupby(messages_with_duplicates)] - for target, message in zip(_PRIVATE_MODULE_ALIAS_TARGETS, messages, strict=True): + + assert len(messages) == len(_PRIVATE_MODULE_ALIAS_TARGETS) + + for target, message in zip(_PRIVATE_MODULE_ALIAS_TARGETS, messages): assert message.endswith(f"Use {target.__name__} instead.") From 5977a6ec9e1646ac94514428a0ad2363be8da2f9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 17:19:39 -0400 Subject: [PATCH 38/89] Fix (and improve wording) of docstrings --- test/deprecation/test_attributes.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index cd26f602b..e4fb39975 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -105,7 +105,7 @@ def test_util_alias_import() -> None: def test_private_module_alias_access() -> None: - """Non-util private alias access works, warns, but is a deliberate mypy error.""" + """Non-util private alias access works but warns and is a deliberate mypy error.""" with pytest.deprecated_call() as ctx: assert ( git.head, # type: ignore[attr-defined] @@ -128,7 +128,7 @@ def test_private_module_alias_access() -> None: def test_private_module_alias_import() -> None: - """Non-util private alias access works, warns, but is a deliberate mypy error.""" + """Non-util private alias import works but warns and is a deliberate mypy error.""" with pytest.deprecated_call() as ctx: from git import head # type: ignore[attr-defined] from git import log # type: ignore[attr-defined] From 5b1fa580400c386932eb9f66c568e4e0090e2779 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 17:46:57 -0400 Subject: [PATCH 39/89] Remove extra import "from typing_extensions" --- test/deprecation/test_attributes.py | 1 - 1 file changed, 1 deletion(-) diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_attributes.py index e4fb39975..eb909b299 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_attributes.py @@ -18,7 +18,6 @@ from typing_extensions import assert_type import pytest -from typing_extensions import assert_type import git import git.index.base From a07be0e35118874f682fa2e2be3b793170bf4853 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 18:12:19 -0400 Subject: [PATCH 40/89] Start on test_compat And rename test_attributes to test_toplevel accordingly. --- test/deprecation/test_compat.py | 33 +++++++++++++++++++ .../{test_attributes.py => test_toplevel.py} | 6 ++-- 2 files changed, 36 insertions(+), 3 deletions(-) create mode 100644 test/deprecation/test_compat.py rename test/deprecation/{test_attributes.py => test_toplevel.py} (95%) diff --git a/test/deprecation/test_compat.py b/test/deprecation/test_compat.py new file mode 100644 index 000000000..dd2f0b0c2 --- /dev/null +++ b/test/deprecation/test_compat.py @@ -0,0 +1,33 @@ +"""Tests for dynamic and static errors and warnings in GitPython's git.compat module. + +These tests verify that the is_ aliases are available, and are even listed in +the output of dir(), but issue warnings, and that bogus (misspelled or unrecognized) +attribute access is still an error both at runtime and with mypy. This is similar to +some of the tests in test_toplevel, but the situation being tested here is simpler +because it does not involve unintuitive module aliasing or import behavior. So this only +tests attribute access, not "from" imports (whose behavior can be intuitively inferred). +""" + +import os +import sys + +import pytest + +import git.compat + + +_MESSAGE_LEADER = "{} and other is_ aliases are deprecated." + + +def test_cannot_access_undefined() -> None: + """Accessing a bogus attribute in git.compat remains a dynamic and static error.""" + with pytest.raises(AttributeError): + git.compat.foo # type: ignore[attr-defined] + + +def test_is_win() -> None: + with pytest.deprecated_call() as ctx: + value = git.compat.is_win + (message,) = [str(entry.message) for entry in ctx] # Exactly one message. + assert message.startswith(_MESSAGE_LEADER.format("git.compat.is_win")) + assert value == (os.name == "nt") diff --git a/test/deprecation/test_attributes.py b/test/deprecation/test_toplevel.py similarity index 95% rename from test/deprecation/test_attributes.py rename to test/deprecation/test_toplevel.py index eb909b299..2a662127b 100644 --- a/test/deprecation/test_attributes.py +++ b/test/deprecation/test_toplevel.py @@ -1,4 +1,4 @@ -"""Tests for dynamic and static attribute errors in GitPython's top-level git module. +"""Tests for dynamic and static errors and warnings in GitPython's top-level git module. Provided mypy has ``warn_unused_ignores = true`` set, running mypy on these test cases checks static typing of the code under test. This is the reason for the many separate @@ -31,13 +31,13 @@ def test_cannot_access_undefined() -> None: - """Accessing a bogus attribute in git remains both a dynamic and static error.""" + """Accessing a bogus attribute in git remains a dynamic and static error.""" with pytest.raises(AttributeError): git.foo # type: ignore[attr-defined] def test_cannot_import_undefined() -> None: - """Importing a bogus attribute from git remains both a dynamic and static error.""" + """Importing a bogus attribute from git remains a dynamic and static error.""" with pytest.raises(ImportError): from git import foo # type: ignore[attr-defined] # noqa: F401 From d4917d0a326d935ee0d6728af3268e9ece8b09df Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 18:19:36 -0400 Subject: [PATCH 41/89] Expand to test all three is_ aliases --- test/deprecation/test_compat.py | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/test/deprecation/test_compat.py b/test/deprecation/test_compat.py index dd2f0b0c2..6d2d87a39 100644 --- a/test/deprecation/test_compat.py +++ b/test/deprecation/test_compat.py @@ -25,9 +25,29 @@ def test_cannot_access_undefined() -> None: git.compat.foo # type: ignore[attr-defined] -def test_is_win() -> None: +def test_is_platform() -> None: + """The is_ aliases work, warn, and mypy accepts code accessing them.""" + fully_qualified_names = [ + "git.compat.is_win", + "git.compat.is_posix", + "git.compat.is_darwin", + ] + with pytest.deprecated_call() as ctx: - value = git.compat.is_win - (message,) = [str(entry.message) for entry in ctx] # Exactly one message. - assert message.startswith(_MESSAGE_LEADER.format("git.compat.is_win")) - assert value == (os.name == "nt") + is_win = git.compat.is_win + is_posix = git.compat.is_posix + is_darwin = git.compat.is_darwin + + messages = [str(entry.message) for entry in ctx] + assert len(messages) == 3 + + for fullname, message in zip(fully_qualified_names, messages): + assert message.startswith(_MESSAGE_LEADER.format(fullname)) + + # These exactly reproduce the expressions in the code under test, so they are not + # good for testing that the values are correct. Instead, the purpose of this test is + # to ensure that any dynamic machinery put in place in git.compat to cause warnings + # to be issued does not get in the way of the intended values being accessed. + assert is_win == (os.name == "nt") + assert is_posix == (os.name == "posix") + assert is_darwin == (sys.platform == "darwin") From f4e5f423f019c7d04798c896118f7fd48b8c3155 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 18:22:30 -0400 Subject: [PATCH 42/89] Slightly improve docstrings --- test/deprecation/test_compat.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/deprecation/test_compat.py b/test/deprecation/test_compat.py index 6d2d87a39..45d631e37 100644 --- a/test/deprecation/test_compat.py +++ b/test/deprecation/test_compat.py @@ -1,7 +1,7 @@ """Tests for dynamic and static errors and warnings in GitPython's git.compat module. -These tests verify that the is_ aliases are available, and are even listed in -the output of dir(), but issue warnings, and that bogus (misspelled or unrecognized) +These tests verify that the is_ attributes are available, and are even listed +in the output of dir(), but issue warnings, and that bogus (misspelled or unrecognized) attribute access is still an error both at runtime and with mypy. This is similar to some of the tests in test_toplevel, but the situation being tested here is simpler because it does not involve unintuitive module aliasing or import behavior. So this only @@ -15,8 +15,8 @@ import git.compat - _MESSAGE_LEADER = "{} and other is_ aliases are deprecated." +"""Form taken by the beginning of the warnings issues for is_ access.""" def test_cannot_access_undefined() -> None: @@ -26,7 +26,7 @@ def test_cannot_access_undefined() -> None: def test_is_platform() -> None: - """The is_ aliases work, warn, and mypy accepts code accessing them.""" + """The is_ attributes work, warn, and mypy accepts code accessing them.""" fully_qualified_names = [ "git.compat.is_win", "git.compat.is_posix", From d54f851d52f4217e904cbd163032aabbf33ec394 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 18:27:55 -0400 Subject: [PATCH 43/89] Add test of dir() on git.compat --- test/deprecation/test_compat.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/deprecation/test_compat.py b/test/deprecation/test_compat.py index 45d631e37..08911d17c 100644 --- a/test/deprecation/test_compat.py +++ b/test/deprecation/test_compat.py @@ -51,3 +51,17 @@ def test_is_platform() -> None: assert is_win == (os.name == "nt") assert is_posix == (os.name == "posix") assert is_darwin == (sys.platform == "darwin") + + +def test_dir() -> None: + """dir() on git.compat lists attributes meant to be public, even if deprecated.""" + expected = { + "defenc", + "safe_decode", + "safe_encode", + "win_encode", + "is_darwin", + "is_win", + "is_posix", + } + assert expected <= set(dir(git.compat)) From aaf046aba4b46fbe0dddf8c10d31f42b6c4d7c57 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 18:28:05 -0400 Subject: [PATCH 44/89] Add static type assertions to is_platform test --- test/deprecation/test_compat.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/deprecation/test_compat.py b/test/deprecation/test_compat.py index 08911d17c..c3cc5b0dd 100644 --- a/test/deprecation/test_compat.py +++ b/test/deprecation/test_compat.py @@ -11,6 +11,11 @@ import os import sys +if sys.version_info >= (3, 11): + from typing import assert_type +else: + from typing_extensions import assert_type + import pytest import git.compat @@ -38,6 +43,10 @@ def test_is_platform() -> None: is_posix = git.compat.is_posix is_darwin = git.compat.is_darwin + assert_type(is_win, bool) + assert_type(is_posix, bool) + assert_type(is_darwin, bool) + messages = [str(entry.message) for entry in ctx] assert len(messages) == 3 From 84d734d5b88cb8a03ab723f92bf83593d53d030f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 18:29:23 -0400 Subject: [PATCH 45/89] Refactor test_compat.test_dir for clarity --- test/deprecation/test_compat.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/deprecation/test_compat.py b/test/deprecation/test_compat.py index c3cc5b0dd..6e42d0209 100644 --- a/test/deprecation/test_compat.py +++ b/test/deprecation/test_compat.py @@ -64,7 +64,7 @@ def test_is_platform() -> None: def test_dir() -> None: """dir() on git.compat lists attributes meant to be public, even if deprecated.""" - expected = { + expected_subset = { "defenc", "safe_decode", "safe_encode", @@ -73,4 +73,5 @@ def test_dir() -> None: "is_win", "is_posix", } - assert expected <= set(dir(git.compat)) + actual = set(dir(git.compat)) + assert expected_subset <= actual From 3a621b38ee98a1d1413a12fcb68aae17d102f396 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 18:45:31 -0400 Subject: [PATCH 46/89] Add top-level dir() tests --- test/deprecation/test_toplevel.py | 49 +++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/test/deprecation/test_toplevel.py b/test/deprecation/test_toplevel.py index 2a662127b..f74f09457 100644 --- a/test/deprecation/test_toplevel.py +++ b/test/deprecation/test_toplevel.py @@ -160,3 +160,52 @@ def test_private_module_alias_import() -> None: for target, message in zip(_PRIVATE_MODULE_ALIAS_TARGETS, messages): assert message.endswith(f"Use {target.__name__} instead.") + + +def test_dir_contains_public_attributes() -> None: + """All public attributes of the git module are present when dir() is called on it. + + This is naturally the case, but some ways of adding dynamic attribute access + behavior can change it, especially if __dir__ is defined but care is not taken to + preserve the contents that should already be present. + + Note that dir() should usually automatically list non-public attributes if they are + actually "physically" present as well, so the approach taken here to test it should + not be reproduced if __dir__ is added (instead, a call to globals() could be used, + as its keys list the automatic values). + """ + expected_subset = set(git.__all__) + actual = set(dir(git)) + assert expected_subset <= actual + + +def test_dir_does_not_contain_util() -> None: + """The util attribute is absent from the dir() of git. + + Because this behavior is less confusing than including it, where its meaning would + be assumed by users examining the dir() for what is available. + """ + assert "util" not in dir(git) + + +def test_dir_does_not_contain_private_module_aliases() -> None: + """Names from inside index and refs only pretend to be there and are not in dir(). + + The reason for omitting these is not that they are private, since private members + are usually included in dir() when actually present. Instead, these are only sort + of even there, no longer being imported and only being resolved dynamically for the + time being. In addition, it would be confusing to list these because doing so would + obscure the module structure of GitPython. + """ + expected_absent = { + "head", + "log", + "reference", + "symbolic", + "tag", + "base", + "fun", + "typ", + } + actual = set(dir(git)) + assert not (expected_absent & actual), "They should be completely disjoint." From 05e0878aef38a5fb1cb3d5860b3649cf7c1d4fda Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 18:47:27 -0400 Subject: [PATCH 47/89] Remove old comment meant as todo (that was done) --- test/deprecation/test_toplevel.py | 1 - 1 file changed, 1 deletion(-) diff --git a/test/deprecation/test_toplevel.py b/test/deprecation/test_toplevel.py index f74f09457..fe7045d46 100644 --- a/test/deprecation/test_toplevel.py +++ b/test/deprecation/test_toplevel.py @@ -89,7 +89,6 @@ def test_util_alias_import() -> None: assert util.TemporaryFileSwap is TemporaryFileSwap -# Split out util and have all its tests be separate, above. _PRIVATE_MODULE_ALIAS_TARGETS = ( git.refs.head, git.refs.log, From 3fe2f15d218744496e4af77b6a7926791480adfe Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 19:01:24 -0400 Subject: [PATCH 48/89] Test that top-level aliases point to modules with normal __name__ --- test/deprecation/test_toplevel.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/deprecation/test_toplevel.py b/test/deprecation/test_toplevel.py index fe7045d46..135cc53cc 100644 --- a/test/deprecation/test_toplevel.py +++ b/test/deprecation/test_toplevel.py @@ -102,6 +102,26 @@ def test_util_alias_import() -> None: """Targets of private aliases in the git module to some modules, not including util.""" +_PRIVATE_MODULE_ALIAS_TARGET_NAMES = ( + "git.refs.head", + "git.refs.log", + "git.refs.reference", + "git.refs.symbolic", + "git.refs.tag", + "git.index.base", + "git.index.fun", + "git.index.typ", +) +"""Expected ``__name__`` attributes of targets of private aliases in the git module.""" + + +def test_alias_target_module_names_are_by_location() -> None: + """The aliases are weird, but their targets are normal, even in ``__name__``.""" + actual = [module.__name__ for module in _PRIVATE_MODULE_ALIAS_TARGETS] + expected = list(_PRIVATE_MODULE_ALIAS_TARGET_NAMES) + assert actual == expected + + def test_private_module_alias_access() -> None: """Non-util private alias access works but warns and is a deliberate mypy error.""" with pytest.deprecated_call() as ctx: From 246cc1703f69e8eba791915bb025945b03abc86b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 19:02:56 -0400 Subject: [PATCH 49/89] Use names directly on other tests The tests are written broadly (per the style elsewhere in this test suite), but narrowing the message-checking tests in this specific way has the further advantage that the logic of the code under test will be less reflected in the logic of the tests, so that bugs are less likely to be missed by being duplicated across code and tests. --- test/deprecation/test_toplevel.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/deprecation/test_toplevel.py b/test/deprecation/test_toplevel.py index 135cc53cc..16c41d4e8 100644 --- a/test/deprecation/test_toplevel.py +++ b/test/deprecation/test_toplevel.py @@ -141,8 +141,8 @@ def test_private_module_alias_access() -> None: assert len(messages) == len(_PRIVATE_MODULE_ALIAS_TARGETS) - for target, message in zip(_PRIVATE_MODULE_ALIAS_TARGETS, messages): - assert message.endswith(f"Use {target.__name__} instead.") + for fullname, message in zip(_PRIVATE_MODULE_ALIAS_TARGET_NAMES, messages): + assert message.endswith(f"Use {fullname} instead.") def test_private_module_alias_import() -> None: @@ -177,8 +177,8 @@ def test_private_module_alias_import() -> None: assert len(messages) == len(_PRIVATE_MODULE_ALIAS_TARGETS) - for target, message in zip(_PRIVATE_MODULE_ALIAS_TARGETS, messages): - assert message.endswith(f"Use {target.__name__} instead.") + for fullname, message in zip(_PRIVATE_MODULE_ALIAS_TARGET_NAMES, messages): + assert message.endswith(f"Use {fullname} instead.") def test_dir_contains_public_attributes() -> None: From d7b6b31f632593bf9e280fbb2be87dd4e16ef7c5 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 19:07:35 -0400 Subject: [PATCH 50/89] Fix a small docstring typo --- test/deprecation/test_compat.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/deprecation/test_compat.py b/test/deprecation/test_compat.py index 6e42d0209..0da5462b2 100644 --- a/test/deprecation/test_compat.py +++ b/test/deprecation/test_compat.py @@ -21,7 +21,7 @@ import git.compat _MESSAGE_LEADER = "{} and other is_ aliases are deprecated." -"""Form taken by the beginning of the warnings issues for is_ access.""" +"""Form taken by the beginning of the warnings issued for is_ access.""" def test_cannot_access_undefined() -> None: From 96089c82c0d8982935bbd3326ccfea36ce72e43b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 19:17:15 -0400 Subject: [PATCH 51/89] Improve description in test module docstrings --- test/deprecation/test_compat.py | 2 +- test/deprecation/test_toplevel.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/deprecation/test_compat.py b/test/deprecation/test_compat.py index 0da5462b2..5007fa1cc 100644 --- a/test/deprecation/test_compat.py +++ b/test/deprecation/test_compat.py @@ -1,4 +1,4 @@ -"""Tests for dynamic and static errors and warnings in GitPython's git.compat module. +"""Tests for dynamic and static characteristics of git.compat module attributes. These tests verify that the is_ attributes are available, and are even listed in the output of dir(), but issue warnings, and that bogus (misspelled or unrecognized) diff --git a/test/deprecation/test_toplevel.py b/test/deprecation/test_toplevel.py index 16c41d4e8..398938616 100644 --- a/test/deprecation/test_toplevel.py +++ b/test/deprecation/test_toplevel.py @@ -1,4 +1,4 @@ -"""Tests for dynamic and static errors and warnings in GitPython's top-level git module. +"""Tests for dynamic and static characteristics of top-level git module attributes. Provided mypy has ``warn_unused_ignores = true`` set, running mypy on these test cases checks static typing of the code under test. This is the reason for the many separate From a0ef53778d4ae665474ebd96bd25ebbb340a8a16 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 23:12:12 -0400 Subject: [PATCH 52/89] Start on test_types --- test/deprecation/test_types.py | 39 ++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 test/deprecation/test_types.py diff --git a/test/deprecation/test_types.py b/test/deprecation/test_types.py new file mode 100644 index 000000000..a2ac45829 --- /dev/null +++ b/test/deprecation/test_types.py @@ -0,0 +1,39 @@ +"""Tests for dynamic and static characteristics of git.types module attributes.""" + +import sys + +if sys.version_info >= (3, 8): + from typing import Literal +else: + from typing_extensions import Literal + +import pytest + +import git.types + + +def test_cannot_access_undefined() -> None: + """Accessing a bogus attribute in git.types remains a dynamic and static error.""" + with pytest.raises(AttributeError): + git.types.foo # type: ignore[attr-defined] + + +def test_lit_commit_ish() -> None: + """ """ + # It would be fine to test attribute access rather than a "from" import. But a + # "from" import is more likely to appear in actual usage, so it is used here. + with pytest.deprecated_call() as ctx: + from git.types import Lit_commit_ish + + # As noted in test_toplevel.test_util_alias_import, there may be multiple warnings, + # but all with the same message. + (message,) = {str(entry.message) for entry in ctx} + assert "Lit_commit_ish is deprecated." in message + assert 'Literal["commit", "tag", "blob", "tree"]' in message, "Has old definition." + assert 'Literal["commit", "tag"]' in message, "Has new definition." + assert "GitObjectTypeString" in message, "Has new type name for old definition." + + _: Lit_commit_ish = "commit" # type: ignore[valid-type] + + # It should be as documented (even though deliberately unusable in static checks). + assert Lit_commit_ish == Literal["commit", "tag"] From 52e7360cad2ebde77a1302b205eb7cf9182a75c2 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 23:13:53 -0400 Subject: [PATCH 53/89] Explain substring assertions in test_toplevel --- test/deprecation/test_toplevel.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/deprecation/test_toplevel.py b/test/deprecation/test_toplevel.py index 398938616..54dc8e358 100644 --- a/test/deprecation/test_toplevel.py +++ b/test/deprecation/test_toplevel.py @@ -76,9 +76,9 @@ def test_util_alias_import() -> None: # prior to retrieving the attribute for actual use. However, all warnings should # have the same message, and it should be our util-specific message. (message,) = {str(entry.message) for entry in ctx} - assert "git.util" in message - assert "git.index.util" in message - assert "should not be relied on" in message + assert "git.util" in message, "Has alias." + assert "git.index.util" in message, "Has target." + assert "should not be relied on" in message, "Distinct from other messages." # As above, we check access through the util alias to the TemporaryFileSwap member. from git.index.util import TemporaryFileSwap From e3675a086fe6ddfb6f8e4050497d47a74e851ed7 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 23:17:55 -0400 Subject: [PATCH 54/89] Expand Lit_commit_ish test name and write docstring --- test/deprecation/test_types.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/deprecation/test_types.py b/test/deprecation/test_types.py index a2ac45829..419435964 100644 --- a/test/deprecation/test_types.py +++ b/test/deprecation/test_types.py @@ -18,8 +18,8 @@ def test_cannot_access_undefined() -> None: git.types.foo # type: ignore[attr-defined] -def test_lit_commit_ish() -> None: - """ """ +def test_can_access_lit_commit_ish_but_it_is_not_usable() -> None: + """Lit_commit_ish_can be accessed, but warns and is an invalid type annotation.""" # It would be fine to test attribute access rather than a "from" import. But a # "from" import is more likely to appear in actual usage, so it is used here. with pytest.deprecated_call() as ctx: From 4857ff08fc9ae0183b65a4b94bd0813bd08a74b4 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 23:26:43 -0400 Subject: [PATCH 55/89] Clarify test_compat.test_dir --- test/deprecation/test_compat.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/test/deprecation/test_compat.py b/test/deprecation/test_compat.py index 5007fa1cc..6699a24d5 100644 --- a/test/deprecation/test_compat.py +++ b/test/deprecation/test_compat.py @@ -63,15 +63,19 @@ def test_is_platform() -> None: def test_dir() -> None: - """dir() on git.compat lists attributes meant to be public, even if deprecated.""" + """dir() on git.compat includes all public attributes, even if deprecated. + + As dir() usually does, it also has nonpublic attributes, which should also not be + removed by a custom __dir__ function, but those are less important to test. + """ expected_subset = { + "is_win", + "is_posix", + "is_darwin", "defenc", "safe_decode", "safe_encode", "win_encode", - "is_darwin", - "is_win", - "is_posix", } actual = set(dir(git.compat)) assert expected_subset <= actual From 488cc13a5b125be886bb361342b8e4709c7944ba Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 23 Mar 2024 23:32:11 -0400 Subject: [PATCH 56/89] Add test of dir() on git.types --- test/deprecation/test_types.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/deprecation/test_types.py b/test/deprecation/test_types.py index 419435964..cd53fa210 100644 --- a/test/deprecation/test_types.py +++ b/test/deprecation/test_types.py @@ -37,3 +37,30 @@ def test_can_access_lit_commit_ish_but_it_is_not_usable() -> None: # It should be as documented (even though deliberately unusable in static checks). assert Lit_commit_ish == Literal["commit", "tag"] + + +def test_dir() -> None: + """dir() on git.types includes public names, even ``Lit_commit_ish``. + + It also contains private names that we don't test. See test_compat.test_dir. + """ + expected_subset = { + "PathLike", + "TBD", + "AnyGitObject", + "Tree_ish", + "Commit_ish", + "GitObjectTypeString", + "Lit_commit_ish", + "Lit_config_levels", + "ConfigLevels_Tup", + "CallableProgress", + "assert_never", + "Files_TD", + "Total_TD", + "HSH_TD", + "Has_Repo", + "Has_id_attribute", + } + actual = set(dir(git.types)) + assert expected_subset <= actual From 19b3c0820b607558b3bc52d3d9f3841295f04ac5 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 24 Mar 2024 01:31:33 -0400 Subject: [PATCH 57/89] Clarify comment about is_ value assertions --- test/deprecation/test_compat.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/deprecation/test_compat.py b/test/deprecation/test_compat.py index 6699a24d5..22f733083 100644 --- a/test/deprecation/test_compat.py +++ b/test/deprecation/test_compat.py @@ -53,10 +53,10 @@ def test_is_platform() -> None: for fullname, message in zip(fully_qualified_names, messages): assert message.startswith(_MESSAGE_LEADER.format(fullname)) - # These exactly reproduce the expressions in the code under test, so they are not - # good for testing that the values are correct. Instead, the purpose of this test is - # to ensure that any dynamic machinery put in place in git.compat to cause warnings - # to be issued does not get in the way of the intended values being accessed. + # These assertions exactly reproduce the expressions in the code under test, so they + # are not good for testing that the values are correct. Instead, their purpose is to + # ensure that any dynamic machinery put in place in git.compat to cause warnings to + # be issued does not get in the way of the intended values being accessed. assert is_win == (os.name == "nt") assert is_posix == (os.name == "posix") assert is_darwin == (sys.platform == "darwin") From 28bd4a3f6e562eadc1018ee7c4d561a3c4352fb5 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 20 Mar 2024 14:31:58 -0400 Subject: [PATCH 58/89] Issue warnings for some deprecated attributes of modules --- git/__init__.py | 97 ++++++++++++++++++++++++++++++++++++------------- git/compat.py | 40 ++++++++++++++++++-- git/types.py | 39 +++++++++++++++----- 3 files changed, 138 insertions(+), 38 deletions(-) diff --git a/git/__init__.py b/git/__init__.py index a13030456..aa838d4f5 100644 --- a/git/__init__.py +++ b/git/__init__.py @@ -88,7 +88,10 @@ __version__ = "git" -from typing import List, Optional, Sequence, TYPE_CHECKING, Tuple, Union +from typing import Any, List, Optional, Sequence, TYPE_CHECKING, Tuple, Union + +if TYPE_CHECKING: + from types import ModuleType from gitdb.util import to_hex_sha @@ -144,11 +147,6 @@ SymbolicReference, Tag, TagReference, - head, # noqa: F401 # Nonpublic. May disappear! Use git.refs.head. - log, # noqa: F401 # Nonpublic. May disappear! Use git.refs.log. - reference, # noqa: F401 # Nonpublic. May disappear! Use git.refs.reference. - symbolic, # noqa: F401 # Nonpublic. May disappear! Use git.refs.symbolic. - tag, # noqa: F401 # Nonpublic. May disappear! Use git.refs.tag. ) from git.diff import ( # @NoMove INDEX, @@ -169,21 +167,6 @@ IndexEntry, IndexFile, StageType, - base, # noqa: F401 # Nonpublic. May disappear! Use git.index.base. - fun, # noqa: F401 # Nonpublic. May disappear! Use git.index.fun. - typ, # noqa: F401 # Nonpublic. May disappear! Use git.index.typ. - # - # NOTE: The expression `git.util` evaluates to git.index.util, and the import - # `from git import util` imports git.index.util, NOT git.util. It may not be - # feasible to change this until the next major version, to avoid breaking code - # inadvertently relying on it. If git.index.util really is what you want, use or - # import from that name, to avoid confusion. To use the "real" git.util module, - # write `from git.util import ...`, or access it as `sys.modules["git.util"]`. - # (This differs from other historical indirect-submodule imports that are - # unambiguously nonpublic and are subject to immediate removal. Here, the public - # git.util module, even though different, makes it less discoverable that the - # expression `git.util` refers to a non-public attribute of the git module.) - util, # noqa: F401 ) from git.util import ( # @NoMove Actor, @@ -196,7 +179,72 @@ except GitError as _exc: raise ImportError("%s: %s" % (_exc.__class__.__name__, _exc)) from _exc + +# NOTE: The expression `git.util` evaluates to git.index.util and `from git import util` +# imports git.index.util, NOT git.util. It may not be feasible to change this until the +# next major version, to avoid breaking code inadvertently relying on it. +# +# - If git.index.util *is* what you want, use or import from that, to avoid confusion. +# +# - To use the "real" git.util module, write `from git.util import ...`, or if necessary +# access it as `sys.modules["git.util"]`. +# +# (This differs from other indirect-submodule imports that are unambiguously non-public +# and subject to immediate removal. Here, the public git.util module, though different, +# makes less discoverable that the expression `git.util` refers to a non-public +# attribute of the git module.) +# +# This had come about by a wildcard import. Now that all intended imports are explicit, +# the intuitive but potentially incompatible binding occurs due to the usual rules for +# Python submodule bindings. So for now we delete that and let __getattr__ handle it. +# +del util # type: ignore[name-defined] # noqa: F821 + + +def _warned_import(message: str, fullname: str) -> "ModuleType": + import importlib + import warnings + + warnings.warn(message, DeprecationWarning, stacklevel=3) + return importlib.import_module(fullname) + + +def _getattr(name: str) -> Any: + # TODO: If __version__ is made dynamic and lazily fetched, put that case right here. + + if name == "util": + return _warned_import( + "The expression `git.util` and the import `from git import util` actually " + "reference git.index.util, and not the git.util module accessed in " + '`from git.util import XYZ` or `sys.modules["git.util"]`. This potentially ' + "confusing behavior is currently preserved for compatibility, but may be " + "changed in the future and should not be relied on.", + fullname="git.index.util", + ) + + for names, prefix in ( + ({"head", "log", "reference", "symbolic", "tag"}, "git.refs"), + ({"base", "fun", "typ"}, "git.index"), + ): + if name not in names: + continue + + fullname = f"{prefix}.{name}" + + return _warned_import( + f"{__name__}.{name} is a private alias of {fullname} and subject to " + f"immediate removal. Use {fullname} instead.", + fullname=fullname, + ) + + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") + + +if not TYPE_CHECKING: # Preserve static checking for undefined/misspelled attributes. + __getattr__ = _getattr + # { Initialize git executable path + GIT_OK = None @@ -232,12 +280,9 @@ def refresh(path: Optional[PathLike] = None) -> None: GIT_OK = True -# } END initialize git executable path - - -################# try: refresh() except Exception as _exc: raise ImportError("Failed to initialize: {0}".format(_exc)) from _exc -################# + +# } END initialize git executable path diff --git a/git/compat.py b/git/compat.py index 4ede8c985..f1b95a80e 100644 --- a/git/compat.py +++ b/git/compat.py @@ -23,7 +23,9 @@ AnyStr, Dict, # noqa: F401 IO, # noqa: F401 + List, Optional, + TYPE_CHECKING, Tuple, # noqa: F401 Type, # noqa: F401 Union, @@ -33,7 +35,39 @@ # --------------------------------------------------------------------------- -is_win = os.name == "nt" +_deprecated_platform_aliases = { + "is_win": os.name == "nt", + "is_posix": os.name == "posix", + "is_darwin": sys.platform == "darwin", +} + + +def _getattr(name: str) -> Any: + try: + value = _deprecated_platform_aliases[name] + except KeyError: + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") from None + + import warnings + + warnings.warn( + f"{__name__}.{name} and other is_ aliases are deprecated. " + "Write the desired os.name or sys.platform check explicitly instead.", + DeprecationWarning, + stacklevel=2, + ) + return value + + +if not TYPE_CHECKING: # Preserve static checking for undefined/misspelled attributes. + __getattr__ = _getattr + + +def __dir__() -> List[str]: + return [*globals(), *_deprecated_platform_aliases] + + +is_win: bool """Deprecated alias for ``os.name == "nt"`` to check for native Windows. This is deprecated because it is clearer to write out :attr:`os.name` or @@ -45,7 +79,7 @@ Cygwin, use ``sys.platform == "cygwin"``. """ -is_posix = os.name == "posix" +is_posix: bool """Deprecated alias for ``os.name == "posix"`` to check for Unix-like ("POSIX") systems. This is deprecated because it clearer to write out :attr:`os.name` or @@ -58,7 +92,7 @@ (Darwin). """ -is_darwin = sys.platform == "darwin" +is_darwin: bool """Deprecated alias for ``sys.platform == "darwin"`` to check for macOS (Darwin). This is deprecated because it clearer to write out :attr:`os.name` or diff --git a/git/types.py b/git/types.py index a93ebdb4f..1be32de1d 100644 --- a/git/types.py +++ b/git/types.py @@ -7,11 +7,13 @@ Any, Callable, Dict, + List, NoReturn, Optional, Sequence as Sequence, Tuple, TYPE_CHECKING, + Type, TypeVar, Union, ) @@ -127,21 +129,40 @@ https://git-scm.com/docs/gitglossary#def_object_type """ -Lit_commit_ish = Literal["commit", "tag"] -"""Deprecated. Type of literal strings identifying sometimes-commitish git object types. +Lit_commit_ish: Type[Literal["commit", "tag"]] +"""Deprecated. Type of literal strings identifying typically-commitish git object types. Prior to a bugfix, this type had been defined more broadly. Any usage is in practice -ambiguous and likely to be incorrect. Instead of this type: +ambiguous and likely to be incorrect. This type has therefore been made a static type +error to appear in annotations. It is preserved, with a deprecated status, to avoid +introducing runtime errors in code that refers to it, but it should not be used. + +Instead of this type: * For the type of the string literals associated with :class:`Commit_ish`, use ``Literal["commit", "tag"]`` or create a new type alias for it. That is equivalent to - this type as currently defined. + this type as currently defined (but usable in statically checked type annotations). * For the type of all four string literals associated with :class:`AnyGitObject`, use :class:`GitObjectTypeString`. That is equivalent to the old definition of this type - prior to the bugfix. + prior to the bugfix (and is also usable in statically checked type annotations). """ + +def _getattr(name: str) -> Any: + if name == "Lit_commit_ish": + return Literal["commit", "tag"] + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") + + +if not TYPE_CHECKING: # Preserve static checking for undefined/misspelled attributes. + __getattr__ = _getattr + + +def __dir__() -> List[str]: + return [*globals(), "Lit_commit_ish"] + + # Config_levels --------------------------------------------------------- Lit_config_levels = Literal["system", "global", "user", "repository"] @@ -188,12 +209,12 @@ def assert_never(inp: NoReturn, raise_error: bool = True, exc: Union[Exception, :param inp: If all members are handled, the argument for `inp` will have the - :class:`~typing.Never`/:class:`~typing.NoReturn` type. Otherwise, the type will - mismatch and cause a mypy error. + :class:`~typing.Never`/:class:`~typing.NoReturn` type. + Otherwise, the type will mismatch and cause a mypy error. :param raise_error: - If ``True``, will also raise :exc:`ValueError` with a general "unhandled - literal" message, or the exception object passed as `exc`. + If ``True``, will also raise :exc:`ValueError` with a general + "unhandled literal" message, or the exception object passed as `exc`. :param exc: It not ``None``, this should be an already-constructed exception object, to be From dffa930a426747d9b30496fceb6efd621ab8795b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 24 Mar 2024 01:38:26 -0400 Subject: [PATCH 59/89] Refine deprecated module attributes and their warnings - In the top-level git module, import util from git.index so there is no ambiguity for tools in detecting which module the util attribute of the top-level git module (i.e., git.util in an *expression*) is, even though it's subsequently deleted (and then dynamically supplied when requested, in a way that is opaque to static type checkers due to being only when `not TYPE_CHECKING`). This seems to be necessary for some tools. Curiously, guarding the del statement under `not TYPE_CHECKING` seems *not* to be needed by any tools of any kind. It should still possibly be done, but that's not included in these changes. - Add the missing deprecation warning for git.types.Lit_commit_ish. - When importing the warnings module, do so with a top-level import as in the other GitPython modules that have long (and reasonably) done so. In git/__init__.py, there already had to be an import of importlib, which seemed like it should be done locally in case of delays. Neither the importlib module nor any of its submodules were already imported anywhere in GitPython, and the code that uses it will most often not be exercised. So there is a potential benefit to avoiding loading it when not needed. When writing a local import for that, I had included the warnings module as a local import as well. But this obscures the potential benefit of locally importing importlib, and could lead to ill-advised changes in the future based on the idea that the degree of justification to be local imports was the same for them both. --- git/__init__.py | 6 ++++-- git/compat.py | 3 +-- git/types.py | 17 ++++++++++++++--- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/git/__init__.py b/git/__init__.py index aa838d4f5..fd1b87707 100644 --- a/git/__init__.py +++ b/git/__init__.py @@ -93,6 +93,8 @@ if TYPE_CHECKING: from types import ModuleType +import warnings + from gitdb.util import to_hex_sha from git.exc import ( @@ -167,6 +169,7 @@ IndexEntry, IndexFile, StageType, + util, ) from git.util import ( # @NoMove Actor, @@ -198,12 +201,11 @@ # the intuitive but potentially incompatible binding occurs due to the usual rules for # Python submodule bindings. So for now we delete that and let __getattr__ handle it. # -del util # type: ignore[name-defined] # noqa: F821 +del util def _warned_import(message: str, fullname: str) -> "ModuleType": import importlib - import warnings warnings.warn(message, DeprecationWarning, stacklevel=3) return importlib.import_module(fullname) diff --git a/git/compat.py b/git/compat.py index f1b95a80e..d7d9a55a9 100644 --- a/git/compat.py +++ b/git/compat.py @@ -13,6 +13,7 @@ import locale import os import sys +import warnings from gitdb.utils.encoding import force_bytes, force_text # noqa: F401 @@ -48,8 +49,6 @@ def _getattr(name: str) -> Any: except KeyError: raise AttributeError(f"module {__name__!r} has no attribute {name!r}") from None - import warnings - warnings.warn( f"{__name__}.{name} and other is_ aliases are deprecated. " "Write the desired os.name or sys.platform check explicitly instead.", diff --git a/git/types.py b/git/types.py index 1be32de1d..584450146 100644 --- a/git/types.py +++ b/git/types.py @@ -17,6 +17,7 @@ TypeVar, Union, ) +import warnings if sys.version_info >= (3, 8): from typing import ( @@ -150,9 +151,19 @@ def _getattr(name: str) -> Any: - if name == "Lit_commit_ish": - return Literal["commit", "tag"] - raise AttributeError(f"module {__name__!r} has no attribute {name!r}") + if name != "Lit_commit_ish": + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") + + warnings.warn( + "Lit_commit_ish is deprecated. It is currently defined as " + '`Literal["commit", "tag"]`, which should be used in its place if desired. It ' + 'had previously been defined as `Literal["commit", "tag", "blob", "tree"]`, ' + "covering all four git object type strings including those that are never " + "commit-ish. For that, use the GitObjectTypeString type instead.", + DeprecationWarning, + stacklevel=2, + ) + return Literal["commit", "tag"] if not TYPE_CHECKING: # Preserve static checking for undefined/misspelled attributes. From 7ab27c5bb1891af9eec7a99e33ea35e234e322d5 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 24 Mar 2024 19:31:00 -0400 Subject: [PATCH 60/89] Start on test module about Git.USE_SHELL and Git attributes --- test/deprecation/test_cmd_git.py | 168 +++++++++++++++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 test/deprecation/test_cmd_git.py diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py new file mode 100644 index 000000000..fc8bab6ee --- /dev/null +++ b/test/deprecation/test_cmd_git.py @@ -0,0 +1,168 @@ +"""Tests for static and dynamic characteristics of Git class and instance attributes. + +Currently this all relates to the deprecated :class:`Git.USE_SHELL` class attribute, +which can also be accessed through instances. Some tests directly verify its behavior, +including deprecation warnings, while others verify that other aspects of attribute +access are not inadvertently broken by mechanisms introduced to issue the warnings. +""" + +import contextlib +import sys +from typing import Generator + +if sys.version_info >= (3, 11): + from typing import assert_type +else: + from typing_extensions import assert_type + +import pytest + +from git.cmd import Git + +_USE_SHELL_DEPRECATED_FRAGMENT = "Git.USE_SHELL is deprecated" +"""Text contained in all USE_SHELL deprecation warnings, and starting most of them.""" + +_USE_SHELL_DANGEROUS_FRAGMENT = "Setting Git.USE_SHELL to True is unsafe and insecure" +"""Beginning text of USE_SHELL deprecation warnings when USE_SHELL is set True.""" + + +@pytest.fixture +def reset_backing_attribute() -> Generator[None, None, None]: + """Fixture to reset the private ``_USE_SHELL`` attribute. + + This is used to decrease the likelihood of state changes leaking out and affecting + other tests. But the goal is not to assert that ``_USE_SHELL`` is used, nor anything + about how or when it is used, which is an implementation detail subject to change. + + This is possible but inelegant to do with pytest's monkeypatch fixture, which only + restores attributes that it has previously been used to change, create, or remove. + """ + no_value = object() + try: + old_value = Git._USE_SHELL + except AttributeError: + old_value = no_value + + yield + + if old_value is no_value: + with contextlib.suppress(AttributeError): + del Git._USE_SHELL + else: + Git._USE_SHELL = old_value + + +def test_cannot_access_undefined_on_git_class() -> None: + """Accessing a bogus attribute on the Git class remains a dynamic and static error. + + This differs from Git instances, where most attribute names will dynamically + synthesize a "bound method" that runs a git subcommand when called. + """ + with pytest.raises(AttributeError): + Git.foo # type: ignore[attr-defined] + + +def test_get_use_shell_on_class_default() -> None: + """USE_SHELL can be read as a class attribute, defaulting to False and warning.""" + with pytest.deprecated_call() as ctx: + use_shell = Git.USE_SHELL + + (message,) = [str(entry.message) for entry in ctx] # Exactly one warning. + assert message.startswith(_USE_SHELL_DEPRECATED_FRAGMENT) + + assert_type(use_shell, bool) + + # This comes after the static assertion, just in case it would affect the inference. + assert not use_shell + + +# FIXME: More robustly check that each operation really issues exactly one deprecation +# warning, even if this requires relying more on reset_backing_attribute doing its job. +def test_use_shell_on_class(reset_backing_attribute) -> None: + """USE_SHELL can be written and re-read as a class attribute, always warning.""" + # We assert in a "safe" order, using reset_backing_attribute only as a backstop. + with pytest.deprecated_call() as ctx: + Git.USE_SHELL = True + set_value = Git.USE_SHELL + Git.USE_SHELL = False + reset_value = Git.USE_SHELL + + # The attribute should take on the values set to it. + assert set_value is True + assert reset_value is False + + messages = [str(entry.message) for entry in ctx] + set_message, check_message, reset_message, recheck_message = messages + + # Setting it to True should produce the special warning for that. + assert _USE_SHELL_DEPRECATED_FRAGMENT in set_message + assert set_message.startswith(_USE_SHELL_DANGEROUS_FRAGMENT) + + # All other operations should produce a usual warning. + assert check_message.startswith(_USE_SHELL_DEPRECATED_FRAGMENT) + assert reset_message.startswith(_USE_SHELL_DEPRECATED_FRAGMENT) + assert recheck_message.startswith(_USE_SHELL_DEPRECATED_FRAGMENT) + + +# FIXME: Test behavior on instances (where we can get but not set). + +# FIXME: Test behavior with multiprocessing (the attribute needs to pickle properly). + + +_EXPECTED_DIR_SUBSET = { + "cat_file_all", + "cat_file_header", + "GIT_PYTHON_TRACE", + "USE_SHELL", # The attribute we get deprecation warnings for. + "GIT_PYTHON_GIT_EXECUTABLE", + "refresh", + "is_cygwin", + "polish_url", + "check_unsafe_protocols", + "check_unsafe_options", + "AutoInterrupt", + "CatFileContentStream", + "__init__", + "__getattr__", + "set_persistent_git_options", + "working_dir", + "version_info", + "execute", + "environment", + "update_environment", + "custom_environment", + "transform_kwarg", + "transform_kwargs", + "__call__", + "_call_process", # Not currently considered public, but unlikely to change. + "get_object_header", + "get_object_data", + "stream_object_data", + "clear_cache", +} +"""Some stable attributes dir() should include on the Git class and its instances. + +This is intentionally incomplete, but includes substantial variety. Most importantly, it +includes both ``USE_SHELL`` and a wide sampling of other attributes. +""" + + +def test_class_dir() -> None: + """dir() on the Git class includes its statically known attributes. + + This tests that the mechanism that adds dynamic behavior to USE_SHELL accesses so + that all accesses issue warnings does not break dir() for the class, neither for + USE_SHELL nor for ordinary (non-deprecated) attributes. + """ + actual = set(dir(Git)) + assert _EXPECTED_DIR_SUBSET <= actual + + +def test_instance_dir() -> None: + """dir() on Git objects includes its statically known attributes. + + This is like test_class_dir, but for Git instance rather than the class itself. + """ + instance = Git() + actual = set(dir(instance)) + assert _EXPECTED_DIR_SUBSET <= actual From af723d5eb04dc919f76369c0b44a087638217352 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 24 Mar 2024 21:05:57 -0400 Subject: [PATCH 61/89] Make test_use_shell_on_class more robust --- test/deprecation/test_cmd_git.py | 54 ++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index fc8bab6ee..319bf7865 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -9,6 +9,7 @@ import contextlib import sys from typing import Generator +import warnings if sys.version_info >= (3, 11): from typing import assert_type @@ -26,9 +27,16 @@ """Beginning text of USE_SHELL deprecation warnings when USE_SHELL is set True.""" +@contextlib.contextmanager +def _suppress_deprecation_warning() -> Generator[None, None, None]: + with warnings.catch_warnings(): + warnings.filterwarnings("ignore", category=DeprecationWarning) + yield + + @pytest.fixture -def reset_backing_attribute() -> Generator[None, None, None]: - """Fixture to reset the private ``_USE_SHELL`` attribute. +def try_restore_use_shell_state() -> Generator[None, None, None]: + """Fixture to attempt to restore state associated with the ``USE_SHELL`` attribute. This is used to decrease the likelihood of state changes leaking out and affecting other tests. But the goal is not to assert that ``_USE_SHELL`` is used, nor anything @@ -38,18 +46,27 @@ def reset_backing_attribute() -> Generator[None, None, None]: restores attributes that it has previously been used to change, create, or remove. """ no_value = object() + try: - old_value = Git._USE_SHELL + old_backing_value = Git._USE_SHELL except AttributeError: - old_value = no_value + old_backing_value = no_value + try: + with _suppress_deprecation_warning(): + old_public_value = Git.USE_SHELL - yield + # This doesn't have its own try-finally because pytest catches exceptions raised + # during the yield. (The outer try-finally catches exceptions in this fixture.) + yield - if old_value is no_value: - with contextlib.suppress(AttributeError): - del Git._USE_SHELL - else: - Git._USE_SHELL = old_value + with _suppress_deprecation_warning(): + Git.USE_SHELL = old_public_value + finally: + if old_backing_value is no_value: + with contextlib.suppress(AttributeError): + del Git._USE_SHELL + else: + Git._USE_SHELL = old_backing_value def test_cannot_access_undefined_on_git_class() -> None: @@ -76,23 +93,26 @@ def test_get_use_shell_on_class_default() -> None: assert not use_shell -# FIXME: More robustly check that each operation really issues exactly one deprecation -# warning, even if this requires relying more on reset_backing_attribute doing its job. -def test_use_shell_on_class(reset_backing_attribute) -> None: +def test_use_shell_on_class(try_restore_use_shell_state) -> None: """USE_SHELL can be written and re-read as a class attribute, always warning.""" - # We assert in a "safe" order, using reset_backing_attribute only as a backstop. - with pytest.deprecated_call() as ctx: + with pytest.deprecated_call() as setting: Git.USE_SHELL = True + with pytest.deprecated_call() as checking: set_value = Git.USE_SHELL + with pytest.deprecated_call() as resetting: Git.USE_SHELL = False + with pytest.deprecated_call() as rechecking: reset_value = Git.USE_SHELL # The attribute should take on the values set to it. assert set_value is True assert reset_value is False - messages = [str(entry.message) for entry in ctx] - set_message, check_message, reset_message, recheck_message = messages + # Each access should warn exactly once. + (set_message,) = [str(entry.message) for entry in setting] + (check_message,) = [str(entry.message) for entry in checking] + (reset_message,) = [str(entry.message) for entry in resetting] + (recheck_message,) = [str(entry.message) for entry in rechecking] # Setting it to True should produce the special warning for that. assert _USE_SHELL_DEPRECATED_FRAGMENT in set_message From bf1388896ac2d052cf956123513f0c8b33f34dd6 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Mar 2024 00:06:40 -0400 Subject: [PATCH 62/89] Write most remaining Git attribute/deprecation tests --- test/deprecation/test_cmd_git.py | 99 ++++++++++++++++++++++++++++---- 1 file changed, 88 insertions(+), 11 deletions(-) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index 319bf7865..ef6f5b5a6 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -17,6 +17,7 @@ from typing_extensions import assert_type import pytest +from pytest import WarningsRecorder from git.cmd import Git @@ -93,17 +94,34 @@ def test_get_use_shell_on_class_default() -> None: assert not use_shell -def test_use_shell_on_class(try_restore_use_shell_state) -> None: - """USE_SHELL can be written and re-read as a class attribute, always warning.""" - with pytest.deprecated_call() as setting: - Git.USE_SHELL = True - with pytest.deprecated_call() as checking: - set_value = Git.USE_SHELL - with pytest.deprecated_call() as resetting: - Git.USE_SHELL = False - with pytest.deprecated_call() as rechecking: - reset_value = Git.USE_SHELL +def test_get_use_shell_on_instance_default() -> None: + """USE_SHELL can be read as an instance attribute, defaulting to False and warning. + + This is the same as test_get_use_shell_on_class_default above, but for instances. + The test is repeated, instead of using parametrization, for clearer static analysis. + """ + instance = Git() + with pytest.deprecated_call() as ctx: + use_shell = instance.USE_SHELL + + (message,) = [str(entry.message) for entry in ctx] # Exactly one warning. + assert message.startswith(_USE_SHELL_DEPRECATED_FRAGMENT) + + assert_type(use_shell, bool) + + # This comes after the static assertion, just in case it would affect the inference. + assert not use_shell + + +def _assert_use_shell_full_results( + set_value: bool, + reset_value: bool, + setting: WarningsRecorder, + checking: WarningsRecorder, + resetting: WarningsRecorder, + rechecking: WarningsRecorder, +) -> None: # The attribute should take on the values set to it. assert set_value is True assert reset_value is False @@ -124,7 +142,66 @@ def test_use_shell_on_class(try_restore_use_shell_state) -> None: assert recheck_message.startswith(_USE_SHELL_DEPRECATED_FRAGMENT) -# FIXME: Test behavior on instances (where we can get but not set). +def test_use_shell_set_and_get_on_class(try_restore_use_shell_state: None) -> None: + """USE_SHELL can be set and re-read as a class attribute, always warning.""" + with pytest.deprecated_call() as setting: + Git.USE_SHELL = True + with pytest.deprecated_call() as checking: + set_value = Git.USE_SHELL + with pytest.deprecated_call() as resetting: + Git.USE_SHELL = False + with pytest.deprecated_call() as rechecking: + reset_value = Git.USE_SHELL + + _assert_use_shell_full_results( + set_value, + reset_value, + setting, + checking, + resetting, + rechecking, + ) + + +def test_use_shell_set_on_class_get_on_instance(try_restore_use_shell_state: None) -> None: + """USE_SHELL can be set on the class and read on an instance, always warning. + + This is like test_use_shell_set_and_get_on_class but it performs reads on an + instance. There is some redundancy here in assertions about warnings when the + attribute is set, but it is a separate test so that any bugs where a read on the + class (or an instance) is needed first before a read on an instance (or the class) + are detected. + """ + instance = Git() + + with pytest.deprecated_call() as setting: + Git.USE_SHELL = True + with pytest.deprecated_call() as checking: + set_value = instance.USE_SHELL + with pytest.deprecated_call() as resetting: + Git.USE_SHELL = False + with pytest.deprecated_call() as rechecking: + reset_value = instance.USE_SHELL + + _assert_use_shell_full_results( + set_value, + reset_value, + setting, + checking, + resetting, + rechecking, + ) + + +@pytest.mark.parametrize("value", [False, True]) +def test_use_shell_cannot_set_on_instance( + value: bool, + try_restore_use_shell_state: None, # In case of a bug where it does set USE_SHELL. +) -> None: + instance = Git() + with pytest.raises(AttributeError): + instance.USE_SHELL = value + # FIXME: Test behavior with multiprocessing (the attribute needs to pickle properly). From 602de0c93572972d29d33c4ecacfd9c6e192484a Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Mar 2024 14:59:13 -0400 Subject: [PATCH 63/89] Begin multiprocessing misadventure There is no per-instance state involved in USE_SHELL, so pickling is far less directly relevant than usual to multiprocessing: the spawn and forkserver methods will not preserve a subsequently changed attribute value unless side effects of loading a module (or other unpickling of a function or its arguments that are submitted to run on a worker subprocess) causes it to run again; the fork method will. This will be (automatically) the same with any combination of metaclasses, properties, and custom descriptors as in the more straightforward case of a simple class attribute. Subtleties arise in the code that uses GitPython and multiprocessing, but should not arise unintentionally from the change in implementation of USE_SHELL done to add deprecation warnings, except possibly with respect to whether warnings will be repeated in worker processes, which is less important than whether the actual state is preserved. --- test/deprecation/test_cmd_git.py | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index ef6f5b5a6..72c1f7cd8 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -6,6 +6,7 @@ access are not inadvertently broken by mechanisms introduced to issue the warnings. """ +from concurrent.futures import ProcessPoolExecutor import contextlib import sys from typing import Generator @@ -36,7 +37,7 @@ def _suppress_deprecation_warning() -> Generator[None, None, None]: @pytest.fixture -def try_restore_use_shell_state() -> Generator[None, None, None]: +def restore_use_shell_state() -> Generator[None, None, None]: """Fixture to attempt to restore state associated with the ``USE_SHELL`` attribute. This is used to decrease the likelihood of state changes leaking out and affecting @@ -142,7 +143,7 @@ def _assert_use_shell_full_results( assert recheck_message.startswith(_USE_SHELL_DEPRECATED_FRAGMENT) -def test_use_shell_set_and_get_on_class(try_restore_use_shell_state: None) -> None: +def test_use_shell_set_and_get_on_class(restore_use_shell_state: None) -> None: """USE_SHELL can be set and re-read as a class attribute, always warning.""" with pytest.deprecated_call() as setting: Git.USE_SHELL = True @@ -163,7 +164,7 @@ def test_use_shell_set_and_get_on_class(try_restore_use_shell_state: None) -> No ) -def test_use_shell_set_on_class_get_on_instance(try_restore_use_shell_state: None) -> None: +def test_use_shell_set_on_class_get_on_instance(restore_use_shell_state: None) -> None: """USE_SHELL can be set on the class and read on an instance, always warning. This is like test_use_shell_set_and_get_on_class but it performs reads on an @@ -196,14 +197,31 @@ class (or an instance) is needed first before a read on an instance (or the clas @pytest.mark.parametrize("value", [False, True]) def test_use_shell_cannot_set_on_instance( value: bool, - try_restore_use_shell_state: None, # In case of a bug where it does set USE_SHELL. + restore_use_shell_state: None, # In case of a bug where it does set USE_SHELL. ) -> None: instance = Git() with pytest.raises(AttributeError): instance.USE_SHELL = value -# FIXME: Test behavior with multiprocessing (the attribute needs to pickle properly). +def _check_use_shell_in_worker(value: bool) -> None: + # USE_SHELL should have the value set in the parent before starting the worker. + assert Git.USE_SHELL is value + + # FIXME: Check that mutation still works and raises the warning. + + +@pytest.mark.filterwarnings("ignore::DeprecationWarning") +@pytest.mark.parametrize("value", [False, True]) +def test_use_shell_preserved_in_multiprocessing( + value: bool, + restore_use_shell_state: None, +) -> None: + """The USE_SHELL class attribute pickles accurately for multiprocessing.""" + Git.USE_SHELL = value + with ProcessPoolExecutor(max_workers=1) as executor: + # Calling result() marshals any exception back to this process and raises it. + executor.submit(_check_use_shell_in_worker, value).result() _EXPECTED_DIR_SUBSET = { From d4b50c94ff3694e3285a1ea8ed9b42c685726baf Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 25 Mar 2024 15:01:23 -0400 Subject: [PATCH 64/89] Somewhat clarify multiprocessing misadventure --- test/deprecation/test_cmd_git.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index 72c1f7cd8..04fb0747c 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -204,11 +204,8 @@ def test_use_shell_cannot_set_on_instance( instance.USE_SHELL = value -def _check_use_shell_in_worker(value: bool) -> None: - # USE_SHELL should have the value set in the parent before starting the worker. - assert Git.USE_SHELL is value - - # FIXME: Check that mutation still works and raises the warning. +def _get_value_in_current_process() -> bool: + return Git.USE_SHELL @pytest.mark.filterwarnings("ignore::DeprecationWarning") @@ -220,8 +217,8 @@ def test_use_shell_preserved_in_multiprocessing( """The USE_SHELL class attribute pickles accurately for multiprocessing.""" Git.USE_SHELL = value with ProcessPoolExecutor(max_workers=1) as executor: - # Calling result() marshals any exception back to this process and raises it. - executor.submit(_check_use_shell_in_worker, value).result() + marshaled_value = executor.submit(_get_value_in_current_process).result() + assert marshaled_value is value _EXPECTED_DIR_SUBSET = { From 02c2f0008082e710ddb73f1c4ff784b74eed4f8f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 26 Mar 2024 13:09:08 -0400 Subject: [PATCH 65/89] Discuss multiprocessing in test module docstring; remove bad test --- test/deprecation/test_cmd_git.py | 44 ++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index 04fb0747c..9dfe37220 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -1,12 +1,35 @@ """Tests for static and dynamic characteristics of Git class and instance attributes. -Currently this all relates to the deprecated :class:`Git.USE_SHELL` class attribute, +Currently this all relates to the deprecated :attr:`Git.USE_SHELL` class attribute, which can also be accessed through instances. Some tests directly verify its behavior, including deprecation warnings, while others verify that other aspects of attribute access are not inadvertently broken by mechanisms introduced to issue the warnings. + +A note on multiprocessing: Because USE_SHELL has no instance state, this module does not +include tests of pickling and multiprocessing. + +- Just as with a simple class attribute, when a class attribute with custom logic is + later set to a new value, it may have either its initial value or the new value when + accessed from a worker process, depending on the process start method. With "fork", + changes are preserved. With "spawn" or "forkserver", re-importing the modules causes + initial values to be set. Then the value in the parent at the time it dispatches the + task is only set in the children if the parent has the task set it, or if it is set as + a side effect of importing needed modules, or of unpickling objects passed to the + child (for example, if it is set in a top-level statement of the module that defines + the function submitted for the child worker process to call). + +- When an attribute gains new logic provided by a property or custom descriptor, and the + attribute involves instance-level state, incomplete or corrupted pickling can break + multiprocessing. (For example, if an instance attribute is reimplemented using a + descriptor that stores data in a global WeakKeyDictionary, pickled instances should be + tested to ensure they are still working correctly.) But nothing like that applies + here, because instance state is not involved. Although the situation is inherently + complex as described above, it is independent of the attribute implementation. + +- That USE_SHELL cannot be set on instances, and that when retrieved on instances it + always gives the same value as on the class, is covered in the tests here. """ -from concurrent.futures import ProcessPoolExecutor import contextlib import sys from typing import Generator @@ -204,23 +227,6 @@ def test_use_shell_cannot_set_on_instance( instance.USE_SHELL = value -def _get_value_in_current_process() -> bool: - return Git.USE_SHELL - - -@pytest.mark.filterwarnings("ignore::DeprecationWarning") -@pytest.mark.parametrize("value", [False, True]) -def test_use_shell_preserved_in_multiprocessing( - value: bool, - restore_use_shell_state: None, -) -> None: - """The USE_SHELL class attribute pickles accurately for multiprocessing.""" - Git.USE_SHELL = value - with ProcessPoolExecutor(max_workers=1) as executor: - marshaled_value = executor.submit(_get_value_in_current_process).result() - assert marshaled_value is value - - _EXPECTED_DIR_SUBSET = { "cat_file_all", "cat_file_header", From 46df79f75e6ee4db9852c398ec99a5788fd966b2 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 26 Mar 2024 16:03:58 -0400 Subject: [PATCH 66/89] Discuss metaclass conflicts in test module docstring --- test/deprecation/test_cmd_git.py | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index 9dfe37220..a5d241cc4 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -5,8 +5,11 @@ including deprecation warnings, while others verify that other aspects of attribute access are not inadvertently broken by mechanisms introduced to issue the warnings. -A note on multiprocessing: Because USE_SHELL has no instance state, this module does not -include tests of pickling and multiprocessing. +A note on multiprocessing +========================= + +Because USE_SHELL has no instance state, this module does not include tests of pickling +and multiprocessing: - Just as with a simple class attribute, when a class attribute with custom logic is later set to a new value, it may have either its initial value or the new value when @@ -28,6 +31,26 @@ - That USE_SHELL cannot be set on instances, and that when retrieved on instances it always gives the same value as on the class, is covered in the tests here. + +A note on metaclass conflicts +============================= + +The most important DeprecationWarning is for the code ``Git.USE_SHELL = True``, which is +a security risk. But this warning may not be possible to implement without a custom +metaclass. This is because a descriptor in a class can customize all forms of attribute +access on its instances, but can only customize getting an attribute on the class. +Retrieving a descriptor from a class calls its ``__get__`` method (if defined), but +replacing or deleting it does not call its ``__set__`` or ``__delete__`` methods. + +Adding a metaclass is a potentially breaking change. This is because derived classes +that use an unrelated metaclass, whether directly or by inheriting from a class such as +abc.ABC that uses one, will raise TypeError when defined. These would have to be +modified to use a newly introduced metaclass that is a lower bound of both. Subclasses +remain unbroken in the far more typical case that they use no custom metaclass. + +The tests in this module do not establish whether the danger of setting Git.USE_SHELL to +True is high enough, and applications of deriving from Git and using an unrelated custom +metaclass marginal enough, to justify introducing a metaclass to issue the warnings. """ import contextlib From 40ed842cf35b0c280dc408d77c764988a2d2746a Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 26 Mar 2024 18:30:34 -0400 Subject: [PATCH 67/89] Revise test module docstring for clarity --- test/deprecation/test_cmd_git.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index a5d241cc4..2ebcb3ad7 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -11,19 +11,19 @@ Because USE_SHELL has no instance state, this module does not include tests of pickling and multiprocessing: -- Just as with a simple class attribute, when a class attribute with custom logic is - later set to a new value, it may have either its initial value or the new value when - accessed from a worker process, depending on the process start method. With "fork", - changes are preserved. With "spawn" or "forkserver", re-importing the modules causes - initial values to be set. Then the value in the parent at the time it dispatches the - task is only set in the children if the parent has the task set it, or if it is set as - a side effect of importing needed modules, or of unpickling objects passed to the - child (for example, if it is set in a top-level statement of the module that defines - the function submitted for the child worker process to call). +- Just as with a simple class attribute, when a class attribute with custom logic is set + to another value, even before a worker process is created that uses the class, the + worker process may see either the initial or new value, depending on the process start + method. With "fork", changes are preserved. With "spawn" or "forkserver", re-importing + the modules causes initial values to be set. Then the value in the parent at the time + it dispatches the task is only set in the children if the parent has the task set it, + or if it is set as a side effect of importing needed modules, or of unpickling objects + passed to the child (for example, if it is set in a top-level statement of the module + that defines the function submitted for the child worker process to call). - When an attribute gains new logic provided by a property or custom descriptor, and the attribute involves instance-level state, incomplete or corrupted pickling can break - multiprocessing. (For example, if an instance attribute is reimplemented using a + multiprocessing. (For example, when an instance attribute is reimplemented using a descriptor that stores data in a global WeakKeyDictionary, pickled instances should be tested to ensure they are still working correctly.) But nothing like that applies here, because instance state is not involved. Although the situation is inherently @@ -35,8 +35,8 @@ A note on metaclass conflicts ============================= -The most important DeprecationWarning is for the code ``Git.USE_SHELL = True``, which is -a security risk. But this warning may not be possible to implement without a custom +The most important DeprecationWarning is for code like ``Git.USE_SHELL = True``, which +is a security risk. But this warning may not be possible to implement without a custom metaclass. This is because a descriptor in a class can customize all forms of attribute access on its instances, but can only customize getting an attribute on the class. Retrieving a descriptor from a class calls its ``__get__`` method (if defined), but From 6a35261ab6a58bfd8fbb2b008aacfb2decf27053 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 03:28:38 -0400 Subject: [PATCH 68/89] Test that USE_SHELL is unittest.mock.patch patchable --- test/deprecation/test_cmd_git.py | 36 ++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index 2ebcb3ad7..5d710b0d4 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -56,6 +56,7 @@ import contextlib import sys from typing import Generator +import unittest.mock import warnings if sys.version_info >= (3, 11): @@ -250,6 +251,41 @@ def test_use_shell_cannot_set_on_instance( instance.USE_SHELL = value +@pytest.mark.filterwarnings("ignore::DeprecationWarning") +@pytest.mark.parametrize("original_value", [False, True]) +def test_use_shell_is_mock_patchable_on_class_as_object_attribute( + original_value: bool, + restore_use_shell_state: None, +) -> None: + """Asymmetric patching looking up USE_SHELL in ``__dict__`` doesn't corrupt state. + + Code using GitPython may temporarily set Git.USE_SHELL to a different value. Ideally + it does not use unittest.mock.patch to do so, because that makes subtle assumptions + about the relationship between attributes and dictionaries. If the attribute can be + retrieved from the ``__dict__`` rather than directly, that value is assumed the + correct one to restore, even by a normal setattr. + + The effect is that some ways of simulating a class attribute with added behavior can + cause a descriptor, such as a property, to be set to its own backing attribute + during unpatching; then subsequent reads raise RecursionError. This happens if both + (a) setting it on the class is customized in a metaclass and (b) getting it on + instances is customized with a descriptor (such as a property) in the class itself. + + Although ideally code outside GitPython would not rely on being able to patch + Git.USE_SHELL with unittest.mock.patch, the technique is widespread. Thus, USE_SHELL + should be implemented in some way compatible with it. This test checks for that. + """ + Git.USE_SHELL = original_value + if Git.USE_SHELL is not original_value: + raise RuntimeError(f"Can't set up the test") + new_value = not original_value + + with unittest.mock.patch.object(Git, "USE_SHELL", new_value): + assert Git.USE_SHELL is new_value + + assert Git.USE_SHELL is original_value + + _EXPECTED_DIR_SUBSET = { "cat_file_all", "cat_file_header", From e725c8254cccd88d02b38b414bac875212a7074b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 11:45:25 -0400 Subject: [PATCH 69/89] Make the restore_use_shell_state fixture more robust --- test/deprecation/test_cmd_git.py | 52 ++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index 5d710b0d4..4183018e6 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -54,6 +54,7 @@ """ import contextlib +import logging import sys from typing import Generator import unittest.mock @@ -75,6 +76,8 @@ _USE_SHELL_DANGEROUS_FRAGMENT = "Setting Git.USE_SHELL to True is unsafe and insecure" """Beginning text of USE_SHELL deprecation warnings when USE_SHELL is set True.""" +_logger = logging.getLogger(__name__) + @contextlib.contextmanager def _suppress_deprecation_warning() -> Generator[None, None, None]: @@ -85,22 +88,44 @@ def _suppress_deprecation_warning() -> Generator[None, None, None]: @pytest.fixture def restore_use_shell_state() -> Generator[None, None, None]: - """Fixture to attempt to restore state associated with the ``USE_SHELL`` attribute. + """Fixture to attempt to restore state associated with the USE_SHELL attribute. This is used to decrease the likelihood of state changes leaking out and affecting - other tests. But the goal is not to assert that ``_USE_SHELL`` is used, nor anything - about how or when it is used, which is an implementation detail subject to change. + other tests. But the goal is not to assert implementation details of USE_SHELL. + + This covers two of the common implementation strategies, for convenience in testing + both. USE_SHELL could be implemented in the metaclass: - This is possible but inelegant to do with pytest's monkeypatch fixture, which only - restores attributes that it has previously been used to change, create, or remove. + * With a separate _USE_SHELL backing attribute. If using a property or other + descriptor, this is the natural way to do it, but custom __getattribute__ and + __setattr__ logic, if it does more than adding warnings, may also use that. + * Like a simple attribute, using USE_SHELL itself, stored as usual in the class + dictionary, with custom __getattribute__/__setattr__ logic only to warn. + + This tries to save private state, tries to save the public attribute value, yields + to the test case, tries to restore the public attribute value, then tries to restore + private state. The idea is that if the getting or setting logic is wrong in the code + under test, the state will still most likely be reset successfully. """ no_value = object() + # Try to save the original private state. try: - old_backing_value = Git._USE_SHELL + old_private_value = Git._USE_SHELL except AttributeError: - old_backing_value = no_value + separate_backing_attribute = False + try: + old_private_value = type.__getattribute__(Git, "USE_SHELL") + except AttributeError: + old_private_value = no_value + _logger.error("Cannot retrieve old private _USE_SHELL or USE_SHELL value") + else: + separate_backing_attribute = True + try: + # Try to save the original public value. Rather than attempt to restore a state + # where the attribute is not set, if we cannot do this we allow AttributeError + # to propagate out of the fixture, erroring the test case before its code runs. with _suppress_deprecation_warning(): old_public_value = Git.USE_SHELL @@ -108,14 +133,15 @@ def restore_use_shell_state() -> Generator[None, None, None]: # during the yield. (The outer try-finally catches exceptions in this fixture.) yield + # Try to restore the original public value. with _suppress_deprecation_warning(): Git.USE_SHELL = old_public_value finally: - if old_backing_value is no_value: - with contextlib.suppress(AttributeError): - del Git._USE_SHELL - else: - Git._USE_SHELL = old_backing_value + # Try to restore the original private state. + if separate_backing_attribute: + Git._USE_SHELL = old_private_value + elif old_private_value is not no_value: + type.__setattr__(Git, "USE_SHELL", old_private_value) def test_cannot_access_undefined_on_git_class() -> None: @@ -277,7 +303,7 @@ def test_use_shell_is_mock_patchable_on_class_as_object_attribute( """ Git.USE_SHELL = original_value if Git.USE_SHELL is not original_value: - raise RuntimeError(f"Can't set up the test") + raise RuntimeError("Can't set up the test") new_value = not original_value with unittest.mock.patch.object(Git, "USE_SHELL", new_value): From 436bcaa85712f73640ac719679d6d1366c376483 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 12:05:52 -0400 Subject: [PATCH 70/89] Add `type: ignore` in test that we can't set USE_SHELL on instances As with other `type: ignore` comments in the deprecation tests, mypy will detect if it is superfluous (provided warn_unused_ignores is set to true), thereby enforcing that such code is a static type error. --- test/deprecation/test_cmd_git.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index 4183018e6..4925fada8 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -274,7 +274,7 @@ def test_use_shell_cannot_set_on_instance( ) -> None: instance = Git() with pytest.raises(AttributeError): - instance.USE_SHELL = value + instance.USE_SHELL = value # type: ignore[misc] # Name not in __slots__. @pytest.mark.filterwarnings("ignore::DeprecationWarning") From febda6f6e9aa56f4b525020153ed88459906641f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 12:09:15 -0400 Subject: [PATCH 71/89] Clarify unittest.mock.patch patchability test docstring --- test/deprecation/test_cmd_git.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index 4925fada8..b792fddb8 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -292,10 +292,11 @@ def test_use_shell_is_mock_patchable_on_class_as_object_attribute( correct one to restore, even by a normal setattr. The effect is that some ways of simulating a class attribute with added behavior can - cause a descriptor, such as a property, to be set to its own backing attribute - during unpatching; then subsequent reads raise RecursionError. This happens if both - (a) setting it on the class is customized in a metaclass and (b) getting it on - instances is customized with a descriptor (such as a property) in the class itself. + cause a descriptor, such as a property, to be set as the value of its own backing + attribute during unpatching; then subsequent reads raise RecursionError. This + happens if both (a) setting it on the class is customized in a metaclass and (b) + getting it on instances is customized with a descriptor (such as a property) in the + class itself. Although ideally code outside GitPython would not rely on being able to patch Git.USE_SHELL with unittest.mock.patch, the technique is widespread. Thus, USE_SHELL From 40371087ac83497f307d2239f36646a321028829 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 13:31:22 -0400 Subject: [PATCH 72/89] Test that Git.execute's own read of USE_SHELL does not warn + Use simplefilter where we can (separate from this test). --- test/deprecation/test_cmd_git.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index b792fddb8..f17756fb6 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -82,7 +82,7 @@ @contextlib.contextmanager def _suppress_deprecation_warning() -> Generator[None, None, None]: with warnings.catch_warnings(): - warnings.filterwarnings("ignore", category=DeprecationWarning) + warnings.simplefilter("ignore", DeprecationWarning) yield @@ -313,6 +313,24 @@ class itself. assert Git.USE_SHELL is original_value +def test_execute_without_shell_arg_does_not_warn() -> None: + """No deprecation warning is issued from operations implemented using Git.execute(). + + When no ``shell`` argument is passed to Git.execute, which is when the value of + USE_SHELL is to be used, the way Git.execute itself accesses USE_SHELL does not + issue a deprecation warning. + """ + with warnings.catch_warnings(): + for category in DeprecationWarning, PendingDeprecationWarning: + warnings.filterwarnings( + action="error", + category=category, + module=r"git(?:\.|$)", + ) + + Git().version() + + _EXPECTED_DIR_SUBSET = { "cat_file_all", "cat_file_header", From 0e311bf5da4332acd314aa6927138b36bbd9e527 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 14:42:26 -0400 Subject: [PATCH 73/89] Suppress type errors in restore_use_shell_state _USE_SHELL branches These conditional branches are kept so alternative implementations can be examined, including if they need to be investigated to satisfy some future requirement. But to be unittest.mock.patch patchable, the approaches that would have a _USE_SHELL backing attribute would be difficult to implement in a straightforward way, which seems not to be needed or justified at this time. Since that is not anticipated (except as an intermediate step in development), these suppressions make sense, and they will also be reported by mypy if the implementation changes to benefit from them (so long as it is configured with warn_unused_ignores set to true). --- test/deprecation/test_cmd_git.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index f17756fb6..b6af5c770 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -111,7 +111,7 @@ def restore_use_shell_state() -> Generator[None, None, None]: # Try to save the original private state. try: - old_private_value = Git._USE_SHELL + old_private_value = Git._USE_SHELL # type: ignore[attr-defined] except AttributeError: separate_backing_attribute = False try: @@ -139,7 +139,7 @@ def restore_use_shell_state() -> Generator[None, None, None]: finally: # Try to restore the original private state. if separate_backing_attribute: - Git._USE_SHELL = old_private_value + Git._USE_SHELL = old_private_value # type: ignore[attr-defined] elif old_private_value is not no_value: type.__setattr__(Git, "USE_SHELL", old_private_value) From df4c5c03d14a53448a232a78eeab94017ac7e7ba Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Mar 2024 15:33:47 -0400 Subject: [PATCH 74/89] Fix wrong/unclear grammar in test_instance_dir docstring --- test/deprecation/test_cmd_git.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index b6af5c770..d312b8778 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -383,7 +383,7 @@ def test_class_dir() -> None: def test_instance_dir() -> None: """dir() on Git objects includes its statically known attributes. - This is like test_class_dir, but for Git instance rather than the class itself. + This is like test_class_dir, but for Git instances rather than the class itself. """ instance = Git() actual = set(dir(instance)) From d38e721c1f35695ecdaf6e88b721881fb7b3ed6f Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Tue, 26 Mar 2024 21:55:04 -0400 Subject: [PATCH 75/89] Issue warnings whenever Git.USE_SHELL is accessed With a special message when it is assigned a True value, which is the dangerous use that underlies its deprecation. The warnings are all DeprecationWarning. --- git/cmd.py | 78 ++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 61 insertions(+), 17 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 03e5d7ffc..0b7536d91 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -19,6 +19,7 @@ import sys from textwrap import dedent import threading +import warnings from git.compat import defenc, force_bytes, safe_decode from git.exc import ( @@ -54,6 +55,7 @@ TYPE_CHECKING, TextIO, Tuple, + Type, Union, cast, overload, @@ -307,8 +309,45 @@ def dict_to_slots_and__excluded_are_none(self: object, d: Mapping[str, Any], exc ## -- End Utilities -- @} +_USE_SHELL_DEFAULT_MESSAGE = ( + "Git.USE_SHELL is deprecated, because only its default value of False is safe. " + "It will be removed in a future release." +) + +_USE_SHELL_DANGER_MESSAGE = ( + "Setting Git.USE_SHELL to True is unsafe and insecure, and should be avoided, " + "because the effect of shell metacharacters and shell expansions cannot usually be " + "accounted for. In addition, Git.USE_SHELL is deprecated and will be removed in a " + "future release." +) + + +def _warn_use_shell(extra_danger: bool) -> None: + warnings.warn( + _USE_SHELL_DANGER_MESSAGE if extra_danger else _USE_SHELL_DEFAULT_MESSAGE, + DeprecationWarning, + stacklevel=3, + ) + + +class _GitMeta(type): + """Metaclass for :class:`Git`. + + This helps issue :class:`DeprecationWarning` if :attr:`Git.USE_SHELL` is used. + """ -class Git: + @property + def USE_SHELL(cls: Type[Git]) -> bool: + _warn_use_shell(False) + return cls._USE_SHELL + + @USE_SHELL.setter + def USE_SHELL(cls: Type[Git], value: bool) -> None: + _warn_use_shell(value) + cls._USE_SHELL = value + + +class Git(metaclass=_GitMeta): """The Git class manages communication with the Git binary. It provides a convenient interface to calling the Git binary, such as in:: @@ -358,25 +397,30 @@ def __setstate__(self, d: Dict[str, Any]) -> None: GIT_PYTHON_TRACE = os.environ.get("GIT_PYTHON_TRACE", False) """Enables debugging of GitPython's git commands.""" - USE_SHELL = False - """Deprecated. If set to ``True``, a shell will be used when executing git commands. + _USE_SHELL: bool = False - Prior to GitPython 2.0.8, this had a narrow purpose in suppressing console windows - in graphical Windows applications. In 2.0.8 and higher, it provides no benefit, as - GitPython solves that problem more robustly and safely by using the - ``CREATE_NO_WINDOW`` process creation flag on Windows. + @property + def USE_SHELL(self) -> bool: + """Deprecated. If set to ``True``, a shell will be used to execute git commands. - Code that uses ``USE_SHELL = True`` or that passes ``shell=True`` to any GitPython - functions should be updated to use the default value of ``False`` instead. ``True`` - is unsafe unless the effect of shell expansions is fully considered and accounted - for, which is not possible under most circumstances. + Prior to GitPython 2.0.8, this had a narrow purpose in suppressing console + windows in graphical Windows applications. In 2.0.8 and higher, it provides no + benefit, as GitPython solves that problem more robustly and safely by using the + ``CREATE_NO_WINDOW`` process creation flag on Windows. - See: + Code that uses ``USE_SHELL = True`` or that passes ``shell=True`` to any + GitPython functions should be updated to use the default value of ``False`` + instead. ``True`` is unsafe unless the effect of shell expansions is fully + considered and accounted for, which is not possible under most circumstances. - - :meth:`Git.execute` (on the ``shell`` parameter). - - https://github.com/gitpython-developers/GitPython/commit/0d9390866f9ce42870d3116094cd49e0019a970a - - https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags - """ + See: + + - :meth:`Git.execute` (on the ``shell`` parameter). + - https://github.com/gitpython-developers/GitPython/commit/0d9390866f9ce42870d3116094cd49e0019a970a + - https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags + """ + _warn_use_shell(False) + return self._USE_SHELL _git_exec_env_var = "GIT_PYTHON_GIT_EXECUTABLE" _refresh_env_var = "GIT_PYTHON_REFRESH" @@ -1138,7 +1182,7 @@ def execute( stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb") if shell is None: - shell = self.USE_SHELL + shell = self._USE_SHELL _logger.debug( "Popen(%s, cwd=%s, stdin=%s, shell=%s, universal_newlines=%s)", redacted_command, From 05de5c0d3bc81f4d56283399417db2f6927fe233 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 03:29:41 -0400 Subject: [PATCH 76/89] Implement instance USE_SHELL lookup in __getattr__ This is with the intention of making it so that Git.USE_SHELL is unittest.mock.patch patchable. However, while this moves in the right direction, it can't be done this way, as the attribute is found to be absent on the class, so when unittest.mock.patch unpatches, it tries to delete the attribute it has set, which fails due to the metaclass's USE_SHELL instance property having no deleter. --- git/cmd.py | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 0b7536d91..d01c15b04 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -399,28 +399,25 @@ def __setstate__(self, d: Dict[str, Any]) -> None: _USE_SHELL: bool = False - @property - def USE_SHELL(self) -> bool: - """Deprecated. If set to ``True``, a shell will be used to execute git commands. + USE_SHELL: bool + """Deprecated. If set to ``True``, a shell will be used to execute git commands. - Prior to GitPython 2.0.8, this had a narrow purpose in suppressing console - windows in graphical Windows applications. In 2.0.8 and higher, it provides no - benefit, as GitPython solves that problem more robustly and safely by using the - ``CREATE_NO_WINDOW`` process creation flag on Windows. + Prior to GitPython 2.0.8, this had a narrow purpose in suppressing console windows + in graphical Windows applications. In 2.0.8 and higher, it provides no benefit, as + GitPython solves that problem more robustly and safely by using the + ``CREATE_NO_WINDOW`` process creation flag on Windows. - Code that uses ``USE_SHELL = True`` or that passes ``shell=True`` to any - GitPython functions should be updated to use the default value of ``False`` - instead. ``True`` is unsafe unless the effect of shell expansions is fully - considered and accounted for, which is not possible under most circumstances. + Code that uses ``USE_SHELL = True`` or that passes ``shell=True`` to any GitPython + functions should be updated to use the default value of ``False`` instead. ``True`` + is unsafe unless the effect of shell expansions is fully considered and accounted + for, which is not possible under most circumstances. - See: + See: - - :meth:`Git.execute` (on the ``shell`` parameter). - - https://github.com/gitpython-developers/GitPython/commit/0d9390866f9ce42870d3116094cd49e0019a970a - - https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags - """ - _warn_use_shell(False) - return self._USE_SHELL + - :meth:`Git.execute` (on the ``shell`` parameter). + - https://github.com/gitpython-developers/GitPython/commit/0d9390866f9ce42870d3116094cd49e0019a970a + - https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags + """ _git_exec_env_var = "GIT_PYTHON_GIT_EXECUTABLE" _refresh_env_var = "GIT_PYTHON_REFRESH" @@ -921,6 +918,11 @@ def __getattr__(self, name: str) -> Any: """ if name.startswith("_"): return super().__getattribute__(name) + + if name == "USE_SHELL": + _warn_use_shell(False) + return self._USE_SHELL + return lambda *args, **kwargs: self._call_process(name, *args, **kwargs) def set_persistent_git_options(self, **kwargs: Any) -> None: From 04eb09c728fbdc4501fbbba1d6f58a0bb7050470 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 03:40:14 -0400 Subject: [PATCH 77/89] Have USE_SHELL warn but work like normal via super() This reimplements Git.USE_SHELL with no properties or descriptors. The metaclass is still needed, but instad of defining properties it defines __getattribute__ and __setattr__, which check for USE_SHELL and warn, then invoke the default attribute access via super(). Likewise, in the Git class itself, a __getatttribute__ override is introduced (not to be confused with __getattr__, which was already present and handles attribute access when an attribute is otherwise absent, unlike __getattribute__ which is always used). This checks for reading USE_SHELL on an instance and warns, then invokes the default attribute access via super(). Advantages: - Git.USE_SHELL is again unittest.mock.patch patchable. - AttributeError messages are automatically as before. - It effectively is a simple attribute, yet with warning, so other unanticipated ways of accessing it may be less likely to break. - The code is simpler, cleaner, and clearer. There is some overhead, but it is small, especially compared to a subprocess invocation as is done for performing most git operations. However, this does introduce disadvantages that must be addressed: - Although attribute access on Git instances was already highly dynamic, as "methods" are synthesized for git subcommands, this was and is not the case for the Git class itself, whose attributes remain exactly those that can be inferred without considering the existence of __getattribute__ and __setattr__ on the metaclass. So static type checkers need to be prevented from accounting for those metaclass methods in a way that causes them to infer that arbitrary class attribute access is allowed. - The occurrence of Git.USE_SHELL in the Git.execute method (where the USE_SHELL attribute is actually examined) should be changed so it does not itself issue DeprecationWarning (it is not enough that by default a DeprecationWarning from there isn't displayed). --- git/cmd.py | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index d01c15b04..1c2aebd3c 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -55,7 +55,6 @@ TYPE_CHECKING, TextIO, Tuple, - Type, Union, cast, overload, @@ -336,15 +335,15 @@ class _GitMeta(type): This helps issue :class:`DeprecationWarning` if :attr:`Git.USE_SHELL` is used. """ - @property - def USE_SHELL(cls: Type[Git]) -> bool: - _warn_use_shell(False) - return cls._USE_SHELL + def __getattribute__(cls, name: str) -> Any: + if name == "USE_SHELL": + _warn_use_shell(False) + return super().__getattribute__(name) - @USE_SHELL.setter - def USE_SHELL(cls: Type[Git], value: bool) -> None: - _warn_use_shell(value) - cls._USE_SHELL = value + def __setattr__(cls, name: str, value: Any) -> Any: + if name == "USE_SHELL": + _warn_use_shell(value) + super().__setattr__(name, value) class Git(metaclass=_GitMeta): @@ -397,9 +396,7 @@ def __setstate__(self, d: Dict[str, Any]) -> None: GIT_PYTHON_TRACE = os.environ.get("GIT_PYTHON_TRACE", False) """Enables debugging of GitPython's git commands.""" - _USE_SHELL: bool = False - - USE_SHELL: bool + USE_SHELL: bool = False """Deprecated. If set to ``True``, a shell will be used to execute git commands. Prior to GitPython 2.0.8, this had a narrow purpose in suppressing console windows @@ -909,6 +906,11 @@ def __init__(self, working_dir: Union[None, PathLike] = None) -> None: self.cat_file_header: Union[None, TBD] = None self.cat_file_all: Union[None, TBD] = None + def __getattribute__(self, name: str) -> Any: + if name == "USE_SHELL": + _warn_use_shell(False) + return super().__getattribute__(name) + def __getattr__(self, name: str) -> Any: """A convenience method as it allows to call the command as if it was an object. @@ -918,11 +920,6 @@ def __getattr__(self, name: str) -> Any: """ if name.startswith("_"): return super().__getattribute__(name) - - if name == "USE_SHELL": - _warn_use_shell(False) - return self._USE_SHELL - return lambda *args, **kwargs: self._call_process(name, *args, **kwargs) def set_persistent_git_options(self, **kwargs: Any) -> None: @@ -1184,7 +1181,7 @@ def execute( stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb") if shell is None: - shell = self._USE_SHELL + shell = self.USE_SHELL _logger.debug( "Popen(%s, cwd=%s, stdin=%s, shell=%s, universal_newlines=%s)", redacted_command, From c6f518b40e42d394dfbcf338aacf101cad58b700 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 14:14:52 -0400 Subject: [PATCH 78/89] Keep mypy from thinking Git has arbitrary class attributes The existence of __getattr__ or __getattribute__ on a class causes static type checkers like mypy to stop inferring that reads of unrecognized instance attributes are static type errors. When the class is a metaclass, this causes static type checkers to stop inferring that reads of unrecognized class attributes, on classes that use (i.e., that have as their type) the metaclass, are static type errors. The Git class itself defines __getattr__ and __getattribute__, but Git objects' instance attributes really are dynamically synthesized (in __getattr__). However, class attributes of Git are not dynamic, even though _GitMeta defines __getattribute__. Therefore, something special is needed so mypy infers nothing about Git class attributes from the existence of _GitMeta.__getattribute__. This takes the same approach as taken to the analogous problem with __getattr__ at module level, defining __getattribute__ with a different name first and then assigning that to __getattribute__ under an `if not TYPE_CHECKING:`. (Allowing static type checkers to see the original definition allows them to find some kinds of type errors in the body of the method, which is why the method isn't just defined in the `if not TYPE_CHECKING`.) Although it may not currently be necessary to hide __setattr__ too, the same is done with it in _GitMeta as well. The reason is that the intent may otherwise be subtly amgiguous to human readers and maybe future tools: when __setattr__ exists, the effect of setting a class attribute that did not previously exist is in principle unclear, and might not make the attribute readble. (I am unsure if this is the right choice; either approach seems reasonable.) --- git/cmd.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 1c2aebd3c..ce19e733c 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -335,16 +335,23 @@ class _GitMeta(type): This helps issue :class:`DeprecationWarning` if :attr:`Git.USE_SHELL` is used. """ - def __getattribute__(cls, name: str) -> Any: + def __getattribute(cls, name: str) -> Any: if name == "USE_SHELL": _warn_use_shell(False) return super().__getattribute__(name) - def __setattr__(cls, name: str, value: Any) -> Any: + def __setattr(cls, name: str, value: Any) -> Any: if name == "USE_SHELL": _warn_use_shell(value) super().__setattr__(name, value) + if not TYPE_CHECKING: + # To preserve static checking for undefined/misspelled attributes while letting + # the methods' bodies be type-checked, these are defined as non-special methods, + # then bound to special names out of view of static type checkers. + __getattribute__ = __getattribute + __setattr__ = __setattr + class Git(metaclass=_GitMeta): """The Git class manages communication with the Git binary. From c5d5b16bfb4829dd510e3ff258b3daca4cd3b57d Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 14:20:54 -0400 Subject: [PATCH 79/89] Clarify that the private name mangling is intentional --- git/cmd.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/git/cmd.py b/git/cmd.py index ce19e733c..de5c6929b 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -348,7 +348,8 @@ def __setattr(cls, name: str, value: Any) -> Any: if not TYPE_CHECKING: # To preserve static checking for undefined/misspelled attributes while letting # the methods' bodies be type-checked, these are defined as non-special methods, - # then bound to special names out of view of static type checkers. + # then bound to special names out of view of static type checkers. (The original + # names invoke name mangling (leading "__") to avoid confusion in other scopes.) __getattribute__ = __getattribute __setattr__ = __setattr From 84bf2ca034721cd54e792f57585083a1dbffc6ea Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 15:25:20 -0400 Subject: [PATCH 80/89] Read USE_SHELL in Git.execute without DeprecationWarning This changes how Git.execute itself accesses the USE_SHELL attribute, so that its own read of it does not issue a warning. --- git/cmd.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/git/cmd.py b/git/cmd.py index de5c6929b..eed82d7dd 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -1189,7 +1189,12 @@ def execute( stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb") if shell is None: - shell = self.USE_SHELL + # Get the value of USE_SHELL with no deprecation warning. Do this without + # warnings.catch_warnings, to avoid a race condition with application code + # configuring warnings. The value could be looked up in type(self).__dict__ + # or Git.__dict__, but those can break under some circumstances. This works + # the same as self.USE_SHELL in more situations; see Git.__getattribute__. + shell = super().__getattribute__("USE_SHELL") _logger.debug( "Popen(%s, cwd=%s, stdin=%s, shell=%s, universal_newlines=%s)", redacted_command, From 5bef7ed7369e1d6f93161607e01dc02bcd1720ab Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Thu, 28 Mar 2024 23:35:03 -0400 Subject: [PATCH 81/89] Add GitPython project top comments to new test modules --- test/deprecation/test_cmd_git.py | 3 +++ test/deprecation/test_compat.py | 3 +++ test/deprecation/test_toplevel.py | 3 +++ test/deprecation/test_types.py | 3 +++ 4 files changed, 12 insertions(+) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index d312b8778..b15f219d8 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -1,3 +1,6 @@ +# This module is part of GitPython and is released under the +# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ + """Tests for static and dynamic characteristics of Git class and instance attributes. Currently this all relates to the deprecated :attr:`Git.USE_SHELL` class attribute, diff --git a/test/deprecation/test_compat.py b/test/deprecation/test_compat.py index 22f733083..2d7805e61 100644 --- a/test/deprecation/test_compat.py +++ b/test/deprecation/test_compat.py @@ -1,3 +1,6 @@ +# This module is part of GitPython and is released under the +# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ + """Tests for dynamic and static characteristics of git.compat module attributes. These tests verify that the is_ attributes are available, and are even listed diff --git a/test/deprecation/test_toplevel.py b/test/deprecation/test_toplevel.py index 54dc8e358..740408193 100644 --- a/test/deprecation/test_toplevel.py +++ b/test/deprecation/test_toplevel.py @@ -1,3 +1,6 @@ +# This module is part of GitPython and is released under the +# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ + """Tests for dynamic and static characteristics of top-level git module attributes. Provided mypy has ``warn_unused_ignores = true`` set, running mypy on these test cases diff --git a/test/deprecation/test_types.py b/test/deprecation/test_types.py index cd53fa210..f97375a85 100644 --- a/test/deprecation/test_types.py +++ b/test/deprecation/test_types.py @@ -1,3 +1,6 @@ +# This module is part of GitPython and is released under the +# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ + """Tests for dynamic and static characteristics of git.types module attributes.""" import sys From 3da47c26db44f287fa2b10e95995689de1f578bc Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 29 Mar 2024 11:46:38 -0400 Subject: [PATCH 82/89] Hide `del util` from type checkers Even though current type checkers don't seem to need it. As noted in dffa930, it appears neither mypy nor pyright currently needs `del util` to be guarded by `if not TYPE_CHECKING:` -- they currently treat util as bound even without it, even with `del util` present. It is not obvious that this is the best type checker behavior or that type checkers will continue to behave this way. (In addition, it may help humans for all statements whose effects are intended not to be considered for purposes of static typing to be guarded by `if not TYPE_CHECKING:`.) So this guards the deletion of util the same as the binding of __getattr__. This also moves, clarifies, and expands the commented explanations of what is going on. --- git/__init__.py | 54 +++++++++++++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/git/__init__.py b/git/__init__.py index fd1b87707..1b2360e3a 100644 --- a/git/__init__.py +++ b/git/__init__.py @@ -169,6 +169,8 @@ IndexEntry, IndexFile, StageType, + # NOTE: This tells type checkers what util resolves to. We delete it, and it is + # really resolved by __getattr__, which warns. See below on what to use instead. util, ) from git.util import ( # @NoMove @@ -183,27 +185,6 @@ raise ImportError("%s: %s" % (_exc.__class__.__name__, _exc)) from _exc -# NOTE: The expression `git.util` evaluates to git.index.util and `from git import util` -# imports git.index.util, NOT git.util. It may not be feasible to change this until the -# next major version, to avoid breaking code inadvertently relying on it. -# -# - If git.index.util *is* what you want, use or import from that, to avoid confusion. -# -# - To use the "real" git.util module, write `from git.util import ...`, or if necessary -# access it as `sys.modules["git.util"]`. -# -# (This differs from other indirect-submodule imports that are unambiguously non-public -# and subject to immediate removal. Here, the public git.util module, though different, -# makes less discoverable that the expression `git.util` refers to a non-public -# attribute of the git module.) -# -# This had come about by a wildcard import. Now that all intended imports are explicit, -# the intuitive but potentially incompatible binding occurs due to the usual rules for -# Python submodule bindings. So for now we delete that and let __getattr__ handle it. -# -del util - - def _warned_import(message: str, fullname: str) -> "ModuleType": import importlib @@ -242,7 +223,36 @@ def _getattr(name: str) -> Any: raise AttributeError(f"module {__name__!r} has no attribute {name!r}") -if not TYPE_CHECKING: # Preserve static checking for undefined/misspelled attributes. +if not TYPE_CHECKING: + # NOTE: The expression `git.util` gives git.index.util and `from git import util` + # imports git.index.util, NOT git.util. It may not be feasible to change this until + # the next major version, to avoid breaking code inadvertently relying on it. + # + # - If git.index.util *is* what you want, use (or import from) that, to avoid + # confusion. + # + # - To use the "real" git.util module, write `from git.util import ...`, or if + # necessary access it as `sys.modules["git.util"]`. + # + # Note also that `import git.util` technically imports the "real" git.util... but + # the *expression* `git.util` after doing so is still git.index.util! + # + # (This situation differs from that of other indirect-submodule imports that are + # unambiguously non-public and subject to immediate removal. Here, the public + # git.util module, though different, makes less discoverable that the expression + # `git.util` refers to a non-public attribute of the git module.) + # + # This had originally come about by a wildcard import. Now that all intended imports + # are explicit, the intuitive but potentially incompatible binding occurs due to the + # usual rules for Python submodule bindings. So for now we replace that binding with + # git.index.util, delete that, and let __getattr__ handle it and issue a warning. + # + # For the same runtime behavior, it would be enough to forgo importing util, and + # delete util as created naturally; __getattr__ would behave the same. But type + # checkers would not know what util refers to when accessed as an attribute of git. + del util + + # This is "hidden" to preserve static checking for undefined/misspelled attributes. __getattr__ = _getattr # { Initialize git executable path From bdabb21fe62063ce51fcae6b5f499f10f55525c5 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 29 Mar 2024 18:52:10 -0400 Subject: [PATCH 83/89] Expand USE_SHELL docstring; clarify a test usage In the USE_SHELL docstring: - Restore the older wording "when executing git commands" rather than "to execute git commands". I've realized that longer phrase, which dates back to the introduction of USE_SHELL in 1c2dd54, is clearer, because readers less familiar with GitPython's overall design and operation will still not be misled into thinking USE_SHELL ever affects whether GitPython uses an external git command, versus some other mechanism, to do something. - Give some more information about why USE_SHELL = True is unsafe (though further revision or clarification may be called for). - Note some non-obvious aspects of USE_SHELL, that some of the way it interacts with other aspects of GitPython is not part of what is or has been documented about it, and in practice changes over time. The first example relates to #1792; the second may help users understand why code that enables USE_SHELL on Windows, in addition to being unsafe there, often breaks immediately on other platforms; the third is included so the warnings in the expanded docstring are not interpreted as a new commitment that any shell syntax that may have a *desired* effect in some application will continue to have the same effect in the future. - Cover a second application that might lead, or have led, to setting USE_SHELL to True, and explain what to do instead. In test_successful_default_refresh_invalidates_cached_version_info: - Rewrite the commented explanation of a special variant of that second application, where the usual easy alternatives are not used because part of the goal of the test is to check a "default" scenario that does not include either of them. This better explains why that choice is made in the test, and also hopefully will prevent anyone from thinking that test is a model for another situation (which, as noted, is unlikely to be the case even in unit tests). --- git/cmd.py | 40 +++++++++++++++++++++++++++++++--------- test/test_git.py | 14 +++++++------- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index eed82d7dd..42e6e927c 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -405,23 +405,45 @@ def __setstate__(self, d: Dict[str, Any]) -> None: """Enables debugging of GitPython's git commands.""" USE_SHELL: bool = False - """Deprecated. If set to ``True``, a shell will be used to execute git commands. + """Deprecated. If set to ``True``, a shell will be used when executing git commands. + + Code that uses ``USE_SHELL = True`` or that passes ``shell=True`` to any GitPython + functions should be updated to use the default value of ``False`` instead. ``True`` + is unsafe unless the effect of syntax treated specially by the shell is fully + considered and accounted for, which is not possible under most circumstances. As + detailed below, it is also no longer needed, even where it had been in the past. + + In addition, how a value of ``True`` interacts with some aspects of GitPython's + operation is not precisely specified and may change without warning, even before + GitPython 4.0.0 when :attr:`USE_SHELL` may be removed. This includes: + + * Whether or how GitPython automatically customizes the shell environment. + + * Whether, outside of Windows (where :class:`subprocess.Popen` supports lists of + separate arguments even when ``shell=True``), this can be used with any GitPython + functionality other than direct calls to the :meth:`execute` method. + + * Whether any GitPython feature that runs git commands ever attempts to partially + sanitize data a shell may treat specially. Currently this is not done. Prior to GitPython 2.0.8, this had a narrow purpose in suppressing console windows in graphical Windows applications. In 2.0.8 and higher, it provides no benefit, as GitPython solves that problem more robustly and safely by using the ``CREATE_NO_WINDOW`` process creation flag on Windows. - Code that uses ``USE_SHELL = True`` or that passes ``shell=True`` to any GitPython - functions should be updated to use the default value of ``False`` instead. ``True`` - is unsafe unless the effect of shell expansions is fully considered and accounted - for, which is not possible under most circumstances. + Because Windows path search differs subtly based on whether a shell is used, in rare + cases changing this from ``True`` to ``False`` may keep an unusual git "executable", + such as a batch file, from being found. To fix this, set the command name or full + path in the :envvar:`GIT_PYTHON_GIT_EXECUTABLE` environment variable or pass the + full path to :func:`git.refresh` (or invoke the script using a ``.exe`` shim). - See: + Further reading: - - :meth:`Git.execute` (on the ``shell`` parameter). - - https://github.com/gitpython-developers/GitPython/commit/0d9390866f9ce42870d3116094cd49e0019a970a - - https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags + * :meth:`Git.execute` (on the ``shell`` parameter). + * https://github.com/gitpython-developers/GitPython/commit/0d9390866f9ce42870d3116094cd49e0019a970a + * https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags + * https://github.com/python/cpython/issues/91558#issuecomment-1100942950 + * https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw """ _git_exec_env_var = "GIT_PYTHON_GIT_EXECUTABLE" diff --git a/test/test_git.py b/test/test_git.py index 112e2f0eb..94e68ecf0 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -669,13 +669,13 @@ def test_successful_default_refresh_invalidates_cached_version_info(self): stack.enter_context(_patch_out_env("GIT_PYTHON_GIT_EXECUTABLE")) if sys.platform == "win32": - # On Windows, use a shell so "git" finds "git.cmd". (In the infrequent - # case that this effect is desired in production code, it should not be - # done with this technique. USE_SHELL=True is less secure and reliable, - # as unintended shell expansions can occur, and is deprecated. Instead, - # use a custom command, by setting the GIT_PYTHON_GIT_EXECUTABLE - # environment variable to git.cmd or by passing git.cmd's full path to - # git.refresh. Or wrap the script with a .exe shim.) + # On Windows, use a shell so "git" finds "git.cmd". The correct and safe + # ways to do this straightforwardly are to set GIT_PYTHON_GIT_EXECUTABLE + # to git.cmd in the environment, or call git.refresh with the command's + # full path. See the Git.USE_SHELL docstring for deprecation details. + # But this tests a "default" scenario where neither is done. The + # approach used here, setting USE_SHELL to True so PATHEXT is honored, + # should not be used in production code (nor even in most test cases). stack.enter_context(mock.patch.object(Git, "USE_SHELL", True)) new_git = Git() From b51b08052821f91a61a40808328aed0ac35935a8 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 29 Mar 2024 19:51:32 -0400 Subject: [PATCH 84/89] Explain the approach in test.deprecation to static checking And why this increases the importance of the warn_unused_ignored mypy configuration option. --- pyproject.toml | 2 +- test/deprecation/__init__.py | 17 +++++++++++++++++ test/deprecation/test_cmd_git.py | 2 +- 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 5eac2be09..6cb05f96e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -25,7 +25,7 @@ files = ["git/", "test/deprecation/"] disallow_untyped_defs = true no_implicit_optional = true warn_redundant_casts = true -warn_unused_ignores = true +warn_unused_ignores = true # Useful in general, but especially in test/deprecation. warn_unreachable = true implicit_reexport = true # strict = true diff --git a/test/deprecation/__init__.py b/test/deprecation/__init__.py index 56b5d89db..fec3126d2 100644 --- a/test/deprecation/__init__.py +++ b/test/deprecation/__init__.py @@ -1,2 +1,19 @@ # This module is part of GitPython and is released under the # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ + +"""Tests of deprecation warnings and possible related attribute bugs. + +Most deprecation warnings are "basic" in the sense that there is no special complexity +to consider, in introducing them. However, to issue deprecation warnings on mere +attribute access can involve adding new dynamic behavior. This can lead to subtle bugs +or less useful dynamic metadata. It can also weaken static typing, as happens if a type +checker sees a method like ``__getattr__`` in a module or class whose attributes it did +not already judge to be dynamic. This test.deprecation submodule covers all three cases: +the basic cases, subtle dynamic behavior, and subtle static type checking issues. + +Static type checking is "tested" by a combination of code that should not be treated as +a type error but would be in the presence of particular bugs, and code that *should* be +treated as a type error and is accordingly marked ``# type: ignore[REASON]`` (for +specific ``REASON``. The latter will only produce mypy errors when the expectation is +not met if it is configured with ``warn_unused_ignores = true``. +""" diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index b15f219d8..3f87037a0 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -1,7 +1,7 @@ # This module is part of GitPython and is released under the # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ -"""Tests for static and dynamic characteristics of Git class and instance attributes. +"""Tests for dynamic and static characteristics of Git class and instance attributes. Currently this all relates to the deprecated :attr:`Git.USE_SHELL` class attribute, which can also be accessed through instances. Some tests directly verify its behavior, From 7cd3aa913077d55bbdb7ca01e6b7df593121f643 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 29 Mar 2024 22:55:36 -0400 Subject: [PATCH 85/89] Make test.performance.lib docstring more specific To distinguish it from the more general test.lib as well as from the forthcoming test.deprecation.lib. --- test/performance/lib.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/performance/lib.py b/test/performance/lib.py index 2ca3c409b..c24599986 100644 --- a/test/performance/lib.py +++ b/test/performance/lib.py @@ -1,7 +1,7 @@ # This module is part of GitPython and is released under the # 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ -"""Support library for tests.""" +"""Support library for performance tests.""" import logging import os From cf2576e406006ec28bcc85565a7ef85864cbd39e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 29 Mar 2024 23:15:52 -0400 Subject: [PATCH 86/89] Make/use test.deprecation.lib; abandon idea to filter by module This creates a module test.deprecation.lib in the test suite for a couple general helpers used in deprecation tests, one of which is now used in two of the test modules and the other of which happens only to be used in one but is concepually related to the first. The assert_no_deprecation_warning context manager function fixes two different flawed approaches to that, which were in use earlier: - In test_basic, only DeprecationWarning and not the less significant PendingDeprecationWarning had been covere, which it should, at least for symmetry, since pytest.deprecated_call() treats it like DeprecationWarning. There was also a comment expressing a plan to make it filter only for warnings originating from GitPython, which was a flawed idea, as detailed below. - In test_cmd_git, the flawed idea of attempting to filter only for warnings originating from GitPython was implemented. The problem with this is that it is heavily affected by stacklevel and its interaction with the pattern of calls through which the warning arises: warnings could actually emanate from code in GitPython's git module, but be registered as having come from test code, a callback in gitdb, smmap, or the standard library, or even the pytest test runner. Some of these are more likely than others, but all are possible especially considering the possibility of a bug in the value passed to warning.warn as stacklevel. (It may be valuable for such bugs to cause tests to fail, but they should not cause otherwise failing tests to wrongly pass.) It is probably feasible to implement such filtering successfully, but I don't think it's worthwhile for the small reduction in likelihood of failing later on an unrealted DeprecationWarning from somewhere else, especially considering that GitPython's main dependencies are so minimal. --- test/deprecation/lib.py | 27 +++++++++++++++++++++++++++ test/deprecation/test_basic.py | 26 ++++++++------------------ test/deprecation/test_cmd_git.py | 24 +++++------------------- 3 files changed, 40 insertions(+), 37 deletions(-) create mode 100644 test/deprecation/lib.py diff --git a/test/deprecation/lib.py b/test/deprecation/lib.py new file mode 100644 index 000000000..9fe623a3a --- /dev/null +++ b/test/deprecation/lib.py @@ -0,0 +1,27 @@ +# This module is part of GitPython and is released under the +# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/ + +"""Support library for deprecation tests.""" + +__all__ = ["assert_no_deprecation_warning", "suppress_deprecation_warning"] + +import contextlib +import warnings + +from typing import Generator + + +@contextlib.contextmanager +def assert_no_deprecation_warning() -> Generator[None, None, None]: + """Context manager to assert that code does not issue any deprecation warnings.""" + with warnings.catch_warnings(): + warnings.simplefilter("error", DeprecationWarning) + warnings.simplefilter("error", PendingDeprecationWarning) + yield + + +@contextlib.contextmanager +def suppress_deprecation_warning() -> Generator[None, None, None]: + with warnings.catch_warnings(): + warnings.simplefilter("ignore", DeprecationWarning) + yield diff --git a/test/deprecation/test_basic.py b/test/deprecation/test_basic.py index 8ee7e72b1..6235a836c 100644 --- a/test/deprecation/test_basic.py +++ b/test/deprecation/test_basic.py @@ -15,9 +15,6 @@ It is inapplicable to the deprecations whose warnings are tested in this module. """ -import contextlib -import warnings - import pytest from git.diff import NULL_TREE @@ -25,6 +22,8 @@ from git.repo import Repo from git.util import Iterable as _Iterable, IterableObj +from .lib import assert_no_deprecation_warning + # typing ----------------------------------------------------------------- from typing import Generator, TYPE_CHECKING @@ -38,15 +37,6 @@ # ------------------------------------------------------------------------ -@contextlib.contextmanager -def _assert_no_deprecation_warning() -> Generator[None, None, None]: - """Context manager to assert that code does not issue any deprecation warnings.""" - with warnings.catch_warnings(): - # FIXME: Refine this to filter for deprecation warnings from GitPython. - warnings.simplefilter("error", DeprecationWarning) - yield - - @pytest.fixture def commit(tmp_path: "Path") -> Generator["Commit", None, None]: """Fixture to supply a one-commit repo's commit, enough for deprecation tests.""" @@ -72,7 +62,7 @@ def test_diff_renamed_warns(diff: "Diff") -> None: def test_diff_renamed_file_does_not_warn(diff: "Diff") -> None: """The preferred Diff.renamed_file property issues no deprecation warning.""" - with _assert_no_deprecation_warning(): + with assert_no_deprecation_warning(): diff.renamed_file @@ -84,13 +74,13 @@ def test_commit_trailers_warns(commit: "Commit") -> None: def test_commit_trailers_list_does_not_warn(commit: "Commit") -> None: """The nondeprecated Commit.trailers_list property issues no deprecation warning.""" - with _assert_no_deprecation_warning(): + with assert_no_deprecation_warning(): commit.trailers_list def test_commit_trailers_dict_does_not_warn(commit: "Commit") -> None: """The nondeprecated Commit.trailers_dict property issues no deprecation warning.""" - with _assert_no_deprecation_warning(): + with assert_no_deprecation_warning(): commit.trailers_dict @@ -102,7 +92,7 @@ def test_traverse_list_traverse_in_base_class_warns(commit: "Commit") -> None: def test_traversable_list_traverse_override_does_not_warn(commit: "Commit") -> None: """Calling list_traverse on concrete subclasses is not deprecated, does not warn.""" - with _assert_no_deprecation_warning(): + with assert_no_deprecation_warning(): commit.list_traverse() @@ -114,7 +104,7 @@ def test_traverse_traverse_in_base_class_warns(commit: "Commit") -> None: def test_traverse_traverse_override_does_not_warn(commit: "Commit") -> None: """Calling traverse on concrete subclasses is not deprecated, does not warn.""" - with _assert_no_deprecation_warning(): + with assert_no_deprecation_warning(): commit.traverse() @@ -128,7 +118,7 @@ class Derived(_Iterable): def test_iterable_obj_inheriting_does_not_warn() -> None: """Subclassing git.util.IterableObj is not deprecated, does not warn.""" - with _assert_no_deprecation_warning(): + with assert_no_deprecation_warning(): class Derived(IterableObj): pass diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index 3f87037a0..6c592454a 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -56,12 +56,10 @@ metaclass marginal enough, to justify introducing a metaclass to issue the warnings. """ -import contextlib import logging import sys from typing import Generator import unittest.mock -import warnings if sys.version_info >= (3, 11): from typing import assert_type @@ -73,6 +71,8 @@ from git.cmd import Git +from .lib import assert_no_deprecation_warning, suppress_deprecation_warning + _USE_SHELL_DEPRECATED_FRAGMENT = "Git.USE_SHELL is deprecated" """Text contained in all USE_SHELL deprecation warnings, and starting most of them.""" @@ -82,13 +82,6 @@ _logger = logging.getLogger(__name__) -@contextlib.contextmanager -def _suppress_deprecation_warning() -> Generator[None, None, None]: - with warnings.catch_warnings(): - warnings.simplefilter("ignore", DeprecationWarning) - yield - - @pytest.fixture def restore_use_shell_state() -> Generator[None, None, None]: """Fixture to attempt to restore state associated with the USE_SHELL attribute. @@ -129,7 +122,7 @@ def restore_use_shell_state() -> Generator[None, None, None]: # Try to save the original public value. Rather than attempt to restore a state # where the attribute is not set, if we cannot do this we allow AttributeError # to propagate out of the fixture, erroring the test case before its code runs. - with _suppress_deprecation_warning(): + with suppress_deprecation_warning(): old_public_value = Git.USE_SHELL # This doesn't have its own try-finally because pytest catches exceptions raised @@ -137,7 +130,7 @@ def restore_use_shell_state() -> Generator[None, None, None]: yield # Try to restore the original public value. - with _suppress_deprecation_warning(): + with suppress_deprecation_warning(): Git.USE_SHELL = old_public_value finally: # Try to restore the original private state. @@ -323,14 +316,7 @@ def test_execute_without_shell_arg_does_not_warn() -> None: USE_SHELL is to be used, the way Git.execute itself accesses USE_SHELL does not issue a deprecation warning. """ - with warnings.catch_warnings(): - for category in DeprecationWarning, PendingDeprecationWarning: - warnings.filterwarnings( - action="error", - category=category, - module=r"git(?:\.|$)", - ) - + with assert_no_deprecation_warning(): Git().version() From f92f4c3bf902c7fc3887cfd969b3e54f581f18f8 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sat, 30 Mar 2024 20:16:45 -0400 Subject: [PATCH 87/89] Clarify security risk in USE_SHELL doc and warnings --- git/cmd.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 42e6e927c..b2829801f 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -314,10 +314,10 @@ def dict_to_slots_and__excluded_are_none(self: object, d: Mapping[str, Any], exc ) _USE_SHELL_DANGER_MESSAGE = ( - "Setting Git.USE_SHELL to True is unsafe and insecure, and should be avoided, " - "because the effect of shell metacharacters and shell expansions cannot usually be " - "accounted for. In addition, Git.USE_SHELL is deprecated and will be removed in a " - "future release." + "Setting Git.USE_SHELL to True is unsafe and insecure, as the effect of special " + "shell syntax cannot usually be accounted for. This can result in a command " + "injection vulnerability and arbitrary code execution. Git.USE_SHELL is deprecated " + "and will be removed in a future release." ) @@ -413,6 +413,13 @@ def __setstate__(self, d: Dict[str, Any]) -> None: considered and accounted for, which is not possible under most circumstances. As detailed below, it is also no longer needed, even where it had been in the past. + It is in many if not most cases a command injection vulnerability for an application + to set :attr:`USE_SHELL` to ``True``. Any attacker who can cause a specially crafted + fragment of text to make its way into any part of any argument to any git command + (including paths, branch names, etc.) can cause the shell to read and write + arbitrary files and execute arbitrary commands. Innocent input may also accidentally + contain special shell syntax, leading to inadvertent malfunctions. + In addition, how a value of ``True`` interacts with some aspects of GitPython's operation is not precisely specified and may change without warning, even before GitPython 4.0.0 when :attr:`USE_SHELL` may be removed. This includes: From 8327b458bdcf73895aa3b0a9a990a61ce2e54ee9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 16:39:20 -0400 Subject: [PATCH 88/89] Test GitMeta alias --- test/deprecation/test_cmd_git.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/test/deprecation/test_cmd_git.py b/test/deprecation/test_cmd_git.py index 6c592454a..e44490273 100644 --- a/test/deprecation/test_cmd_git.py +++ b/test/deprecation/test_cmd_git.py @@ -69,7 +69,7 @@ import pytest from pytest import WarningsRecorder -from git.cmd import Git +from git.cmd import Git, GitMeta from .lib import assert_no_deprecation_warning, suppress_deprecation_warning @@ -377,3 +377,15 @@ def test_instance_dir() -> None: instance = Git() actual = set(dir(instance)) assert _EXPECTED_DIR_SUBSET <= actual + + +def test_metaclass_alias() -> None: + """GitMeta aliases Git's metaclass, whether that is type or a custom metaclass.""" + + def accept_metaclass_instance(cls: GitMeta) -> None: + """Check that cls is statically recognizable as an instance of GitMeta.""" + + accept_metaclass_instance(Git) # assert_type would expect Type[Git], not GitMeta. + + # This comes after the static check, just in case it would affect the inference. + assert type(Git) is GitMeta From f6060df576acda613227a57f03c01d235eceaeae Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 27 Mar 2024 17:38:31 -0400 Subject: [PATCH 89/89] Add GitMeta alias Hopefully no one will ever need it. --- git/cmd.py | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/git/cmd.py b/git/cmd.py index b2829801f..90fc39cd6 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -5,7 +5,7 @@ from __future__ import annotations -__all__ = ["Git"] +__all__ = ["GitMeta", "Git"] import contextlib import io @@ -354,6 +354,32 @@ def __setattr(cls, name: str, value: Any) -> Any: __setattr__ = __setattr +GitMeta = _GitMeta +"""Alias of :class:`Git`'s metaclass, whether it is :class:`type` or a custom metaclass. + +Whether the :class:`Git` class has the default :class:`type` as its metaclass or uses a +custom metaclass is not documented and may change at any time. This statically checkable +metaclass alias is equivalent at runtime to ``type(Git)``. This should almost never be +used. Code that benefits from it is likely to be remain brittle even if it is used. + +In view of the :class:`Git` class's intended use and :class:`Git` objects' dynamic +callable attributes representing git subcommands, it rarely makes sense to inherit from +:class:`Git` at all. Using :class:`Git` in multiple inheritance can be especially tricky +to do correctly. Attempting uses of :class:`Git` where its metaclass is relevant, such +as when a sibling class has an unrelated metaclass and a shared lower bound metaclass +might have to be introduced to solve a metaclass conflict, is not recommended. + +:note: + The correct static type of the :class:`Git` class itself, and any subclasses, is + ``Type[Git]``. (This can be written as ``type[Git]`` in Python 3.9 later.) + + :class:`GitMeta` should never be used in any annotation where ``Type[Git]`` is + intended or otherwise possible to use. This alias is truly only for very rare and + inherently precarious situations where it is necessary to deal with the metaclass + explicitly. +""" + + class Git(metaclass=_GitMeta): """The Git class manages communication with the Git binary.