Skip to content

Commit

Permalink
Remove unused TASKKILL fallback in AutoInterrupt
Browse files Browse the repository at this point in the history
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 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.)
  • Loading branch information
EliahKagan committed Dec 1, 2023
1 parent a30b3b7 commit 36b7433
Showing 1 changed file with 1 addition and 11 deletions.
12 changes: 1 addition & 11 deletions git/cmd.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down

0 comments on commit 36b7433

Please sign in to comment.