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.

Copilot AI 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 thread git/cmd.py Outdated
@EliahKagan
EliahKagan force-pushed the claude/execute-string-platform-docs branch 2 times, most recently from 19bc03b to c871adc Compare May 10, 2026 00:10
@EliahKagan
EliahKagan requested a review from Copilot May 10, 2026 00:13

Copilot AI 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 thread git/cmd.py Outdated
@EliahKagan
EliahKagan force-pushed the claude/execute-string-platform-docs branch from c871adc to 1a3e3dd Compare May 10, 2026 00:34
…opers#2146

gitpython-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 fixes gitpython-developers#2146. Only documentation is changed.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@EliahKagan
EliahKagan force-pushed the claude/execute-string-platform-docs branch from 1a3e3dd to 010a7bb Compare May 10, 2026 00:37
@EliahKagan
EliahKagan requested a review from Copilot May 10, 2026 00:39

Copilot AI 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 thread git/cmd.py
Comment thread git/cmd.py
Comment thread git/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
EliahKagan force-pushed the claude/execute-string-platform-docs branch from c42fc0b to 38d62c4 Compare May 10, 2026 01:22
@EliahKagan
EliahKagan requested a review from Copilot May 10, 2026 01:25

Copilot AI 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.

@EliahKagan EliahKagan left a comment

Copy link
Copy Markdown
Member Author

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:main May 10, 2026
34 checks passed
@EliahKagan
EliahKagan deleted the claude/execute-string-platform-docs branch May 10, 2026 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants