From ad56fb2b8021ec5086d4d0cf7ea3ece0aa283de9 Mon Sep 17 00:00:00 2001 From: Idan Varsano Date: Fri, 5 Aug 2022 11:50:55 -0400 Subject: [PATCH 01/10] Fix over recursion of git serve --- internal/servegit/serve.go | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/internal/servegit/serve.go b/internal/servegit/serve.go index 1ee5c13320..f9f091ba7f 100644 --- a/internal/servegit/serve.go +++ b/internal/servegit/serve.go @@ -178,8 +178,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 } @@ -209,15 +209,10 @@ func (s *Serve) Repos() ([]Repo, error) { URI: pathpkg.Join("/repos", name), }) - // 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 - } - - return nil + // 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 ite useful to support git submodules + return filepath.SkipDir }) if err != nil { @@ -248,4 +243,4 @@ func explainAddr(addr string) string { See https://docs.sourcegraph.com/admin/external_service/src_serve_git for instructions to configure in Sourcegraph. `, addr) -} +} \ No newline at end of file From 018133552b2bacb8404187a802025169e382e6a3 Mon Sep 17 00:00:00 2001 From: Idan Varsano Date: Tue, 9 Aug 2022 14:15:31 -0400 Subject: [PATCH 02/10] add isBare flag to repos --- internal/servegit/serve.go | 10 ++++++---- internal/servegit/serve_test.go | 8 +++++--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/internal/servegit/serve.go b/internal/servegit/serve.go index f9f091ba7f..104d0eca8a 100644 --- a/internal/servegit/serve.go +++ b/internal/servegit/serve.go @@ -60,8 +60,9 @@ var indexHTML = template.Must(template.New("").Parse(` `)) type Repo struct { - Name string - URI string + Name string + URI string + IsBare bool } func (s *Serve) handler() http.Handler { @@ -205,8 +206,9 @@ func (s *Serve) Repos() ([]Repo, error) { name := filepath.ToSlash(subpath) reposRootIsRepo = reposRootIsRepo || name == "." repos = append(repos, Repo{ - Name: name, - URI: pathpkg.Join("/repos", name), + Name: name, + URI: pathpkg.Join("/repos", name), + IsBare: isBare, }) // At this point we know the directory is either a git repo or a bare git repo, diff --git a/internal/servegit/serve_test.go b/internal/servegit/serve_test.go index df80603666..47ecd49e36 100644 --- a/internal/servegit/serve_test.go +++ b/internal/servegit/serve_test.go @@ -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) { @@ -47,7 +47,9 @@ func TestReposHandler(t *testing.T) { var want []Repo for _, name := range tc.repos { - want = append(want, Repo{Name: name, URI: path.Join("/repos", name)}) + isBare := strings.HasSuffix(name, ".bare") + want = append(want, Repo{Name: name, URI: path.Join("/repos", name), IsBare: isBare}) + } testReposHandler(t, h, want) }) @@ -245,4 +247,4 @@ type testWriter struct { func (tw testWriter) Write(p []byte) (n int, err error) { tw.T.Log(string(p)) return len(p), nil -} +} \ No newline at end of file From 2f98bef5a3d0f95da3583a92fcec55a41b7209b3 Mon Sep 17 00:00:00 2001 From: Idan Varsano Date: Tue, 9 Aug 2022 16:00:40 -0400 Subject: [PATCH 03/10] Remove subproject support and fix test's git repo init --- internal/servegit/serve.go | 12 +++--- internal/servegit/serve_test.go | 75 ++++++--------------------------- 2 files changed, 19 insertions(+), 68 deletions(-) diff --git a/internal/servegit/serve.go b/internal/servegit/serve.go index 104d0eca8a..e39cac7d6b 100644 --- a/internal/servegit/serve.go +++ b/internal/servegit/serve.go @@ -60,9 +60,9 @@ var indexHTML = template.Must(template.New("").Parse(` `)) type Repo struct { - Name string - URI string - IsBare bool + Name string + URI string + Bare bool } func (s *Serve) handler() http.Handler { @@ -206,9 +206,9 @@ func (s *Serve) Repos() ([]Repo, error) { name := filepath.ToSlash(subpath) reposRootIsRepo = reposRootIsRepo || name == "." repos = append(repos, Repo{ - Name: name, - URI: pathpkg.Join("/repos", name), - IsBare: isBare, + Name: name, + URI: pathpkg.Join("/repos", name), + Bare: isBare, }) // At this point we know the directory is either a git repo or a bare git repo, diff --git a/internal/servegit/serve_test.go b/internal/servegit/serve_test.go index 47ecd49e36..c2a1e7c56c 100644 --- a/internal/servegit/serve_test.go +++ b/internal/servegit/serve_test.go @@ -48,69 +48,11 @@ func TestReposHandler(t *testing.T) { var want []Repo for _, name := range tc.repos { isBare := strings.HasSuffix(name, ".bare") - want = append(want, Repo{Name: name, URI: path.Join("/repos", name), IsBare: isBare}) + want = append(want, Repo{Name: name, URI: path.Join("/repos", name), Bare: isBare}) } 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) - } - root = symlink - } - - 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) - }) } } @@ -175,6 +117,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") @@ -185,10 +135,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 From 00f07b67af0f2848881acaa86d47d9c496290fe4 Mon Sep 17 00:00:00 2001 From: Idan Varsano Date: Mon, 22 Aug 2022 17:14:26 -0400 Subject: [PATCH 04/10] add newlines --- internal/servegit/serve.go | 2 +- internal/servegit/serve_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/servegit/serve.go b/internal/servegit/serve.go index e39cac7d6b..492471fe02 100644 --- a/internal/servegit/serve.go +++ b/internal/servegit/serve.go @@ -245,4 +245,4 @@ func explainAddr(addr string) string { See https://docs.sourcegraph.com/admin/external_service/src_serve_git for instructions to configure in Sourcegraph. `, addr) -} \ No newline at end of file +} diff --git a/internal/servegit/serve_test.go b/internal/servegit/serve_test.go index c2a1e7c56c..62486a8d63 100644 --- a/internal/servegit/serve_test.go +++ b/internal/servegit/serve_test.go @@ -198,4 +198,4 @@ type testWriter struct { func (tw testWriter) Write(p []byte) (n int, err error) { tw.T.Log(string(p)) return len(p), nil -} \ No newline at end of file +} From 200f5b6ae765d78ecc010e7c8b18dfa9117ebc9a Mon Sep 17 00:00:00 2001 From: Idan Varsano Date: Wed, 24 Aug 2022 16:59:37 -0400 Subject: [PATCH 05/10] use clonepath to denote where to clone repo from --- internal/servegit/serve.go | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/internal/servegit/serve.go b/internal/servegit/serve.go index 492471fe02..604faaebf0 100644 --- a/internal/servegit/serve.go +++ b/internal/servegit/serve.go @@ -60,9 +60,9 @@ var indexHTML = template.Must(template.New("").Parse(` `)) type Repo struct { - Name string - URI string - Bare bool + Name string + URI string + ClonePath string } func (s *Serve) handler() http.Handler { @@ -205,10 +205,16 @@ func (s *Serve) Repos() ([]Repo, error) { name := filepath.ToSlash(subpath) reposRootIsRepo = reposRootIsRepo || name == "." + cloneURI := pathpkg.Join("/repos", name) + clonePath := cloneURI + + if !isBare { + clonePath += "/.git" + } repos = append(repos, Repo{ - Name: name, - URI: pathpkg.Join("/repos", name), - Bare: isBare, + Name: name, + URI: cloneURI, + ClonePath: clonePath, }) // At this point we know the directory is either a git repo or a bare git repo, @@ -245,4 +251,4 @@ func explainAddr(addr string) string { See https://docs.sourcegraph.com/admin/external_service/src_serve_git for instructions to configure in Sourcegraph. `, addr) -} +} \ No newline at end of file From 6b923b7d68c354a50b04222135ebff642d605112 Mon Sep 17 00:00:00 2001 From: Idan Varsano Date: Wed, 24 Aug 2022 17:02:01 -0400 Subject: [PATCH 06/10] fix test --- internal/servegit/serve_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/servegit/serve_test.go b/internal/servegit/serve_test.go index 62486a8d63..4f106fe1c0 100644 --- a/internal/servegit/serve_test.go +++ b/internal/servegit/serve_test.go @@ -48,7 +48,12 @@ func TestReposHandler(t *testing.T) { var want []Repo for _, name := range tc.repos { isBare := strings.HasSuffix(name, ".bare") - want = append(want, Repo{Name: name, URI: path.Join("/repos", name), Bare: isBare}) + uri := path.Join("/repos", name) + clonePath := uri + if !isBare { + clonePath += "/.git" + } + want = append(want, Repo{Name: name, URI: uri, ClonePath: clonePath}) } testReposHandler(t, h, want) @@ -198,4 +203,4 @@ type testWriter struct { func (tw testWriter) Write(p []byte) (n int, err error) { tw.T.Log(string(p)) return len(p), nil -} +} \ No newline at end of file From 8025890b10a2806607ba2b3110661efaa1eb5235 Mon Sep 17 00:00:00 2001 From: Idan Varsano Date: Thu, 25 Aug 2022 14:07:09 -0400 Subject: [PATCH 07/10] Address PR comments --- internal/servegit/serve.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/servegit/serve.go b/internal/servegit/serve.go index 604faaebf0..4e3b99a2f1 100644 --- a/internal/servegit/serve.go +++ b/internal/servegit/serve.go @@ -208,9 +208,11 @@ func (s *Serve) Repos() ([]Repo, error) { cloneURI := pathpkg.Join("/repos", name) clonePath := cloneURI - if !isBare { + // Regular git repos won't clone without the full path to the .git directory. + if isGit { clonePath += "/.git" } + repos = append(repos, Repo{ Name: name, URI: cloneURI, @@ -219,7 +221,7 @@ func (s *Serve) Repos() ([]Repo, error) { // 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 ite useful to support git submodules + // TODO: Look into whether it is useful to support git submodules return filepath.SkipDir }) @@ -251,4 +253,4 @@ func explainAddr(addr string) string { See https://docs.sourcegraph.com/admin/external_service/src_serve_git for instructions to configure in Sourcegraph. `, addr) -} \ No newline at end of file +} From 1ed940de6588f0f652a2b8f4217c415732c0f218 Mon Sep 17 00:00:00 2001 From: Idan Varsano Date: Thu, 25 Aug 2022 14:13:36 -0400 Subject: [PATCH 08/10] add changelog changes --- CHANGELOG.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 210e5affe6..3876270dc0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,8 +19,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.42.2 ### Fixed @@ -814,4 +819,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) \ No newline at end of file From 1a59bfc068b06d2ab8e40b91ae41e60caf114ec5 Mon Sep 17 00:00:00 2001 From: Idan Varsano Date: Thu, 25 Aug 2022 14:42:54 -0400 Subject: [PATCH 09/10] fix lint --- internal/servegit/serve_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/servegit/serve_test.go b/internal/servegit/serve_test.go index 4f106fe1c0..b314b76de4 100644 --- a/internal/servegit/serve_test.go +++ b/internal/servegit/serve_test.go @@ -203,4 +203,4 @@ type testWriter struct { func (tw testWriter) Write(p []byte) (n int, err error) { tw.T.Log(string(p)) return len(p), nil -} \ No newline at end of file +} From a584d66cf0822d096f2d82376c5d502db33fadf7 Mon Sep 17 00:00:00 2001 From: Idan Varsano Date: Thu, 25 Aug 2022 15:01:17 -0400 Subject: [PATCH 10/10] Fix changelog --- CHANGELOG.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 95ae32feb8..eba4031fac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,8 +43,6 @@ All notable changes to `src-cli` are documented in this file. ### Fixed -### Removed -======= - INTERNAL ONLY: Fixed src batch exec not logging errors.