|
The failure may have been caused by a sporadic network outage. Let's try again. |
Sorry, something went wrong.
There was a problem hiding this comment.
Updates process execution to rely on subprocess’s native timeout= support (instead of a custom watchdog) and adjusts related tests/cleanup.
Changes:
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| 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.
|
@Byron : this is ready for a re-review. |
Sorry, something went wrong.
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.
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.
| def __init__( | ||
| self, | ||
| proc: subprocess.Popen | None, | ||
| args: Any, | ||
| timeout: float | None = None, | ||
| ) -> None: | ||
| self.proc = proc | ||
| self.args = args | ||
| self.status: Union[int, None] = None | ||
| self.timeout = timeout |
There was a problem hiding this comment.
_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().
Sorry, something went wrong.
| try: | ||
| proc.terminate() | ||
| status = proc.wait() # Ensure the process goes away. | ||
| status = proc.wait(timeout=self.timeout) # Ensure the process goes away. | ||
|
|
||
| self.status = self._status_code_if_terminate or status | ||
| except (OSError, AttributeError) as ex: | ||
| except (OSError, AttributeError, subprocess.TimeoutExpired) as ex: | ||
| # On interpreter shutdown (notably on Windows), parts of the stdlib used by | ||
| # subprocess can already be torn down (e.g. `subprocess._winapi` becomes None), | ||
| # which can cause AttributeError during terminate(). In that case, we prefer | ||
| # to silently ignore to avoid noisy "Exception ignored in: __del__" messages. | ||
| _logger.info("Ignored error while terminating process: %r", ex) |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| "error: process killed because it timed out. kill_after_timeout=%s seconds", | ||
| kill_after_timeout, | ||
| ) | ||
| proc.terminate() | ||
| stdout_value, stderr_value = proc.communicate() |
There was a problem hiding this comment.
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.
| "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) | |
| except subprocess.TimeoutExpired: | |
| _logger.info("error: process did not terminate in time; killing it") | |
| proc.kill() | |
| stdout_value, stderr_value = proc.communicate() | |
| if stdout_value is not None and stdout_value.endswith(newline) and strip_newline_in_stdout: # type: ignore[arg-type] | |
| stdout_value = stdout_value[:-1] | |
| if stderr_value is not None and stderr_value.endswith(newline): # type: ignore[arg-type] | |
| stderr_value = stderr_value[:-1] | |
| status = proc.returncode |
Sorry, something went wrong.
|
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.
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