Merge pull request #1761 from EliahKagan/callback-limitations

Clarify some Git.execute kill_after_timeout limitations

Author: Byron

git/cmd.py

@@ -879,11 +879,19 @@ def execute(
             Specifies a timeout in seconds for the git command, after which the process
             should be killed. This will have no effect if `as_process` is set to True.
             It is set to None by default and will let the process run until the timeout
-            is explicitly specified. This feature is not supported on Windows. It's also
-            worth noting that `kill_after_timeout` uses SIGKILL, which can have negative
-            side effects on a repository. For example, stale locks in case of ``git gc``
-            could render the repository incapable of accepting changes until the lock is
-            manually removed.
+            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
+               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
+               terminate as a consequence of their parent processes being killed.
+            4. `kill_after_timeout` uses ``SIGKILL``, which can have negative side
+               effects on a repository. For example, stale locks in case of ``git gc``
+               could render the repository incapable of accepting changes until the lock
+               is manually removed.
 
         :param with_stdout:
             If True, default True, we open stdout on the created process.
@@ -1007,11 +1015,9 @@ def execute(
 
         def kill_process(pid: int) -> None:
             """Callback to kill a process."""
-            p = Popen(
-                ["ps", "--ppid", str(pid)],
-                stdout=PIPE,
-                creationflags=PROC_CREATIONFLAGS,
-            )
+            if os.name == "nt":
+                raise AssertionError("Bug: This callback would be ineffective and unsafe on Windows, stopping.")
+            p = Popen(["ps", "--ppid", str(pid)], stdout=PIPE)
             child_pids = []
             if p.stdout is not None:
                 for line in p.stdout:
@@ -1020,18 +1026,16 @@ def kill_process(pid: int) -> None:
                         if local_pid.isdigit():
                             child_pids.append(int(local_pid))
             try:
-                # Windows does not have SIGKILL, so use SIGTERM instead.
-                sig = getattr(signal, "SIGKILL", signal.SIGTERM)
-                os.kill(pid, sig)
+                os.kill(pid, signal.SIGKILL)
                 for child_pid in child_pids:
                     try:
-                        os.kill(child_pid, sig)
+                        os.kill(child_pid, signal.SIGKILL)
                     except OSError:
                         pass
                 kill_check.set()  # Tell the main routine that the process was killed.
             except OSError:
-                # It is possible that the process gets completed in the duration after timeout
-                # happens and before we try to kill the process.
+                # It is possible that the process gets completed in the duration after
+                # timeout happens and before we try to kill the process.
                 pass
             return