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/git/__init__.py b/git/__init__.py index a13030456..1b2360e3a 100644 --- a/git/__init__.py +++ b/git/__init__.py @@ -88,7 +88,12 @@ __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 + +import warnings from gitdb.util import to_hex_sha @@ -144,11 +149,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 +169,9 @@ 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 + # 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 Actor, @@ -196,7 +184,79 @@ except GitError as _exc: raise ImportError("%s: %s" % (_exc.__class__.__name__, _exc)) from _exc + +def _warned_import(message: str, fullname: str) -> "ModuleType": + import importlib + + 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: + # 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 + GIT_OK = None @@ -232,12 +292,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/cmd.py b/git/cmd.py index 03e5d7ffc..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 @@ -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 ( @@ -307,8 +308,79 @@ 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, 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." +) + + +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. + """ + + 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: + 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. (The original + # names invoke name mangling (leading "__") to avoid confusion in other scopes.) + __getattribute__ = __getattribute + __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. -class Git: +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. It provides a convenient interface to calling the Git binary, such as in:: @@ -358,24 +430,53 @@ 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 + USE_SHELL: bool = False """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. + + 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: + + * 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" @@ -868,6 +969,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. @@ -1138,7 +1244,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, diff --git a/git/compat.py b/git/compat.py index 4ede8c985..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 @@ -23,7 +24,9 @@ AnyStr, Dict, # noqa: F401 IO, # noqa: F401 + List, Optional, + TYPE_CHECKING, Tuple, # noqa: F401 Type, # noqa: F401 Union, @@ -33,7 +36,37 @@ # --------------------------------------------------------------------------- -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 + + 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 +78,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 +91,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/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 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 diff --git a/git/types.py b/git/types.py index a93ebdb4f..584450146 100644 --- a/git/types.py +++ b/git/types.py @@ -7,14 +7,17 @@ Any, Callable, Dict, + List, NoReturn, Optional, Sequence as Sequence, Tuple, TYPE_CHECKING, + Type, TypeVar, Union, ) +import warnings if sys.version_info >= (3, 8): from typing import ( @@ -127,21 +130,50 @@ 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": + 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. + __getattr__ = _getattr + + +def __dir__() -> List[str]: + return [*globals(), "Lit_commit_ish"] + + # Config_levels --------------------------------------------------------- Lit_config_levels = Literal["system", "global", "user", "repository"] @@ -188,12 +220,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 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: diff --git a/pyproject.toml b/pyproject.toml index 1dc1e6aed..6cb05f96e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -21,10 +21,11 @@ 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 -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-requirements.txt b/test-requirements.txt index e1f5e2ed4..75e9e81fa 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -8,3 +8,4 @@ pytest-cov pytest-instafail pytest-mock pytest-sugar +typing-extensions ; python_version < "3.11" diff --git a/test/deprecation/__init__.py b/test/deprecation/__init__.py new file mode 100644 index 000000000..fec3126d2 --- /dev/null +++ b/test/deprecation/__init__.py @@ -0,0 +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/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 new file mode 100644 index 000000000..6235a836c --- /dev/null +++ b/test/deprecation/test_basic.py @@ -0,0 +1,124 @@ +# 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 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 pytest + +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 + +from .lib import assert_no_deprecation_warning + +# 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 + +# ------------------------------------------------------------------------ + + +@pytest.fixture +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) + repo.index.add(["a.txt"]) + yield repo.index.commit("Initial commit") + repo.close() + + +@pytest.fixture +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: "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: "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: "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: "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: "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: "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: "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: "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: "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() -> None: + """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() -> None: + """Subclassing git.util.IterableObj is not deprecated, does not warn.""" + 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 new file mode 100644 index 000000000..e44490273 --- /dev/null +++ b/test/deprecation/test_cmd_git.py @@ -0,0 +1,391 @@ +# 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 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, +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 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, 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 + 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. + +A note on metaclass conflicts +============================= + +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 +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 logging +import sys +from typing import Generator +import unittest.mock + +if sys.version_info >= (3, 11): + from typing import assert_type +else: + from typing_extensions import assert_type + +import pytest +from pytest import WarningsRecorder + +from git.cmd import Git, GitMeta + +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.""" + +_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__) + + +@pytest.fixture +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 + 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: + + * 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_private_value = Git._USE_SHELL # type: ignore[attr-defined] + except AttributeError: + 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 + + # 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 + + # Try to restore the original public value. + with suppress_deprecation_warning(): + Git.USE_SHELL = old_public_value + finally: + # Try to restore the original private state. + if separate_backing_attribute: + 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) + + +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 + + +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 + + # 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 + 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) + + +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 + 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(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, + 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 # type: ignore[misc] # Name not in __slots__. + + +@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 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 + 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("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 + + +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 assert_no_deprecation_warning(): + Git().version() + + +_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 instances rather than the class itself. + """ + 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 diff --git a/test/deprecation/test_compat.py b/test/deprecation/test_compat.py new file mode 100644 index 000000000..2d7805e61 --- /dev/null +++ b/test/deprecation/test_compat.py @@ -0,0 +1,84 @@ +# 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 +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 + +if sys.version_info >= (3, 11): + from typing import assert_type +else: + from typing_extensions import assert_type + +import pytest + +import git.compat + +_MESSAGE_LEADER = "{} and other is_ aliases are deprecated." +"""Form taken by the beginning of the warnings issued for is_ access.""" + + +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_platform() -> None: + """The is_ attributes 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: + is_win = git.compat.is_win + 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 + + for fullname, message in zip(fully_qualified_names, messages): + assert message.startswith(_MESSAGE_LEADER.format(fullname)) + + # 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") + + +def test_dir() -> None: + """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", + } + actual = set(dir(git.compat)) + assert expected_subset <= actual diff --git a/test/deprecation/test_toplevel.py b/test/deprecation/test_toplevel.py new file mode 100644 index 000000000..740408193 --- /dev/null +++ b/test/deprecation/test_toplevel.py @@ -0,0 +1,233 @@ +# 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 +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. +""" + +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 + +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: + """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 a dynamic and static error.""" + with pytest.raises(ImportError): + from git import foo # type: ignore[attr-defined] # noqa: F401 + + +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 + + # 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 + + # 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 + + assert_type(util.TemporaryFileSwap, Type[TemporaryFileSwap]) + + # This comes after the static assertion, just in case it would affect the inference. + assert util.TemporaryFileSwap is TemporaryFileSwap + + +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: + from git import util + + # 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, "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 + + assert_type(util.TemporaryFileSwap, Type[TemporaryFileSwap]) + + # This comes after the static assertion, just in case it would affect the inference. + assert util.TemporaryFileSwap is TemporaryFileSwap + + +_PRIVATE_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, +) +"""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: + 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] + ) == _PRIVATE_MODULE_ALIAS_TARGETS + + # Each should have warned exactly once, and note what to use instead. + messages = [str(w.message) for w in ctx] + + assert len(messages) == len(_PRIVATE_MODULE_ALIAS_TARGETS) + + 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: + """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 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] + + assert ( + head, + log, + reference, + symbolic, + tag, + base, + fun, + typ, + ) == _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 + # 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 itertools.groupby(messages_with_duplicates)] + + assert len(messages) == len(_PRIVATE_MODULE_ALIAS_TARGETS) + + 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: + """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." diff --git a/test/deprecation/test_types.py b/test/deprecation/test_types.py new file mode 100644 index 000000000..f97375a85 --- /dev/null +++ b/test/deprecation/test_types.py @@ -0,0 +1,69 @@ +# 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 + +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_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 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"] + + +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 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 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() 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]