Skip to content

Commit

Permalink
Expand USE_SHELL docstring; clarify a test usage
Browse files Browse the repository at this point in the history
In the USE_SHELL docstring:

- Restore the older wording "when executing git commands" rather
  than "to execute git commands". I've realized that longer phrase,
  which dates back to the introduction of USE_SHELL in 1c2dd54, is
  clearer, because readers less familiar with GitPython's overall
  design and operation will still not be misled into thinking
  USE_SHELL ever affects whether GitPython uses an external git
  command, versus some other mechanism, to do something.

- Give some more information about why USE_SHELL = True is unsafe
  (though further revision or clarification may be called for).

- Note some non-obvious aspects of USE_SHELL, that some of the way
  it interacts with other aspects of GitPython is not part of what
  is or has been documented about it, and in practice changes over
  time. The first example relates to #1792; the second may help
  users understand why code that enables USE_SHELL on Windows, in
  addition to being unsafe there, often breaks immediately on other
  platforms; the third is included so the warnings in the expanded
  docstring are not interpreted as a new commitment that any shell
  syntax that may have a *desired* effect in some application will
  continue to have the same effect in the future.

- Cover a second application that might lead, or have led, to
  setting USE_SHELL to True, and explain what to do instead.

In test_successful_default_refresh_invalidates_cached_version_info:

- Rewrite the commented explanation of a special variant of that
  second application, where the usual easy alternatives are not
  used because part of the goal of the test is to check a "default"
  scenario that does not include either of them. This better
  explains why that choice is made in the test, and also hopefully
  will prevent anyone from thinking that test is a model for
  another situation (which, as noted, is unlikely to be the case
  even in unit tests).
  • Loading branch information
EliahKagan committed Mar 29, 2024
1 parent 3da47c2 commit bdabb21
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 16 deletions.
40 changes: 31 additions & 9 deletions git/cmd.py
Original file line number Diff line number Diff line change
Expand Up @@ -405,23 +405,45 @@ def __setstate__(self, d: Dict[str, Any]) -> None:
"""Enables debugging of GitPython's git commands."""

USE_SHELL: bool = False
"""Deprecated. If set to ``True``, a shell will be used to execute git commands.
"""Deprecated. If set to ``True``, a shell will be used when executing git commands.
Code that uses ``USE_SHELL = True`` or that passes ``shell=True`` to any GitPython
functions should be updated to use the default value of ``False`` instead. ``True``
is unsafe unless the effect of syntax treated specially by the shell is fully
considered and accounted for, which is not possible under most circumstances. As
detailed below, it is also no longer needed, even where it had been in the past.
In addition, how a value of ``True`` interacts with some aspects of GitPython's
operation is not precisely specified and may change without warning, even before
GitPython 4.0.0 when :attr:`USE_SHELL` may be removed. This includes:
* Whether or how GitPython automatically customizes the shell environment.
* Whether, outside of Windows (where :class:`subprocess.Popen` supports lists of
separate arguments even when ``shell=True``), this can be used with any GitPython
functionality other than direct calls to the :meth:`execute` method.
* Whether any GitPython feature that runs git commands ever attempts to partially
sanitize data a shell may treat specially. Currently this is not done.
Prior to GitPython 2.0.8, this had a narrow purpose in suppressing console windows
in graphical Windows applications. In 2.0.8 and higher, it provides no benefit, as
GitPython solves that problem more robustly and safely by using the
``CREATE_NO_WINDOW`` process creation flag on Windows.
Code that uses ``USE_SHELL = True`` or that passes ``shell=True`` to any GitPython
functions should be updated to use the default value of ``False`` instead. ``True``
is unsafe unless the effect of shell expansions is fully considered and accounted
for, which is not possible under most circumstances.
Because Windows path search differs subtly based on whether a shell is used, in rare
cases changing this from ``True`` to ``False`` may keep an unusual git "executable",
such as a batch file, from being found. To fix this, set the command name or full
path in the :envvar:`GIT_PYTHON_GIT_EXECUTABLE` environment variable or pass the
full path to :func:`git.refresh` (or invoke the script using a ``.exe`` shim).
See:
Further reading:
- :meth:`Git.execute` (on the ``shell`` parameter).
- https://github.com/gitpython-developers/GitPython/commit/0d9390866f9ce42870d3116094cd49e0019a970a
- https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags
* :meth:`Git.execute` (on the ``shell`` parameter).
* https://github.com/gitpython-developers/GitPython/commit/0d9390866f9ce42870d3116094cd49e0019a970a
* https://learn.microsoft.com/en-us/windows/win32/procthread/process-creation-flags
* https://github.com/python/cpython/issues/91558#issuecomment-1100942950
* https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw
"""

_git_exec_env_var = "GIT_PYTHON_GIT_EXECUTABLE"
Expand Down
14 changes: 7 additions & 7 deletions test/test_git.py
Original file line number Diff line number Diff line change
Expand Up @@ -669,13 +669,13 @@ def test_successful_default_refresh_invalidates_cached_version_info(self):
stack.enter_context(_patch_out_env("GIT_PYTHON_GIT_EXECUTABLE"))

if sys.platform == "win32":
# On Windows, use a shell so "git" finds "git.cmd". (In the infrequent
# case that this effect is desired in production code, it should not be
# done with this technique. USE_SHELL=True is less secure and reliable,
# as unintended shell expansions can occur, and is deprecated. Instead,
# use a custom command, by setting the GIT_PYTHON_GIT_EXECUTABLE
# environment variable to git.cmd or by passing git.cmd's full path to
# git.refresh. Or wrap the script with a .exe shim.)
# On Windows, use a shell so "git" finds "git.cmd". The correct and safe
# ways to do this straightforwardly are to set GIT_PYTHON_GIT_EXECUTABLE
# to git.cmd in the environment, or call git.refresh with the command's
# full path. See the Git.USE_SHELL docstring for deprecation details.
# But this tests a "default" scenario where neither is done. The
# approach used here, setting USE_SHELL to True so PATHEXT is honored,
# should not be used in production code (nor even in most test cases).
stack.enter_context(mock.patch.object(Git, "USE_SHELL", True))

new_git = Git()
Expand Down

0 comments on commit bdabb21

Please sign in to comment.