Pre-deprecate setting Git.USE_SHELL

[EliahKagan]

Fixes #1781

This does two things:

• Converts some comments on constants and other attributes to docstrings of the kind discussed in #1734, throughout the codebase (not limited to the git.cmd module).
• Rewrites the comment, now in the form of a docstring, for Git.USE_SHELL, to discourage setting it to True, briefly explain why that can be unsafe, and explain the Windows console suppression use case for which it had previously been recommended and that it is no longer necessary or useful for that case (#1781).

I have held off on calling it "deprecated," which I think is a decision that could be made later. There may be ways to limit the downsides of shell=True that arise with dynamic methods that call Git.execute (including those that happen indirectly through other GitPython code). If so, and a change that improves the safety of most existing client code that uses shell=True or Git.USE_SHELL = True would be a breaking change, then that would be a further reason to officially deprecate setting Git.USE_SHELL to True. If so, but it could be done in a way that is not a breaking change, then that might be a reason not to deprecate it. In any case, issuing a DeprecationWarning when the attribute is set on the class will probably require a metaclass or new custom descriptor to be written, which is more complicated than changing the message and may benefit from being reviewed separately.

The reason I've converted attribute/constant comments to non-reified docstrings and "pre-deprecated" USE_SHELL together in one pull request, rather than separately, is:

• Turning them into something that editors such as VS Code recognize and render as docstrings makes it so the revised "docstring" on USE_SHELL is more salient, increasing the benefit of the change.
• Making the changes separately, in the event that a release for an unrelated reason were to end up being done between them, would--depending on the order--either forgo some of the benefit, or risk the opposite effect of making the old and currently incorrect message more salient.

However, I would be pleased to sever this into two separate PRs or otherwise change this.

My commit message for 106bbe6 notes some of the above-mentioned considerations that may affect deprecation. If you'd prefer those thoughts not go in a commit message, I'd be pleased to amend it (or, if you prefer, I think GitHub will let you force push to my branch for the PR).

[Byron]

Thanks so much for figuring this out and for making the necessary improvements to the USE_SHELL docstring.

As mentioned, using a metaclass for deprecation seems more complicated than it's worth, but I 'd think the calling it deprecated in the doc string would be sufficient.

That way, for when there is a version 4 of GitPython, some or all of the deprecated items can be removed.

[EliahKagan]

As mentioned, using a metaclass for deprecation seems more complicated than it's worth

I agree using a metaclass is not justified here. But I suspect that using a custom descriptor, which avoids interfering with other aspects of the class, would be justified, to make it more likely that developers writing code that sets the attribute will become aware of the deprecation. I say this especially in light of how DeprecationWarning is issued for most other deprecated features in GitPython and, even more so, how a docstring on an attribute with this syntax, while more discoverable than a comment because many editors/IDEs show it, is not reified. Because no __doc__ attribute is added or extended with the text, Sphinx does not generate anything for it (I have just checked this, to be sure).

In any case, I will not add a custom descriptor either in this PR. If I ever propose that, I'll use a separate, subsequent pull request, ensuring it can be easily declined without forgoing any of the changes here.

but I 'd think the calling it deprecated in the doc string would be sufficient.

Should I add a commit to this pull request for that change, rewording the docstring to explicitly say it is deprecated? Based on your comment, it sounds like you may be calling for that to be done here. However, based on the "approved" status taken together with the presence of an alternative explanation for not yet merging this PR -- the temporarily broken CI on main due to the bug #1784 fixed -- I'm not sure. I'd be pleased to make that change here (and retitle the PR accordingly) if desired.

[Byron]

Apologies, communication here was ambiguous as I definitely missed to hit the merge button somehow.

I also leave it to your judgement to decide in which way a deprecation notice should be added - to me its main purpose would be to help finding the deprecated item to be removed. Users who upgrade would then hopefully realise that the item is gone, maybe with help of exhaustive change-notes that can easily be searched.

[EliahKagan]

I think another purpose of deprecation can be to inform developers of software using GitPython of the risks of setting Git.USE_SHELL to True. However, even if it were decided that DeprecationWarning should be raised, there is the question of whether it should be raised on any access to the attribute, or only setting the attribute (which I think would be my preference), or only setting the attribute to True. I don't mean we shouldn't deprecate all use of it, only that I'm not sure DeprecationWarning should be raised on the less dangerous uses.

Therefore, while the changes I have proposed in #1787 deprecate USE_SHELL by documenting it as such, they do not issue DeprecationWarning. If I propose a change to issue the warning, I can do so in a separate PR (not just from this one, which has been merged, but also separate from #1787).

[Byron]

Yes, I agree that the PR is a good first step and that a deprecation warning could be emitted as a follow-up. Since the attribute is bound for removal, in theory all access would ideally trigger a warning to help codebases to be cleared of it. It really depends on the purpose of the warning, as I can also imagine only warning on mutation or when set to true to prevent incorrect usage, which an option to fully deprecate it later and before 4.0 lands. In a way, one could implement a deprecation-ladder if one was inclined, even though I think that this might capture resources that are better spent elsewhere.

[EliahKagan]

On further reflection and in view of what you're saying, I think the best thing to do is to have a DeprecationWarning issued on all accesses to the attribute, but have two versions of the warning: one for when it is set to True with a little extra text saying why that is bad (or at least mentioning that it is bad), and the other for all other access scenarios.

I should acknowledge, though, that when I looked through the various places deprecation notices appeared when working on a0fa2bd (#1787), I found that it was more of an even split between issuing a warning and not issuing one than I had thought. As such, the expectation that a warning be issued may not be as high as I had anticipated, at least among developers who have been working with GitPython for a long time.

With that said, I think in most of the other situations where no warning is issued, one could and probably should be. I might look at them all together and open a PR proposing more such additions than just for Git.USE_SHELL.

[Byron]

With that said, I think in most of the other situations where no warning is issued, one could and probably should be. I might look at them all together and open a PR proposing more such additions than just for Git.USE_SHELL.

Thanks a lot! I like that change in stance as it also serves as a reminder that getting a 4.0 ready can be a long-term goal. To me, this would be the GitPython as it should have been. Modern, clean, and with fewer bugs around encodings.