Skip to content

Fix over recursion of git serve #810

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
varsanojidan merged 11 commits into from
Aug 25, 2022
Merged

Fix over recursion of git serve #810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
ad56fb2
Fix over recursion of git serve
varsanojidan Aug 5, 2022
0181335
add isBare flag to repos
varsanojidan Aug 9, 2022
2f98bef
Remove subproject support and fix test's git repo init
varsanojidan Aug 9, 2022
00f07b6
add newlines
varsanojidan Aug 22, 2022
200f5b6
use clonepath to denote where to clone repo from
varsanojidan Aug 24, 2022
6b923b7
fix test
varsanojidan Aug 24, 2022
8025890
Address PR comments
varsanojidan Aug 25, 2022
1ed940d
add changelog changes
varsanojidan Aug 25, 2022
1a59bfc
fix lint
varsanojidan Aug 25, 2022
978387d
Merge branch 'main' into iv/fix-walking-too-far
varsanojidan Aug 25, 2022
a584d66
Fix changelog
varsanojidan Aug 25, 2022
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
8 changes: 7 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,13 @@ All notable changes to `src-cli` are documented in this file.

### Fixed

- Fixed a performance issue when serving git repos where it would take an exponentially large amount of time to list the repos. [#810](https://github.com/sourcegraph/src-cli/pull/810)
- Fixed Bare git repo support when serving git repos. [#810](https://github.com/sourcegraph/src-cli/pull/810)

### Removed

- Removed git sub-repo support when serving git repos as it introduced a huge performance hit. [#810](https://github.com/sourcegraph/src-cli/pull/810)

## 3.43.0

### Changed
Expand All @@ -40,6 +45,7 @@ All notable changes to `src-cli` are documented in this file.

- INTERNAL ONLY: Fixed src batch exec not logging errors.


## 3.42.2

### Fixed
Expand Down Expand Up @@ -833,4 +839,4 @@ Re-release of 3.29.3 for Sourcegraph 3.30.
### Changed

- The terminal UI has been replaced by the logger-based UI that was previously only visible in verbose-mode (`-v`). [#228](https://github.com/sourcegraph/src-cli/pull/228)
- Deprecated the `-endpoint` flag. Instead, use the `SRC_ENDPOINT` environment variable. [#235](https://github.com/sourcegraph/src-cli/pull/235)
- Deprecated the `-endpoint` flag. Instead, use the `SRC_ENDPOINT` environment variable. [#235](https://github.com/sourcegraph/src-cli/pull/235)
35 changes: 20 additions & 15 deletions internal/servegit/serve.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,9 @@ var indexHTML = template.Must(template.New("").Parse(`<html>
</html>`))

type Repo struct {
Name string
URI string
Name string
URI string
ClonePath string
}

func (s *Serve) handler() http.Handler {
Expand Down Expand Up @@ -178,8 +179,8 @@ func (s *Serve) Repos() ([]Repo, error) {
return nil
}

// We recurse into bare repositories to find subprojects. Prevent
// recursing into .git
// Previously we recursed into bare repositories which is why this check was here.
// Now we use this as a sanity check to make sure we didn't somehow stumble into a .git dir.
if filepath.Base(path) == ".git" {
return filepath.SkipDir
}
Expand All @@ -204,20 +205,24 @@ func (s *Serve) Repos() ([]Repo, error) {

name := filepath.ToSlash(subpath)
reposRootIsRepo = reposRootIsRepo || name == "."
repos = append(repos, Repo{
Name: name,
URI: pathpkg.Join("/repos", name),
})
cloneURI := pathpkg.Join("/repos", name)
clonePath := cloneURI

// Check whether a repository is a bare repository or not.
//
// Bare repositories shouldn't have any further child
// repositories. Only regular git worktrees can have children.
if isBare {
return filepath.SkipDir
// Regular git repos won't clone without the full path to the .git directory.
if isGit {
clonePath += "/.git"
}

return nil
repos = append(repos, Repo{
Name: name,
URI: cloneURI,
ClonePath: clonePath,
})

// At this point we know the directory is either a git repo or a bare git repo,
// we don't need to recurse further to save time.
// TODO: Look into whether it is useful to support git submodules
return filepath.SkipDir
})

if err != nil {
Expand Down
80 changes: 19 additions & 61 deletions internal/servegit/serve_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func TestReposHandler(t *testing.T) {
repos: []string{"project1", "project2"},
}, {
name: "nested",
repos: []string{"project1", "project1/subproject", "project2", "dir/project3", "dir/project4.bare"},
repos: []string{"project1", "project2", "dir/project3", "dir/project4.bare"},
}}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
Expand All @@ -47,65 +47,14 @@ func TestReposHandler(t *testing.T) {

var want []Repo
for _, name := range tc.repos {
want = append(want, Repo{Name: name, URI: path.Join("/repos", name)})
}
testReposHandler(t, h, want)
})

// Now do the same test, but we root it under a repo we serve. This is
// to test we properly serve up the root repo as something other than
// "."
t.Run("rooted-"+tc.name, func(t *testing.T) {
repos := []string{"project-root"}
for _, name := range tc.repos {
repos = append(repos, filepath.Join("project-root", name))
}

root := gitInitRepos(t, repos...)

// This is the difference to above, we point our root at the git repo
root = filepath.Join(root, "project-root")

h := (&Serve{
Info: testLogger(t),
Debug: discardLogger,
Addr: testAddress,
Root: root,
}).handler()

// project-root is served from /repos, etc
want := []Repo{{Name: "project-root", URI: "/repos"}}
for _, name := range tc.repos {
want = append(want, Repo{Name: path.Join("project-root", name), URI: path.Join("/repos", name)})
}
testReposHandler(t, h, want)
})

// Ensure everything still works if root is a symlink
t.Run("rooted-"+tc.name, func(t *testing.T) {
root := gitInitRepos(t, tc.repos...)

// This is the difference, we create a symlink for root
{
tmp := t.TempDir()

symlink := filepath.Join(tmp, "symlink-root")
if err := os.Symlink(root, symlink); err != nil {
t.Fatal(err)
isBare := strings.HasSuffix(name, ".bare")
uri := path.Join("/repos", name)
clonePath := uri
if !isBare {
clonePath += "/.git"
}
root = symlink
}
want = append(want, Repo{Name: name, URI: uri, ClonePath: clonePath})

h := (&Serve{
Info: testLogger(t),
Debug: discardLogger,
Addr: testAddress,
Root: root,
}).handler()

var want []Repo
for _, name := range tc.repos {
want = append(want, Repo{Name: name, URI: path.Join("/repos", name)})
}
testReposHandler(t, h, want)
})
Expand Down Expand Up @@ -173,6 +122,14 @@ func gitInitBare(t *testing.T, path string) {
}
}

func gitInit(t *testing.T, path string) {
cmd := exec.Command("git", "init")
cmd.Dir = path
if err := cmd.Run(); err != nil {
t.Fatal(err)
}
}

func gitInitRepos(t *testing.T, names ...string) string {
root := t.TempDir()
root = filepath.Join(root, "repos-root")
Expand All @@ -183,10 +140,11 @@ func gitInitRepos(t *testing.T, names ...string) string {
t.Fatal(err)
}

if !strings.HasSuffix(p, ".bare") {
p = filepath.Join(p, ".git")
if strings.HasSuffix(p, ".bare") {
gitInitBare(t, p)
} else {
gitInit(t, p)
}
gitInitBare(t, p)
}

return root
Expand Down