Commit 04eb09c
committed
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).1 parent 05de5c0 commit 04eb09c
1 file changed
Lines changed: 15 additions & 18 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -55,7 +55,6 @@ | |||
| 55 | 55 | TYPE_CHECKING, | |
| 56 | 56 | TextIO, | |
| 57 | 57 | Tuple, | |
| 58 | - Type, | ||
| 59 | 58 | Union, | |
| 60 | 59 | cast, | |
| 61 | 60 | overload, | |
@@ -336,15 +335,15 @@ class _GitMeta(type): | |||
| 336 | 335 | This helps issue :class:`DeprecationWarning` if :attr:`Git.USE_SHELL` is used. | |
| 337 | 336 | """ | |
| 338 | 337 | ||
| 339 | - @property | ||
| 340 | - def USE_SHELL(cls: Type[Git]) -> bool: | ||
| 341 | - _warn_use_shell(False) | ||
| 342 | - return cls._USE_SHELL | ||
| 338 | + def __getattribute__(cls, name: str) -> Any: | ||
| 339 | + if name == "USE_SHELL": | ||
| 340 | + _warn_use_shell(False) | ||
| 341 | + return super().__getattribute__(name) | ||
| 343 | 342 | ||
| 344 | - @USE_SHELL.setter | ||
| 345 | - def USE_SHELL(cls: Type[Git], value: bool) -> None: | ||
| 346 | - _warn_use_shell(value) | ||
| 347 | - cls._USE_SHELL = value | ||
| 343 | + def __setattr__(cls, name: str, value: Any) -> Any: | ||
| 344 | + if name == "USE_SHELL": | ||
| 345 | + _warn_use_shell(value) | ||
| 346 | + super().__setattr__(name, value) | ||
| 348 | 347 | ||
| 349 | 348 | ||
| 350 | 349 | class Git(metaclass=_GitMeta): | |
@@ -397,9 +396,7 @@ def __setstate__(self, d: Dict[str, Any]) -> None: | |||
| 397 | 396 | GIT_PYTHON_TRACE = os.environ.get("GIT_PYTHON_TRACE", False) | |
| 398 | 397 | """Enables debugging of GitPython's git commands.""" | |
| 399 | 398 | ||
| 400 | - _USE_SHELL: bool = False | ||
| 401 | - | ||
| 402 | - USE_SHELL: bool | ||
| 399 | + USE_SHELL: bool = False | ||
| 403 | 400 | """Deprecated. If set to ``True``, a shell will be used to execute git commands. | |
| 404 | 401 | ||
| 405 | 402 | 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: | |||
| 909 | 906 | self.cat_file_header: Union[None, TBD] = None | |
| 910 | 907 | self.cat_file_all: Union[None, TBD] = None | |
| 911 | 908 | ||
| 909 | + def __getattribute__(self, name: str) -> Any: | ||
| 910 | + if name == "USE_SHELL": | ||
| 911 | + _warn_use_shell(False) | ||
| 912 | + return super().__getattribute__(name) | ||
| 913 | + | ||
| 912 | 914 | def __getattr__(self, name: str) -> Any: | |
| 913 | 915 | """A convenience method as it allows to call the command as if it was an object. | |
| 914 | 916 | ||
@@ -918,11 +920,6 @@ def __getattr__(self, name: str) -> Any: | |||
| 918 | 920 | """ | |
| 919 | 921 | if name.startswith("_"): | |
| 920 | 922 | return super().__getattribute__(name) | |
| 921 | - | ||
| 922 | - if name == "USE_SHELL": | ||
| 923 | - _warn_use_shell(False) | ||
| 924 | - return self._USE_SHELL | ||
| 925 | - | ||
| 926 | 923 | return lambda *args, **kwargs: self._call_process(name, *args, **kwargs) | |
| 927 | 924 | ||
| 928 | 925 | def set_persistent_git_options(self, **kwargs: Any) -> None: | |
@@ -1184,7 +1181,7 @@ def execute( | |||
| 1184 | 1181 | ||
| 1185 | 1182 | stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb") | |
| 1186 | 1183 | if shell is None: | |
| 1187 | - shell = self._USE_SHELL | ||
| 1184 | + shell = self.USE_SHELL | ||
| 1188 | 1185 | _logger.debug( | |
| 1189 | 1186 | "Popen(%s, cwd=%s, stdin=%s, shell=%s, universal_newlines=%s)", | |
| 1190 | 1187 | redacted_command, | |
0 commit comments