Skip to content

Fix Git.execute shell use and reporting bugs #1687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Oct 2, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
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.
  • Loading branch information
EliahKagan committed Oct 2, 2023
commit 0f19fb0be1bd3ccd3ff8f35dba9e924c9d379e41
4 changes: 2 additions & 2 deletions test/test_git.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)

Expand All @@ -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,
)

Expand Down