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

Diff zoekt git settings #635

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

xvandish
Copy link
Contributor

@xvandish xvandish commented Aug 2, 2023

This is an optimization on top of what I introduced in #600.

Rather than always updating the zoekt settings for every repo, which can be at least 10 git calls per repo (10 is the current number of zoekt settings we store), we instead get all zoekt-* settings with one call, and then only make calls to update the changed values. I remember testing, and the code handled new and deleted zoekt-* keys fine. This is just a constant speedup, but it really limits the number of git calls and IO we perform, especially with larger sets of repos.

We also make another change, in that if the zoekt-settings are updated, we return the repoDest which triggers a re-index right away.

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 code looks a little tricky. Maybe you want to take this opportunity to switch to using go-git to do this? Look at the implementation of setFetch to see how you could do this.

outBuf := &bytes.Buffer{}
errBuf := &bytes.Buffer{}
cmd.Stdout = outBuf
cmd.Stderr = errBuf
Copy link
Member

Choose a reason for hiding this comment

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

you don't use errBuf

Comment on lines +82 to +85
settingsToUpdate, err := getZoektSettingsToUpdate(repoDest, settings, keys)
if err != nil {
return false, err
}
Copy link
Member

Choose a reason for hiding this comment

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

if this fails do you maybe wanna fallback to just updating everything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure that makes sense, good idea

@xvandish
Copy link
Contributor Author

xvandish commented Aug 3, 2023

this code looks a little tricky. Maybe you want to take this opportunity to switch to using go-git to do this? Look at the implementation of setFetch to see how you could do this.

Sure, I'll give it a shot! thanks for taking a look

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.

2 participants