-
-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
Worktree support #238
base: master
Are you sure you want to change the base?
Worktree support #238
Conversation
@@ -44,8 +44,18 @@ def get_git_root_path(filepath): | |||
|
|||
# search up to five directories above for the git repo root | |||
for _ in range(6): | |||
if dir_exists(os.path.join(gitroot_path, ".git")): | |||
dot_git = os.path.join(gitroot_path, ".git") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before you assume that .git
exists at the root of a repository, first check $GIT_DIR
from the environment. The defaults are obviously in common use but exceptions are not so uncommon that it is smart to just ignore them. Unfortunately anywhere you are running font-v
might not actually by inside a shell environment where the Git dir has been setup for the benefit of other tools, but it's better than nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not mistaken, this is more of a gripe with the original implementation, rather than the logic I added for worktree support?
I can add a check for $GIT_DIR
as a fast path before the for
loop if you'd like? I can validate it with is_git_dir()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just something I happened to notice when reviewing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some logic for this and a corresponding test
0bb2e00
to
8e5b8bb
Compare
Okay, this implementation I'm actually happy with I used the Wayback Machine & GitHub to get the actual documentation / source code for GitPython I changed the test for "is I switched my manual reading/parsing of the I also updated the test to use a fixture, as otherwise if As an aside, not sure if the pipeline failure is my fault and/or how to fix it |
8e5b8bb
to
42ea6fa
Compare
Thank you very much Ricky. And thanks for the review Caleb. Let me fix the CI and I'll have a look at this PR this week. I really appreciate it! |
Fixes #169
Adds git worktree support to font-v, as well as improving the robustness of
.git/
folder detectionOld, out-dated description
I'm not saying it's a good implementation, but it's something that works to start this conversationUnhelpfully, the documentation link @madig recommended was just a page with headings and no actual docs for me to do this properly. I'd be all for using the library's support for this if I could actually see how to :/