Fix Git.execute shell use and reporting bugs by EliahKagan · Pull Request #1687 · gitpython-developers/GitPython
added 7 commits
October 2, 2023 04:27That 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, 2023This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters