◐ Shell
reader mode source ↗
Skip to content

Clarify Git.execute and Popen arguments#1688

Merged
Byron merged 11 commits into
gitpython-developers:mainfrom
EliahKagan:execute-args
Oct 3, 2023
Merged

Clarify Git.execute and Popen arguments#1688
Byron merged 11 commits into
gitpython-developers:mainfrom
EliahKagan:execute-args

Conversation

@EliahKagan

@EliahKagan EliahKagan commented Oct 3, 2023

Copy link
Copy Markdown
Member

This is a sequel to #1687, improving how parameters to Git.execute are documented, and improving debug logging and tests of how they affect the arguments passed to Popen.

In the Git.execute docstring, the method is no longer described as using a shell, since this is typically (and by default) not the case; the items documenting each parameter are listed in the order of the parameters; and some copyediting is done, for clarity, consistency, spacing, and in a few cases other formatting. I copyedited some other docstrings in the module as well.

The order of names in the execute_kwargs set is tweaked to also match the order in which they appear as parameters to Git.execute. (This is just for the purpose of code clarity, as set objects guarantee no particular iteration order.) Since not all unintentional mismatches between this set and the defined method parameters would cause existing tests to fail, and the failures that would occur would not always immediately show the cause of the problem, I also added a test that execute_kwargs has the exact expected relationship to the parameters of Git.execute. (Though an alternative might be to generate execute_kwargs programmatically from Git.execute using a similar technique.)

One part of the test logic in #1687 was unnecessarily complicated, due to swallowing an exception produced by running git with no arguments. This changes that by passing with_exceptions=False so that exception is never generated.

Another part of the the test logic in #1687 combined claims about the code under test with custom assertion logic in a way that made it hard to see what claims were being made by reading the test code. This fixes that by generalizing, and extracting out, an _assert_logged_for_popen test helper method. I also took this opportunity to remove its unstated incompatibility with value representations containing regular expression metacharacters. (Please note that the new code is still only robust enough for the special purpose for which it is intended; it does not actually parse the debug message rigorously.)

As requested in #1686 (comment), I changed istream= to stdin= in the debug logging message documenting the Popen call. I reused the test helper in writing a new test, test_it_logs_istream_summary_for_stdin,
which checks both that it has the new name and that it has the expected simplified value representation. I did not change any parameters or variables called istream to stdin or any other name; rather, this changes the message to better reflect the Popen call it exists to document.

If I would only have changed the displayed parameter name, I would likely not have written a test, but I also wanted to change two other things, in which the test helped verify that correctness was maintained: I reordered the displayed name=value representations in the log message to match the relative order in which they are passed to Popen. And I eliminated the istream_ok variable (which was named like a boolean flag but was not one), because having the logic for producing the string in the logging call makes it better resemble the nonidentical but corresponding logic in the Popen call, so they can be compared.

Although this is peripheral to the core concept of the clarity of function arguments, I also renamed and fixed the docstring of a local function that appeared (including by claiming) to be a method.

- 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.
(Not specific to git.cmd.Git.execute.)
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.
@EliahKagan EliahKagan marked this pull request as ready for review October 3, 2023 15:34
@Byron Byron added this to the v3.1.38 - Bugfixes milestone Oct 3, 2023

@Byron Byron left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Thanks a lot for this second round of improvements!

It's at the core of GitPython and any improvements is most certainly valued downstream as long as it's not subtly breaking, which you made sure of.

With that said, I remember that this module is also used to start two long-running python processes lazily when objects are accessed, along with various calls to git for doing pretty much anything else.

Also thanks to your work the idea of using gitoxide (or rather its still to be created python-bindings) has been forming in my head, which could lead to git invocations to be reduced to zero while probably performing as well or better. Of course, anything like it is still years away, but it's something I look forward to.

@Byron Byron merged commit 91f63cd into gitpython-developers:main Oct 3, 2023
@EliahKagan EliahKagan deleted the execute-args branch October 3, 2023 17:54
renovate Bot referenced this pull request in allenporter/flux-local Oct 20, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [GitPython](https://togithub.com/gitpython-developers/GitPython) |
`==3.1.37` -> `==3.1.40` |
[![age](https://developer.mend.io/api/mc/badges/age/pypi/GitPython/3.1.40?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/GitPython/3.1.40?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/GitPython/3.1.37/3.1.40?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/GitPython/3.1.37/3.1.40?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>gitpython-developers/GitPython (GitPython)</summary>

###
[`v3.1.40`](https://togithub.com/gitpython-developers/GitPython/compare/3.1.38...3.1.40)

[Compare
Source](https://togithub.com/gitpython-developers/GitPython/compare/3.1.38...3.1.40)

###
[`v3.1.38`](https://togithub.com/gitpython-developers/GitPython/releases/tag/3.1.38)

[Compare
Source](https://togithub.com/gitpython-developers/GitPython/compare/3.1.37...3.1.38)

#### What's Changed

- Add missing assert keywords by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1678](https://togithub.com/gitpython-developers/GitPython/pull/1678)
- Make clear every test's status in every CI run by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1679](https://togithub.com/gitpython-developers/GitPython/pull/1679)
- Fix new link to license in readme by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1680](https://togithub.com/gitpython-developers/GitPython/pull/1680)
- Drop unneeded flake8 suppressions by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1681](https://togithub.com/gitpython-developers/GitPython/pull/1681)
- Update instructions and test helpers for git-daemon by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1684](https://togithub.com/gitpython-developers/GitPython/pull/1684)
- Fix Git.execute shell use and reporting bugs by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1687](https://togithub.com/gitpython-developers/GitPython/pull/1687)
- No longer allow CI to select a prerelease for 3.12 by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1689](https://togithub.com/gitpython-developers/GitPython/pull/1689)
- Clarify Git.execute and Popen arguments by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1688](https://togithub.com/gitpython-developers/GitPython/pull/1688)
- Ask git where its daemon is and use that by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1697](https://togithub.com/gitpython-developers/GitPython/pull/1697)
- Fix bugs affecting exception wrapping in rmtree callback by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1700](https://togithub.com/gitpython-developers/GitPython/pull/1700)
- Fix dynamically-set **all** variable by
[@&#8203;DeflateAwning](https://togithub.com/DeflateAwning) in
[https://github.com/gitpython-developers/GitPython/pull/1659](https://togithub.com/gitpython-developers/GitPython/pull/1659)
- Fix small
[#&#8203;1662](https://togithub.com/gitpython-developers/GitPython/issues/1662)
regression due to
[#&#8203;1659](https://togithub.com/gitpython-developers/GitPython/issues/1659)
by [@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1701](https://togithub.com/gitpython-developers/GitPython/pull/1701)
- Drop obsolete info on yanking from security policy by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1703](https://togithub.com/gitpython-developers/GitPython/pull/1703)
- Have Dependabot offer submodule updates by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1702](https://togithub.com/gitpython-developers/GitPython/pull/1702)
- Bump git/ext/gitdb from `49c3178` to `8ec2390` by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/gitpython-developers/GitPython/pull/1704](https://togithub.com/gitpython-developers/GitPython/pull/1704)
- Bump git/ext/gitdb from `8ec2390` to `6a22706` by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[https://github.com/gitpython-developers/GitPython/pull/1705](https://togithub.com/gitpython-developers/GitPython/pull/1705)
- Update readme for milestone-less releasing by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1707](https://togithub.com/gitpython-developers/GitPython/pull/1707)
- Run Cygwin CI workflow commands in login shells by
[@&#8203;EliahKagan](https://togithub.com/EliahKagan) in
[https://github.com/gitpython-developers/GitPython/pull/1709](https://togithub.com/gitpython-developers/GitPython/pull/1709)

#### New Contributors

- [@&#8203;DeflateAwning](https://togithub.com/DeflateAwning) made their
first contribution in
[https://github.com/gitpython-developers/GitPython/pull/1659](https://togithub.com/gitpython-developers/GitPython/pull/1659)

**Full Changelog**:
gitpython-developers/GitPython@3.1.37...3.1.38

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/allenporter/flux-local).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xOS4yIiwidXBkYXRlZEluVmVyIjoiMzcuMTkuMiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants