Use subprocess timeout

[ngie-eign]

subprocess's APIs in 3.3+ support passing timeout to calls, such as
.communicate(..), .wait(..), etc. Pass kill_after_timeout to those
APIs and remove the watchdog handler code as it's not needed once
timeout= is used.

This enables kill_after_timeout on Windows platforms by side-effect as
upstream implements timeout for all supported platforms.

Requires: #2126

[ngie-eign]

The failure may have been caused by a sporadic network outage. Let's try again.

[Copilot]

Pull request overview

Updates process execution to rely on subprocess’s native timeout= support (instead of a custom watchdog) and adjusts related tests/cleanup.

Changes:

• Remove watchdog-based timeout logic and use timeout= with Popen.communicate() / Popen.wait().
• Add tests covering Git.execute(..., with_stdout=False) behavior (including with output_stream).
• Improve test cleanup for unwritable directories/symlink scenarios; update AUTHORS.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.

File	Description
git/cmd.py	Replaces custom watchdog timeout with subprocess timeouts; adjusts stream handling and AutoInterrupt waits.
test/test_git.py	Adds regression tests for with_stdout=False with/without output_stream.
test/test_util.py	Adds a pytest finalizer to ensure cleanup of unwritable temp directories after the test.
AUTHORS	Adds contributor entry.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Byron]

Thanks a lot for your continued work on this.

Let's take a look at the auto review before I merge it.

[ngie-eign]

@Byron : this is ready for a re-review.

[Byron]

Thanks a lot, and sorry forward shall follow.

Because I realized that we are definitely missing an actual timeout test. So maybe just make it so that it runs some fixture binary that sleeps for a while and set the timeout short enough so it reliably times out. What matters here is that we validate that communicate still picks up the stdout and stderr that was emitted so far.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

_AutoInterrupt now has a timeout attribute and uses it in wait(timeout=...), but no callers pass a value (the only construction is self.AutoInterrupt(proc, command)). As a result, timeout stays None and the new timeout-aware wait()/_terminate() behavior is never applied. If the intent is to enforce kill_after_timeout for as_process=True flows (e.g., via handle_process_output), wire the timeout through when constructing AutoInterrupt or set it before calling _terminate()/wait().

[Copilot]

In _terminate(), if proc.wait(timeout=self.timeout) raises TimeoutExpired, the exception is logged and ignored, potentially leaving the child process running and leaked. Consider escalating to proc.kill() (or another stronger termination strategy) after a terminate+wait timeout, and/or ensuring the process is reaped.

[Copilot]

execute()'s TimeoutExpired handling calls proc.terminate() and then proc.communicate() without any timeout, which can hang indefinitely if the process ignores termination. Also, status is not updated in this path, so with with_exceptions=False the method may return status=0 even though a timeout occurred. Consider: (1) setting status = proc.returncode after reaping, (2) using a bounded second communicate(timeout=...) followed by proc.kill() as a fallback, and (3) avoiding log text that claims the process was "killed" when only terminate() was attempted.

Suggested change

	"error: process killed because it timed out. kill_after_timeout=%s seconds",
	kill_after_timeout,
	)
	proc.terminate()
	stdout_value, stderr_value=proc.communicate()
	"error: process timed out after %s seconds; attempting to terminate it",
	kill_after_timeout,
	)
	proc.terminate()
	try:
	stdout_value, stderr_value=proc.communicate(timeout=kill_after_timeout)
	exceptsubprocess.TimeoutExpired:
	_logger.info("error: process did not terminate in time; killing it")
	proc.kill()
	stdout_value, stderr_value=proc.communicate()
	ifstdout_valueisnotNoneandstdout_value.endswith(newline) andstrip_newline_in_stdout: # type: ignore[arg-type]
	stdout_value=stdout_value[:-1]
	ifstderr_valueisnotNoneandstderr_value.endswith(newline): # type: ignore[arg-type]
	stderr_value=stderr_value[:-1]
	status=proc.returncode

[ngie-eign]

Some of my recent changes upset win32 according to the test failures. Let me see if I can track down what's going on there then bring this out of draft again.

[Byron]

I think it's unrelated.

The following was suggested elsewhere: #2128 (comment). I recommend to watch it and rebase once the PR is merged.

[Byron]

This is a supposed fix, maybe fragile, maybe too specific, but I also don't know what else to do here.
Also it seems unrelated to this PR.

diff --git a/git/index/base.py b/git/index/base.py
index 2276343f..xxxxxxx 100644
--- a/git/index/base.py+++ b/git/index/base.py@@ -1284,6 +1284,18 @@ class IndexFile(LazyMixin, git_diff.Diffable, Serializable):
)
for line in stderr.splitlines():
+ # Git for Windows may emit non-fatal warnings when creating symlinks, e.g.:+ # "warning: created file symlink 'name' pointing to 'target';"+ # followed by:+ # "set the `symlink` gitattribute to `dir` if a directory symlink is required"+ # These are not checkout failures and should not be treated as unknown errors.+ if line.startswith("warning: created file symlink "):+ continue+ if line.startswith("set the `symlink` gitattribute to `dir`"):+ continue+
if not line.startswith("git checkout-index: ") and not line.startswith("git-checkout-index: "):
is_a_dir = " is a directory"
unlink_issue = "unable to unlink old '"

Better would be to set xfail:

@pytest.mark.xfail(sys.platform=="win32"andGit().config("core.symlinks") =="true", ...)

@ngie-eign Your help here would be appreciated. Maybe then the PR can be taken out of draft as well.

[Byron]

Closing as the PR is conflicted and lacks interaction. Please feel free to recreate the PR if there is still interest.