Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate Git.USE_SHELL #1787

Merged
merged 2 commits into from
Dec 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions git/cmd.py
Original file line number Diff line number Diff line change
Expand Up @@ -280,20 +280,20 @@ def __setstate__(self, d: Dict[str, Any]) -> None:
"""Enables debugging of GitPython's git commands."""

USE_SHELL = False
"""If True, a shell will be used when executing git commands.
"""Deprecated. If set to True, a shell will be used when executing git commands.

This exists to avoid breaking old code that may access it, but it is no longer
needed and should rarely if ever be used. Prior to GitPython 2.0.8, it 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.

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
"""
Expand Down Expand Up @@ -919,7 +919,15 @@ def execute(

:param shell:
Whether to invoke commands through a shell (see `Popen(..., shell=True)`).
It overrides :attr:`USE_SHELL` if it is not `None`.
If this is not `None`, it overrides :attr:`USE_SHELL`.

Passing ``shell=True`` to this or any other GitPython function should be
avoided, as it is unsafe under most circumstances. This is because it is
typically not feasible to fully consider and account for the effect of shell
expansions, especially when passing ``shell=True`` to other methods that
forward it to :meth:`Git.execute`. Passing ``shell=True`` is also no longer
needed (nor useful) to work around any known operating system specific
issues.

:param env:
A dictionary of environment variables to be passed to :class:`subprocess.Popen`.
Expand Down
37 changes: 30 additions & 7 deletions git/compat.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,41 @@
# ---------------------------------------------------------------------------


# DEPRECATED attributes providing shortcuts to operating system checks based on os.name.
#
# These are deprecated because it is clearer, and helps avoid bugs, to write out the
# os.name or sys.platform checks explicitly, especially in cases where it matters which
# is used. For example, is_win is False on Cygwin, but is often assumed True. To detect
# Cygwin, use sys.platform == "cygwin". (Also, in the past, is_darwin was unreliable.)
#
is_win = os.name == "nt"
"""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
:attr:`sys.platform` checks explicitly, especially in cases where it matters which is
used.

:note: ``is_win`` is ``False`` on Cygwin, but is often wrongly assumed ``True``. To
detect Cygwin, use ``sys.platform == "cygwin"``.
"""

is_posix = os.name == "posix"
"""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
:attr:`sys.platform` checks explicitly, especially in cases where it matters which is
used.

:note: For POSIX systems, more detailed information is available in
:attr:`sys.platform`, while :attr:`os.name` is always ``"posix"`` on such systems,
including macOS (Darwin).
"""

is_darwin = sys.platform == "darwin"
"""Deprecated alias for ``sys.platform == "darwin"`` to check for macOS (Darwin).

This is deprecated because it clearer to write out :attr:`os.name` or
:attr:`sys.platform` checks explicitly.

:note: For macOS (Darwin), ``os.name == "posix"`` as in other Unix-like systems, while
``sys.platform == "darwin"`.
"""

defenc = sys.getfilesystemencoding()
"""The encoding used to convert between Unicode and bytes filenames."""


@overload
Expand Down
15 changes: 9 additions & 6 deletions git/diff.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/

import re

from git.cmd import handle_process_output
from git.compat import defenc
from git.util import finalize_process, hex_to_bin
Expand Down Expand Up @@ -208,13 +209,15 @@ class DiffIndex(List[T_Diff]):
The class improves the diff handling convenience.
"""

# Change type invariant identifying possible ways a blob can have changed:
# A = Added
# D = Deleted
# R = Renamed
# M = Modified
# T = Changed in the type
change_type = ("A", "C", "D", "R", "M", "T")
"""Change type invariant identifying possible ways a blob can have changed:

* ``A`` = Added
* ``D`` = Deleted
* ``R`` = Renamed
* ``M`` = Modified
* ``T`` = Changed in the type
"""

def iter_change_type(self, change_type: Lit_change_type) -> Iterator[T_Diff]:
"""
Expand Down
4 changes: 2 additions & 2 deletions git/objects/commit.py
Original file line number Diff line number Diff line change
Expand Up @@ -352,8 +352,8 @@ def stats(self) -> Stats:
def trailers(self) -> Dict[str, str]:
"""Get the trailers of the message as a dictionary

:note: This property is deprecated, please use either ``Commit.trailers_list``
or ``Commit.trailers_dict``.
:note: This property is deprecated, please use either :attr:`trailers_list` or
:attr:`trailers_dict``.

:return:
Dictionary containing whitespace stripped trailer information. Only contains
Expand Down
98 changes: 54 additions & 44 deletions git/objects/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ class TraverseNT(NamedTuple):
T_TIobj = TypeVar("T_TIobj", bound="TraversableIterableObj") # For TraversableIterableObj.traverse()

TraversedTup = Union[
Tuple[Union["Traversable", None], "Traversable"], # For commit, submodule
"TraversedTreeTup",
] # for tree.traverse()
Tuple[Union["Traversable", None], "Traversable"], # For Commit, Submodule.
"TraversedTreeTup", # For Tree.traverse().
]

# --------------------------------------------------------------------

Expand Down Expand Up @@ -380,11 +380,15 @@ class Tree:: (cls, Tree) -> Tuple[Tree, ...]

@abstractmethod
def list_traverse(self, *args: Any, **kwargs: Any) -> Any:
""" """
"""Traverse self and collect all items found.

Calling this directly only the abstract base class, including via a ``super()``
proxy, is deprecated. Only overridden implementations should be called.
"""
warnings.warn(
"list_traverse() method should only be called from subclasses."
"Calling from Traversable abstract class will raise NotImplementedError in 3.1.20"
"Builtin sublclasses are 'Submodule', 'Tree' and 'Commit",
" Calling from Traversable abstract class will raise NotImplementedError in 4.0.0."
" The concrete subclasses in GitPython itself are 'Commit', 'RootModule', 'Submodule', and 'Tree'.",
DeprecationWarning,
stacklevel=2,
)
Expand All @@ -393,12 +397,14 @@ def list_traverse(self, *args: Any, **kwargs: Any) -> Any:
def _list_traverse(
self, as_edge: bool = False, *args: Any, **kwargs: Any
) -> IterableList[Union["Commit", "Submodule", "Tree", "Blob"]]:
"""
"""Traverse self and collect all items found.

:return: IterableList with the results of the traversal as produced by
traverse()
Commit -> IterableList['Commit']
Submodule -> IterableList['Submodule']
Tree -> IterableList[Union['Submodule', 'Tree', 'Blob']]
:meth:`traverse`::

Commit -> IterableList['Commit']
Submodule -> IterableList['Submodule']
Tree -> IterableList[Union['Submodule', 'Tree', 'Blob']]
"""
# Commit and Submodule have id.__attribute__ as IterableObj.
# Tree has id.__attribute__ inherited from IndexObject.
Expand All @@ -421,11 +427,15 @@ def _list_traverse(

@abstractmethod
def traverse(self, *args: Any, **kwargs: Any) -> Any:
""" """
"""Iterator yielding items found when traversing self.

Calling this directly on the abstract base class, including via a ``super()``
proxy, is deprecated. Only overridden implementations should be called.
"""
warnings.warn(
"traverse() method should only be called from subclasses."
"Calling from Traversable abstract class will raise NotImplementedError in 3.1.20"
"Builtin sublclasses are 'Submodule', 'Tree' and 'Commit",
" Calling from Traversable abstract class will raise NotImplementedError in 4.0.0."
" The concrete subclasses in GitPython itself are 'Commit', 'RootModule', 'Submodule', and 'Tree'.",
DeprecationWarning,
stacklevel=2,
)
Expand All @@ -441,7 +451,7 @@ def _traverse(
ignore_self: int = 1,
as_edge: bool = False,
) -> Union[Iterator[Union["Traversable", "Blob"]], Iterator[TraversedTup]]:
""":return: Iterator yielding items found when traversing self
"""Iterator yielding items found when traversing self.

:param predicate: f(i,d) returns False if item i at depth d should not be
included in the result.
Expand Down Expand Up @@ -471,18 +481,18 @@ def _traverse(
if True, return a pair of items, first being the source, second the
destination, i.e. tuple(src, dest) with the edge spanning from source to
destination
"""

"""
Commit -> Iterator[Union[Commit, Tuple[Commit, Commit]]
Submodule -> Iterator[Submodule, Tuple[Submodule, Submodule]]
Tree -> Iterator[Union[Blob, Tree, Submodule,
Tuple[Union[Submodule, Tree], Union[Blob, Tree, Submodule]]]

ignore_self=True is_edge=True -> Iterator[item]
ignore_self=True is_edge=False --> Iterator[item]
ignore_self=False is_edge=True -> Iterator[item] | Iterator[Tuple[src, item]]
ignore_self=False is_edge=False -> Iterator[Tuple[src, item]]
:return: Iterator yielding items found when traversing self::

Commit -> Iterator[Union[Commit, Tuple[Commit, Commit]]
Submodule -> Iterator[Submodule, Tuple[Submodule, Submodule]]
Tree -> Iterator[Union[Blob, Tree, Submodule,
Tuple[Union[Submodule, Tree], Union[Blob, Tree, Submodule]]]

ignore_self=True is_edge=True -> Iterator[item]
ignore_self=True is_edge=False --> Iterator[item]
ignore_self=False is_edge=True -> Iterator[item] | Iterator[Tuple[src, item]]
ignore_self=False is_edge=False -> Iterator[Tuple[src, item]]
"""

visited = set()
Expand Down Expand Up @@ -547,7 +557,7 @@ class Serializable(Protocol):
def _serialize(self, stream: "BytesIO") -> "Serializable":
"""Serialize the data of this object into the given data stream.

:note: A serialized object would ``_deserialize`` into the same object.
:note: A serialized object would :meth:`_deserialize` into the same object.

:param stream: a file-like object

Expand Down Expand Up @@ -627,24 +637,24 @@ def traverse(
ignore_self: int = 1,
as_edge: bool = False,
) -> Union[Iterator[T_TIobj], Iterator[Tuple[T_TIobj, T_TIobj]], Iterator[TIobj_tuple]]:
"""For documentation, see util.Traversable._traverse()"""
"""For documentation, see :meth:`Traversable._traverse`."""

## To typecheck instead of using cast:
#
# import itertools
# from git.types import TypeGuard
# def is_commit_traversed(inp: Tuple) -> TypeGuard[Tuple[Iterator[Tuple['Commit', 'Commit']]]]:
# for x in inp[1]:
# if not isinstance(x, tuple) and len(x) != 2:
# if all(isinstance(inner, Commit) for inner in x):
# continue
# return True
#
# ret = super(Commit, self).traverse(predicate, prune, depth, branch_first, visit_once, ignore_self, as_edge)
# ret_tup = itertools.tee(ret, 2)
# assert is_commit_traversed(ret_tup), f"{[type(x) for x in list(ret_tup[0])]}"
# return ret_tup[0]

"""
# To typecheck instead of using cast.
import itertools
from git.types import TypeGuard
def is_commit_traversed(inp: Tuple) -> TypeGuard[Tuple[Iterator[Tuple['Commit', 'Commit']]]]:
for x in inp[1]:
if not isinstance(x, tuple) and len(x) != 2:
if all(isinstance(inner, Commit) for inner in x):
continue
return True

ret = super(Commit, self).traverse(predicate, prune, depth, branch_first, visit_once, ignore_self, as_edge)
ret_tup = itertools.tee(ret, 2)
assert is_commit_traversed(ret_tup), f"{[type(x) for x in list(ret_tup[0])]}"
return ret_tup[0]
"""
return cast(
Union[Iterator[T_TIobj], Iterator[Tuple[Union[None, T_TIobj], T_TIobj]]],
super()._traverse(predicate, prune, depth, branch_first, visit_once, ignore_self, as_edge), # type: ignore
Expand Down
8 changes: 4 additions & 4 deletions git/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -1233,10 +1233,10 @@ def __init__(cls, name: str, bases: Tuple, clsdict: Dict) -> None:
for base in bases:
if type(base) is IterableClassWatcher:
warnings.warn(
f"GitPython Iterable subclassed by {name}. "
"Iterable is deprecated due to naming clash since v3.1.18"
" and will be removed in 3.1.20, "
"Use IterableObj instead \n",
f"GitPython Iterable subclassed by {name}."
" Iterable is deprecated due to naming clash since v3.1.18"
" and will be removed in 4.0.0."
EliahKagan marked this conversation as resolved.
Show resolved Hide resolved
" Use IterableObj instead.",
DeprecationWarning,
stacklevel=2,
)
Expand Down
Loading