Skip to content

Commit 5b2869f

Browse files
committed
Fix old Commit_ish annotations in git.remote
The combination of changes here is a bit unintuitive. - In PushInfo.__init__, the old_commit parameter is an optional string, whose value is bound to the _old_commit_sha instance attribute. The old_commit property attempts to create a Commit object using this string. When _old_commit_sha is None, this property returns None. Otherwise it calls Repo.commit on the Repo object for the PushInfo's associated remote. Repo.commit should return, and is annotated to return, a Commit object. Its return value is produced by calling rev_parse, which is actually the implementation in git.fun, which always returns an Object (and more specifically an AnyGitObject) and whose annotation was recently fixed in fe7f9f2. The way rev_parse is used appears to only be able to get a Commit, but even if it were to get something else, it would still be an Object and not a SymbolicReference or string. Therefore, the return annotation, which contained the old Commit_ish, was overly broad, in part because the old Commit_ish was defined over-broadly, but also in other ways. This changes it to declare that it returns a Commit object or None, not allowing any kind of refs, strings, or instances representing git objects other than commits. The new return annotation is also what type checkers infer for the operand of the return statement, ever since git.fun.rev_parse's own return annotation was fixed. - In contrast, the situation with FetchInfo is almost the opposite. FetchInfo.__init__ had declared its old_commit parameter as being of the old Commit_ish type or None. Given the name old_commit, this may seem at first glance to be a case where only actually commit-ish values should be accepted, such that the annotation may have been overly broad due the overbroad old definition of Commit_ish. But on closer inspection it seems that may not be so. Specifically, when "git fetch" reports the refs it updated, and a ref points to something that is not commit-ish, the "old_commit" can be something that is not a commit. In particular, if a remote lightweight tag points to a blob or tree (rather than the usual case of pointing to a commit or tag), and that tag is then changed, then a fetch -- if done with flags that allow it to be reset -- should result in an "old_commit" of the blob or tree. More specifically, in FetchInfo._from_line (in the "tag update" case), in a line with ".." or "...", old_commit is set by parsing a field with repo.rev_parse, which is git.fun.rev_parse, which will return an Object that may be any of the four types. This is later passed as the old_commmit parameter when constructing a FetchInfo instance. Since a lightweight tag is a ref that can refer to any of the four types, any could end up being parsed into the old_commit attribute. This therefore keeps those as broad as before, channging their old Commit_ish annotations to AnyGitObject rather than to the newer Commit_ish type or anything narrower. Note also that it is pure coincidence that entities named old_commit were temporarily annotated as Old_commit_ish. (The latter, as noted in 04a2753, is just what the old Commit_ish type was temporarily renamed to.)
1 parent b4b6e1e commit 5b2869f

File tree

1 file changed

+4
-4
lines changed

1 file changed

+4
-4
lines changed

Diff for: git/remote.py

+4-4
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@
4141
overload,
4242
)
4343

44-
from git.types import PathLike, Literal, Old_commit_ish
44+
from git.types import AnyGitObject, Literal, PathLike
4545

4646
if TYPE_CHECKING:
4747
from git.objects.commit import Commit
@@ -194,7 +194,7 @@ def __init__(
194194
self.summary = summary
195195

196196
@property
197-
def old_commit(self) -> Union[str, SymbolicReference, Old_commit_ish, None]:
197+
def old_commit(self) -> Union["Commit", None]:
198198
return self._old_commit_sha and self._remote.repo.commit(self._old_commit_sha) or None
199199

200200
@property
@@ -360,7 +360,7 @@ def __init__(
360360
ref: SymbolicReference,
361361
flags: int,
362362
note: str = "",
363-
old_commit: Union[Old_commit_ish, None] = None,
363+
old_commit: Union[AnyGitObject, None] = None,
364364
remote_ref_path: Optional[PathLike] = None,
365365
) -> None:
366366
"""Initialize a new instance."""
@@ -436,7 +436,7 @@ def _from_line(cls, repo: "Repo", line: str, fetch_line: str) -> "FetchInfo":
436436

437437
# Parse operation string for more info.
438438
# This makes no sense for symbolic refs, but we parse it anyway.
439-
old_commit: Union[Old_commit_ish, None] = None
439+
old_commit: Union[AnyGitObject, None] = None
440440
is_tag_operation = False
441441
if "rejected" in operation:
442442
flags |= cls.REJECTED

0 commit comments

Comments
 (0)