Content-Length: 17459 | pFad | http://github.com/gitpython-developers/GitPython/pull/1687.patch

thub.com From 5e71c270b2ea0adfc5c0a103fce33ab6acf275b1 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 1 Oct 2023 22:43:36 -0400 Subject: [PATCH 1/7] Fix the name of the "executes git" test 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. --- test/test_git.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_git.py b/test/test_git.py index 481309538..a21a3c78e 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -73,7 +73,7 @@ def test_it_transforms_kwargs_into_git_command_arguments(self): res = self.git.transform_kwargs(**{"s": True, "t": True}) self.assertEqual({"-s", "-t"}, set(res)) - def test_it_executes_git_to_shell_and_returns_result(self): + def test_it_executes_git_and_returns_result(self): self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$") def test_it_executes_git_not_from_cwd(self): From 59440607406873a28788ca38526871749c5549f9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 2 Oct 2023 00:17:05 -0400 Subject: [PATCH 2/7] Test whether a shell is used In the Popen calls in Git.execute, for all combinations of allowed values for Git.USE_SHELL and the shell= keyword argument. --- test/test_git.py | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index a21a3c78e..6fd2c8268 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -4,23 +4,24 @@ # # This module is part of GitPython and is released under # the BSD License: https://opensource.org/license/bsd-3-clause/ +import contextlib import os +import os.path as osp import shutil import subprocess import sys from tempfile import TemporaryDirectory, TemporaryFile from unittest import mock, skipUnless -from git import Git, refresh, GitCommandError, GitCommandNotFound, Repo, cmd -from test.lib import TestBase, fixture_path -from test.lib import with_rw_directory -from git.util import cwd, finalize_process - -import os.path as osp +import ddt +from git import Git, refresh, GitCommandError, GitCommandNotFound, Repo, cmd from git.compat import is_win +from git.util import cwd, finalize_process +from test.lib import TestBase, fixture_path, with_rw_directory +@ddt.ddt class TestGit(TestBase): @classmethod def setUpClass(cls): @@ -73,6 +74,28 @@ def test_it_transforms_kwargs_into_git_command_arguments(self): res = self.git.transform_kwargs(**{"s": True, "t": True}) self.assertEqual({"-s", "-t"}, set(res)) + @ddt.data( + (None, False, False), + (None, True, True), + (False, True, False), + (False, False, False), + (True, False, True), + (True, True, True), + ) + @mock.patch.object(cmd, "Popen", wraps=cmd.Popen) # Since it is gotten via a "from" import. + def test_it_uses_shell_or_not_as_specified(self, case, mock_popen): + """A bool passed as ``shell=`` takes precedence over `Git.USE_SHELL`.""" + value_in_call, value_from_class, expected_popen_arg = case + # FIXME: Check what gets logged too! + with mock.patch.object(Git, "USE_SHELL", value_from_class): + with contextlib.suppress(GitCommandError): + self.git.execute( + "git", # No args, so it runs with or without a shell, on all OSes. + shell=value_in_call, + ) + mock_popen.assert_called_once() + self.assertIs(mock_popen.call_args.kwargs["shell"], expected_popen_arg) + def test_it_executes_git_and_returns_result(self): self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$") From aa5e2f6b24e36d2d38e84e7f2241b104318396c3 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 2 Oct 2023 01:45:17 -0400 Subject: [PATCH 3/7] Test if whether a shell is used is logged 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). --- test/test_git.py | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index 6fd2c8268..389e80552 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -5,8 +5,10 @@ # This module is part of GitPython and is released under # the BSD License: https://opensource.org/license/bsd-3-clause/ import contextlib +import logging import os import os.path as osp +import re import shutil import subprocess import sys @@ -74,7 +76,8 @@ def test_it_transforms_kwargs_into_git_command_arguments(self): res = self.git.transform_kwargs(**{"s": True, "t": True}) self.assertEqual({"-s", "-t"}, set(res)) - @ddt.data( + _shell_cases = ( + # value_in_call, value_from_class, expected_popen_arg (None, False, False), (None, True, True), (False, True, False), @@ -82,20 +85,42 @@ def test_it_transforms_kwargs_into_git_command_arguments(self): (True, False, True), (True, True, True), ) + + @ddt.idata(_shell_cases) @mock.patch.object(cmd, "Popen", wraps=cmd.Popen) # Since it is gotten via a "from" import. def test_it_uses_shell_or_not_as_specified(self, case, mock_popen): """A bool passed as ``shell=`` takes precedence over `Git.USE_SHELL`.""" value_in_call, value_from_class, expected_popen_arg = case - # FIXME: Check what gets logged too! + with mock.patch.object(Git, "USE_SHELL", value_from_class): with contextlib.suppress(GitCommandError): self.git.execute( "git", # No args, so it runs with or without a shell, on all OSes. shell=value_in_call, ) + mock_popen.assert_called_once() self.assertIs(mock_popen.call_args.kwargs["shell"], expected_popen_arg) + @ddt.idata(full_case[:2] for full_case in _shell_cases) + @mock.patch.object(cmd, "Popen", wraps=cmd.Popen) # Since it is gotten via a "from" import. + def test_it_logs_if_it_uses_a_shell(self, case, mock_popen): + """``shell=`` in the log message agrees with what is passed to `Popen`.""" + value_in_call, value_from_class = case + + with self.assertLogs(cmd.log, level=logging.DEBUG) as log_watcher: + with mock.patch.object(Git, "USE_SHELL", value_from_class): + with contextlib.suppress(GitCommandError): + self.git.execute( + "git", # No args, so it runs with or without a shell, on all OSes. + shell=value_in_call, + ) + + popen_shell_arg = mock_popen.call_args.kwargs["shell"] + expected_message = re.compile(rf"DEBUG:git.cmd:Popen$.*\bshell={popen_shell_arg}\b.*$") + match_attempts = [expected_message.fullmatch(message) for message in log_watcher.output] + self.assertTrue(any(match_attempts), repr(log_watcher.output)) + def test_it_executes_git_and_returns_result(self): self.assertRegex(self.git.execute(["git", "version"]), r"^git version [\d\.]{2}.*$") From 0f19fb0be1bd3ccd3ff8f35dba9e924c9d379e41 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 2 Oct 2023 02:21:10 -0400 Subject: [PATCH 4/7] Fix tests so they don't try to run "g" 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. --- test/test_git.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index 389e80552..7e8e7c805 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -95,7 +95,7 @@ def test_it_uses_shell_or_not_as_specified(self, case, mock_popen): with mock.patch.object(Git, "USE_SHELL", value_from_class): with contextlib.suppress(GitCommandError): self.git.execute( - "git", # No args, so it runs with or without a shell, on all OSes. + ["git"], # No args, so it runs with or without a shell, on all OSes. shell=value_in_call, ) @@ -112,7 +112,7 @@ def test_it_logs_if_it_uses_a_shell(self, case, mock_popen): with mock.patch.object(Git, "USE_SHELL", value_from_class): with contextlib.suppress(GitCommandError): self.git.execute( - "git", # No args, so it runs with or without a shell, on all OSes. + ["git"], # No args, so it runs with or without a shell, on all OSes. shell=value_in_call, ) From da3460c6cc3a7c6981dfd1d4675d167a7a5f2b0c Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 2 Oct 2023 02:56:37 -0400 Subject: [PATCH 5/7] Extract shared test logic to a helper 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). --- test/test_git.py | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/test/test_git.py b/test/test_git.py index 7e8e7c805..343bf7a19 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -86,35 +86,33 @@ def test_it_transforms_kwargs_into_git_command_arguments(self): (True, True, True), ) + def _do_shell_combo(self, value_in_call, value_from_class): + with mock.patch.object(Git, "USE_SHELL", value_from_class): + # git.cmd gets Popen via a "from" import, so patch it there. + with mock.patch.object(cmd, "Popen", wraps=cmd.Popen) as mock_popen: + # Use a command with no arguments (besides the program name), so it runs + # with or without a shell, on all OSes, with the same effect. Since git + # errors out when run with no arguments, we swallow that error. + with contextlib.suppress(GitCommandError): + self.git.execute(["git"], shell=value_in_call) + + return mock_popen + @ddt.idata(_shell_cases) - @mock.patch.object(cmd, "Popen", wraps=cmd.Popen) # Since it is gotten via a "from" import. - def test_it_uses_shell_or_not_as_specified(self, case, mock_popen): + def test_it_uses_shell_or_not_as_specified(self, case): """A bool passed as ``shell=`` takes precedence over `Git.USE_SHELL`.""" value_in_call, value_from_class, expected_popen_arg = case - - with mock.patch.object(Git, "USE_SHELL", value_from_class): - with contextlib.suppress(GitCommandError): - self.git.execute( - ["git"], # No args, so it runs with or without a shell, on all OSes. - shell=value_in_call, - ) - + mock_popen = self._do_shell_combo(value_in_call, value_from_class) mock_popen.assert_called_once() self.assertIs(mock_popen.call_args.kwargs["shell"], expected_popen_arg) @ddt.idata(full_case[:2] for full_case in _shell_cases) - @mock.patch.object(cmd, "Popen", wraps=cmd.Popen) # Since it is gotten via a "from" import. - def test_it_logs_if_it_uses_a_shell(self, case, mock_popen): + def test_it_logs_if_it_uses_a_shell(self, case): """``shell=`` in the log message agrees with what is passed to `Popen`.""" value_in_call, value_from_class = case with self.assertLogs(cmd.log, level=logging.DEBUG) as log_watcher: - with mock.patch.object(Git, "USE_SHELL", value_from_class): - with contextlib.suppress(GitCommandError): - self.git.execute( - ["git"], # No args, so it runs with or without a shell, on all OSes. - shell=value_in_call, - ) + mock_popen = self._do_shell_combo(value_in_call, value_from_class) popen_shell_arg = mock_popen.call_args.kwargs["shell"] expected_message = re.compile(rf"DEBUG:git.cmd:Popen$.*\bshell={popen_shell_arg}\b.*$") From 41294d578471f7f63c02bf59e8abc3459f9d6390 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 2 Oct 2023 03:26:02 -0400 Subject: [PATCH 6/7] Use the mock backport on Python 3.7 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. --- test-requirements.txt | 3 ++- test/test_git.py | 7 ++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/test-requirements.txt b/test-requirements.txt index 1c08c736f..9414da09c 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -1,6 +1,7 @@ black coverage[toml] -ddt>=1.1.1, !=1.4.3 +ddt >= 1.1.1, != 1.4.3 +mock ; python_version < "3.8" mypy pre-commit pytest diff --git a/test/test_git.py b/test/test_git.py index 343bf7a19..583c74fa3 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -13,7 +13,12 @@ import subprocess import sys from tempfile import TemporaryDirectory, TemporaryFile -from unittest import mock, skipUnless +from unittest import skipUnless + +if sys.version_info >= (3, 8): + from unittest import mock +else: + import mock # To be able to examine call_args.kwargs on a mock. import ddt From baf3457ec8f92c64d2481b812107e6acc4059ddd Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 2 Oct 2023 04:11:18 -0400 Subject: [PATCH 7/7] Fix Git.execute shell use and reporting bugs 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. --- git/cmd.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/git/cmd.py b/git/cmd.py index 9921dd6c9..53b8b9050 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -974,6 +974,8 @@ def execute( istream_ok = "None" if istream: istream_ok = "" + if shell is None: + shell = self.USE_SHELL log.debug( "Popen(%s, cwd=%s, universal_newlines=%s, shell=%s, istream=%s)", redacted_command, @@ -992,7 +994,7 @@ def execute( stdin=istream or DEVNULL, stderr=PIPE, stdout=stdout_sink, - shell=shell is not None and shell or self.USE_SHELL, + shell=shell, close_fds=is_posix, # unsupported on windows universal_newlines=universal_newlines, creationflags=PROC_CREATIONFLAGS,

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!