Repository navigation
Commit 5b2869f
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
1 file changed
Lines changed: 4 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -41,7 +41,7 @@ | |||
| 41 | 41 | overload, | |
| 42 | 42 | ) | |
| 43 | 43 | ||
| 44 | - from git.types import PathLike, Literal, Old_commit_ish | ||
| 44 | + from git.types import AnyGitObject, Literal, PathLike | ||
| 45 | 45 | ||
| 46 | 46 | if TYPE_CHECKING: | |
| 47 | 47 | from git.objects.commit import Commit | |
@@ -194,7 +194,7 @@ def __init__( | |||
| 194 | 194 | self.summary = summary | |
| 195 | 195 | ||
| 196 | 196 | @property | |
| 197 | - def old_commit(self) -> Union[str, SymbolicReference, Old_commit_ish, None]: | ||
| 197 | + def old_commit(self) -> Union["Commit", None]: | ||
| 198 | 198 | return self._old_commit_sha and self._remote.repo.commit(self._old_commit_sha) or None | |
| 199 | 199 | ||
| 200 | 200 | @property | |
@@ -360,7 +360,7 @@ def __init__( | |||
| 360 | 360 | ref: SymbolicReference, | |
| 361 | 361 | flags: int, | |
| 362 | 362 | note: str = "", | |
| 363 | - old_commit: Union[Old_commit_ish, None] = None, | ||
| 363 | + old_commit: Union[AnyGitObject, None] = None, | ||
| 364 | 364 | remote_ref_path: Optional[PathLike] = None, | |
| 365 | 365 | ) -> None: | |
| 366 | 366 | """Initialize a new instance.""" | |
@@ -436,7 +436,7 @@ def _from_line(cls, repo: "Repo", line: str, fetch_line: str) -> "FetchInfo": | |||
| 436 | 436 | ||
| 437 | 437 | # Parse operation string for more info. | |
| 438 | 438 | # 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 | ||
| 440 | 440 | is_tag_operation = False | |
| 441 | 441 | if "rejected" in operation: | |
| 442 | 442 | flags |= cls.REJECTED | |
0 commit comments