Skip to content

Clarify USE_SHELL warning helper signature #2045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

EliahKagan
Copy link
Member

This is a minor refactor of how _warn_use_shell can be, and is, invoked.

The _warn_use_shell helper function in git.cmd takes a single bool-valued argument extra_danger, which is conceptually associated with having a True value of USE_SHELL, but the association is not necessarily obvious. Specifically:

  • For the warning given when reading USE_SHELL on the Git class or through an instance, extra_danger is always False. This is so even if the USE_SHELL value is currently True, because the danger that arises from True occurs internally.
  • For the warning given when writing USE_SHELL, which can only be done on the Git class and not on or through an instance, extra_danger is the value set for the attribute. This is because setting USE_SHELL to True incurs the danger described in #1896.

When reading the code, which passed extra_danger positionally, the meaning of the parameter may not always have been obvious.

This makes the extra_danger parameter keyword-only, and passes it by keyword in all invocations, so that its meaning is clearer.

This is a minor refactor of how `_warn_use_shell` can be, and is,
invoked.

The `_warn_use_shell` helper function in `git.cmd` takes a single
`bool`-valued argument `extra_danger`, which is conceptually
associated with having a `True` value of `USE_SHELL`, but the
association is not necessarily obvious. Specifically:

- For the warning given when reading `USE_SHELL` on the `Git` class
  or through an instance, `extra_danger` is always `False`. This is
  so even if the `USE_SHELL` value is currently `True`, because the
  danger that arises from `True` occurs internally.

- For the warning given when writing `USE_SHELL`, which can only be
  done on the `Git` class and not on or through an instance,
  `extra_danger` is the value set for the attribute. This is
  because setting `USE_SHELL` to `True` incurs the danger described
  in gitpython-developers#1896.

When reading the code, which passed `extra_danger` positionally,
the meaning of the parameter may not always have been obvious.

This makes the `extra_danger` parameter keyword-only, and passes
it by keyword in all invocations, so that its meaning is clearer.
@EliahKagan EliahKagan force-pushed the shell-warning-refactor branch from f556490 to 253099f Compare June 7, 2025 22:47
@EliahKagan EliahKagan marked this pull request as ready for review June 7, 2025 23:05
@EliahKagan EliahKagan merged commit 2e0d835 into gitpython-developers:main Jun 7, 2025
27 checks passed
@EliahKagan EliahKagan deleted the shell-warning-refactor branch June 7, 2025 23:05
Sign up for free to 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.

1 participant