8000 Merge pull request #1687 from EliahKagan/popen-shell · gitpython-developers/GitPython@874f0bf · GitHub
[go: up one dir, main page]
Skip to content

Navigation Menu

Sign in

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 874f0bf

Browse files
ByronByron
authored
Merge pull request #1687 from EliahKagan/popen-shell
Fix Git.execute shell use and reporting bugs
2 parents f9a3b83 + baf3457 commit 874f0bf

File tree

3 files changed

+63
-9
lines changed

3 files changed

+63
-9
lines changed

‎git/cmd.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -974,6 +974,8 @@ def execute(
974974
istream_ok = "None"
975975
if istream:
976976
istream_ok = "<valid stream>"
977+
if shell is None:
978+
shell = self.USE_SHELL
977979
log.debug(
978980
"Popen(%s, cwd=%s, universal_newlines=%s, shell=%s, istream=%s)",
979981
redacted_command,
@@ -992,7 +994,7 @@ def execute(
992994
stdin=istream or DEVNULL,
993995
stderr=PIPE,
994996
stdout=stdout_sink,
995-
shell=shell is not None and shell or self.USE_SHELL,
997+
shell=shell,
996998
close_fds=is_posix, # unsupported on windows
997999
universal_newlines=universal_newlines,
9981000
creationflags=PROC_CREATIONFLAGS,

‎test-requirements.txt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
black
22
coverage[toml]
3-
ddt>=1.1.1, !=1.4.3
3+
ddt >= 1.1.1, != 1.4.3
4+
mock ; python_version < "3.8"
45
mypy
56
pre-commit
67
pytest

‎test/test_git.py

Lines changed: 58 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,23 +4,31 @@
44
#
55
# This module is part of GitPython and is released under
66
# the BSD License: https://opensource.org/license/bsd-3-clause/
7+
import contextlib
8+
import logging
79
import os
10+
import os.path as osp
11+
import re
812
import shutil
913
import subprocess
1014
import sys
1115
from tempfile import TemporaryDirectory, TemporaryFile
12-
from unittest import mock, skipUnless
16+
from unittest import skipUnless
1317

14-
from git import Git, refresh, GitCommandError, GitCommandNotFound, Repo, cmd
15-
from test.lib import TestBase, fixture_path
16-
from test.lib import with_rw_directory
17-
from git.util import cwd, finalize_process
18+
if sys.version_info >= (3, 8):
19+
from unittest import mock
20+
else:
21+
import mock # To be able to examine call_args.kwargs on a mock.
1822

19-
import os.path as osp
23+
import ddt
2024

25+
from git import Git, refresh, GitCommandError, GitCommandNotFound, Repo, cmd
2126
from git.compat import is_win
27+
from git.util import cwd, finalize_process
28+
from test.lib import TestBase, fixture_path, with_rw_directory
2229

2330

31+
@ddt.ddt
2432
class TestGit(TestBase):
2533
@classmethod
2634
def setUpClass(cls):
@@ -73,7 +81,50 @@ def test_it_transforms_kwargs_into_git_command_arguments(self):
7381
res = self.git.transform_kwargs(**{"s": True, "t": True})
7482
self.assertEqual({"-s", "-t"}, set(res))
7583

76-
def test_it_executes_git_to_shell_and_returns_result(self):
84+
_shell_cases = (
85+
# value_in_call, value_from_class, expected_popen_arg
86+
(None, False, False),
87+
(None, True, True),
88+
(False, True, False),
89+
(False, False, False),
90+
(True, False, True),
91+
(True, True, True),
92+
)
93+
94+
def _do_shell_combo(self, value_in_call, value_from_class):
95+
with mock.patch.object(Git, "USE_SHELL", value_from_class):
96+
# git.cmd gets Popen via a "from" import, so patch it there.
97+
with mock.patch.object(cmd, "Popen", wraps=cmd.Popen) as mock_popen:
98+
# Use a command with no arguments (besides the program name), so it runs
99+
# with or without a shell, on all OSes, with the same effect. Since git
100+
# errors out when run with no arguments, we swallow that error.
101+
with contextlib.suppress(GitCommandError):
102+
self.git.execute(["git"], shell=value_in_call)
103+
104+
return mock_popen
105+
106+
@ddt.idata(_shell_cases)
107+
def test_it_uses_shell_or_not_as_specified(self, case):
108+
"""A bool passed as ``shell=`` takes precedence over `Git.USE_SHELL`."""
109+
value_in_call, value_from_class, expected_popen_arg = case
110+
mock_popen = self._do_shell_combo(value_in_call, value_from_class)
111+
mock_popen.assert_called_once()
112+
self.assertIs(mock_popen.call_args.kwargs["shell"], expected_popen_arg)
113+
114+
@ddt.idata(full_case[:2] for full_case in _shell_cases)
115+
def test_it_logs_if_it_uses_a_shell(self, case):
116+
"""``shell=`` in the log message agrees with what is passed to `Popen`."""
117+
value_in_call, value_from_class = case
118+
119+
with self.assertLogs(cmd.log, level=logging.DEBUG) as log_watcher:
120+
mock_popen = self._do_shell_combo(value_in_call, value_from_class)
121+
122+
popen_shell_arg = mock_popen.call_args.kwargs["shell"]
123+
expected_message = re.compile(rf"DEBUG:git.cmd:Popen\(.*\bshell={popen_shell_arg}\b.*\)")
124+
match_attempts = [expected_message.fullmatch(message) for message in log_watcher.output]
125+
self.assertTrue(any(match_attempts), repr(log_watcher.output))
126+
127+
def test_it_executes_git_and_returns_result(self):
77128
self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$")
78129

79130
def test_it_executes_git_not_from_cwd(self):
 388E

0 commit comments

Comments
 (0)
0