git.cmd.Git.execute(..): fix with_stdout=False#2126
git.cmd.Git.execute(..): fix with_stdout=False#2126ngie-eign wants to merge 2 commits intogitpython-developers:mainfrom
with_stdout=False#2126Conversation
In the event the end-user called one of the APIs with `with_stdout=False`, i.e., they didn't want to capture stdout, the code would crash with an AttributeError or ValueError when trying to dereference the stdout/stderr streams attached to `Popen(..)` objects. Be more defensive by checking the streams first to make sure they're not `None` before trying to access their corresponding attributes. Add myself to AUTHORS and add corresponding regression tests for the change. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>
ngie-eign
force-pushed
the
fix-execute-with_stdout-no-issues
branch
3 times, most recently
from
d3d587c to
a64bde9
Compare
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
Thanks a lot. This looks good to me.
If the machine doesn't find anything horrendous, we can merge this.
There was a problem hiding this comment.
Pull request overview
Fixes Git.execute(..., with_stdout=False) crashing when stdout/stderr streams are None, and adds regression coverage to prevent future regressions.
Changes:
- Make
Git.execute()defensive againstNonestdout/stderr streams when stripping newlines, copying output, and closing handles. - Add regression tests for
with_stdout=Falsescenarios (with and withoutoutput_stream). - Add contributor entry to
AUTHORSand improve test cleanup robustness inrmtreetests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
git/cmd.py |
Adds None checks around stdout/stderr usage to avoid crashes when stdout isn’t piped. |
test/test_git.py |
Adds regression tests covering with_stdout=False behavior. |
test/test_util.py |
Adds a pytest finalizer to ensure temp directories are cleaned up on platforms with stricter permissions. |
AUTHORS |
Adds a new contributor entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if symlink.exists(): | ||
| symlink.chmod(rwx) |
There was a problem hiding this comment.
preen_dir2() calls symlink.chmod(rwx), but Path.chmod() follows symlinks on most platforms and will change the permissions of the target file (dir1 / "file"). That undermines the intent of this test (avoiding permission changes outside dir2) and can leave surprising side effects in the temp tree. Consider removing the symlink chmod entirely (only dir2.chmod(rwx) is needed for cleanup), or if you really need it, use follow_symlinks=False with a best-effort fallback.
| if symlink.exists(): | |
| symlink.chmod(rwx) |
| max_chunk_size = max_chunk_size if max_chunk_size and max_chunk_size > 0 else io.DEFAULT_BUFFER_SIZE | ||
| stream_copy(proc.stdout, output_stream, max_chunk_size) | ||
| stdout_value = proc.stdout.read() | ||
| stderr_value = proc.stderr.read() | ||
| if proc.stdout is not None: | ||
| stream_copy(proc.stdout, output_stream, max_chunk_size) | ||
| stdout_value = proc.stdout.read() | ||
| if proc.stderr is not None: | ||
| stderr_value = proc.stderr.read() |
There was a problem hiding this comment.
In the output_stream branch, if with_stdout=False then proc.stdout will be None, so stdout_value stays as the initial b"" and the method can return an empty string/bytes even though stdout was intentionally not captured. For consistency with the communicate() path (where stdout becomes None), explicitly set stdout_value = None when proc.stdout is None (and update the local typing/overloads accordingly, since stdout can now be None when with_stdout=False).
| # Strip trailing "\n". | ||
| if stdout_value.endswith(newline) and strip_newline_in_stdout: # type: ignore[arg-type] | ||
| 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.endswith(newline): # type: ignore[arg-type] | ||
| if stderr_value is not None and stderr_value.endswith(newline): # type: ignore[arg-type] | ||
| stderr_value = stderr_value[:-1] |
There was a problem hiding this comment.
The new stdout_value is not None / stderr_value is not None guards indicate communicate() can return None (e.g., when with_stdout=False). However, stdout_value/stderr_value are currently annotated as non-optional and the execute() overloads/return type don’t include None for stdout. Updating those annotations to reflect Optional[...] (and the with_extended_output tuple element types) would prevent type-checking mismatches and better document the API behavior.
There was a problem hiding this comment.
Yes, we should definitely find a solution for this. Maybe a type change might already do the trick.
There was a problem hiding this comment.
I tried fudging around with types, but it's really messy dealing with these issues. I quite frankly think that the whole subprocess interface could be simplified with 3.8+'s version of subprocess... give me a bit of time and I'll noodle on how to do that.
There was a problem hiding this comment.
Also, the pre-3.10 type hints are cringy... do you have to support pre-3.8, even though 3.8 has been technically EOL upstream for the past 1~1.5 years?
There was a problem hiding this comment.
I don't like the typing and would be happy to streamline them if there is a choice.
There was a problem hiding this comment.
Also sounds good to update to 3.8+ version of subprocess, if all this helps with types.
There was a problem hiding this comment.
Ah, interesting... you didn't set the minimum version of the project to 3.7 explicitly, but you set it implicitly. from __future__ import annotations is available, which allows the project to use much cleaner type hinting than prior versions of 3.x allowed.
There was a problem hiding this comment.
Oh wait: it's specified in setup.py (the legacy packaging file) instead of pyproject.toml (the new packaging file).
There was a problem hiding this comment.
And someone already added from __future__ import annotations to the file... ok, time to pull up my sleeves and leverage it in the file I suppose.
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
Needs auto-review comments to be addressed.
Prior to this the test would fail [silently] on my macOS host during the test and then pytest would complain loudly about it being an issue post-session (regardless of whether or not the test was being run). Squash the unwritable directory to mute noise complaints from pytest. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>
ngie-eign
force-pushed
the
fix-execute-with_stdout-no-issues
branch
from
a64bde9 to
0ae13c9
Compare
In the event the end-user called one of the APIs with
with_stdout=False, i.e., they didn't want to capture stdout, the code would crash with an AttributeError or ValueError when trying to dereference the stdout/stderr streams attached toPopen(..)objects.Be more defensive by checking the streams first to make sure they're not
Nonebefore trying to access their corresponding attributes.Add myself to AUTHORS and add corresponding regression tests for the change.