Clarify Git.execute and Popen arguments by EliahKagan · Pull Request #1688 · gitpython-developers/GitPython
added 11 commits
October 3, 2023 04:58- Reorder the items in the git.cmd.Git.execute docstring that document its parameters, to be in the same order the parameters are actually defined in. - Use consistent spacing, by having a blank line between successive items that document parameters. Before, most of them were separated this way, but some of them were not. - Reorder the elements of execute_kwargs (which list all those parameters except the first parameter, command) to be listed in that order as well. These were mostly in order, but a couple were out of order. This is just about the order they appear in the definition, since sets in Python (unlike dicts) have no key order guarantees.
The top line of the Git.execute docstring said that it used a shell, which is not necessarily the case (and is not usually the case, since the default is not to use one). This removes that claim while keeping the top-line wording otherwise the same. It also rephrases the description of the command parameter, in a way that does not change its meaning but reflects the more common practice of passing a sequence of arguments (since portable calls that do not use a shell must do that).
These are some small clarity and consistency revisions to the docstring of git.cmd.Git.execute that didn't specifically fit in the narrow topics of either of the preceding two commits.
The kill_process local function defined in the Git.execute method is a local function and not a method, but it was easy to misread as a method for two reasons: - Its docstring described it as a method. - It was named with a leading underscore, as though it were a nonpublic method. But this name is a local variable, and local variables are always nonpublic (except when they are function parameters, in which case they are in a sense public). A leading underscore in a local variable name usually means the variable is unused in the function. This fixes the docstring and drops the leading underscore from the name. If this is ever extracted from the Git.execute method and placed at class or module scope, then the name can be changed back.
Instead of swallowing GitCommandError exceptions in the helper used by test_it_uses_shell_or_not_as_specified and test_it_logs_if_it_uses_a_shell, this modifies the helper so it prevents Git.execute from raising the exception in the first place.
This extracts the logic of searching log messages, and asserting that (at least) one matches a pattern for the report of a Popen call with a given argument, from test_it_logs_if_it_uses_a_shell into a new nonpublic test helper method _assert_logged_for_popen. The extracted version is modified to make it slightly more general, and slightly more robust. This is still not extremely robust: the notation used to log Popen calls is informal, so it wouldn't make sense to really parse it as code. But this no longer assumes that the representation of a value ends at a word boundary, nor that the value is free of regular expression metacharacters.
This changes how the Popen call debug logging line shows the informal summary of what kind of thing is being passed as the stdin argument to Popen, showing it with stdin= rather than istream=. The new test, with "istream" in place of "stdin", passed before the code change in the git.cmd module, failed once "istream" was changed to "stdin" in the test, and then, as expected, passed again once "istream=" was changed to "stdin=" in the log.debug call in git.cmd.Git.execute.
This is still not including all or even most of the arguments, nor are all the logged arguments literal (nor should either of those things likely be changed). It is just to facilitate easier comparison of what is logged to the Popen call in the code.
In Git.execute, the stdin argument to Popen is the only one where a compound expression (rather than a single term) is currently passed. So having that be the same in the log message makes it easier to understand what is going on, as well as to see how the information shown in the log corresponds to what Popen receives.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters