Skip to content

Commit e0c8290

Browse files
committed
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.3 (2.6 and 3.2 reached EoL in 2013 and 2016, 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 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 absent, which the _terminate method handles 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 uses a global variable and a module attribute, indicating that the intent is 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.)
1 parent a30b3b7 commit e0c8290

File tree

1 file changed

+1
-11
lines changed

1 file changed

+1
-11
lines changed

git/cmd.py

+1-11
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
import logging
1212
import os
1313
import signal
14-
from subprocess import call, Popen, PIPE, DEVNULL
14+
from subprocess import Popen, PIPE, DEVNULL
1515
import subprocess
1616
import threading
1717
from textwrap import dedent
@@ -544,16 +544,6 @@ def _terminate(self) -> None:
544544
self.status = self._status_code_if_terminate or status
545545
except OSError as ex:
546546
log.info("Ignored error after process had died: %r", ex)
547-
except AttributeError:
548-
# Try Windows.
549-
# For some reason, providing None for stdout/stderr still prints something. This is why
550-
# we simply use the shell and redirect to nul. Slower than CreateProcess. The question
551-
# is whether we really want to see all these messages. It's annoying no matter what.
552-
if os.name == "nt":
553-
call(
554-
("TASKKILL /F /T /PID %s 2>nul 1>nul" % str(proc.pid)),
555-
shell=True,
556-
)
557547
# END exception handling
558548

559549
def __del__(self) -> None:

0 commit comments

Comments
 (0)