Skip to content

Rewrite Git.execute() command parameter docstring per #2146 - #2147

Merged
EliahKagan merged 2 commits into
gitpython-developers:mainfrom
EliahKagan:claude/execute-string-platform-docs
May 10, 2026
Merged

Rewrite Git.execute() command parameter docstring per #2146#2147
EliahKagan merged 2 commits into
gitpython-developers:mainfrom
EliahKagan:claude/execute-string-platform-docs

Conversation

@EliahKagan

Copy link
Copy Markdown
Member

Fixes#2146

This was written using Claude Code, with extensive back-and-forth about wording. The following description is written by Claude (also with extensive back and forth). Because I've been carefully reading everything the whole way through, I know what this change is exactly. Nonetheless, I'll review it again before marking it non-draft and merging it.

#2146 identifies two factual problems in #2144's :param command: rewrite, both of which this fixes:

  1. The claim that with shell=False a string "is passed as a single executable name to subprocess.Popen" is accurate on Unix-like systems, but on Windows subprocess.Popen forwards the string to CreateProcessW and Windows command-line parsing produces argv. So multi-word strings happen to work on Windows -- which makes the docstring misleading for Windows readers and unhelpful for anyone whose actual problem is portability.

  2. The blanket recommendation to use shlex.split read as a general string-to-argv splitter. shlex.split parses POSIX shell syntax on all systems, and the resulting tokens are still unsafe for anything but fixed, fully trusted strings -- in particular, strings built by interpolating values can still inject extra arguments via embedded whitespace or quoting.

Restructure :param command: into four paragraphs (description, platform-specific string handling, shell=True / Git.USE_SHELL warning, qualified shlex.split note) and cross-reference USE_SHELL for the long-form security discussion rather than reproducing it.

Add a related paragraph to :param shell: noting shlex.split's narrow value as a transitional tool when migrating existing string-command shell=True calls on Unix-like systems, with extreme care, and cross-referencing :param command: for the risks.

This only changes documentation, not behavior.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the Git.execute() docstring in git/cmd.py to clarify how the command parameter behaves when passed as a string vs a sequence, with emphasis on platform differences (Unix-like vs Windows) and the security implications of shell=True / Git.USE_SHELL. This aligns the documentation with the Windows behavior discussed in #2146 and refines guidance around shlex.split.

Changes:

  • Rewrites :param command: to describe platform-dependent behavior of string commands and to strengthen security guidance around shells.
  • Qualifies the shlex.split mention to position it as safer than shell=True but still unsafe for untrusted/interpolated input.
  • Adds a :param shell: note about shlex.split as a rare transitional aid when migrating away from shell=True on Unix-like systems.

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

Comment threadgit/cmd.py Outdated
@EliahKagan
EliahKaganforce-pushed the claude/execute-string-platform-docs branch 2 times, most recently from 19bc03b to c871adcCompareMay 10, 2026 00:10
@EliahKagan
EliahKagan requested a review from CopilotMay 10, 2026 00:13

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment threadgit/cmd.py Outdated
@EliahKagan
EliahKaganforce-pushed the claude/execute-string-platform-docs branch from c871adc to 1a3e3ddCompareMay 10, 2026 00:34
…opers#2146gitpython-developers#2146 identifies two factual problems in gitpython-developers#2144's `:param command:`
rewrite, both of which this fixes:
1. The claim that with `shell=False` a string "is passed as a single
executable name to `subprocess.Popen`" is accurate on Unix-like
systems, but on Windows `subprocess.Popen` forwards the string to
`CreateProcessW` and Windows command-line parsing produces argv.
So multi-word strings happen to work on Windows -- which makes
the docstring misleading for Windows readers and unhelpful for
anyone whose actual problem is portability.
2. The blanket recommendation to use `shlex.split` read as a general
string-to-argv splitter. `shlex.split` parses POSIX shell syntax
on all systems, and the resulting tokens are still unsafe for
anything but fixed, fully trusted strings -- in particular,
strings built by interpolating values can still inject extra
arguments via embedded whitespace or quoting.
Restructure `:param command:` into four paragraphs (description,
platform-specific string handling, `shell=True` / `Git.USE_SHELL`
warning, qualified `shlex.split` note) and cross-reference
`USE_SHELL` for the long-form security discussion rather than
reproducing it.
Add a related paragraph to `:param shell:` noting `shlex.split`'s
narrow value as a transitional tool when migrating existing
string-command `shell=True` calls on Unix-like systems, with extreme
care, and cross-referencing `:param command:` for the risks.
This fixesgitpython-developers#2146. Only documentation is changed.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@EliahKagan
EliahKaganforce-pushed the claude/execute-string-platform-docs branch from 1a3e3dd to 010a7bbCompareMay 10, 2026 00:37
@EliahKagan
EliahKagan requested a review from CopilotMay 10, 2026 00:39

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

Comment threadgit/cmd.py
Comment threadgit/cmd.py
Comment threadgit/cmd.py Outdated
To avert the mistake of not removing `shell=True` when using it.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@EliahKagan
EliahKaganforce-pushed the claude/execute-string-platform-docs branch from c42fc0b to 38d62c4CompareMay 10, 2026 01:22
@EliahKagan
EliahKagan requested a review from CopilotMay 10, 2026 01:25

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@EliahKaganEliahKagan left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is in good shape. By building on #2144, I've tried to combine the benefits of the way things were both before and after #2144, while addressing the disadvantages of both. I plan to merge this once CI passes.

@EliahKagan
EliahKagan marked this pull request as ready for review May 10, 2026 01:38
@EliahKagan
EliahKagan merged commit 830471c into gitpython-developers:mainMay 10, 2026
34 checks passed
@EliahKagan
EliahKagan deleted the claude/execute-string-platform-docs branch May 10, 2026 01:39
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Git.execute doc is inaccurate on Windows and recommends shlex too broadly

2 participants

@EliahKagan