-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Search backend: simplify repo:contains
resolution
#40280
Conversation
Codenotify: Notifying subscribers in CODENOTIFY files for diff 57bf297...1e478e5.
|
and = append(and, &zoekt.Type{Type: zoekt.TypeRepo, Child: zoekt.NewAnd(repoHasFilters...)}) | ||
and = append(and, zoekt.NewAnd(repoHasFilters...)) |
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.
QueryForFileContentArgs
now returns a zoekt.Type
query, so we don't need to wrap here anymore
@@ -586,25 +585,23 @@ func (r *Resolver) filterRepoHasFileContent( | |||
q := searchzoekt.QueryForFileContentArgs(opt, op.CaseSensitiveRepoFilters) | |||
q = zoektquery.NewAnd(&zoektquery.BranchesRepos{List: indexed.BranchRepos()}, q) | |||
|
|||
repos, err := r.zoekt.List(ctx, q, &zoekt.ListOptions{Minimal: true}) |
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.
Becuase QueryForFileContentArgs
now returns a zoekt.Type{}
query, the query will only ever return repos and it will work with zoekt.List()
.
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
25596b0
to
f062cde
Compare
98a876c
to
1e478e5
Compare
I think I figured out the right way to use Zoekt's
Type
andList()
queries, which simplifies therepo:contains...
resolution during repo resolution. This change is also required for correct behavior of the negated form of the predicate (which is in the works).Stacked on #40239
Test plan
Integration tests, manual testing, pulled this off another larger change that I've been testing more extensively.