From 3f21391e9aab5eb89aa3bac6fd32293b129167fc Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 1 Dec 2023 14:34:11 -0500 Subject: [PATCH] Remove unused TASKKILL fallback in AutoInterrupt When Git.AutoInterrupt was first implemented in ead94f2, it used os.kill (sending SIGINT to the process to terminate it). This was in 2009, and not all supported versions of Python provided os.kill on Windows, since it was added for Windows in 2.7 and 3.2 (2.6 and 3.1 reached EoL in 2013 and 2012, respectively). Catching AttributeError and running TASKKILL provided a fallback for Python versions in which os.kill was absent on Windows. Since then, all supported versions have os.kill on Windows, and also the code of GitPython, in the try-block, has changed, and no longer uses os.kill. It now contains four attribute access expressions: - proc.terminate. All currently suppported versions of Python (including 3.7, which GitPython still supports, and some before that) have Popen.terminate. - proc.wait. Same situation as proc.terminate. Popen.wait exists. - self._status_code_if_terminate. This is a class attribute of AutoInterrupt, accessible through its instances. - self.status. This is assigned to. AutoInterrupt is slotted, but "status" appears in __slots__, so this isn't an AttributeError either. The "except AttributeError" clause is thus no longer used and can be removed, which is done here. Removing it shortens and simplifies the code, better expresses the logic for how the wrapped process is actually being terminated, and eliminates the need to engage with any subtleties of TASKKILL (and of how it is called) when reading the code to verify its correctness. In addition, because they are now expected always to be available, if somehow an AttributeError managed to be raised in the terminate or wait calls, this would be very strange and we probably shouldn't silently catch that. (Because this AutoInterrupt._terminate method is sometimes called due to __del__ methods running as the interpreter is shutting down, it is possible for some attributes to unexpectedly be set to None, which could cause AttributeError indirectly if another attribute is looked up on the None object; and perhaps on some non-CPython implementations some attributes might even be deleted during shutdown. The _terminate method handles this where relevant. But the TASKKILL fallback removed here seems unrelated to that, which affects module attributes and global variables. The names used in the try-block are proc, status, and self, which are local variables; the except clause itself accessed os.name, which used a global variable and a module attribute, indicating that the intent was not to handle this interpreter shutdown scenario; and this whole issue, while not gone completely, is much less significant since Python 3.4, due to https://docs.python.org/3/whatsnew/3.4.html#whatsnew-pep-442.) --- git/cmd.py | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/git/cmd.py b/git/cmd.py index 05e199dc0..4dc76556e 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -11,7 +11,7 @@ import logging import os import signal -from subprocess import call, Popen, PIPE, DEVNULL +from subprocess import Popen, PIPE, DEVNULL import subprocess import threading from textwrap import dedent @@ -544,16 +544,6 @@ def _terminate(self) -> None: self.status = self._status_code_if_terminate or status except OSError as ex: log.info("Ignored error after process had died: %r", ex) - except AttributeError: - # Try Windows. - # For some reason, providing None for stdout/stderr still prints something. This is why - # we simply use the shell and redirect to nul. Slower than CreateProcess. The question - # is whether we really want to see all these messages. It's annoying no matter what. - if os.name == "nt": - call( - ("TASKKILL /F /T /PID %s 2>nul 1>nul" % str(proc.pid)), - shell=True, - ) # END exception handling def __del__(self) -> None: