From a40ade62435bad5a1664d9060a4678389b506a8e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Fri, 8 Mar 2024 17:37:00 -0500 Subject: [PATCH] Correct and clarify Diffable.diff docstring In cd16a35 (#1725), I had taken "Treeish" to mean the type of that exact name, git.index.base.Treeish. But that type is only used within the git.index package (actually only in git.index.base itself). It is also nonpublic: git.index.base.__all__ exists and does not list it. So most likely this was not intended in the git.diff.Diffable.diff docstring. Even if intended, it does not appear accurate, since the git.index.base.Treeish union includes bytes, and the logic in Diffable.diff and its helpers does not appear to accommodate bytes. A closer type is the public git.types.Tree_ish union, which is narrower than git.index.base.Treeish, including neither str nor bytes. However, it does not include str, and Diffable.diff does accept str to specify a tree-ish for diff-ing. It may be that "Treeish" in the pre-#1725 docstring was capitalized for some reason other than to identify a type defined in GitPython's code. For now, I've changed it to refer to git.types.Tree_ish, but also explicitly documented that a string can be used to specify a tree-ish -- which is independently valuable, since previously the effect of passing a str instance to the diff method was not stated anywhere in the method docstring. To clarify further, I included a link to tree-ish in gitglossary(7) as well. In addition, the original wording before cd16a35 had included "(type)", which I had erroneously assumed was just meant to state that it is a type (i.e. a class), so I had wrongly removed it without replacing it with anything when making it into a reference to a type. But it was really an attempt to clarify that Diffable.Index should be used directly, rather than an instance of it. That is in effect the opposite of merely pointing out that it is a class; it is to express that it should be used in a way that does not depend in any way on it being a class. This commit has the docstring explicitly state that. --- git/diff.py | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/git/diff.py b/git/diff.py index 16e6be151..967db9654 100644 --- a/git/diff.py +++ b/git/diff.py @@ -122,12 +122,21 @@ def diff( This the item to compare us with. * If ``None``, we will be compared to the working tree. - * If :class:`~git.index.base.Treeish`, it will be compared against the - respective tree. - * If :class:`Diffable.Index`, it will be compared against the index. - * If :attr:`git.NULL_TREE`, it will compare against the empty tree. - * It defaults to :class:`Diffable.Index` so that the method will not by - default fail on bare repositories. + + * If :class:`~git.types.Tree_ish`, it will be compared against the + respective tree. (See https://git-scm.com/docs/gitglossary#def_tree-ish.) + This can also be specified as a string. + + * If :class:`Diffable.Index`, it will be compared against the index. Use the + type object :class:`Index` itself, without attempting to instantiate it. + (That is, you should treat :class:`Index` as an opqaue constant. Don't + rely on it being a class or even callable.) + + * If :attr:`git.NULL_TREE `, it will compare against the empty + tree. + + This parameter defaults to :class:`Diffable.Index` (rather than ``None``) so + that the method will not by default fail on bare repositories. :param paths: This a list of paths or a single path to limit the diff to. It will only @@ -143,7 +152,7 @@ def diff( sides of the diff. :return: - :class:`DiffIndex` + A :class:`DiffIndex` representing the computed diff. :note: On a bare repository, `other` needs to be provided as