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

Fix over recursion of git serve #810

Merged
merged 11 commits into from
Aug 25, 2022
Merged

Fix over recursion of git serve #810

merged 11 commits into from
Aug 25, 2022

Conversation

varsanojidan
Copy link
Contributor

@varsanojidan varsanojidan commented Aug 5, 2022

Fixes https://github.com/sourcegraph/customer/issues/1210

So there was some weird behavior circling around this change.

Before the change:

  • When searching for repos, we would only continue recursing on bare git repos, looking for subprojects underneath them
  • Our tests were testing as if we were recursing on non-bare git repos (even though we weren't in the code), and they were passing because of the strange way we were initializing the non-bare git repos in our tests (--bare with a path to the .git directory)
  • Our tests would pass, but functionality wise we would never continue searching for git repos once we hit our first .git directory

After the change:

  • The change corrected the behavior of the code to match what we were testing
  • The extra recursion after the change, now that we check every single directory to see if its a repo, caused an exponential hit in performance to the server i.e. serving 100 avg sized repos and querying http://localhost:3434/v1/list-repos takes several minutes to return as opposed to a couple of seconds.
  • This performance hit directly affected the issue linked at the top of the thread.

In this PR I have removed the support for subprojects making the code return the moment it finds either a git or a bare git repo. My assumption here was that because it was broken before this change, there didn't seem to be much of a need for us to support subprojects, and once reverted, the performance is now back to where it was previously.

I am open to feedback on if it is important for us to server subprojects this way, in which case we would need to think about a significantly more performant way of doing so.

Test plan

Tested manually to validate the improved runtime. Existing non-removed tests pass.

@@ -62,6 +62,7 @@ var indexHTML = template.Must(template.New("").Parse(`<html>
type Repo struct {
Name string
URI string
Bare bool
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to the changes here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@mrnugget mrnugget left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving final review to @keegancsmith but let some formatting drive-by comments

internal/servegit/serve.go Outdated Show resolved Hide resolved
internal/servegit/serve_test.go Outdated Show resolved Hide resolved
Copy link
Member

@keegancsmith keegancsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behaviour change is changelog worthy incase it breaks someone who is now relying on this new behaviour.

@mike-r-mclaughlin anyone you know who is using src serve-git on non-bare repos with subrepos? (maybe reply in slack, public PR)

@@ -62,6 +62,7 @@ var indexHTML = template.Must(template.New("").Parse(`<html>
type Repo struct {
Name string
URI string
Bare bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mike-r-mclaughlin
Copy link

@mike-r-mclaughlin anyone you know who is using src serve-git on non-bare repos with subrepos? (maybe reply in slack, public PR)

I can't think of anyone off the top of my head. I don't know of anyone that is using src-serve-git with subrepos. But, still good idea to call it out in changelog.

internal/servegit/serve.go Outdated Show resolved Hide resolved
internal/servegit/serve.go Outdated Show resolved Hide resolved
internal/servegit/serve.go Outdated Show resolved Hide resolved
@varsanojidan varsanojidan enabled auto-merge (squash) August 25, 2022 18:59
@varsanojidan varsanojidan enabled auto-merge (squash) August 25, 2022 19:01
@varsanojidan varsanojidan merged commit 96c8c8c into main Aug 25, 2022
@varsanojidan varsanojidan deleted the iv/fix-walking-too-far branch August 25, 2022 19:03
scjohns pushed a commit that referenced this pull request Apr 24, 2023
* Fix over recursion of git serve
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants