feat(search): Make search aware of perforce changelist id mapping (#63563)

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

```
context:global repo:^perforce-sgdev-org/rhia-depot-test$@345c17c` some text
```

But for perforce they want to do the same thing but with the change list
ids. Which would look like this

```
context:global repo:^perforce-sgdev-org/rhia-depot-test$@changelist/83732` some text
```

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
1. Is perforce changelist mapping feature flag enabled
2. Is this a perforce repo?
3. Is the revision request a integer ?

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_ID` instead of supporting
`@CL_ID` to 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

![image](https://github.com/sourcegraph/sourcegraph/assets/304410/a673b9bd-d11f-4b36-bd95-c21ab8a5c4af)


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

<!-- REQUIRED; info at
https://docs-legacy.sourcegraph.com/dev/background-information/testing_principles
-->

## Changelog
- For perforce depots, support searching within a specific changelist by
specifying a ref like `context:global repo:^repo/name$@changelist/83854`
This commit is contained in:
Matthew Manela 2024-07-09 14:01:05 -04:00 committed by GitHub
parent 64ebfca904
commit 814aceb46f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
13 changed files with 643 additions and 14 deletions

View File

@ -36,7 +36,6 @@
"editor.formatOnSave": true,
"go.useLanguageServer": true,
"gopls": {
"build.allowImplicitNetworkAccess": true,
"local": "github.com/sourcegraph/sourcegraph"
},
"npm.packageManager": "pnpm",

View File

@ -271,7 +271,7 @@ export const GitCommitNode: React.FunctionComponent<React.PropsWithChildren<GitC
const treeCanonicalURL =
isPerforceChangelistMappingEnabled() && isPerforceDepot
? node.tree.canonicalURL.replace(node.oid, refID)
? node.tree.canonicalURL.replace(node.oid, `changelist/${refID}`)
: node.tree.canonicalURL
const viewFilesCommitElement = node.tree && (

View File

@ -5,6 +5,7 @@ import (
"fmt"
"net/url"
"strconv"
"strings"
"sync"
"time"
@ -318,6 +319,8 @@ func (r *RepositoryResolver) Changelist(ctx context.Context, args *RepositoryCha
attribute.String("changelist", args.CID))
defer tr.EndWithErr(&err)
// Strip "changelist/" prefix if present since we will sometimes append it in the UI.
args.CID = strings.TrimPrefix(args.CID, "changelist/")
cid, err := strconv.ParseInt(args.CID, 10, 64)
if err != nil {
// NOTE: From the UI, the user may visit a URL like:

View File

@ -111,6 +111,36 @@ func TestRepository_Changelist(t *testing.T) {
"canonicalURL": "/perforce.sgdev.org/gorilla/mux/-/changelist/123",
"commit": {
"oid": "%s"
}
}
}
}
`, exampleCommitSHA1),
})
RunTest(t, &Test{
Schema: mustParseGraphQLSchema(t, db),
Query: `
{
repository(name: "perforce.sgdev.org/gorilla/mux") {
changelist(cid: "changelist/123") {
cid
canonicalURL
commit {
oid
}
}
}
}
`,
ExpectedResult: fmt.Sprintf(`
{
"repository": {
"changelist": {
"cid": "123",
"canonicalURL": "/perforce.sgdev.org/gorilla/mux/-/changelist/123",
"commit": {
"oid": "%s"
}
}
}

View File

@ -57924,6 +57924,10 @@ func (c RecentViewSignalStoreListFuncCall) Results() []interface{} {
// github.com/sourcegraph/sourcegraph/internal/database) used for unit
// testing.
type MockRepoCommitsChangelistsStore struct {
// BatchGetRepoCommitChangelistFunc is an instance of a mock function
// object controlling the behavior of the method
// BatchGetRepoCommitChangelist.
BatchGetRepoCommitChangelistFunc *RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFunc
// BatchInsertCommitSHAsWithPerforceChangelistIDFunc is an instance of a
// mock function object controlling the behavior of the method
// BatchInsertCommitSHAsWithPerforceChangelistID.
@ -57941,6 +57945,11 @@ type MockRepoCommitsChangelistsStore struct {
// all results, unless overwritten.
func NewMockRepoCommitsChangelistsStore() *MockRepoCommitsChangelistsStore {
return &MockRepoCommitsChangelistsStore{
BatchGetRepoCommitChangelistFunc: &RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFunc{
defaultHook: func(context.Context, ...database.RepoChangelistIDs) (r0 map[api.RepoID]map[int64]*types.RepoCommit, r1 error) {
return
},
},
BatchInsertCommitSHAsWithPerforceChangelistIDFunc: &RepoCommitsChangelistsStoreBatchInsertCommitSHAsWithPerforceChangelistIDFunc{
defaultHook: func(context.Context, api.RepoID, []types.PerforceChangelist) (r0 error) {
return
@ -57964,6 +57973,11 @@ func NewMockRepoCommitsChangelistsStore() *MockRepoCommitsChangelistsStore {
// unless overwritten.
func NewStrictMockRepoCommitsChangelistsStore() *MockRepoCommitsChangelistsStore {
return &MockRepoCommitsChangelistsStore{
BatchGetRepoCommitChangelistFunc: &RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFunc{
defaultHook: func(context.Context, ...database.RepoChangelistIDs) (map[api.RepoID]map[int64]*types.RepoCommit, error) {
panic("unexpected invocation of MockRepoCommitsChangelistsStore.BatchGetRepoCommitChangelist")
},
},
BatchInsertCommitSHAsWithPerforceChangelistIDFunc: &RepoCommitsChangelistsStoreBatchInsertCommitSHAsWithPerforceChangelistIDFunc{
defaultHook: func(context.Context, api.RepoID, []types.PerforceChangelist) error {
panic("unexpected invocation of MockRepoCommitsChangelistsStore.BatchInsertCommitSHAsWithPerforceChangelistID")
@ -57987,6 +58001,9 @@ func NewStrictMockRepoCommitsChangelistsStore() *MockRepoCommitsChangelistsStore
// given implementation, unless overwritten.
func NewMockRepoCommitsChangelistsStoreFrom(i database.RepoCommitsChangelistsStore) *MockRepoCommitsChangelistsStore {
return &MockRepoCommitsChangelistsStore{
BatchGetRepoCommitChangelistFunc: &RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFunc{
defaultHook: i.BatchGetRepoCommitChangelist,
},
BatchInsertCommitSHAsWithPerforceChangelistIDFunc: &RepoCommitsChangelistsStoreBatchInsertCommitSHAsWithPerforceChangelistIDFunc{
defaultHook: i.BatchInsertCommitSHAsWithPerforceChangelistID,
},
@ -57999,6 +58016,127 @@ func NewMockRepoCommitsChangelistsStoreFrom(i database.RepoCommitsChangelistsSto
}
}
// RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFunc describes the
// behavior when the BatchGetRepoCommitChangelist method of the parent
// MockRepoCommitsChangelistsStore instance is invoked.
type RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFunc struct {
defaultHook func(context.Context, ...database.RepoChangelistIDs) (map[api.RepoID]map[int64]*types.RepoCommit, error)
hooks []func(context.Context, ...database.RepoChangelistIDs) (map[api.RepoID]map[int64]*types.RepoCommit, error)
history []RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFuncCall
mutex sync.Mutex
}
// BatchGetRepoCommitChangelist delegates to the next hook function in the
// queue and stores the parameter and result values of this invocation.
func (m *MockRepoCommitsChangelistsStore) BatchGetRepoCommitChangelist(v0 context.Context, v1 ...database.RepoChangelistIDs) (map[api.RepoID]map[int64]*types.RepoCommit, error) {
r0, r1 := m.BatchGetRepoCommitChangelistFunc.nextHook()(v0, v1...)
m.BatchGetRepoCommitChangelistFunc.appendCall(RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFuncCall{v0, v1, r0, r1})
return r0, r1
}
// SetDefaultHook sets function that is called when the
// BatchGetRepoCommitChangelist method of the parent
// MockRepoCommitsChangelistsStore instance is invoked and the hook queue is
// empty.
func (f *RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFunc) SetDefaultHook(hook func(context.Context, ...database.RepoChangelistIDs) (map[api.RepoID]map[int64]*types.RepoCommit, error)) {
f.defaultHook = hook
}
// PushHook adds a function to the end of hook queue. Each invocation of the
// BatchGetRepoCommitChangelist method of the parent
// MockRepoCommitsChangelistsStore instance invokes the hook at the front of
// the queue and discards it. After the queue is empty, the default hook
// function is invoked for any future action.
func (f *RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFunc) PushHook(hook func(context.Context, ...database.RepoChangelistIDs) (map[api.RepoID]map[int64]*types.RepoCommit, error)) {
f.mutex.Lock()
f.hooks = append(f.hooks, hook)
f.mutex.Unlock()
}
// SetDefaultReturn calls SetDefaultHook with a function that returns the
// given values.
func (f *RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFunc) SetDefaultReturn(r0 map[api.RepoID]map[int64]*types.RepoCommit, r1 error) {
f.SetDefaultHook(func(context.Context, ...database.RepoChangelistIDs) (map[api.RepoID]map[int64]*types.RepoCommit, error) {
return r0, r1
})
}
// PushReturn calls PushHook with a function that returns the given values.
func (f *RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFunc) PushReturn(r0 map[api.RepoID]map[int64]*types.RepoCommit, r1 error) {
f.PushHook(func(context.Context, ...database.RepoChangelistIDs) (map[api.RepoID]map[int64]*types.RepoCommit, error) {
return r0, r1
})
}
func (f *RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFunc) nextHook() func(context.Context, ...database.RepoChangelistIDs) (map[api.RepoID]map[int64]*types.RepoCommit, error) {
f.mutex.Lock()
defer f.mutex.Unlock()
if len(f.hooks) == 0 {
return f.defaultHook
}
hook := f.hooks[0]
f.hooks = f.hooks[1:]
return hook
}
func (f *RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFunc) appendCall(r0 RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFuncCall) {
f.mutex.Lock()
f.history = append(f.history, r0)
f.mutex.Unlock()
}
// History returns a sequence of
// RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFuncCall objects
// describing the invocations of this function.
func (f *RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFunc) History() []RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFuncCall {
f.mutex.Lock()
history := make([]RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFuncCall, len(f.history))
copy(history, f.history)
f.mutex.Unlock()
return history
}
// RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFuncCall is an
// object that describes an invocation of method
// BatchGetRepoCommitChangelist on an instance of
// MockRepoCommitsChangelistsStore.
type RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFuncCall struct {
// Arg0 is the value of the 1st argument passed to this method
// invocation.
Arg0 context.Context
// Arg1 is a slice containing the values of the variadic arguments
// passed to this method invocation.
Arg1 []database.RepoChangelistIDs
// Result0 is the value of the 1st result returned from this method
// invocation.
Result0 map[api.RepoID]map[int64]*types.RepoCommit
// Result1 is the value of the 2nd result returned from this method
// invocation.
Result1 error
}
// Args returns an interface slice containing the arguments of this
// invocation. The variadic slice argument is flattened in this array such
// that one positional argument and three variadic arguments would result in
// a slice of four, not two.
func (c RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFuncCall) Args() []interface{} {
trailing := []interface{}{}
for _, val := range c.Arg1 {
trailing = append(trailing, val)
}
return append([]interface{}{c.Arg0}, trailing...)
}
// Results returns an interface slice containing the results of this
// invocation.
func (c RepoCommitsChangelistsStoreBatchGetRepoCommitChangelistFuncCall) Results() []interface{} {
return []interface{}{c.Result0, c.Result1}
}
// RepoCommitsChangelistsStoreBatchInsertCommitSHAsWithPerforceChangelistIDFunc
// describes the behavior when the
// BatchInsertCommitSHAsWithPerforceChangelistID method of the parent

View File

@ -22,9 +22,12 @@ type RepoCommitsChangelistsStore interface {
// GetLatestForRepo will return the latest commit that has been mapped in the database.
GetLatestForRepo(ctx context.Context, repoID api.RepoID) (*types.RepoCommit, error)
// GetRepoCommit will return the mathcing row from the table for the given repo ID and the
// GetRepoCommit will return the matching row from the table for the given repo ID and the
// given changelist ID.
GetRepoCommitChangelist(ctx context.Context, repoID api.RepoID, changelistID int64) (*types.RepoCommit, error)
// BatchGetRepoCommitChangelist bulk loads repo commits for given repo ids and changelistIds
BatchGetRepoCommitChangelist(ctx context.Context, rcs ...RepoChangelistIDs) (map[api.RepoID]map[int64]*types.RepoCommit, error)
}
type repoCommitsChangelistsStore struct {
@ -119,3 +122,66 @@ func (s *repoCommitsChangelistsStore) GetRepoCommitChangelist(ctx context.Contex
}
return repoCommit, nil
}
type RepoChangelistIDs struct {
RepoID api.RepoID
ChangelistIDs []int64
}
var getRepoCommitFmtBatchStr = `
SELECT
id,
repo_id,
commit_sha,
perforce_changelist_id
FROM
repo_commits_changelists
WHERE
%s;
`
func (s *repoCommitsChangelistsStore) BatchGetRepoCommitChangelist(ctx context.Context, rcs ...RepoChangelistIDs) (map[api.RepoID]map[int64]*types.RepoCommit, error) {
res := make(map[api.RepoID]map[int64]*types.RepoCommit, len(rcs))
for _, rc := range rcs {
res[rc.RepoID] = make(map[int64]*types.RepoCommit, len(rc.ChangelistIDs))
}
var where []*sqlf.Query
for _, rc := range rcs {
changeListIdsLength := len(rc.ChangelistIDs)
if changeListIdsLength == 0 {
continue
}
items := make([]*sqlf.Query, changeListIdsLength)
for i, id := range rc.ChangelistIDs {
items[i] = sqlf.Sprintf("%d", id)
}
where = append(where, sqlf.Sprintf("(repo_id=%d AND perforce_changelist_id IN (%s))", rc.RepoID, sqlf.Join(items, ",")))
}
var whereClause *sqlf.Query
if len(where) > 0 {
whereClause = sqlf.Join(where, "\n OR ")
} else {
// If input has no changeList ids just return an empty result
return res, nil
}
q := sqlf.Sprintf(getRepoCommitFmtBatchStr, whereClause)
rows, err := s.Query(ctx, q)
if err != nil {
return nil, err
}
defer rows.Close()
for rows.Next() {
if repoCommit, err := scanRepoCommitRow(rows); err != nil {
return nil, err
} else {
res[repoCommit.RepoID][repoCommit.PerforceChangelistID] = repoCommit
}
}
return res, rows.Err()
}

View File

@ -27,14 +27,18 @@ func TestRepoCommitsChangelists(t *testing.T) {
repos := db.Repos()
err := repos.Create(ctx, &types.Repo{ID: 1, Name: "foo"})
require.NoError(t, err, "failed to insert repo")
err = repos.Create(ctx, &types.Repo{ID: 2, Name: "bar"})
require.NoError(t, err, "failed to insert repo")
repoID := int32(1)
repoID1 := int32(1)
repoID2 := int32(2)
commitSHA1 := "98d3ec26623660f17f6c298943f55aa339aa894a"
commitSHA2 := "4b6152a804c4c177f5fe0dfd61e71cacb64d64dd"
commitSHA3 := "e9c83398bbd4c4e9481fd20f100a7e49d68d7510"
commitSHA4 := "aac83398bbd4c4e9481fd20f100a7e49d68d7510"
data := []types.PerforceChangelist{
dataForRepo1 := []types.PerforceChangelist{
{
CommitSHA: api.CommitID(commitSHA1),
ChangelistID: 123,
@ -48,16 +52,25 @@ func TestRepoCommitsChangelists(t *testing.T) {
ChangelistID: 125,
},
}
dataForRepo2 := []types.PerforceChangelist{
{
CommitSHA: api.CommitID(commitSHA4),
ChangelistID: 126,
},
}
s := RepoCommitsChangelistsWith(logger, db)
err = s.BatchInsertCommitSHAsWithPerforceChangelistID(ctx, api.RepoID(repoID), data)
err = s.BatchInsertCommitSHAsWithPerforceChangelistID(ctx, api.RepoID(repoID1), dataForRepo1)
if err != nil {
t.Fatal(err)
}
err = s.BatchInsertCommitSHAsWithPerforceChangelistID(ctx, api.RepoID(repoID2), dataForRepo2)
if err != nil {
t.Fatal(err)
}
t.Run("BatchInsertCommitSHAsWithPerforceChangelistID", func(t *testing.T) {
rows, err := db.QueryContext(ctx, `SELECT repo_id, commit_sha, perforce_changelist_id, created_at FROM repo_commits_changelists ORDER by id`)
rows, err := db.QueryContext(ctx, `SELECT repo_id, commit_sha, perforce_changelist_id, created_at FROM repo_commits_changelists WHERE repo_id=1 ORDER by id`)
if err != nil {
t.Fatal(err)
}
@ -112,14 +125,14 @@ func TestRepoCommitsChangelists(t *testing.T) {
t.Run("GetLatestForRepo", func(t *testing.T) {
t.Run("existing repo", func(t *testing.T) {
repoCommit, err := s.GetLatestForRepo(ctx, api.RepoID(repoID))
repoCommit, err := s.GetLatestForRepo(ctx, api.RepoID(repoID1))
require.NoError(t, err, "unexpected error in GetLatestForRepo")
require.NotNil(t, repoCommit, "repoCommit was not expected to be nil")
require.Equal(
t,
&types.RepoCommit{
ID: 3,
RepoID: api.RepoID(repoID),
RepoID: api.RepoID(repoID1),
CommitSHA: dbutil.CommitBytea(commitSHA3),
PerforceChangelistID: 125,
},
@ -129,7 +142,7 @@ func TestRepoCommitsChangelists(t *testing.T) {
})
t.Run("non existing repo", func(t *testing.T) {
repoCommit, err := s.GetLatestForRepo(ctx, api.RepoID(2))
repoCommit, err := s.GetLatestForRepo(ctx, api.RepoID(3))
require.Error(t, err)
require.True(t, errors.Is(err, sql.ErrNoRows))
require.Nil(t, repoCommit)
@ -163,4 +176,34 @@ func TestRepoCommitsChangelists(t *testing.T) {
}
})
})
t.Run("BatchGetRepoCommitChangelist", func(t *testing.T) {
changelistIds := []RepoChangelistIDs{
{
RepoID: api.RepoID(1),
ChangelistIDs: []int64{123, 124, 125},
},
{
RepoID: api.RepoID(2),
ChangelistIDs: []int64{126},
},
}
t.Run("existing rows", func(t *testing.T) {
got, err := s.BatchGetRepoCommitChangelist(ctx, changelistIds...)
require.NoError(t, err)
// Make sure every items from changelist ids is present in the result.
for _, ids := range changelistIds {
for _, id := range ids.ChangelistIDs {
_, found := got[ids.RepoID][id]
require.True(t, found, "row for repo %d and changelist %d was not found", ids.RepoID, id)
}
}
})
t.Run("return empty map if no result", func(t *testing.T) {
got, err := s.BatchGetRepoCommitChangelist(ctx, RepoChangelistIDs{RepoID: 3})
require.NoError(t, err)
require.Len(t, got[3], 0)
})
})
}

View File

@ -431,6 +431,9 @@ var minimalRepoColumns = []string{
"repo.name",
"repo.private",
"repo.stars",
"repo.external_id",
"repo.external_service_type",
"repo.external_service_id",
}
var repoColumns = []string{
@ -834,7 +837,14 @@ func (s *repoStore) StreamMinimalRepos(ctx context.Context, opt ReposListOptions
err = s.list(ctx, tr, opt, func(rows *sql.Rows) error {
var r types.MinimalRepo
var private bool
err := rows.Scan(&r.ID, &r.Name, &private, &dbutil.NullInt{N: &r.Stars})
err := rows.Scan(
&r.ID,
&r.Name,
&private,
&dbutil.NullInt{N: &r.Stars},
&dbutil.NullString{S: &r.ExternalRepo.ID},
&dbutil.NullString{S: &r.ExternalRepo.ServiceType},
&dbutil.NullString{S: &r.ExternalRepo.ServiceID})
if err != nil {
return err
}

View File

@ -145,7 +145,7 @@ func setZoektIndexed(t *testing.T, db DB, name api.RepoName) {
func repoNamesFromRepos(repos []*types.Repo) []types.MinimalRepo {
rnames := make([]types.MinimalRepo, 0, len(repos))
for _, repo := range repos {
rnames = append(rnames, types.MinimalRepo{ID: repo.ID, Name: repo.Name})
rnames = append(rnames, types.MinimalRepo{ID: repo.ID, Name: repo.Name, ExternalRepo: repo.ExternalRepo})
}
return rnames

View File

@ -17,6 +17,7 @@ go_library(
"//internal/database",
"//internal/dotcom",
"//internal/endpoint",
"//internal/extsvc",
"//internal/gitserver",
"//internal/gitserver/gitdomain",
"//internal/grpc/defaults",
@ -60,10 +61,12 @@ go_test(
],
deps = [
"//internal/api",
"//internal/conf",
"//internal/database",
"//internal/database/dbmocks",
"//internal/database/dbtest",
"//internal/endpoint",
"//internal/extsvc",
"//internal/gitserver",
"//internal/gitserver/gitdomain",
"//internal/grpc/defaults",
@ -76,6 +79,7 @@ go_test(
"//internal/types",
"//lib/errors",
"//lib/iterator",
"//schema",
"@com_github_derision_test_go_mockgen_v2//testutil/require",
"@com_github_google_go_cmp//cmp",
"@com_github_grafana_regexp//:regexp",

View File

@ -24,6 +24,7 @@ import (
"github.com/sourcegraph/sourcegraph/internal/database"
"github.com/sourcegraph/sourcegraph/internal/dotcom"
"github.com/sourcegraph/sourcegraph/internal/endpoint"
"github.com/sourcegraph/sourcegraph/internal/extsvc"
"github.com/sourcegraph/sourcegraph/internal/gitserver"
"github.com/sourcegraph/sourcegraph/internal/gitserver/gitdomain"
"github.com/sourcegraph/sourcegraph/internal/grpc/defaults"
@ -347,6 +348,10 @@ func (r *Resolver) doQueryDB(ctx context.Context, tr trace.Trace, op search.Repo
associatedRepoRevs, missingRepoRevs := r.associateReposWithRevs(repos, searchContextRepositoryRevisions, includePatternRevs)
tr.AddEvent("completed rev association")
tr.AddEvent("starting remapping for perforce")
associatedRepoRevs = r.resolvePerforceChangeListIdsToCommitSHAs(ctx, associatedRepoRevs)
tr.AddEvent("completed remapping for perforce")
return dbResolved{
Associated: associatedRepoRevs,
Missing: missingRepoRevs,
@ -466,6 +471,85 @@ func (r *Resolver) associateReposWithRevs(
return associatedRevs[:notMissingCount], associatedRevs[notMissingCount:]
}
var changelistRegex = regexp.MustCompile(`^changelist/(\d+)$`)
func extractChangelistNumber(revSpec string) (int64, error) {
matches := changelistRegex.FindStringSubmatch(revSpec)
if matches == nil {
return 0, errors.Newf("invalid changelist format: %s", revSpec)
}
numberStr := matches[1]
number, err := strconv.ParseInt(numberStr, 10, 0)
if err != nil {
return 0, errors.Newf("failed to parse changelist number: %w", err)
}
return number, nil
}
// resolvePerforceChangeListIds re-writes resolved refs for perforce repos
// to use the sha of the changelist instead of the changelist id
func (r *Resolver) resolvePerforceChangeListIdsToCommitSHAs(
ctx context.Context,
repoRevs []RepoRevSpecs,
) []RepoRevSpecs {
c := conf.Get()
isPerforceChangelistMappingEnabled := c.ExperimentalFeatures != nil && c.ExperimentalFeatures.PerforceChangelistMapping == "enabled"
if !isPerforceChangelistMappingEnabled {
return repoRevs
}
reposToMap := []database.RepoChangelistIDs{}
for _, repoRev := range repoRevs {
if repoRev.Repo.ExternalRepo.ServiceType == extsvc.TypePerforce && len(repoRev.Revs) > 0 {
repoToMap := database.RepoChangelistIDs{
RepoID: repoRev.Repo.ID,
}
for _, rev := range repoRev.Revs {
// We assume that if a repo is a perforce repo and the revs looks like changelist ids
// then we want to map those to underlying shas
if changelistNumber, err := extractChangelistNumber(rev.RevSpec); err == nil {
repoToMap.ChangelistIDs = append(repoToMap.ChangelistIDs, changelistNumber)
}
}
if len(repoToMap.ChangelistIDs) > 0 {
reposToMap = append(reposToMap, repoToMap)
}
}
}
if len(reposToMap) <= 0 {
return repoRevs
}
changelistIDsToCommits, err := r.db.RepoCommitsChangelists().BatchGetRepoCommitChangelist(ctx, reposToMap...)
if err != nil {
r.logger.Warn("failed to get repo commit changelists", log.Error(err))
return repoRevs
}
// Remap the revs if we resolved in the db
for i := range repoRevs {
repoRev := &repoRevs[i]
if subMap, ok := changelistIDsToCommits[repoRev.Repo.ID]; ok &&
repoRev.Repo.ExternalRepo.ServiceType == extsvc.TypePerforce &&
len(repoRev.Revs) > 0 {
for j := range repoRev.Revs {
rev := &repoRev.Revs[j]
if changelistNumber, err := extractChangelistNumber(rev.RevSpec); err == nil {
if commit, ok := subMap[changelistNumber]; ok {
rev.RevSpec = string(commit.CommitSHA)
}
}
}
}
}
return repoRevs
}
// normalizeRefs handles three jobs:
// 1) expanding each ref glob into a set of refs
// 2) checking that every revision (except HEAD) exists

View File

@ -3,6 +3,8 @@ package repos
import (
stdcmp "cmp"
"context"
"crypto/sha1"
"encoding/hex"
"flag"
"fmt"
"os"
@ -20,10 +22,12 @@ import (
"github.com/sourcegraph/log/logtest"
"github.com/sourcegraph/sourcegraph/internal/api"
"github.com/sourcegraph/sourcegraph/internal/conf"
"github.com/sourcegraph/sourcegraph/internal/database"
"github.com/sourcegraph/sourcegraph/internal/database/dbmocks"
"github.com/sourcegraph/sourcegraph/internal/database/dbtest"
"github.com/sourcegraph/sourcegraph/internal/endpoint"
"github.com/sourcegraph/sourcegraph/internal/extsvc"
"github.com/sourcegraph/sourcegraph/internal/gitserver"
"github.com/sourcegraph/sourcegraph/internal/gitserver/gitdomain"
"github.com/sourcegraph/sourcegraph/internal/grpc/defaults"
@ -34,6 +38,7 @@ import (
"github.com/sourcegraph/sourcegraph/internal/types"
"github.com/sourcegraph/sourcegraph/lib/errors"
"github.com/sourcegraph/sourcegraph/lib/iterator"
"github.com/sourcegraph/sourcegraph/schema"
)
func TestMain(m *testing.M) {
@ -427,6 +432,244 @@ func TestResolverIterator(t *testing.T) {
}
}
func TestResolverIterator_Perforce(t *testing.T) {
conf.Mock(&conf.Unified{
SiteConfiguration: schema.SiteConfiguration{
ExperimentalFeatures: &schema.ExperimentalFeatures{
PerforceChangelistMapping: "enabled",
},
},
})
t.Cleanup(func() {
conf.Mock(nil)
})
ctx := context.Background()
logger := logtest.Scoped(t)
db := database.NewDB(logger, dbtest.NewDB(t))
var revSpecs []RepoRevSpecs
for i := 1; i <= 5; i++ {
r := &types.Repo{
Name: api.RepoName(fmt.Sprintf("p4/foo/bar%d", i)),
Stars: i * 100,
ExternalRepo: api.ExternalRepoSpec{
ID: "my-external-id",
ServiceType: extsvc.TypePerforce,
},
}
if err := db.Repos().Create(ctx, r); err != nil {
t.Fatal(err)
}
revSpecs = append(revSpecs, RepoRevSpecs{Repo: types.MinimalRepo{
ID: r.ID,
Name: r.Name,
Stars: r.Stars,
ExternalRepo: api.ExternalRepoSpec{
ID: "my-external-id",
ServiceType: extsvc.TypePerforce,
},
}})
}
// Populate perforce changelist mapping table
s := db.RepoCommitsChangelists()
data := []types.PerforceChangelist{}
for i := range revSpecs {
repo := &revSpecs[i]
sha := gitSha(fmt.Sprintf("%d", repo.Repo.ID))
data = append(data, types.PerforceChangelist{
CommitSHA: api.CommitID(sha),
ChangelistID: int64(i),
})
repo.Revs = []query.RevisionSpecifier{
{
RevSpec: sha,
},
}
err := s.BatchInsertCommitSHAsWithPerforceChangelistID(ctx, api.RepoID(repo.Repo.ID), data)
if err != nil {
t.Fatal(err)
}
}
gsClient := gitserver.NewMockClient()
gsClient.ResolveRevisionFunc.SetDefaultHook(func(_ context.Context, name api.RepoName, spec string, _ gitserver.ResolveRevisionOptions) (api.CommitID, error) {
if spec == "bad_commit" || spec == "changelist/bad_commit" {
return "", &gitdomain.BadCommitError{}
}
// All repos have the revision except foo/bar5
if name == "p4/foo/bar5" {
return "", &gitdomain.RevisionNotFoundError{}
}
if len(spec) > 0 {
shortSpec := api.CommitID(spec).Short()
for _, revSpec := range revSpecs {
shortRevSpec := api.CommitID(revSpec.Revs[0].RevSpec).Short()
if name == revSpec.Repo.Name && shortSpec == shortRevSpec {
return api.CommitID(revSpec.Revs[0].RevSpec), nil
}
}
return "", &gitdomain.RevisionNotFoundError{}
}
return "", nil
})
resolver := NewResolver(logtest.Scoped(t), db, gsClient, nil, defaults.NewConnectionCache(logtest.Scoped(t)), nil)
all, _, err := resolver.resolve(ctx, search.RepoOptions{})
if err != nil {
t.Fatal(err)
}
// Assert that we get the cursor we expect
{
want := types.MultiCursor{
{Column: "stars", Direction: "prev", Value: fmt.Sprint(all.RepoRevs[3].Repo.Stars)},
{Column: "id", Direction: "prev", Value: fmt.Sprint(all.RepoRevs[3].Repo.ID)},
}
_, next, err := resolver.resolve(ctx, search.RepoOptions{
Limit: 3,
})
if err != nil {
t.Fatal(err)
}
if diff := cmp.Diff(next, want); diff != "" {
t.Errorf("unexpected cursor (-have, +want):\n%s", diff)
}
}
withRepoRevs := func(rrs []*search.RepositoryRevisions, revs ...string) []*search.RepositoryRevisions {
var with []*search.RepositoryRevisions
for _, r := range rrs {
with = append(with, &search.RepositoryRevisions{
Repo: r.Repo,
Revs: revs,
})
}
return with
}
for _, tc := range []struct {
name string
opts search.RepoOptions
pages []Resolved
err error
}{
{
name: "with sha rev",
opts: search.RepoOptions{
RepoFilters: toParsedRepoFilters(fmt.Sprintf("foo/bar[0-4]@%s", data[2].CommitSHA.Short())),
},
pages: []Resolved{
{
RepoRevs: withRepoRevs(all.RepoRevs[2:3], data[2].CommitSHA.Short()),
},
}, err: &MissingRepoRevsError{Missing: []RepoRevSpecs{
{
Repo: all.RepoRevs[0].Repo,
Revs: []query.RevisionSpecifier{
{
RevSpec: "rev",
},
},
},
{
Repo: all.RepoRevs[1].Repo,
Revs: []query.RevisionSpecifier{
{
RevSpec: "rev",
},
},
},
}},
},
{
name: "with perforce changelist id rev",
opts: search.RepoOptions{
RepoFilters: toParsedRepoFilters(fmt.Sprintf("foo/bar[0-4]@changelist/%d", data[2].ChangelistID)),
},
pages: []Resolved{
{
RepoRevs: withRepoRevs(all.RepoRevs[2:3], string(data[2].CommitSHA)),
},
}, err: &MissingRepoRevsError{Missing: []RepoRevSpecs{
{
Repo: all.RepoRevs[0].Repo,
Revs: []query.RevisionSpecifier{
{
RevSpec: "rev",
},
},
},
{
Repo: all.RepoRevs[1].Repo,
Revs: []query.RevisionSpecifier{
{
RevSpec: "rev",
},
},
},
}},
},
{
name: "single perforce changelist id rev",
opts: search.RepoOptions{
RepoFilters: toParsedRepoFilters(fmt.Sprintf("foo/bar3@changelist/%d", data[2].ChangelistID)),
},
pages: []Resolved{
{
RepoRevs: withRepoRevs(all.RepoRevs[2:3], string(data[2].CommitSHA)),
},
}, err: nil,
},
{
name: "with limit 3 and fatal error",
opts: search.RepoOptions{
Limit: 3,
RepoFilters: toParsedRepoFilters("foo/bar[0-5]@bad_commit"),
},
err: &gitdomain.BadCommitError{},
pages: nil,
},
{
name: "with bad changelist id",
opts: search.RepoOptions{
Limit: 3,
RepoFilters: toParsedRepoFilters("foo/bar[0-5]@changelist/bad_commit"),
},
err: &gitdomain.BadCommitError{},
pages: nil,
},
} {
t.Run(tc.name, func(t *testing.T) {
r := NewResolver(logtest.Scoped(t), db, gsClient, nil, defaults.NewConnectionCache(logtest.Scoped(t)), nil)
it := r.Iterator(ctx, tc.opts)
var pages []Resolved
for it.Next() {
page := it.Current()
pages = append(pages, page)
}
err = it.Err()
if !errors.Is(err, tc.err) {
diff := cmp.Diff(errors.UnwrapAll(err), tc.err)
t.Errorf("%s unexpected error (-have, +want):\n%s", tc.name, diff)
}
if diff := cmp.Diff(pages, tc.pages); diff != "" {
t.Errorf("%s unexpected pages (-have, +want):\n%s", tc.name, diff)
}
})
}
}
func TestResolverIterateRepoRevs(t *testing.T) {
ctx := context.Background()
logger := logtest.Scoped(t)
@ -887,3 +1130,9 @@ func TestRepoHasCommitAfter(t *testing.T) {
})
}
}
func gitSha(val string) string {
writer := sha1.New()
writer.Write([]byte(val))
return hex.EncodeToString(writer.Sum(nil))
}

View File

@ -521,11 +521,14 @@ type RepoIDName struct {
Name api.RepoName
}
// MinimalRepo represents a source code repository name, its ID and number of stars.
// MinimalRepo represents a source code repository name, its ID, number of stars and service type.
type MinimalRepo struct {
ID api.RepoID
Name api.RepoName
Stars int
// ExternalRepo identifies this repository by its ID on the external service where it resides (and the external
// service itself).
ExternalRepo api.ExternalRepoSpec
}
// MinimalRepos is an utility type with convenience methods for operating on lists of repo names