Skip to content

Clarify Git.execute and Popen arguments #1688

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.

Sign up for GitHub

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 11 commits into from
Oct 3, 2023
+91 −74
Merged

Clarify Git.execute and Popen arguments #1688

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b79198a
Document Git.execute parameters in definition order
EliahKagan Oct 3, 2023
13e1593
Don't say Git.execute uses a shell, in its summary
EliahKagan Oct 3, 2023
74b68e9
Copyedit Git.execute docstring
EliahKagan Oct 3, 2023
133271b
Other copyediting in the git.cmd module
EliahKagan Oct 3, 2023
fc755da
Avoid having a local function seem to be a method
EliahKagan Oct 3, 2023
2d1efdc
Test that git.cmd.execute_kwargs is correct
EliahKagan Oct 3, 2023
a8a43fe
Simplify shell test helper with with_exceptions=False
EliahKagan Oct 3, 2023
9fa1cee
Extract a _assert_logged_for_popen method
EliahKagan Oct 3, 2023
790a790
Log stdin arg as such, and test that this is done
EliahKagan Oct 3, 2023
c3fde7f
Log args in the order they are passed to Popen
EliahKagan Oct 3, 2023
ab95886
Eliminate istream_ok variable
EliahKagan Oct 3, 2023
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
Test that git.cmd.execute_kwargs is correct
  • Loading branch information
@EliahKagan
EliahKagan committed Oct 3, 2023
commit 2d1efdca84e266a422f4298ee94ee9b8dae6c32e
9 changes: 9 additions & 0 deletions test/test_git.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
# This module is part of GitPython and is released under
# the BSD License: https://opensource.org/license/bsd-3-clause/
import contextlib
import inspect
import logging
import os
import os.path as osp
Expand Down Expand Up @@ -364,3 +365,11 @@ def counter_stderr(line):

self.assertEqual(count[1], line_count)
self.assertEqual(count[2], line_count)

def test_execute_kwargs_set_agrees_with_method(self):
parameter_names = inspect.signature(cmd.Git.execute).parameters.keys()
self_param, command_param, *most_params, extra_kwargs_param = parameter_names
self.assertEqual(self_param, "self")
self.assertEqual(command_param, "command")
self.assertEqual(set(most_params), cmd.execute_kwargs) # Most important.
self.assertEqual(extra_kwargs_param, "subprocess_kwargs")
Comment on lines +378 to +384
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or git.cmd.execute_kwargs could be generated in a manner similar to this, and this test, the need to manually update it, and the note in the Git.execute docstring about that need, could all be done away with. Maybe something like this:

execute_kwargs = inspect.signature(Git.execute).parameters.keys() - {"self", "command", "subprocess_kwargs"}

Right now it is defined very high up in the module, even higher than __all__, and this suggests that it may be intended to be available for static inspection. But I don't think any tools will statically inspect a set of strings like that (plus, static analysis tools can examine the parameters of Git.execute... though the incomplete lists of parameters in the @overload-decorated stubs that precede it confuse the situation somewhat).

git.cmd.__all__ contains only "Git" and I hope that means code that uses GitPython should not be using git.cmd.execute_kwargs or relying on its presence. If possible, perhaps it could be made more explicitly private (_execute_kwargs, or if it needs to remain statically defined, maybe _EXECUTE_KWARGS) or, even better, removed altogether if the Git.execute method's arguments can be inspected efficiently enough without it where execute_kwargs is currently used. On the other hand, people may have come to depend on it even though the presence of __all__ that omits it means no one should have depended on it.

Anyway, I do not think any of the changes I suggest in this comment need to be made in this pull request. But I wanted to mention the issue just in case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing execute_kwargs seems like a reduction of complexity, which would always be a valuable reduction of maintenance costs should changes need to be made.

As execute_kwargs was never advertised in __all__ I'd think that it's fair to say that those who depend on it nonetheless new the risk. I think the same argument is valid knowing that nothing is ever truly private, everything can be introspected if one truly wants to, yet it's something one simply has to ignore in order to be able to make any changes to python software once released.