-
Notifications
You must be signed in to change notification settings - Fork 64
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
Support serving bare repositories #639
Conversation
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.
Looks good to me, but I'll defer ultimate approval to @keegancsmith.
Thanks so much for the PR :)
This should be more robus than the previous check where we only checked for the presence of a .git folder.
We do already check each candidate if it is a folder before calling the git related functions.
Rebased and addressed the latest review comments. |
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.
LGTM. Sorry about the slow review.
@@ -32,7 +32,7 @@ func TestReposHandler(t *testing.T) { | |||
repos: []string{"project1", "project2"}, | |||
}, { | |||
name: "nested", | |||
repos: []string{"project1", "project1/subproject", "project2", "dir/project3"}, | |||
repos: []string{"project1", "project1/subproject", "project2", "dir/project3", "dir/project4.bare"}, |
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.
This doesn't seem like a hack at all, succinct. So I actually like it :) FYI I think there is a more traditional way to indicate a repo is base, and that is if the name has the suffix .git
. I believe git used to check the suffix to decide, but it has a more robust check these days based on actual presence of git files.
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.
Oh yeah, The repos actually end up with .git
in my deployment. I just felt like being explicit there.
This allows bare repos in addition to worktree style git repos in detection.
This is in response to my own issue (#634) where I was asking for support for serving bare repositories from a directory.
This PR has a couple of changes. Some of them can be seen as a refactoring of sorts. The comments in the original implementation were a bit misleading as they appeared to be the other way around (compared to what the tests wanted). I took the liberty to rewrite them a bit. All of the original tests still pass. I've added one case where a bare repository is created (by suffixing the name of a repo with
.bare
) which seems to work. I am not happy about the repository name suffix hack that I had to add but it also doesn't seem like it is worth the effort to refactor the entire test machinery just because of this.I am open for feedback on this.
cc @mrnugget & @keegancsmith who where very responsive to my issue