fixup! Use subprocess's timeout feature instead of reinventing the wheel

Author: ngie-eign

git/cmd.py

@@ -138,7 +138,7 @@ def handle_process_output(
         - decoding must happen later, such as for :class:`~git.diff.Diff`\s.
 
     :param kill_after_timeout:
-        :class:`int`, `float`, or ``None`` (block indefinitely), 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.
@@ -373,7 +373,7 @@ def _terminate(self) -> None:
             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:
+        except (OSError, AttributeError, subprocess.TimeoutExpired) as ex:
             # On interpreter shutdown (notably on Windows), parts of the stdlib used by
             # subprocess can already be torn down (e.g. `subprocess._winapi` becomes None),
             # which can cause AttributeError during terminate(). In that case, we prefer
@@ -1111,7 +1111,7 @@ def execute(
         as_process: bool = False,
         output_stream: Union[None, BinaryIO] = None,
         stdout_as_string: bool = True,
-        kill_after_timeout: Union[None, float] = None,
+        kill_after_timeout: float | int | None = None,
         with_stdout: bool = True,
         universal_newlines: bool = False,
         shell: Union[None, bool] = None,
@@ -1163,13 +1163,12 @@ def execute(
             until the timeout is explicitly specified. Uses of this feature should be
             carefully considered, due to the following limitations:
 
-            1. This feature is not supported at all on Windows.
-            2. Effectiveness may vary by operating system. ``ps --ppid`` is used to
+            1. Effectiveness may vary by operating system. ``ps --ppid`` is used to
                enumerate child processes, which is available on most GNU/Linux systems
                but not most others.
-            3. Deeper descendants do not receive signals, though they may sometimes
+            2. Deeper descendants do not receive signals, though they may sometimes
                terminate as a consequence of their parent processes being killed.
-            4. `kill_after_timeout` uses ``SIGKILL``, which can have negative side
+            3. `kill_after_timeout` uses ``SIGKILL``, which can have negative side
                effects on a repository. For example, stale locks in case of
                :manpage:`git-gc(1)` could render the repository incapable of accepting
                changes until the lock is manually removed.
@@ -1257,15 +1256,7 @@ def execute(
         if inline_env is not None:
             env.update(inline_env)
 
-        if sys.platform == "win32":
-            if kill_after_timeout is not None:
-                raise GitCommandError(
-                    redacted_command,
-                    '"kill_after_timeout" feature is not supported on Windows.',
-                )
-            cmd_not_found_exception = OSError
-        else:
-            cmd_not_found_exception = FileNotFoundError
+        cmd_not_found_exception = OSError if sys.platform == "win32" else FileNotFoundError
         # END handle
 
         stdout_sink = PIPE if with_stdout else getattr(subprocess, "DEVNULL", None) or open(os.devnull, "wb")
@@ -1335,6 +1326,12 @@ def execute(
                     stderr_value = stderr_value[:-1]
                 status = proc.wait(timeout=kill_after_timeout)
             # END stdout handling
+        except subprocess.TimeoutExpired as err:
+            _logger.info(
+                "error: process killed because it timed out. kill_after_timeout=%s seconds", kill_after_timeout,
+            )
+            if with_exceptions:
+                raise GitCommandError(redacted_command, status or 255, err.stderr, err.stdout) from err
         finally:
             if proc.stdout is not None:
                 proc.stdout.close()

git/remote.py

@@ -5,6 +5,8 @@
 
 """Module implementing a remote object allowing easy access to git remotes."""
 
+from __future__ import annotations
+
 __all__ = ["RemoteProgress", "PushInfo", "FetchInfo", "Remote"]
 
 import contextlib
@@ -873,7 +875,7 @@ def _get_fetch_info_from_stderr(
         self,
         proc: "Git.AutoInterrupt",
         progress: Union[Callable[..., Any], RemoteProgress, None],
-        kill_after_timeout: Union[None, float] = None,
+        kill_after_timeout: float | int | None = None,
     ) -> IterableList["FetchInfo"]:
         progress = to_progress_instance(progress)
 
@@ -944,7 +946,7 @@ def _get_push_info(
         self,
         proc: "Git.AutoInterrupt",
         progress: Union[Callable[..., Any], RemoteProgress, None],
-        kill_after_timeout: Union[None, float] = None,
+        kill_after_timeout: float | int | None = None,
     ) -> PushInfoList:
         progress = to_progress_instance(progress)
 
@@ -1002,7 +1004,7 @@ def fetch(
         refspec: Union[str, List[str], None] = None,
         progress: Union[RemoteProgress, None, "UpdateProgress"] = None,
         verbose: bool = True,
-        kill_after_timeout: Union[None, float] = None,
+        kill_after_timeout: float | int | None = None,
         allow_unsafe_protocols: bool = False,
         allow_unsafe_options: bool = False,
         **kwargs: Any,
@@ -1082,7 +1084,7 @@ def pull(
         self,
         refspec: Union[str, List[str], None] = None,
         progress: Union[RemoteProgress, "UpdateProgress", None] = None,
-        kill_after_timeout: Union[None, float] = None,
+        kill_after_timeout: float | int | None = None,
         allow_unsafe_protocols: bool = False,
         allow_unsafe_options: bool = False,
         **kwargs: Any,
@@ -1136,7 +1138,7 @@ def push(
         self,
         refspec: Union[str, List[str], None] = None,
         progress: Union[RemoteProgress, "UpdateProgress", Callable[..., RemoteProgress], None] = None,
-        kill_after_timeout: Union[None, float] = None,
+        kill_after_timeout: float | int | None = None,
         allow_unsafe_protocols: bool = False,
         allow_unsafe_options: bool = False,
         **kwargs: Any,

test/test_remote.py

@@ -434,7 +434,7 @@ def _assert_push_and_pull(self, remote, rw_repo, remote_repo):
         TagReference.delete(rw_repo, new_tag, other_tag)
         remote.push(":%s" % other_tag.path, kill_after_timeout=10.0)
 
-    @skipIf(HIDE_WINDOWS_FREEZE_ERRORS, "FIXME: Freezes!")
+    #@skipIf(HIDE_WINDOWS_FREEZE_ERRORS, "FIXME: Freezes!")
     @with_rw_and_rw_remote_repo("0.1.6")
     def test_base(self, rw_repo, remote_repo):
         num_remotes = 0