Fix Git.execute shell use and reporting bugs by EliahKagan · Pull Request #1687 · gitpython-developers/GitPython

added 7 commits

October 2, 2023 04:27
That test is not testing whether or not a shell is used (nor does
it need to test that), but just whether the library actually runs
git, passes an argument to it, and returns text from its stdout.
In the Popen calls in Git.execute, for all combinations of allowed
values for Git.USE_SHELL and the shell= keyword argument.
The log message shows "Popen(...)", not "execute(...)". So it
should faithfully report what is about to be passed to Popen in
cases where a user reading the log would otherwise be misled into
thinking this is what has happened.

Reporting the actual "shell=" argument passed to Popen is also more
useful to log than the argument passed to Git.execute (at least if
only one of them is to be logged).
Both new shell-related tests were causing the code under test to
split "git" into ["g", "i", "t"] and thus causing the code under
test to attempt to execute a "g" command. This passes the command
as a one-element list of strings rather than as a string, which
avoids this on all operating systems regardless of whether the code
under test has a bug being tested for.

This would not occur on Windows, which does not iterate commands of
type str character-by-character even when the command is run
without a shell. But it did happen on other systems.

Most of the time, the benefit of using a command that actually runs
"git" rather than "g" is the avoidance of confusion in the error
message. But this is also important because it is possible for the
user who runs the tests to have a "g" command, in which case it
could be very inconvenient, or even unsafe, to run "g". This should
be avoided even when the code under test has a bug that causes a
shell to be used when it shouldn't or not used when it should, so
having separate commands (list and str) per test case parameters
would not be a sufficient solution: it would only guard against
running "g" when a bug in the code under test were absent.
This also helps mock Popen over a smaller scope, which may be
beneficial (especially if it is mocked in the subprocess module,
rather than the git.cmd module, in the future).
Because mock.call.kwargs, i.e. the ability to examine
m.call_args.kwargs where m is a Mock or MagicMock, was introduced
in Python 3.8.

Currently it is only in test/test_git.py that any use of mocks
requires this, so I've put the conditional import logic to import
mock (the top-level package) rather than unittest.mock only there.

The mock library is added as a development (testing) dependency
only when the Python version is lower than 3.8, so it is not
installed when not needed.

This fixes a problem in the new tests of whether a shell is used,
and reported as used, in the Popen call in Git.execute. Those
just-introduced tests need this, to be able to use
mock_popen.call_args.kwargs on Python 3.7.
This updates the shell variable itself, only when it is None, from
self.USE_SHELL.

(That attribute is usually Git.USE_SHELL rather than an instance
attribute. But although people probably shouldn't set it on
instances, people will expect that this is permitted.)

Now:

- USE_SHELL is used as a fallback only. When shell=False is passed,
  USE_SHELL is no longer consuled. Thus shell=False always keeps a
  shell from being used, even in the non-default case where the
  USE_SHELL attribue has been set to True.

- The debug message printed to the log shows the actual value that
  is being passed to Popen, because the updated shell variable is
  used both to produce that message and in the Popen call.

This was referenced

Oct 2, 2023

renovate bot referenced this pull request in allenporter/flux-local

Oct 20, 2023

@renovate