Use subprocess timeout#2127
Conversation
Sorry, something went wrong.
ngie-eign
force-pushed
the
use-subprocess-timeout
branch
2 times, most recently
from
2d09ccf to
1435486
Compare
April 18, 2026 17:29
|
The failure may have been caused by a sporadic network outage. Let's try again. |
Sorry, something went wrong.
ngie-eign
force-pushed
the
use-subprocess-timeout
branch
from
1435486 to
14190cb
Compare
April 18, 2026 17:55
There was a problem hiding this comment.
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=withPopen.communicate()/Popen.wait(). - Add tests covering
Git.execute(..., with_stdout=False)behavior (including withoutput_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.
Sorry, something went wrong.
|
Thanks a lot for your continued work on this. Let's take a look at the auto review before I merge it. |
Sorry, something went wrong.
ngie-eign
force-pushed
the
use-subprocess-timeout
branch
from
14190cb to
5124e0c
Compare
April 21, 2026 07:27
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ngie-eign
force-pushed
the
use-subprocess-timeout
branch
4 times, most recently
from
8ddff20 to
d946809
Compare
April 21, 2026 16:51
ngie-eign
force-pushed
the
use-subprocess-timeout
branch
from
d946809 to
07362e7
Compare
April 21, 2026 17:12
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
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. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>
ngie-eign
force-pushed
the
use-subprocess-timeout
branch
2 times, most recently
from
1c08f40 to
4c1fda8
Compare
April 22, 2026 21:25
ngie-eign
force-pushed
the
use-subprocess-timeout
branch
from
4c1fda8 to
ec130d6
Compare
April 22, 2026 21:26
|
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. |
Sorry, something went wrong.
|
I think it's unrelated. The following was suggested elsewhere: #2128 (comment). I recommend to watch it and rebase once the PR is merged. |
Sorry, something went wrong.
|
This is a supposed fix, maybe fragile, maybe too specific, but I also don't know what else to do here. 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" and Git().config("core.symlinks") == "true",
...
)@ngie-eign Your help here would be appreciated. Maybe then the PR can be taken out of draft as well. |
Sorry, something went wrong.
Reviewers
Copilot code review Copilot Copilot left review comments
+1 more reviewer
subprocess's APIs in 3.3+ support passing timeout to calls, such as.communicate(..),.wait(..), etc. Passkill_after_timeoutto thoseAPIs and remove the watchdog handler code as it's not needed once
timeout=is used.This enables
kill_after_timeouton Windows platforms by side-effect asupstream implements
timeoutfor all supported platforms.Requires: #2126