That test is not testing whether or not a shell is used (nor doesit need to test that), but just whether the library actually runsgit, passes an argument to it, and returns text from its stdout.

In the Popen calls in Git.execute, for all combinations of allowedvalues for Git.USE_SHELL and the shell= keyword argument.

The log message shows "Popen(...)", not "execute(...)". So itshould faithfully report what is about to be passed to Popen incases where a user reading the log would otherwise be misled intothinking this is what has happened.Reporting the actual "shell=" argument passed to Popen is also moreuseful to log than the argument passed to Git.execute (at least ifonly one of them is to be logged).

Both new shell-related tests were causing the code under test tosplit "git" into ["g", "i", "t"] and thus causing the code undertest to attempt to execute a "g" command. This passes the commandas a one-element list of strings rather than as a string, whichavoids this on all operating systems regardless of whether the codeunder test has a bug being tested for.This would not occur on Windows, which does not iterate commands oftype str character-by-character even when the command is runwithout 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 errormessage. But this is also important because it is possible for theuser who runs the tests to have a "g" command, in which case itcould be very inconvenient, or even unsafe, to run "g". This shouldbe avoided even when the code under test has a bug that causes ashell to be used when it shouldn't or not used when it should, sohaving separate commands (list and str) per test case parameterswould not be a sufficient solution: it would only guard againstrunning "g" when a bug in the code under test were absent.

This also helps mock Popen over a smaller scope, which may bebeneficial (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 examinem.call_args.kwargs where m is a Mock or MagicMock, was introducedin Python 3.8.Currently it is only in test/test_git.py that any use of mocksrequires this, so I've put the conditional import logic to importmock (the top-level package) rather than unittest.mock only there.The mock library is added as a development (testing) dependencyonly when the Python version is lower than 3.8, so it is notinstalled 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. Thosejust-introduced tests need this, to be able to usemock_popen.call_args.kwargs on Python 3.7.

This updates the shell variable itself, only when it is None, fromself.USE_SHELL.(That attribute is usually Git.USE_SHELL rather than an instanceattribute. But although people probably shouldn't set it oninstances, 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.

Byron left a comment