Skip to content

Commit 91f63cd

Browse files
authored
Merge pull request #1688 from EliahKagan/execute-args
Clarify Git.execute and Popen arguments
2 parents 881b464 + ab95886 commit 91f63cd

File tree

2 files changed

+91
-74
lines changed

2 files changed

+91
-74
lines changed

git/cmd.py

+63-64
Original file line numberDiff line numberDiff line change
@@ -66,10 +66,10 @@
6666
"with_extended_output",
6767
"with_exceptions",
6868
"as_process",
69-
"stdout_as_string",
7069
"output_stream",
71-
"with_stdout",
70+
"stdout_as_string",
7271
"kill_after_timeout",
72+
"with_stdout",
7373
"universal_newlines",
7474
"shell",
7575
"env",
@@ -105,7 +105,7 @@ def handle_process_output(
105105
) -> None:
106106
"""Registers for notifications to learn that process output is ready to read, and dispatches lines to
107107
the respective line handlers.
108-
This function returns once the finalizer returns
108+
This function returns once the finalizer returns.
109109
110110
:return: result of finalizer
111111
:param process: subprocess.Popen instance
@@ -294,9 +294,7 @@ def __setstate__(self, d: Dict[str, Any]) -> None:
294294

295295
@classmethod
296296
def refresh(cls, path: Union[None, PathLike] = None) -> bool:
297-
"""This gets called by the refresh function (see the top level
298-
__init__).
299-
"""
297+
"""This gets called by the refresh function (see the top level __init__)."""
300298
# discern which path to refresh with
301299
if path is not None:
302300
new_git = os.path.expanduser(path)
@@ -446,9 +444,9 @@ def polish_url(cls, url: str, is_cygwin: Union[None, bool] = None) -> PathLike:
446444
if is_cygwin:
447445
url = cygpath(url)
448446
else:
449-
"""Remove any backslahes from urls to be written in config files.
447+
"""Remove any backslashes from urls to be written in config files.
450448
451-
Windows might create config-files containing paths with backslashed,
449+
Windows might create config files containing paths with backslashes,
452450
but git stops liking them as it will escape the backslashes.
453451
Hence we undo the escaping just to be sure.
454452
"""
@@ -464,8 +462,8 @@ def check_unsafe_protocols(cls, url: str) -> None:
464462
Check for unsafe protocols.
465463
466464
Apart from the usual protocols (http, git, ssh),
467-
Git allows "remote helpers" that have the form `<transport>::<address>`,
468-
one of these helpers (`ext::`) can be used to invoke any arbitrary command.
465+
Git allows "remote helpers" that have the form ``<transport>::<address>``,
466+
one of these helpers (``ext::``) can be used to invoke any arbitrary command.
469467
470468
See:
471469
@@ -517,7 +515,7 @@ def __init__(self, proc: Union[None, subprocess.Popen], args: Any) -> None:
517515
self.status: Union[int, None] = None
518516

519517
def _terminate(self) -> None:
520-
"""Terminate the underlying process"""
518+
"""Terminate the underlying process."""
521519
if self.proc is None:
522520
return
523521

@@ -572,7 +570,7 @@ def wait(self, stderr: Union[None, str, bytes] = b"") -> int:
572570
"""Wait for the process and return its status code.
573571
574572
:param stderr: Previously read value of stderr, in case stderr is already closed.
575-
:warn: may deadlock if output or error pipes are used and not handled separately.
573+
:warn: May deadlock if output or error pipes are used and not handled separately.
576574
:raise GitCommandError: if the return status is not 0"""
577575
if stderr is None:
578576
stderr_b = b""
@@ -605,13 +603,12 @@ def read_all_from_possibly_closed_stream(stream: Union[IO[bytes], None]) -> byte
605603
# END auto interrupt
606604

607605
class CatFileContentStream(object):
608-
609606
"""Object representing a sized read-only stream returning the contents of
610607
an object.
611608
It behaves like a stream, but counts the data read and simulates an empty
612609
stream once our sized content region is empty.
613-
If not all data is read to the end of the objects's lifetime, we read the
614-
rest to assure the underlying stream continues to work"""
610+
If not all data is read to the end of the object's lifetime, we read the
611+
rest to assure the underlying stream continues to work."""
615612

616613
__slots__: Tuple[str, ...] = ("_stream", "_nbr", "_size")
617614

@@ -740,11 +737,11 @@ def __getattr__(self, name: str) -> Any:
740737

741738
def set_persistent_git_options(self, **kwargs: Any) -> None:
742739
"""Specify command line options to the git executable
743-
for subsequent subcommand calls
740+
for subsequent subcommand calls.
744741
745742
:param kwargs:
746743
is a dict of keyword arguments.
747-
these arguments are passed as in _call_process
744+
These arguments are passed as in _call_process
748745
but will be passed to the git command rather than
749746
the subcommand.
750747
"""
@@ -775,7 +772,7 @@ def version_info(self) -> Tuple[int, int, int, int]:
775772
"""
776773
:return: tuple(int, int, int, int) tuple with integers representing the major, minor
777774
and additional version numbers as parsed from git version.
778-
This value is generated on demand and is cached"""
775+
This value is generated on demand and is cached."""
779776
return self._version_info
780777

781778
@overload
@@ -842,16 +839,16 @@ def execute(
842839
strip_newline_in_stdout: bool = True,
843840
**subprocess_kwargs: Any,
844841
) -> Union[str, bytes, Tuple[int, Union[str, bytes], str], AutoInterrupt]:
845-
"""Handles executing the command on the shell and consumes and returns
846-
the returned information (stdout)
842+
"""Handles executing the command and consumes and returns the returned
843+
information (stdout).
847844
848845
:param command:
849846
The command argument list to execute.
850-
It should be a string, or a sequence of program arguments. The
847+
It should be a sequence of program arguments, or a string. The
851848
program to execute is the first item in the args sequence or string.
852849
853850
:param istream:
854-
Standard input filehandle passed to subprocess.Popen.
851+
Standard input filehandle passed to `subprocess.Popen`.
855852
856853
:param with_extended_output:
857854
Whether to return a (status, stdout, stderr) tuple.
@@ -862,8 +859,7 @@ def execute(
862859
:param as_process:
863860
Whether to return the created process instance directly from which
864861
streams can be read on demand. This will render with_extended_output and
865-
with_exceptions ineffective - the caller will have
866-
to deal with the details himself.
862+
with_exceptions ineffective - the caller will have to deal with the details.
867863
It is important to note that the process will be placed into an AutoInterrupt
868864
wrapper that will interrupt the process once it goes out of scope. If you
869865
use the command in iterators, you should pass the whole process instance
@@ -876,13 +872,34 @@ def execute(
876872
always be created with a pipe due to issues with subprocess.
877873
This merely is a workaround as data will be copied from the
878874
output pipe to the given output stream directly.
879-
Judging from the implementation, you shouldn't use this flag !
875+
Judging from the implementation, you shouldn't use this parameter!
880876
881877
:param stdout_as_string:
882-
if False, the commands standard output will be bytes. Otherwise, it will be
883-
decoded into a string using the default encoding (usually utf-8).
878+
If False, the command's standard output will be bytes. Otherwise, it will be
879+
decoded into a string using the default encoding (usually UTF-8).
884880
The latter can fail, if the output contains binary data.
885881
882+
:param kill_after_timeout:
883+
Specifies a timeout in seconds for the git command, after which the process
884+
should be killed. This will have no effect if as_process is set to True. It is
885+
set to None by default and will let the process run until the timeout is
886+
explicitly specified. This feature is not supported on Windows. It's also worth
887+
noting that kill_after_timeout uses SIGKILL, which can have negative side
888+
effects on a repository. For example, stale locks in case of ``git gc`` could
889+
render the repository incapable of accepting changes until the lock is manually
890+
removed.
891+
892+
:param with_stdout:
893+
If True, default True, we open stdout on the created process.
894+
895+
:param universal_newlines:
896+
if True, pipes will be opened as text, and lines are split at
897+
all known line endings.
898+
899+
:param shell:
900+
Whether to invoke commands through a shell (see `Popen(..., shell=True)`).
901+
It overrides :attr:`USE_SHELL` if it is not `None`.
902+
886903
:param env:
887904
A dictionary of environment variables to be passed to `subprocess.Popen`.
888905
@@ -891,38 +908,23 @@ def execute(
891908
one invocation of write() method. If the given number is not positive then
892909
the default value is used.
893910
911+
:param strip_newline_in_stdout:
912+
Whether to strip the trailing ``\\n`` of the command stdout.
913+
894914
:param subprocess_kwargs:
895-
Keyword arguments to be passed to subprocess.Popen. Please note that
896-
some of the valid kwargs are already set by this method, the ones you
915+
Keyword arguments to be passed to `subprocess.Popen`. Please note that
916+
some of the valid kwargs are already set by this method; the ones you
897917
specify may not be the same ones.
898918
899-
:param with_stdout: If True, default True, we open stdout on the created process
900-
:param universal_newlines:
901-
if True, pipes will be opened as text, and lines are split at
902-
all known line endings.
903-
:param shell:
904-
Whether to invoke commands through a shell (see `Popen(..., shell=True)`).
905-
It overrides :attr:`USE_SHELL` if it is not `None`.
906-
:param kill_after_timeout:
907-
To specify a timeout in seconds for the git command, after which the process
908-
should be killed. This will have no effect if as_process is set to True. It is
909-
set to None by default and will let the process run until the timeout is
910-
explicitly specified. This feature is not supported on Windows. It's also worth
911-
noting that kill_after_timeout uses SIGKILL, which can have negative side
912-
effects on a repository. For example, stale locks in case of git gc could
913-
render the repository incapable of accepting changes until the lock is manually
914-
removed.
915-
:param strip_newline_in_stdout:
916-
Whether to strip the trailing ``\\n`` of the command stdout.
917919
:return:
918920
* str(output) if extended_output = False (Default)
919921
* tuple(int(status), str(stdout), str(stderr)) if extended_output = True
920922
921-
if output_stream is True, the stdout value will be your output stream:
923+
If output_stream is True, the stdout value will be your output stream:
922924
* output_stream if extended_output = False
923925
* tuple(int(status), output_stream, str(stderr)) if extended_output = True
924926
925-
Note git is executed with LC_MESSAGES="C" to ensure consistent
927+
Note that git is executed with ``LC_MESSAGES="C"`` to ensure consistent
926928
output regardless of system language.
927929
928930
:raise GitCommandError:
@@ -971,18 +973,15 @@ def execute(
971973
# end handle
972974

973975
stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb")
974-
istream_ok = "None"
975-
if istream:
976-
istream_ok = "<valid stream>"
977976
if shell is None:
978977
shell = self.USE_SHELL
979978
log.debug(
980-
"Popen(%s, cwd=%s, universal_newlines=%s, shell=%s, istream=%s)",
979+
"Popen(%s, cwd=%s, stdin=%s, shell=%s, universal_newlines=%s)",
981980
redacted_command,
982981
cwd,
983-
universal_newlines,
982+
"<valid stream>" if istream else "None",
984983
shell,
985-
istream_ok,
984+
universal_newlines,
986985
)
987986
try:
988987
with maybe_patch_caller_env:
@@ -1010,8 +1009,8 @@ def execute(
10101009
if as_process:
10111010
return self.AutoInterrupt(proc, command)
10121011

1013-
def _kill_process(pid: int) -> None:
1014-
"""Callback method to kill a process."""
1012+
def kill_process(pid: int) -> None:
1013+
"""Callback to kill a process."""
10151014
p = Popen(
10161015
["ps", "--ppid", str(pid)],
10171016
stdout=PIPE,
@@ -1044,7 +1043,7 @@ def _kill_process(pid: int) -> None:
10441043

10451044
if kill_after_timeout is not None:
10461045
kill_check = threading.Event()
1047-
watchdog = threading.Timer(kill_after_timeout, _kill_process, args=(proc.pid,))
1046+
watchdog = threading.Timer(kill_after_timeout, kill_process, args=(proc.pid,))
10481047

10491048
# Wait for the process to return
10501049
status = 0
@@ -1208,7 +1207,7 @@ def _unpack_args(cls, arg_list: Sequence[str]) -> List[str]:
12081207

12091208
def __call__(self, **kwargs: Any) -> "Git":
12101209
"""Specify command line options to the git executable
1211-
for a subcommand call
1210+
for a subcommand call.
12121211
12131212
:param kwargs:
12141213
is a dict of keyword arguments.
@@ -1246,7 +1245,7 @@ def _call_process(
12461245
self, method: str, *args: Any, **kwargs: Any
12471246
) -> Union[str, bytes, Tuple[int, Union[str, bytes], str], "Git.AutoInterrupt"]:
12481247
"""Run the given git command with the specified arguments and return
1249-
the result as a String
1248+
the result as a string.
12501249
12511250
:param method:
12521251
is the command. Contained "_" characters will be converted to dashes,
@@ -1255,7 +1254,7 @@ def _call_process(
12551254
:param args:
12561255
is the list of arguments. If None is included, it will be pruned.
12571256
This allows your commands to call git more conveniently as None
1258-
is realized as non-existent
1257+
is realized as non-existent.
12591258
12601259
:param kwargs:
12611260
It contains key-values for the following:
@@ -1385,7 +1384,7 @@ def get_object_header(self, ref: str) -> Tuple[str, str, int]:
13851384
return self.__get_object_header(cmd, ref)
13861385

13871386
def get_object_data(self, ref: str) -> Tuple[str, str, int, bytes]:
1388-
"""As get_object_header, but returns object data as well
1387+
"""As get_object_header, but returns object data as well.
13891388
13901389
:return: (hexsha, type_string, size_as_int, data_string)
13911390
:note: not threadsafe"""
@@ -1395,10 +1394,10 @@ def get_object_data(self, ref: str) -> Tuple[str, str, int, bytes]:
13951394
return (hexsha, typename, size, data)
13961395

13971396
def stream_object_data(self, ref: str) -> Tuple[str, str, int, "Git.CatFileContentStream"]:
1398-
"""As get_object_header, but returns the data as a stream
1397+
"""As get_object_header, but returns the data as a stream.
13991398
14001399
:return: (hexsha, type_string, size_as_int, stream)
1401-
:note: This method is not threadsafe, you need one independent Command instance per thread to be safe !"""
1400+
:note: This method is not threadsafe, you need one independent Command instance per thread to be safe!"""
14021401
cmd = self._get_persistent_cmd("cat_file_all", "cat_file", batch=True)
14031402
hexsha, typename, size = self.__get_object_header(cmd, ref)
14041403
cmd_stdout = cmd.stdout if cmd.stdout is not None else io.BytesIO()

test/test_git.py

+28-10
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
#
55
# This module is part of GitPython and is released under
66
# the BSD License: https://opensource.org/license/bsd-3-clause/
7-
import contextlib
7+
import inspect
88
import logging
99
import os
1010
import os.path as osp
@@ -40,6 +40,13 @@ def tearDown(self):
4040

4141
gc.collect()
4242

43+
def _assert_logged_for_popen(self, log_watcher, name, value):
44+
re_name = re.escape(name)
45+
re_value = re.escape(str(value))
46+
re_line = re.compile(fr"DEBUG:git.cmd:Popen\(.*\b{re_name}={re_value}[,)]")
47+
match_attempts = [re_line.match(message) for message in log_watcher.output]
48+
self.assertTrue(any(match_attempts), repr(log_watcher.output))
49+
4350
@mock.patch.object(Git, "execute")
4451
def test_call_process_calls_execute(self, git):
4552
git.return_value = ""
@@ -96,10 +103,8 @@ def _do_shell_combo(self, value_in_call, value_from_class):
96103
# git.cmd gets Popen via a "from" import, so patch it there.
97104
with mock.patch.object(cmd, "Popen", wraps=cmd.Popen) as mock_popen:
98105
# Use a command with no arguments (besides the program name), so it runs
99-
# with or without a shell, on all OSes, with the same effect. Since git
100-
# errors out when run with no arguments, we swallow that error.
101-
with contextlib.suppress(GitCommandError):
102-
self.git.execute(["git"], shell=value_in_call)
106+
# with or without a shell, on all OSes, with the same effect.
107+
self.git.execute(["git"], with_exceptions=False, shell=value_in_call)
103108

104109
return mock_popen
105110

@@ -115,14 +120,19 @@ def test_it_uses_shell_or_not_as_specified(self, case):
115120
def test_it_logs_if_it_uses_a_shell(self, case):
116121
"""``shell=`` in the log message agrees with what is passed to `Popen`."""
117122
value_in_call, value_from_class = case
118-
119123
with self.assertLogs(cmd.log, level=logging.DEBUG) as log_watcher:
120124
mock_popen = self._do_shell_combo(value_in_call, value_from_class)
125+
self._assert_logged_for_popen(log_watcher, "shell", mock_popen.call_args.kwargs["shell"])
121126

122-
popen_shell_arg = mock_popen.call_args.kwargs["shell"]
123-
expected_message = re.compile(rf"DEBUG:git.cmd:Popen\(.*\bshell={popen_shell_arg}\b.*\)")
124-
match_attempts = [expected_message.fullmatch(message) for message in log_watcher.output]
125-
self.assertTrue(any(match_attempts), repr(log_watcher.output))
127+
@ddt.data(
128+
("None", None),
129+
("<valid stream>", subprocess.PIPE),
130+
)
131+
def test_it_logs_istream_summary_for_stdin(self, case):
132+
expected_summary, istream_argument = case
133+
with self.assertLogs(cmd.log, level=logging.DEBUG) as log_watcher:
134+
self.git.execute(["git", "version"], istream=istream_argument)
135+
self._assert_logged_for_popen(log_watcher, "stdin", expected_summary)
126136

127137
def test_it_executes_git_and_returns_result(self):
128138
self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$")
@@ -364,3 +374,11 @@ def counter_stderr(line):
364374

365375
self.assertEqual(count[1], line_count)
366376
self.assertEqual(count[2], line_count)
377+
378+
def test_execute_kwargs_set_agrees_with_method(self):
379+
parameter_names = inspect.signature(cmd.Git.execute).parameters.keys()
380+
self_param, command_param, *most_params, extra_kwargs_param = parameter_names
381+
self.assertEqual(self_param, "self")
382+
self.assertEqual(command_param, "command")
383+
self.assertEqual(set(most_params), cmd.execute_kwargs) # Most important.
384+
self.assertEqual(extra_kwargs_param, "subprocess_kwargs")

0 commit comments

Comments
 (0)