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
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
Next Next commit
Document Git.execute parameters in definition order 
- 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.
  • Loading branch information
@EliahKagan
EliahKagan committed Oct 3, 2023
commit b79198a880982e6768fec4d0ef244338420efbdc
46 changes: 26 additions & 20 deletions git/cmd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,10 @@
"with_extended_output",
"with_exceptions",
"as_process",
"stdout_as_string",
"output_stream",
"with_stdout",
"stdout_as_string",
"kill_after_timeout",
"with_stdout",
Comment on lines -69 to +72
Copy link
Member Author

@EliahKagan EliahKagan Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This changes only the order in which they are given, not the contents, and the set is equal to the same value as before.

However, I am wondering if command should actually be added here. Although the @overload-decorated stubs for Git.execute define some parameters as keyword-only, in the actual definition no parameter is keyword-only and there is definitional symmetry between command and the others. Should I worry about what happens if someone has a git-command script that they can usually run as git command and tries to run g.command() where g is a git.cmd.Git instance? Or related situations?

Another ramification of the parameters not being keyword-only is that, because they can be passed positionally, it is a breaking change to add a new one to Git.execute elsewhere than the end. Still, regarding #1686 (comment), if you think a stdin parameter should be added as a synonym of istream, this could still be done even with stdin at the end, with the docstring items that document the parameters referring to each other to overcome any confusion. I am inclined to say the added complexity is not worthwhile (for example, the function would have to handle the case where they are both passed and with inconsistent values). But a possible argument against this is that the text synonym of universal_newlines could also be added.

Copy link
Member

Choose a reason for hiding this comment

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

Should I worry about what happens if someone has a git-command script that they can usually run as git command and tries to run g.command() where g is a git.cmd.Git instance? Or related situations?

In these situations it would be great to know why command was put there in the first place, and I for one do not remember. I know there never was an issue related to command specifically, which seems to indicate it's a limitation worth ignoring or fixing in a non-breaking fashion (which seems possible).

Regarding istream, I'd think maintaining stability would let me sleep better at night especially since you seem to agree that it's used consistently here and in gitdb. Probably along with the documentation improvements, all is well and better than it was before, which even in the past didn't seem to have caused enough confusion to cause an issue to be opened.

Copy link
Member Author

@EliahKagan EliahKagan Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In these situations it would be great to know why command was put there in the first place, and I for one do not remember.

Do you mean why it was omitted from the execute_kwargs set? If so, my guess is that it was intended to be used positionally while the others were intended to be keyword-only. This is judging by how they are given as keyword-only arguments in the @overloads, where they are preceded by a *, item in the list of formal parameters, which is absent in the actual function definition (thus causing them to accept both positional and keyword arguments).

But it may be that I am misunderstanding what you mean here. It is also possible that their keyword-only status in the @overloads was intentionally different from the ultimate definition and intended to provide guidance. (I'm not sure. The @overloads are missing most of the arguments, which confuses tooling and seems unintentional.)

Copy link
Member

Choose a reason for hiding this comment

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

I am happy to adopt your conclusion in that this is not intentional and since there are negative side-effects, like tooling not working correctly, it's certainly something that could be improved.
If against all odds such an action does create downstream breakage, it could also be undone with a patch release.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should make the parameters in the real definition actually keyword-only, since that is a breaking change. Or, at least, I would not rush to that. It seems likely to me that, at least for the first few, people are passing them positionally.

I believe it is merely the absence of many parameters from the @overload-decorated stubs (which precede the real definition) that is causing VS Code not to show or autocomplete some arguments. Adding the missing parameters to the @overload-decorated stubs should be sufficient to fix that, and it shouldn't be a breaking change because (a) they are just type stubs, so it doesn't break runtime behavior, (b) the actual change seems compatible even relative to what the type stubs said before, and (c) GitPython doesn't have static typing stability currently anyway, because although py.typed is included in the package, mypy checks don't pass (nor, per #1659, do pyright checks pass).

"universal_newlines",
"shell",
"env",
Expand Down Expand Up @@ -883,6 +883,27 @@ def execute(
decoded into a string using the default encoding (usually utf-8).
The latter can fail, if the output contains binary data.

:param kill_after_timeout:
To specify a timeout in seconds for the git command, after which the process
should be killed. This will have no effect if as_process is set to True. It is
set to None by default and will let the process run until the timeout is
explicitly specified. This feature is not supported on Windows. It's also worth
noting that kill_after_timeout uses SIGKILL, which can have negative side
effects on a repository. For example, stale locks in case of git gc could
render the repository incapable of accepting changes until the lock is manually
removed.

:param with_stdout:
If True, default True, we open stdout on the created process

:param universal_newlines:
if True, pipes will be opened as text, and lines are split at
all known line endings.

:param shell:
Whether to invoke commands through a shell (see `Popen(..., shell=True)`).
It overrides :attr:`USE_SHELL` if it is not `None`.

:param env:
A dictionary of environment variables to be passed to `subprocess.Popen`.

Expand All @@ -891,29 +912,14 @@ def execute(
one invocation of write() method. If the given number is not positive then
the default value is used.

:param strip_newline_in_stdout:
Whether to strip the trailing ``\\n`` of the command stdout.

:param subprocess_kwargs:
Keyword arguments to be passed to subprocess.Popen. Please note that
some of the valid kwargs are already set by this method, the ones you
specify may not be the same ones.

:param with_stdout: If True, default True, we open stdout on the created process
:param universal_newlines:
if True, pipes will be opened as text, and lines are split at
all known line endings.
:param shell:
Whether to invoke commands through a shell (see `Popen(..., shell=True)`).
It overrides :attr:`USE_SHELL` if it is not `None`.
:param kill_after_timeout:
To specify a timeout in seconds for the git command, after which the process
should be killed. This will have no effect if as_process is set to True. It is
set to None by default and will let the process run until the timeout is
explicitly specified. This feature is not supported on Windows. It's also worth
noting that kill_after_timeout uses SIGKILL, which can have negative side
effects on a repository. For example, stale locks in case of git gc could
render the repository incapable of accepting changes until the lock is manually
removed.
:param strip_newline_in_stdout:
Whether to strip the trailing ``\\n`` of the command stdout.
:return:
* str(output) if extended_output = False (Default)
* tuple(int(status), str(stdout), str(stderr)) if extended_output = True
Expand Down