-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Check for bare repositories #2067
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
Merged
jesseduffield
merged 15 commits into
jesseduffield:master
from
nullishamy:feat/detect-bare-repo
Aug 18, 2022
+61
−8
Merged
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
41b54d7
Check for bare repositories
nullishamy 9987e65
Merge branch 'master' into feat/detect-bare-repo
nullishamy 2866827
Apply suggestions from code review
nullishamy bdb0b9a
Merge branch 'master' into feat/detect-bare-repo
nullishamy b9b2f58
Format, bug fixes
nullishamy 69718fb
Factor out redundant statement
nullishamy a658cd4
Factor out opening of recent repos
nullishamy 0b4f9f8
Refactor branching logic
nullishamy a91d977
Merge branch 'master' into feat/detect-bare-repo
nullishamy d072b0c
Merge branch 'master' into feat/detect-bare-repo
nullishamy 3016469
Merge branch 'master' into feat/detect-bare-repo
nullishamy 154bd97
Apply refactoring suggestions
nullishamy 21a4522
Merge branch 'master' into feat/detect-bare-repo
nullishamy 290e865
Merge branch 'master' into feat/detect-bare-repo
nullishamy 956372c
Run gofumpt
nullishamy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
@jesseduffield should we somehow conflate this
isBareRepoand this one?Currently I think the one from
recent_repos_panelisn't reachable fromapp.go, but maybe it's better to make it public rather than have a duplicate?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.
Good idea, let's make the existing one public like you say
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.
How would that look, and do you want that implemented in this PR? Also how do I go about accessing the existing function?
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.
It seems like a trivial change, I see no reason why to postpone it.
Should be accessible through
app.IsBareRepoonce you make it public (capitalize it)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.
I'll give it a go.
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.
I say we wait for Jesse to see if he can
deus ex machinathis pickle.I also say we extract it to something like
pkg/commands/git_commands/misc.go, throw inIsDirectoryAGitRepositoryas well and call it a day.We'll see!
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.
I meaaaaaan that's not even a bad idea per-se. But we can wait for Jesse to comment on this one. Go is a real pain sometimes.
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.
the app package can depend on the commands package (which contains git.go) but not vice-versa. I see two approaches we can take here:
pkg/commands/git_commands/status.goLike so:Then in app.go we call IsBareRepo, passing in our os struct.
I would prefer option 1 but that deserves its own PR, so I'm happy to go with option 2 for now.
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.
the go-git method is very fast btw because it's just checking if a value is nil, but this function is only called when you want to view recent repos so it's not a hot path.
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.
That should be that sorted, took me a while to figure out how to import the function lol.