Skip to content

Remove optional from two member variables #1550

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 2, 2023
Merged

Remove optional from two member variables #1550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cc71f02
Remove optional from two member variables
Sineaggi Jan 27, 2023
bc8f119
Add check to test bare repo
Sineaggi Feb 1, 2023
fb6eb3d
Add additional assertions to test_base.py
Sineaggi Feb 2, 2023
4a44fdf
Update docs
Sineaggi Feb 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 29 additions & 27 deletions git/repo/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,17 +115,17 @@ class Repo(object):
'working_dir' is the working directory of the git command, which is the working tree
directory if available or the .git directory in case of bare repositories

'working_tree_dir' is the working tree directory, but will raise AssertionError
'working_tree_dir' is the working tree directory, but will return None
if we are a bare repository.

'git_dir' is the .git repository directory, which is always set."""

DAEMON_EXPORT_FILE = "git-daemon-export-ok"

git = cast("Git", None) # Must exist, or __del__ will fail in case we raise on `__init__()`
working_dir: Optional[PathLike] = None
working_dir: PathLike
_working_tree_dir: Optional[PathLike] = None
git_dir: PathLike = ""
git_dir: PathLike
_common_dir: PathLike = ""

# precompiled regex
Expand Down Expand Up @@ -215,13 +215,14 @@ def __init__(
## Walk up the path to find the `.git` dir.
#
curpath = epath
git_dir = None
while curpath:
# ABOUT osp.NORMPATH
# It's important to normalize the paths, as submodules will otherwise initialize their
# repo instances with paths that depend on path-portions that will not exist after being
# removed. It's just cleaner.
if is_git_dir(curpath):
self.git_dir = curpath
git_dir = curpath
# from man git-config : core.worktree
# Set the path to the root of the working tree. If GIT_COMMON_DIR environment
# variable is set, core.worktree is ignored and not used for determining the
Expand All @@ -230,9 +231,9 @@ def __init__(
# directory, which is either specified by GIT_DIR, or automatically discovered.
# If GIT_DIR is specified but none of GIT_WORK_TREE and core.worktree is specified,
# the current working directory is regarded as the top level of your working tree.
self._working_tree_dir = os.path.dirname(self.git_dir)
self._working_tree_dir = os.path.dirname(git_dir)
if os.environ.get("GIT_COMMON_DIR") is None:
gitconf = self.config_reader("repository")
gitconf = self._config_reader("repository", git_dir)
if gitconf.has_option("core", "worktree"):
self._working_tree_dir = gitconf.get("core", "worktree")
if "GIT_WORK_TREE" in os.environ:
Expand All @@ -242,14 +243,14 @@ def __init__(
dotgit = osp.join(curpath, ".git")
sm_gitpath = find_submodule_git_dir(dotgit)
if sm_gitpath is not None:
self.git_dir = osp.normpath(sm_gitpath)
git_dir = osp.normpath(sm_gitpath)

sm_gitpath = find_submodule_git_dir(dotgit)
if sm_gitpath is None:
sm_gitpath = find_worktree_git_dir(dotgit)

if sm_gitpath is not None:
self.git_dir = expand_path(sm_gitpath, expand_vars)
git_dir = expand_path(sm_gitpath, expand_vars)
self._working_tree_dir = curpath
break

Expand All @@ -260,8 +261,9 @@ def __init__(
break
# END while curpath

if self.git_dir is None:
if git_dir is None:
raise InvalidGitRepositoryError(epath)
self.git_dir = git_dir

self._bare = False
try:
Expand All @@ -282,7 +284,7 @@ def __init__(
self._working_tree_dir = None
# END working dir handling

self.working_dir: Optional[PathLike] = self._working_tree_dir or self.common_dir
self.working_dir: PathLike = self._working_tree_dir or self.common_dir
self.git = self.GitCommandWrapperType(self.working_dir)

# special handling, in special times
Expand Down Expand Up @@ -320,7 +322,7 @@ def close(self) -> None:
gc.collect()

def __eq__(self, rhs: object) -> bool:
if isinstance(rhs, Repo) and self.git_dir:
if isinstance(rhs, Repo):
return self.git_dir == rhs.git_dir
return False

Expand All @@ -332,14 +334,12 @@ def __hash__(self) -> int:

# Description property
def _get_description(self) -> str:
if self.git_dir:
filename = osp.join(self.git_dir, "description")
filename = osp.join(self.git_dir, "description")
with open(filename, "rb") as fp:
return fp.read().rstrip().decode(defenc)

def _set_description(self, descr: str) -> None:
if self.git_dir:
filename = osp.join(self.git_dir, "description")
filename = osp.join(self.git_dir, "description")
with open(filename, "wb") as fp:
fp.write((descr + "\n").encode(defenc))

Expand All @@ -357,13 +357,7 @@ def common_dir(self) -> PathLike:
"""
:return: The git dir that holds everything except possibly HEAD,
FETCH_HEAD, ORIG_HEAD, COMMIT_EDITMSG, index, and logs/."""
if self._common_dir:
return self._common_dir
elif self.git_dir:
return self.git_dir
else:
# or could return ""
raise InvalidGitRepositoryError()
return self._common_dir or self.git_dir

@property
def bare(self) -> bool:
Expand Down Expand Up @@ -532,7 +526,9 @@ def delete_remote(self, remote: "Remote") -> str:
"""Delete the given remote."""
return Remote.remove(self, remote)

def _get_config_path(self, config_level: Lit_config_levels) -> str:
def _get_config_path(self, config_level: Lit_config_levels, git_dir: Optional[PathLike] = None) -> str:
if git_dir is None:
git_dir = self.git_dir
# we do not support an absolute path of the gitconfig on windows ,
# use the global config instead
if is_win and config_level == "system":
Expand All @@ -546,7 +542,7 @@ def _get_config_path(self, config_level: Lit_config_levels) -> str:
elif config_level == "global":
return osp.normpath(osp.expanduser("~/.gitconfig"))
elif config_level == "repository":
repo_dir = self._common_dir or self.git_dir
repo_dir = self._common_dir or git_dir
if not repo_dir:
raise NotADirectoryError
else:
Expand Down Expand Up @@ -575,15 +571,21 @@ def config_reader(
you know which file you wish to read to prevent reading multiple files.
:note: On windows, system configuration cannot currently be read as the path is
unknown, instead the global path will be used."""
files = None
return self._config_reader(config_level=config_level)

def _config_reader(
self,
config_level: Optional[Lit_config_levels] = None,
git_dir: Optional[PathLike] = None,
) -> GitConfigParser:
if config_level is None:
files = [
self._get_config_path(cast(Lit_config_levels, f))
self._get_config_path(cast(Lit_config_levels, f), git_dir)
for f in self.config_level
if cast(Lit_config_levels, f)
]
else:
files = [self._get_config_path(config_level)]
files = [self._get_config_path(config_level, git_dir)]
return GitConfigParser(files, read_only=True, repo=self)

def config_writer(self, config_level: Lit_config_levels = "repository") -> GitConfigParser:
Expand Down
9 changes: 7 additions & 2 deletions test/test_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import tempfile
from unittest import SkipTest, skipIf

from git import Repo
from git.objects import Blob, Tree, Commit, TagObject
from git.compat import is_win
from git.objects.util import get_object_type_by_name
Expand Down Expand Up @@ -95,14 +96,18 @@ def test_object_resolution(self):
self.assertEqual(self.rorepo.head.reference.object, self.rorepo.active_branch.object)

@with_rw_repo("HEAD", bare=True)
def test_with_bare_rw_repo(self, bare_rw_repo):
def test_with_bare_rw_repo(self, bare_rw_repo: Repo):
assert bare_rw_repo.config_reader("repository").getboolean("core", "bare")
assert osp.isfile(osp.join(bare_rw_repo.git_dir, "HEAD"))
assert osp.isdir(bare_rw_repo.working_dir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Could you also add an assertion of the working_tree_dir for completeness? It's easy to confuse both which is what I did apparently. The working_dir is the CWD of the git repository, either the working_tree_dir or the git_dir, which I think should be an assertion for the bare and non-bare cases.

If that assertion holds, of course, I was going by the docs.

Screenshot 2023-02-02 at 07 15 07

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so the docs in base.py say that working_tree_dir can return none

    @property
    def working_tree_dir(self) -> Optional[PathLike]:
        """:return: The working tree directory of our git repository. If this is a bare repository, None is returned."""
        return self._working_tree_dir

I've added a few more assertions, please check if I understood you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also updated the python docs of the class to mention returning None

assert bare_rw_repo.working_tree_dir is None

@with_rw_repo("0.1.6")
def test_with_rw_repo(self, rw_repo):
def test_with_rw_repo(self, rw_repo: Repo):
assert not rw_repo.config_reader("repository").getboolean("core", "bare")
assert osp.isdir(rw_repo.working_tree_dir)
assert osp.isdir(osp.join(rw_repo.working_tree_dir, "lib"))
assert osp.isdir(rw_repo.working_dir)

@skipIf(HIDE_WINDOWS_FREEZE_ERRORS, "FIXME: Freezes! sometimes...")
@with_rw_and_rw_remote_repo("0.1.6")
Expand Down