Skip to content

Commit

Permalink
Fix mypy warning "Missing return statement"
Browse files Browse the repository at this point in the history
In git.index.base.IndexFile.from_tree, control was never able to
fall off the end, but mypy reported a "Missing return statement"
because it could not infer that the context manager didn't suppress
any exceptions.

This was for two reasons. An ExitStack context manager suppresses
exceptions in some uses and not others, depending on the context
manager objects its enter_context method is called with. In this
case, that was sufficient to produce the mypy warning regardless of
other changes, so I converted it to nested with-statements. The
nesting depth is only 2, so this is not really any worse. (I also
reworded the comments for clarity and adjusted their spacing.)

The more important cause, however, could also produce this warning
in code that uses GitPython, as git.index.util.TemporaryFileSwap is
public. Its __exit__ method was annotated to return bool. This was
itself reported by mypy as an error, because a context manager
__exit__ method that never suppresses exceptions should either
always (implicitly) return None and be annotated as such, or it can
return False as this implementation does but should then have its
return type annotated as Literal[False].

So this also changes TemporaryFileSwap.__exit__'s return type
annotation from bool to Literal[False]. If this were nonpublic or
being newly designed, it may be better for it to instead return
None implicitly, but I think that would technically be a breaking
change (though it would be very unusual, and not a good idea, for
any code to ever rely on the distinction).

With both these changes made, both those mypy warnings go away, and
code using GitPython will not get a warning if it makes similar
direct use of TemporaryFileSwap.

(An alternative might be to dedent the return statement out of the
with-statement in IndexFile.from_tree, but this would make less
clear to readers that it does not suppress exceptions. Furthermore,
the change to TemporaryFileSwap.__exit__ would still have to be
made for mypy to consider it correctly typed.)
  • Loading branch information
EliahKagan committed Dec 25, 2023
1 parent 86d0177 commit e194d46
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 13 deletions.
20 changes: 9 additions & 11 deletions git/index/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -381,23 +381,21 @@ def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile
arg_list.append("--aggressive")
# END merge handling

# tmp file created in git home directory to be sure renaming
# works - /tmp/ dirs could be on another device.
with contextlib.ExitStack() as stack:
tmp_index = stack.enter_context(_named_temporary_file_for_subprocess(repo.git_dir))
# Create the temporary file in the .git directory to be sure renaming
# works - /tmp/ directories could be on another device.
with _named_temporary_file_for_subprocess(repo.git_dir) as tmp_index:
arg_list.append("--index-output=%s" % tmp_index)
arg_list.extend(treeish)

# Move current index out of the way - otherwise the merge may fail
# Move the current index out of the way - otherwise the merge may fail
# as it considers existing entries. Moving it essentially clears the index.
# Unfortunately there is no 'soft' way to do it.
# The TemporaryFileSwap ensures the original file gets put back.

stack.enter_context(TemporaryFileSwap(join_path_native(repo.git_dir, "index")))
repo.git.read_tree(*arg_list, **kwargs)
index = cls(repo, tmp_index)
index.entries # Force it to read the file as we will delete the temp-file.
return index
with TemporaryFileSwap(join_path_native(repo.git_dir, "index")):
repo.git.read_tree(*arg_list, **kwargs)
index = cls(repo, tmp_index)
index.entries # Force it to read the file as we will delete the temp-file.
return index
# END index merge handling

# UTILITIES
Expand Down
4 changes: 2 additions & 2 deletions git/index/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

from typing import Any, Callable, TYPE_CHECKING, Optional, Type

from git.types import PathLike, _T
from git.types import Literal, PathLike, _T

if TYPE_CHECKING:
from git.index import IndexFile
Expand Down Expand Up @@ -55,7 +55,7 @@ def __exit__(
exc_type: Optional[Type[BaseException]],
exc_val: Optional[BaseException],
exc_tb: Optional[TracebackType],
) -> bool:
) -> Literal[False]:
if osp.isfile(self.tmp_file_path):
os.replace(self.tmp_file_path, self.file_path)
return False
Expand Down

0 comments on commit e194d46

Please sign in to comment.