From d966a0deabe3c8cf09ba3d1b0f54a29bdbdb4f1d Mon Sep 17 00:00:00 2001 From: Enji Cooper Date: Fri, 17 Apr 2026 01:00:40 -0700 Subject: [PATCH 1/3] git.cmd.Git.execute(..): fix `with_stdout=False` In the event the end-user called one of the APIs with `with_stdout=False`, i.e., they didn't want to capture stdout, the code would crash with an AttributeError or ValueError when trying to dereference the stdout/stderr streams attached to `Popen(..)` objects. Be more defensive by checking the streams first to make sure they're not `None` before trying to access their corresponding attributes. Add myself to AUTHORS and add corresponding regression tests for the change. Signed-off-by: Enji Cooper --- AUTHORS | 1 + git/cmd.py | 20 ++++++++++++-------- test/test_git.py | 20 ++++++++++++++++++++ 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/AUTHORS b/AUTHORS index b57113edd..15333e1e5 100644 --- a/AUTHORS +++ b/AUTHORS @@ -56,5 +56,6 @@ Contributors are: -Ethan Lin -Jonas Scharpf -Gordon Marx +-Enji Cooper Portions derived from other open source works and are clearly marked. diff --git a/git/cmd.py b/git/cmd.py index b529bcc10..d5fbc7736 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -1364,25 +1364,29 @@ def communicate() -> Tuple[AnyStr, AnyStr]: if output_stream is None: stdout_value, stderr_value = communicate() # Strip trailing "\n". - if stdout_value.endswith(newline) and strip_newline_in_stdout: # type: ignore[arg-type] + if stdout_value is not None and stdout_value.endswith(newline) and strip_newline_in_stdout: # type: ignore[arg-type] stdout_value = stdout_value[:-1] - if stderr_value.endswith(newline): # type: ignore[arg-type] + if stderr_value is not None and stderr_value.endswith(newline): # type: ignore[arg-type] stderr_value = stderr_value[:-1] status = proc.returncode else: max_chunk_size = max_chunk_size if max_chunk_size and max_chunk_size > 0 else io.DEFAULT_BUFFER_SIZE - stream_copy(proc.stdout, output_stream, max_chunk_size) - stdout_value = proc.stdout.read() - stderr_value = proc.stderr.read() + if proc.stdout is not None: + stream_copy(proc.stdout, output_stream, max_chunk_size) + stdout_value = proc.stdout.read() + if proc.stderr is not None: + stderr_value = proc.stderr.read() # Strip trailing "\n". - if stderr_value.endswith(newline): # type: ignore[arg-type] + if stderr_value is not None and stderr_value.endswith(newline): # type: ignore[arg-type] stderr_value = stderr_value[:-1] status = proc.wait() # END stdout handling finally: - proc.stdout.close() - proc.stderr.close() + if proc.stdout is not None: + proc.stdout.close() + if proc.stderr is not None: + proc.stderr.close() if self.GIT_PYTHON_TRACE == "full": cmdstr = " ".join(redacted_command) diff --git a/test/test_git.py b/test/test_git.py index 4a54d0d9b..da50fdfe8 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -6,6 +6,7 @@ import contextlib import gc import inspect +import io import logging import os import os.path as osp @@ -201,6 +202,25 @@ def test_it_logs_istream_summary_for_stdin(self, case): def test_it_executes_git_and_returns_result(self): self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$") + def test_it_output_stream_with_stdout_is_false(self): + temp_stream = io.BytesIO() + self.git.execute( + ["git", "version"], + output_stream=temp_stream, + with_stdout=False, + ) + self.assertEqual(temp_stream.tell(), 0) + + def test_it_executes_git_without_stdout_redirect(self): + returncode, stdout, stderr = self.git.execute( + ["git", "version"], + with_extended_output=True, + with_stdout=False, + ) + self.assertEqual(returncode, 0) + self.assertIsNone(stdout) + self.assertIsNotNone(stderr) + @ddt.data( # chdir_to_repo, shell, command, use_shell_impostor (False, False, ["git", "version"], False), From 6fc474265d863cbb9fbabdbfcc957f27cea2b5c4 Mon Sep 17 00:00:00 2001 From: Enji Cooper Date: Fri, 17 Apr 2026 09:40:43 -0700 Subject: [PATCH 2/3] test_avoids_changing...: don't leave test artifacts behind Prior to this the test would fail [silently] on my macOS host during the test and then pytest would complain loudly about it being an issue post-session (regardless of whether or not the test was being run). Squash the unwritable directory to mute noise complaints from pytest. Signed-off-by: Enji Cooper --- test/test_util.py | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/test/test_util.py b/test/test_util.py index 000830f41..e7453769a 100644 --- a/test/test_util.py +++ b/test/test_util.py @@ -113,7 +113,7 @@ def test_deletes_dir_with_readonly_files(self, tmp_path): sys.platform == "cygwin", reason="Cygwin can't set the permissions that make the test meaningful.", ) - def test_avoids_changing_permissions_outside_tree(self, tmp_path): + def test_avoids_changing_permissions_outside_tree(self, tmp_path, request): # Automatically works on Windows, but on Unix requires either special handling # or refraining from attempting to fix PermissionError by making chmod calls. @@ -125,9 +125,32 @@ def test_avoids_changing_permissions_outside_tree(self, tmp_path): dir2 = tmp_path / "dir2" dir2.mkdir() - (dir2 / "symlink").symlink_to(dir1 / "file") + symlink = dir2 / "symlink" + symlink.symlink_to(dir1 / "file") dir2.chmod(stat.S_IRUSR | stat.S_IXUSR) + def preen_dir2(): + """Don't leave unwritable directories behind. + + pytest has difficulties cleaning up after the fact on some platforms, + e.g., macOS, and whines incessantly until the issue is resolved--regardless + of the pytest session. + """ + rwx = stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR + if not dir2.exists(): + return + if symlink.exists(): + try: + # Try lchmod first, if the platform supports it. + symlink.lchmod(rwx) + except NotImplementedError: + # The platform (probably win32) doesn't support lchmod; fall back to chmod. + symlink.chmod(rwx) + dir2.chmod(rwx) + rmtree(dir2) + + request.addfinalizer(preen_dir2) + try: rmtree(dir2) except PermissionError: From 14190cbee694444c658e70aad9548fae19243971 Mon Sep 17 00:00:00 2001 From: Enji Cooper Date: Sat, 18 Apr 2026 10:23:39 -0700 Subject: [PATCH 3/3] Use subprocess's timeout feature instead of reinventing the wheel subprocess's APIs in 3.3+ support passing timeout to calls, such as .communicate(..), .wait(..), etc. Pass `kill_after_timeout` to those APIs and remove the watchdog handler code as it's not needed once `timeout=` is used. This enables `kill_after_timeout` on Windows platforms by side-effect as upstream implements `timeout` for all supported platforms. Signed-off-by: Enji Cooper --- git/cmd.py | 75 ++++++++++-------------------------------------------- 1 file changed, 14 insertions(+), 61 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index d5fbc7736..d9fe72760 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -13,7 +13,6 @@ import logging import os import re -import signal import subprocess from subprocess import DEVNULL, PIPE, Popen import sys @@ -110,7 +109,7 @@ def handle_process_output( stderr_handler: Union[None, Callable[[AnyStr], None], Callable[[List[AnyStr]], None]], finalizer: Union[None, Callable[[Union[Popen, "Git.AutoInterrupt"]], None]] = None, decode_streams: bool = True, - kill_after_timeout: Union[None, float] = None, + kill_after_timeout: float | int | None = None, ) -> None: R"""Register for notifications to learn that process output is ready to read, and dispatch lines to the respective line handlers. @@ -139,7 +138,7 @@ def handle_process_output( - decoding must happen later, such as for :class:`~git.diff.Diff`\s. :param kill_after_timeout: - :class:`float` or ``None``, Default = ``None`` + :class:`int`, `float`, or ``None`` (block indefinitely), Default = ``None``. To specify a timeout in seconds for the git command, after which the process should be killed. @@ -326,16 +325,22 @@ class _AutoInterrupt: raise. """ - __slots__ = ("proc", "args", "status") + __slots__ = ("proc", "args", "status", "timeout") # If this is non-zero it will override any status code during _terminate, used # to prevent race conditions in testing. _status_code_if_terminate: int = 0 - def __init__(self, proc: Union[None, subprocess.Popen], args: Any) -> None: + def __init__( + self, + proc: subprocess.Popen | None, + args: Any, + timeout: float | int | None = None, + ) -> None: self.proc = proc self.args = args self.status: Union[int, None] = None + self.timeout = timeout def _terminate(self) -> None: """Terminate the underlying process.""" @@ -365,7 +370,7 @@ def _terminate(self) -> None: # Try to kill it. try: proc.terminate() - status = proc.wait() # Ensure the process goes away. + status = proc.wait(timeout=self.timeout) # Ensure the process goes away. self.status = self._status_code_if_terminate or status except (OSError, AttributeError) as ex: @@ -400,7 +405,7 @@ def wait(self, stderr: Union[None, str, bytes] = b"") -> int: stderr_b = force_bytes(data=stderr, encoding="utf-8") status: Union[int, None] if self.proc is not None: - status = self.proc.wait() + status = self.proc.wait(timeout=self.timeout) p_stderr = self.proc.stderr else: # Assume the underlying proc was killed earlier or never existed. status = self.status @@ -1303,58 +1308,6 @@ def execute( if as_process: return self.AutoInterrupt(proc, command) - if sys.platform != "win32" and kill_after_timeout is not None: - # Help mypy figure out this is not None even when used inside communicate(). - timeout = kill_after_timeout - - def kill_process(pid: int) -> None: - """Callback to kill a process. - - This callback implementation would be ineffective and unsafe on Windows. - """ - p = Popen(["ps", "--ppid", str(pid)], stdout=PIPE) - child_pids = [] - if p.stdout is not None: - for line in p.stdout: - if len(line.split()) > 0: - local_pid = (line.split())[0] - if local_pid.isdigit(): - child_pids.append(int(local_pid)) - try: - os.kill(pid, signal.SIGKILL) - for child_pid in child_pids: - try: - os.kill(child_pid, signal.SIGKILL) - except OSError: - pass - # Tell the main routine that the process was killed. - kill_check.set() - except OSError: - # It is possible that the process gets completed in the duration - # after timeout happens and before we try to kill the process. - pass - return - - def communicate() -> Tuple[AnyStr, AnyStr]: - watchdog.start() - out, err = proc.communicate() - watchdog.cancel() - if kill_check.is_set(): - err = 'Timeout: the command "%s" did not complete in %d secs.' % ( - " ".join(redacted_command), - timeout, - ) - if not universal_newlines: - err = err.encode(defenc) - return out, err - - # END helpers - - kill_check = threading.Event() - watchdog = threading.Timer(timeout, kill_process, args=(proc.pid,)) - else: - communicate = proc.communicate - # Wait for the process to return. status = 0 stdout_value: Union[str, bytes] = b"" @@ -1362,7 +1315,7 @@ def communicate() -> Tuple[AnyStr, AnyStr]: newline = "\n" if universal_newlines else b"\n" try: if output_stream is None: - stdout_value, stderr_value = communicate() + stdout_value, stderr_value = proc.communicate(timeout=kill_after_timeout) # Strip trailing "\n". if stdout_value is not None and stdout_value.endswith(newline) and strip_newline_in_stdout: # type: ignore[arg-type] stdout_value = stdout_value[:-1] @@ -1380,7 +1333,7 @@ def communicate() -> Tuple[AnyStr, AnyStr]: # Strip trailing "\n". if stderr_value is not None and stderr_value.endswith(newline): # type: ignore[arg-type] stderr_value = stderr_value[:-1] - status = proc.wait() + status = proc.wait(timeout=kill_after_timeout) # END stdout handling finally: if proc.stdout is not None: