Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

[Backport 5.0] Structural Search: Modify revisions values to match on Zoekt branches#51419

Merged
unknwon merged 1 commit into
5.0from
backport-51076-to-5.0
May 3, 2023
Merged

[Backport 5.0] Structural Search: Modify revisions values to match on Zoekt branches#51419
unknwon merged 1 commit into
5.0from
backport-51076-to-5.0

Conversation

@github-actions

@github-actions github-actions Bot commented May 3, 2023

Copy link
Copy Markdown
Contributor

Issue: https://github.com/sourcegraph/sourcegraph/issues/50506

The issue manifests for a structural search for any revisions specified in the query that have both of the following:

  1. the revision is a commit and is the latest commit of an indexed branch in Zoekt
  2. the revision is not the branch name indexed in Zoekt (i.e. it is not a revision specified in experimentalFeatures.search.index.branches)

The following fix works for all cases where the indexed branch to query is HEAD. Therefore this will resolve the code insights use case since code insights repo filter does not accept revisions.

The fix in this PR is intended to be something we can ship quickly and therefore I hoped to avoid modifying existing logic as much as possible. For this reason we keep the SearchJob arguments and logic unchanged but just modify the repo revisions passed in.

Note that there is still a remaining issue where if we specify two revisions for our query and both of those revisions match both criteria described above, then we do not have enough information to determine which branch to use since we'll have two branches and neither of them have a map to their latest commit. To resolve this I believe we'd have to modify PartitionRepos() and/or IndexedRepoRevs which would be a bit more involved so I am planning to spin off to a separate ticket.

Test plan

<!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles -->

New method has test cases added
Backport 633c37b from #51076

…#51076)

Issue: https://github.com/sourcegraph/sourcegraph/issues/50506

The issue manifests for a structural search for any revisions specified
in the query that have both of the following:
1) the revision is a commit and is the latest commit of an indexed
branch in Zoekt
2) the revision is not the branch name indexed in Zoekt (i.e. it is not
a revision specified in `experimentalFeatures.search.index.branches`)

The following fix works for all cases where the indexed branch to query
is `HEAD`. Therefore this will resolve the code insights use case since
code insights repo filter does not accept revisions.

The fix in this PR is intended to be something we can ship quickly and
therefore I hoped to avoid modifying existing logic as much as possible.
For this reason we keep the `SearchJob` arguments and logic unchanged
but just modify the repo revisions passed in.

Note that there is still a remaining issue where if we specify two
revisions for our query and both of those revisions match both criteria
described above, then we do not have enough information to determine
which branch to use since we'll have two branches and neither of them
have a map to their latest commit. To resolve this I believe we'd have
to modify
[`PartitionRepos()`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/search/zoekt/indexed_search.go?L177:6)
and/or
[`IndexedRepoRevs`](https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/search/zoekt/indexed_search.go?L38)
which would be a bit more involved so I am planning to spin off to a
separate ticket.

## Test plan

<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->

New method has test cases added

(cherry picked from commit 633c37b)
@unknwon unknwon enabled auto-merge (squash) May 3, 2023 15:42
@unknwon unknwon merged commit 6d37df2 into 5.0 May 3, 2023
@unknwon unknwon deleted the backport-51076-to-5.0 branch May 3, 2023 16:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants