Fixes #1685
Fixes #1686
Changes to git module code
This updates the shell variable itself, only when it is None, from self.USE_SHELL.
(That attribute usually gets its value from Git.USE_SHELL rather than being separately an instance attribute. But self.USE_SHELL is an idiomatic way to access it. Furthermore, although people probably shouldn't set it on instances, people may expect that this is permitted.)
Now:
USE_SHELL is used as a fallback only. When shell=False is passed, USE_SHELL is no longer consulted. Thus shell=False always keeps a shell from being used, even in the non-default case where the USE_SHELL attribute 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.
New tests
This also (actually, first) adds tests, some to check that the correct value is always passed as shell= when Git.execute calls Popen, and others to check that whatever it passes is the same as it claims to be passing in the log. I've made them two separate test methods rather than a test method with multiple assertions, because I think the results of these particular tests are easier to understand that way (even compared to the option of using self.subTest). But the major shared logic is extracted to a helper function, where its important subtleties are commented. I have verified that the tests fail, in the expected way, before the bugs are fixed, and then pass, and that these are both the case even on native Windows where we don't (yet) have CI jobs.
The tests cover the six combinations of valid values of the shell= argument to Git.execute (None, False, and True) and Git.USE_SHELL (False and True). I used ddt for this, so there are only two test methods, even though there are six test cases. The tests are simple in principle, with the main source of complexity being the question of how to deal with how the command argument to Popen typically differs, including by type, based on the shell argument, yet the tests must not misbehave, nor induce extraneous misbehavior, even when the code under test has a bug that would ordinarily prevent such agreement.
On Python 3.7 only, this adds mock as a development dependency (in the test extra, not as a regular dependency). mock is a backport of unittest.mock, and a couple of my assertions use a handy feature that was introduced to the mock library in Python 3.8. Besides being harder to write without it, I believe it would be less clearly expressed. (Details are commented with the import logic.)
While adding the new tests, I also renamed test_it_executes_git_to_shell_and_returns_result to test_it_executes_git_and_returns_result, because that test case does not test that a shell is used to run git (and a shell is not used in that test).
%5D(https%3A%2F%2Frenovatebot.com){kind=link}