Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

RickyDaMa
Copy link

@RickyDaMa RickyDaMa commented Mar 20, 2024

Fixes #169

Adds git worktree support to font-v, as well as improving the robustness of .git/ folder detection

Old, out-dated description I'm not saying it's a good implementation, but it's something that works to start this conversation

Unhelpfully, 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 :/

@@ -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")
Copy link
Member

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.

Copy link
Author

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()

Copy link
Member

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.

Copy link
Author

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

@RickyDaMa
Copy link
Author

RickyDaMa commented Mar 21, 2024

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 .git a folder?" to use GitPython's provided is_git_dir() so that's more robust

I switched my manual reading/parsing of the .git file for worktrees with GitPython's find_worktree_git_dir (which has pretty similar logic to my original more naive implementation lol)

I also updated the test to use a fixture, as otherwise if get_git_root_path raised then the worktree wouldn't get cleaned up; now it does

As an aside, not sure if the pipeline failure is my fault and/or how to fix it

@RickyDaMa RickyDaMa changed the title Worktree support (super bad awful implementation) Worktree support Mar 21, 2024
@chrissimpkins
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

get_git_root_path should support worktrees
3 participants