Orgit.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 theGit.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 aset of strings like that (plus, static analysis toolscan examine the parameters ofGit.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 usinggit.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 theGit.execute method's arguments can be inspected efficiently enough without it whereexecute_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 oneshould 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.

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

Asexecute_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.

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 ifcommand should actually be added here. Although the@overload-decorated stubs forGit.execute define some parameters as keyword-only, in the actual definition no parameter is keyword-only and there is definitional symmetry betweencommand and the others. Should I worry about what happens if someone has agit-command script that they can usually run asgit command and tries to rung.command() whereg is agit.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 toGit.execute elsewhere than the end. Still, regarding#1686 (comment), if you think astdin parameter should be added as a synonym ofistream, this could still be done even withstdin 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 thetext synonym ofuniversal_newlines could also be added.

Should I worry about what happens if someone has agit-command script that they can usually run asgit command and tries to rung.command() whereg is agit.cmd.Git instance? Or related situations?

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

Regardingistream, 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 ingitdb. 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.

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

Do you mean why it wasomitted from theexecute_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.)

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.

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 althoughpy.typed is included in the package,mypy checks don't pass (nor, per#1659, dopyright checks pass).

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

Asexecute_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.