feat(search): Make search aware of perforce changelist id mapping#63563
Conversation
eseliger
left a comment
There was a problem hiding this comment.
not a search expert - so leaving the real review to someone else. but this seems like a nice low-level integration to me
left a few suggestions/considerations inline
jtibshirani
left a comment
There was a problem hiding this comment.
This looks very reasonable to me and doesn't feel too messy. The repo resolver is the central place in search where we interpret the repository specifications in the query. We will need good test coverage though, this is core code and there may be subtle interactions!
A couple high-level comments:
- I'd prefer simpler semantics for the translation: if the changelist ID mapping feature is enabled, then all revisions in queries should be interpreted as changelists. If they cannot be, then we fail with a clear error like "invalid changelist ID". Our goal would be for end users not to be aware of the changelist ID -> commit hash mapping. What do you think?
- We should double-check this interacts okay with Zoekt's repo partitioning and other downstream revision logic. It looks fine to me, just something to be aware of!
…ist-id' of https://github.com/sourcegraph/sourcegraph into mmanela-splf-116-perforce-searching-by-perforce-changelist-id
camdencheek
left a comment
There was a problem hiding this comment.
General comment: is this the right layer to handle this?
My thinking is: what if we handled this with git refs during perforce -> git translation? If a changelist really does just resolve to a commit, couldn't we just create a symbolic "refs/changelists/80132"? That feels less risky to me than trying to add perforce emulation back on top of our git translation.
This is a good question -- I believe we discussed this offline, but would be good to document the reasoning here! |
jtibshirani
left a comment
There was a problem hiding this comment.
This looks good to me. I left a few small comments.
…ist-id' of https://github.com/sourcegraph/sourcegraph into mmanela-splf-116-perforce-searching-by-perforce-changelist-id
…rce-changelist-id
| GetRepoCommitChangelist(ctx context.Context, repoID api.RepoID, changelistID int64) (*types.RepoCommit, error) | ||
|
|
||
| // GetRepoCommitChangelistBatch bulk loads repo commits for given repo ids and changelistIds | ||
| GetRepoCommitChangelistBatch(ctx context.Context, rcs ...RepoChangelistIDs) (map[api.RepoID]map[int64]*types.RepoCommit, error) |
There was a problem hiding this comment.
Tiny thing I noticed: It looks like our naming standard is to use Batch at the start as in BatchInsertCommitSHAsWithPerforceChangelistID.
…ist-id' of https://github.com/sourcegraph/sourcegraph into mmanela-splf-116-perforce-searching-by-perforce-changelist-id
https://linear.app/sourcegraph/issue/SPLF-116/perforce-searching-by-perforce-changelist-id
Details
We have had requests from our customers using Perforce to be able to search inside of a changelist id. For a commit sha we support doing this
But for perforce they want to do the same thing but with the change list ids. Which would look like this
To support this, I am attempting to smartly detect when we should do a DB round trip and look up the proper mapping. I built a simple heuristic that is
This mapping is just a best effort, if it fails it just falls back on current behavior.
We are doing with a syntax of
@changelist/CL_IDinstead of supporting@CL_IDto future proof us. This lookup focuses on finding the mapping in the DB but in the future we may want to pre-create these refs in the db duing mapping of perforce CLs to git commits.Limitations
This works well in testing however, the repo name@changelist/rev we return contains the sha
I investigated changing this but it would required a larger change in resolving the stream results. While that would be nice to have, I decided to keep this minimal for now and add that later if needed
Test plan
Changelog
context:global repo:^repo/name$@changelist/83854