diff --git a/cmd/gitserver/internal/integration_tests/BUILD.bazel b/cmd/gitserver/internal/integration_tests/BUILD.bazel index d601c971fc5..0bf59d1301c 100644 --- a/cmd/gitserver/internal/integration_tests/BUILD.bazel +++ b/cmd/gitserver/internal/integration_tests/BUILD.bazel @@ -62,7 +62,6 @@ go_test( "//internal/extsvc", "//internal/gitserver", "//internal/gitserver/gitdomain", - "//internal/gitserver/protocol", "//internal/gitserver/v1:gitserver", "//internal/grpc", "//internal/grpc/defaults", diff --git a/cmd/gitserver/internal/integration_tests/resolverevisions_test.go b/cmd/gitserver/internal/integration_tests/resolverevisions_test.go index 6e2b8775bb8..f07eebd23c4 100644 --- a/cmd/gitserver/internal/integration_tests/resolverevisions_test.go +++ b/cmd/gitserver/internal/integration_tests/resolverevisions_test.go @@ -23,7 +23,6 @@ import ( "github.com/sourcegraph/sourcegraph/internal/extsvc" "github.com/sourcegraph/sourcegraph/internal/gitserver" "github.com/sourcegraph/sourcegraph/internal/gitserver/gitdomain" - "github.com/sourcegraph/sourcegraph/internal/gitserver/protocol" proto "github.com/sourcegraph/sourcegraph/internal/gitserver/v1" internalgrpc "github.com/sourcegraph/sourcegraph/internal/grpc" "github.com/sourcegraph/sourcegraph/internal/grpc/defaults" @@ -33,7 +32,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/wrexec" ) -func TestClient_ResolveRevisions(t *testing.T) { +func TestClient_ResolveRevision(t *testing.T) { root := t.TempDir() remote := createSimpleGitRepo(t, root) // These hashes should be stable since we set the timestamps @@ -42,27 +41,27 @@ func TestClient_ResolveRevisions(t *testing.T) { hash2 := "c5151eceb40d5e625716589b745248e1a6c6228d" tests := []struct { - input []protocol.RevisionSpecifier - want []string + input string + want api.CommitID err error }{{ - input: []protocol.RevisionSpecifier{{}}, - want: []string{hash2}, + input: "", + want: api.CommitID(hash2), }, { - input: []protocol.RevisionSpecifier{{RevSpec: "HEAD"}}, - want: []string{hash2}, + input: "HEAD", + want: api.CommitID(hash2), }, { - input: []protocol.RevisionSpecifier{{RevSpec: "HEAD~1"}}, - want: []string{hash1}, + input: "HEAD~1", + want: api.CommitID(hash1), }, { - input: []protocol.RevisionSpecifier{{RevSpec: "test-ref"}}, - want: []string{hash1}, + input: "test-ref", + want: api.CommitID(hash1), }, { - input: []protocol.RevisionSpecifier{{RevSpec: "test-nested-ref"}}, - want: []string{hash1}, + input: "test-nested-ref", + want: api.CommitID(hash1), }, { - input: []protocol.RevisionSpecifier{{RevSpec: "test-fake-ref"}}, - err: &gitdomain.RevisionNotFoundError{Repo: api.RepoName(remote), Spec: "test-fake-ref"}, + input: "test-fake-ref", + err: &gitdomain.RevisionNotFoundError{Repo: api.RepoName(remote), Spec: "test-fake-ref^0"}, }} logger := logtest.Scoped(t) @@ -107,7 +106,7 @@ func TestClient_ResolveRevisions(t *testing.T) { _, err := cli.RequestRepoUpdate(ctx, api.RepoName(remote), 0) require.NoError(t, err) - got, err := cli.ResolveRevisions(ctx, api.RepoName(remote), test.input) + got, err := cli.ResolveRevision(ctx, api.RepoName(remote), test.input, gitserver.ResolveRevisionOptions{NoEnsureRevision: true}) if test.err != nil { require.Equal(t, test.err, err) return diff --git a/internal/batches/sources/mocks_test.go b/internal/batches/sources/mocks_test.go index 5e49df77d22..7e7eed741fc 100644 --- a/internal/batches/sources/mocks_test.go +++ b/internal/batches/sources/mocks_test.go @@ -10986,9 +10986,6 @@ type MockGitserverClient struct { // ResolveRevisionFunc is an instance of a mock function object // controlling the behavior of the method ResolveRevision. ResolveRevisionFunc *GitserverClientResolveRevisionFunc - // ResolveRevisionsFunc is an instance of a mock function object - // controlling the behavior of the method ResolveRevisions. - ResolveRevisionsFunc *GitserverClientResolveRevisionsFunc // RevListFunc is an instance of a mock function object controlling the // behavior of the method RevList. RevListFunc *GitserverClientRevListFunc @@ -11231,11 +11228,6 @@ func NewMockGitserverClient() *MockGitserverClient { return }, }, - ResolveRevisionsFunc: &GitserverClientResolveRevisionsFunc{ - defaultHook: func(context.Context, api.RepoName, []protocol.RevisionSpecifier) (r0 []string, r1 error) { - return - }, - }, RevListFunc: &GitserverClientRevListFunc{ defaultHook: func(context.Context, string, string, func(commit string) (bool, error)) (r0 error) { return @@ -11493,11 +11485,6 @@ func NewStrictMockGitserverClient() *MockGitserverClient { panic("unexpected invocation of MockGitserverClient.ResolveRevision") }, }, - ResolveRevisionsFunc: &GitserverClientResolveRevisionsFunc{ - defaultHook: func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, error) { - panic("unexpected invocation of MockGitserverClient.ResolveRevisions") - }, - }, RevListFunc: &GitserverClientRevListFunc{ defaultHook: func(context.Context, string, string, func(commit string) (bool, error)) error { panic("unexpected invocation of MockGitserverClient.RevList") @@ -11670,9 +11657,6 @@ func NewMockGitserverClientFrom(i gitserver.Client) *MockGitserverClient { ResolveRevisionFunc: &GitserverClientResolveRevisionFunc{ defaultHook: i.ResolveRevision, }, - ResolveRevisionsFunc: &GitserverClientResolveRevisionsFunc{ - defaultHook: i.ResolveRevisions, - }, RevListFunc: &GitserverClientRevListFunc{ defaultHook: i.RevList, }, @@ -16563,120 +16547,6 @@ func (c GitserverClientResolveRevisionFuncCall) Results() []interface{} { return []interface{}{c.Result0, c.Result1} } -// GitserverClientResolveRevisionsFunc describes the behavior when the -// ResolveRevisions method of the parent MockGitserverClient instance is -// invoked. -type GitserverClientResolveRevisionsFunc struct { - defaultHook func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, error) - hooks []func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, error) - history []GitserverClientResolveRevisionsFuncCall - mutex sync.Mutex -} - -// ResolveRevisions delegates to the next hook function in the queue and -// stores the parameter and result values of this invocation. -func (m *MockGitserverClient) ResolveRevisions(v0 context.Context, v1 api.RepoName, v2 []protocol.RevisionSpecifier) ([]string, error) { - r0, r1 := m.ResolveRevisionsFunc.nextHook()(v0, v1, v2) - m.ResolveRevisionsFunc.appendCall(GitserverClientResolveRevisionsFuncCall{v0, v1, v2, r0, r1}) - return r0, r1 -} - -// SetDefaultHook sets function that is called when the ResolveRevisions -// method of the parent MockGitserverClient instance is invoked and the hook -// queue is empty. -func (f *GitserverClientResolveRevisionsFunc) SetDefaultHook(hook func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, error)) { - f.defaultHook = hook -} - -// PushHook adds a function to the end of hook queue. Each invocation of the -// ResolveRevisions method of the parent MockGitserverClient 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 *GitserverClientResolveRevisionsFunc) PushHook(hook func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, 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 *GitserverClientResolveRevisionsFunc) SetDefaultReturn(r0 []string, r1 error) { - f.SetDefaultHook(func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, error) { - return r0, r1 - }) -} - -// PushReturn calls PushHook with a function that returns the given values. -func (f *GitserverClientResolveRevisionsFunc) PushReturn(r0 []string, r1 error) { - f.PushHook(func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, error) { - return r0, r1 - }) -} - -func (f *GitserverClientResolveRevisionsFunc) nextHook() func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, 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 *GitserverClientResolveRevisionsFunc) appendCall(r0 GitserverClientResolveRevisionsFuncCall) { - f.mutex.Lock() - f.history = append(f.history, r0) - f.mutex.Unlock() -} - -// History returns a sequence of GitserverClientResolveRevisionsFuncCall -// objects describing the invocations of this function. -func (f *GitserverClientResolveRevisionsFunc) History() []GitserverClientResolveRevisionsFuncCall { - f.mutex.Lock() - history := make([]GitserverClientResolveRevisionsFuncCall, len(f.history)) - copy(history, f.history) - f.mutex.Unlock() - - return history -} - -// GitserverClientResolveRevisionsFuncCall is an object that describes an -// invocation of method ResolveRevisions on an instance of -// MockGitserverClient. -type GitserverClientResolveRevisionsFuncCall struct { - // Arg0 is the value of the 1st argument passed to this method - // invocation. - Arg0 context.Context - // Arg1 is the value of the 2nd argument passed to this method - // invocation. - Arg1 api.RepoName - // Arg2 is the value of the 3rd argument passed to this method - // invocation. - Arg2 []protocol.RevisionSpecifier - // Result0 is the value of the 1st result returned from this method - // invocation. - Result0 []string - // 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. -func (c GitserverClientResolveRevisionsFuncCall) Args() []interface{} { - return []interface{}{c.Arg0, c.Arg1, c.Arg2} -} - -// Results returns an interface slice containing the results of this -// invocation. -func (c GitserverClientResolveRevisionsFuncCall) Results() []interface{} { - return []interface{}{c.Result0, c.Result1} -} - // GitserverClientRevListFunc describes the behavior when the RevList method // of the parent MockGitserverClient instance is invoked. type GitserverClientRevListFunc struct { diff --git a/internal/codemonitors/search.go b/internal/codemonitors/search.go index 4a4f850d12b..959baf0acd4 100644 --- a/internal/codemonitors/search.go +++ b/internal/codemonitors/search.go @@ -107,15 +107,20 @@ func Snapshot(ctx context.Context, logger log.Logger, db database.DB, query stri ) hook := func(ctx context.Context, db database.DB, gs commit.GitserverClient, args *gitprotocol.SearchRequest, repoID api.RepoID, _ commit.DoSearchFunc) error { - // Resolve the requested revisions into a static set of commit hashes - commitHashes, err := gs.ResolveRevisions(ctx, args.Repo, args.Revisions) - if err != nil { - return err - } + for _, rev := range args.Revisions { + // Fail early for context cancelation. + if err := ctx.Err(); err != nil { + return ctx.Err() + } - mu.Lock() - resolvedRevisions[repoID] = commitHashes - mu.Unlock() + res, err := gs.ResolveRevision(ctx, args.Repo, rev, gitserver.ResolveRevisionOptions{NoEnsureRevision: true}) + if err != nil { + return err + } + mu.Lock() + resolvedRevisions[repoID] = append(resolvedRevisions[repoID], string(res)) + mu.Unlock() + } return nil } @@ -174,9 +179,18 @@ func hookWithID( cm := db.CodeMonitors() // Resolve the requested revisions into a static set of commit hashes - commitHashes, err := gs.ResolveRevisions(ctx, args.Repo, args.Revisions) - if err != nil { - return err + commitHashes := make([]string, 0, len(args.Revisions)) + for _, rev := range args.Revisions { + // Fail early for context cancelation. + if err := ctx.Err(); err != nil { + return ctx.Err() + } + + res, err := gs.ResolveRevision(ctx, args.Repo, rev, gitserver.ResolveRevisionOptions{NoEnsureRevision: true}) + if err != nil { + return err + } + commitHashes = append(commitHashes, string(res)) } // Look up the previously searched set of commit hashes @@ -190,12 +204,10 @@ func hookWithID( } // Merge requested hashes and excluded hashes - newRevs := make([]gitprotocol.RevisionSpecifier, 0, len(commitHashes)+len(lastSearched)) - for _, hash := range commitHashes { - newRevs = append(newRevs, gitprotocol.RevisionSpecifier{RevSpec: hash}) - } + newRevs := make([]string, 0, len(commitHashes)+len(lastSearched)) + newRevs = append(newRevs, commitHashes...) for _, exclude := range lastSearched { - newRevs = append(newRevs, gitprotocol.RevisionSpecifier{RevSpec: "^" + exclude}) + newRevs = append(newRevs, "^"+exclude) } // Update args with the new set of revisions diff --git a/internal/codemonitors/search_test.go b/internal/codemonitors/search_test.go index 16778bd9b50..f84ac2f8de0 100644 --- a/internal/codemonitors/search_test.go +++ b/internal/codemonitors/search_test.go @@ -138,44 +138,35 @@ func TestCodeMonitorHook(t *testing.T) { ctx := context.Background() gs := gitserver.NewMockClient() - gs.ResolveRevisionsFunc.PushReturn([]string{"hash1", "hash2"}, nil) - gs.ResolveRevisionsFunc.PushReturn([]string{"hash3", "hash4"}, nil) - gs.ResolveRevisionsFunc.PushReturn([]string{"hash5", "hash6"}, nil) + gs.ResolveRevisionFunc.PushReturn("hash1", nil) + gs.ResolveRevisionFunc.PushReturn("hash2", nil) + gs.ResolveRevisionFunc.PushReturn("hash3", nil) + gs.ResolveRevisionFunc.PushReturn("hash4", nil) + gs.ResolveRevisionFunc.PushReturn("hash5", nil) + gs.ResolveRevisionFunc.PushReturn("hash6", nil) // The first time, doSearch should receive only the resolved hashes doSearch := func(args *gitprotocol.SearchRequest) error { - require.Equal(t, args.Revisions, []gitprotocol.RevisionSpecifier{{ - RevSpec: "hash1", - }, { - RevSpec: "hash2", - }}) + require.Equal(t, args.Revisions, []string{"hash1", "hash2"}) return nil } - err := hookWithID(ctx, db, gs, fixtures.Monitor.ID, fixtures.Repo.ID, &gitprotocol.SearchRequest{}, doSearch) + err := hookWithID(ctx, db, gs, fixtures.Monitor.ID, fixtures.Repo.ID, &gitprotocol.SearchRequest{Revisions: []string{"rev1", "rev2"}}, doSearch) require.NoError(t, err) // The next time, doSearch should receive the new resolved hashes plus the // hashes from last time, but excluded doSearch = func(args *gitprotocol.SearchRequest) error { - require.Equal(t, args.Revisions, []gitprotocol.RevisionSpecifier{{ - RevSpec: "hash3", - }, { - RevSpec: "hash4", - }, { - RevSpec: "^hash1", - }, { - RevSpec: "^hash2", - }}) + require.Equal(t, args.Revisions, []string{"hash3", "hash4", "^hash1", "^hash2"}) return nil } - err = hookWithID(ctx, db, gs, fixtures.Monitor.ID, fixtures.Repo.ID, &gitprotocol.SearchRequest{}, doSearch) + err = hookWithID(ctx, db, gs, fixtures.Monitor.ID, fixtures.Repo.ID, &gitprotocol.SearchRequest{Revisions: []string{"rev1", "rev2"}}, doSearch) require.NoError(t, err) t.Run("deadline exceeded is propagated", func(t *testing.T) { doSearch = func(args *gitprotocol.SearchRequest) error { return context.DeadlineExceeded } - err := hookWithID(ctx, db, gs, fixtures.Monitor.ID, fixtures.Repo.ID, &gitprotocol.SearchRequest{}, doSearch) + err := hookWithID(ctx, db, gs, fixtures.Monitor.ID, fixtures.Repo.ID, &gitprotocol.SearchRequest{Revisions: []string{"rev1", "rev2"}}, doSearch) require.ErrorContains(t, err, "some commits may be skipped") }) } diff --git a/internal/gitserver/client.go b/internal/gitserver/client.go index 3327e9a8a37..adeab02d46b 100644 --- a/internal/gitserver/client.go +++ b/internal/gitserver/client.go @@ -27,7 +27,6 @@ import ( "github.com/sourcegraph/sourcegraph/internal/gitserver/protocol" proto "github.com/sourcegraph/sourcegraph/internal/gitserver/v1" "github.com/sourcegraph/sourcegraph/internal/grpc/streamio" - "github.com/sourcegraph/sourcegraph/internal/lazyregexp" "github.com/sourcegraph/sourcegraph/internal/observation" "github.com/sourcegraph/sourcegraph/internal/perforce" "github.com/sourcegraph/sourcegraph/lib/errors" @@ -361,9 +360,6 @@ type Client interface { // * Other unexpected errors. ResolveRevision(ctx context.Context, repo api.RepoName, spec string, opt ResolveRevisionOptions) (api.CommitID, error) - // ResolveRevisions expands a set of RevisionSpecifiers into an equivalent set of commit hashes. - ResolveRevisions(_ context.Context, repo api.RepoName, _ []protocol.RevisionSpecifier) ([]string, error) - // RequestRepoUpdate is the new protocol endpoint for synchronous requests // with more detailed responses. Do not use this if you are not repo-updater. // @@ -1263,78 +1259,6 @@ func (c *clientImplementor) GetObject(ctx context.Context, repo api.RepoName, ob return &res.Object, nil } -var ambiguousArgPattern = lazyregexp.New(`ambiguous argument '([^']+)'`) - -func (c *clientImplementor) ResolveRevisions(ctx context.Context, repo api.RepoName, revs []protocol.RevisionSpecifier) (_ []string, err error) { - ctx, _, endObservation := c.operations.resolveRevisions.With(ctx, &err, observation.Args{ - MetricLabelValues: []string{c.scope}, - Attrs: []attribute.KeyValue{ - repo.Attr(), - attribute.Int("revs", len(revs)), - }, - }) - defer endObservation(1, observation.Args{}) - - args := append([]string{"rev-parse"}, revsToGitArgs(revs)...) - - cmd := c.gitCommand(repo, args...) - stdout, stderr, err := cmd.DividedOutput(ctx) - if err != nil { - if gitdomain.IsRepoNotExist(err) { - return nil, err - } - if match := ambiguousArgPattern.FindSubmatch(stderr); match != nil { - return nil, &gitdomain.RevisionNotFoundError{Repo: repo, Spec: string(match[1])} - } - return nil, errors.WithMessage(err, fmt.Sprintf("git command %v failed (stderr: %q)", cmd.Args(), stderr)) - } - - return strings.Fields(string(stdout)), nil -} - -func revsToGitArgs(revSpecs []protocol.RevisionSpecifier) []string { - args := make([]string, 0, len(revSpecs)) - for _, r := range revSpecs { - if r.RevSpec != "" { - args = append(args, r.RevSpec) - } else { - args = append(args, "HEAD") - } - } - - // If revSpecs is empty, git treats it as equivalent to HEAD - if len(revSpecs) == 0 { - args = append(args, "HEAD") - } - return args -} - -// readResponseBody will attempt to read the body of the HTTP response and return it as a -// string. However, in the unlikely scenario that it fails to read the body, it will encode and -// return the error message as a string. -// -// This allows us to use this function directly without yet another if err != nil check. As a -// result, this function should **only** be used when we're attempting to return the body's content -// as part of an error. In such scenarios we don't need to return the potential error from reading -// the body, but can get away with returning that error as a string itself. -// -// This is an unusual pattern of not returning an error. Be careful of replicating this in other -// parts of the code. -func readResponseBody(body io.Reader) string { - content, err := io.ReadAll(body) - if err != nil { - return fmt.Sprintf("failed to read response body, error: %v", err) - } - - // strings.TrimSpace is needed to remove trailing \n characters that is added by the - // server. We use http.Error in the server which in turn uses fmt.Fprintln to format - // the error message. And in translation that newline gets escaped into a \n - // character. For what the error message would look in the UI without - // strings.TrimSpace, see attached screenshots in this pull request: - // https://github.com/sourcegraph/sourcegraph/pull/39358. - return strings.TrimSpace(string(content)) -} - func stringsToByteSlices(in []string) [][]byte { res := make([][]byte, len(in)) for i, s := range in { diff --git a/internal/gitserver/commands.go b/internal/gitserver/commands.go index be9e4832856..c4104b733f0 100644 --- a/internal/gitserver/commands.go +++ b/internal/gitserver/commands.go @@ -34,7 +34,6 @@ import ( "github.com/sourcegraph/sourcegraph/internal/byteutils" "github.com/sourcegraph/sourcegraph/internal/fileutil" "github.com/sourcegraph/sourcegraph/internal/gitserver/gitdomain" - "github.com/sourcegraph/sourcegraph/internal/gitserver/protocol" proto "github.com/sourcegraph/sourcegraph/internal/gitserver/v1" "github.com/sourcegraph/sourcegraph/internal/grpc/streamio" "github.com/sourcegraph/sourcegraph/internal/lazyregexp" @@ -436,12 +435,6 @@ func (c *clientImplementor) DiffSymbols(ctx context.Context, repo api.RepoName, }) defer endObservation(1, observation.Args{}) - // Ensure commits exist to provide a better error message - _, err = c.ResolveRevisions(ctx, repo, []protocol.RevisionSpecifier{{RevSpec: string(commitA)}, {RevSpec: string(commitB)}}) - if err != nil { - return nil, errors.Wrapf(err, "failed to lookup revisions for git diff on %s between %s and %s", repo, commitA, commitB) - } - command := c.gitCommand(repo, "diff", "-z", "--name-status", "--no-renames", string(commitA), string(commitB)) out, err := command.Output(ctx) if err != nil { diff --git a/internal/gitserver/internal_test.go b/internal/gitserver/internal_test.go index 86f56a75e7d..e560d1271fd 100644 --- a/internal/gitserver/internal_test.go +++ b/internal/gitserver/internal_test.go @@ -1,11 +1,8 @@ package gitserver import ( - "bytes" "fmt" "testing" - - "github.com/google/go-cmp/cmp" ) func BenchmarkAddrForKey(b *testing.B) { @@ -22,14 +19,3 @@ func BenchmarkAddrForKey(b *testing.B) { }) } } - -func Test_readResponseBody(t *testing.T) { - // The \n in the end is important to test that readResponseBody correctly removes it from the returned string. - reader := bytes.NewReader([]byte("A test string that is more than 40 bytes long. Lorem ipsum whatever whatever\n")) - - expected := "A test string that is more than 40 bytes long. Lorem ipsum whatever whatever" - got := readResponseBody(reader) - if diff := cmp.Diff([]byte(expected), []byte(got)); diff != "" { - t.Fatalf("Mismatch (-want +got):\n%s", diff) - } -} diff --git a/internal/gitserver/mocks_temp.go b/internal/gitserver/mocks_temp.go index 03411fb621a..5f772fcb364 100644 --- a/internal/gitserver/mocks_temp.go +++ b/internal/gitserver/mocks_temp.go @@ -154,9 +154,6 @@ type MockClient struct { // ResolveRevisionFunc is an instance of a mock function object // controlling the behavior of the method ResolveRevision. ResolveRevisionFunc *ClientResolveRevisionFunc - // ResolveRevisionsFunc is an instance of a mock function object - // controlling the behavior of the method ResolveRevisions. - ResolveRevisionsFunc *ClientResolveRevisionsFunc // RevListFunc is an instance of a mock function object controlling the // behavior of the method RevList. RevListFunc *ClientRevListFunc @@ -399,11 +396,6 @@ func NewMockClient() *MockClient { return }, }, - ResolveRevisionsFunc: &ClientResolveRevisionsFunc{ - defaultHook: func(context.Context, api.RepoName, []protocol.RevisionSpecifier) (r0 []string, r1 error) { - return - }, - }, RevListFunc: &ClientRevListFunc{ defaultHook: func(context.Context, string, string, func(commit string) (bool, error)) (r0 error) { return @@ -661,11 +653,6 @@ func NewStrictMockClient() *MockClient { panic("unexpected invocation of MockClient.ResolveRevision") }, }, - ResolveRevisionsFunc: &ClientResolveRevisionsFunc{ - defaultHook: func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, error) { - panic("unexpected invocation of MockClient.ResolveRevisions") - }, - }, RevListFunc: &ClientRevListFunc{ defaultHook: func(context.Context, string, string, func(commit string) (bool, error)) error { panic("unexpected invocation of MockClient.RevList") @@ -837,9 +824,6 @@ func NewMockClientFrom(i Client) *MockClient { ResolveRevisionFunc: &ClientResolveRevisionFunc{ defaultHook: i.ResolveRevision, }, - ResolveRevisionsFunc: &ClientResolveRevisionsFunc{ - defaultHook: i.ResolveRevisions, - }, RevListFunc: &ClientRevListFunc{ defaultHook: i.RevList, }, @@ -5646,117 +5630,6 @@ func (c ClientResolveRevisionFuncCall) Results() []interface{} { return []interface{}{c.Result0, c.Result1} } -// ClientResolveRevisionsFunc describes the behavior when the -// ResolveRevisions method of the parent MockClient instance is invoked. -type ClientResolveRevisionsFunc struct { - defaultHook func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, error) - hooks []func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, error) - history []ClientResolveRevisionsFuncCall - mutex sync.Mutex -} - -// ResolveRevisions delegates to the next hook function in the queue and -// stores the parameter and result values of this invocation. -func (m *MockClient) ResolveRevisions(v0 context.Context, v1 api.RepoName, v2 []protocol.RevisionSpecifier) ([]string, error) { - r0, r1 := m.ResolveRevisionsFunc.nextHook()(v0, v1, v2) - m.ResolveRevisionsFunc.appendCall(ClientResolveRevisionsFuncCall{v0, v1, v2, r0, r1}) - return r0, r1 -} - -// SetDefaultHook sets function that is called when the ResolveRevisions -// method of the parent MockClient instance is invoked and the hook queue is -// empty. -func (f *ClientResolveRevisionsFunc) SetDefaultHook(hook func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, error)) { - f.defaultHook = hook -} - -// PushHook adds a function to the end of hook queue. Each invocation of the -// ResolveRevisions method of the parent MockClient 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 *ClientResolveRevisionsFunc) PushHook(hook func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, 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 *ClientResolveRevisionsFunc) SetDefaultReturn(r0 []string, r1 error) { - f.SetDefaultHook(func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, error) { - return r0, r1 - }) -} - -// PushReturn calls PushHook with a function that returns the given values. -func (f *ClientResolveRevisionsFunc) PushReturn(r0 []string, r1 error) { - f.PushHook(func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, error) { - return r0, r1 - }) -} - -func (f *ClientResolveRevisionsFunc) nextHook() func(context.Context, api.RepoName, []protocol.RevisionSpecifier) ([]string, 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 *ClientResolveRevisionsFunc) appendCall(r0 ClientResolveRevisionsFuncCall) { - f.mutex.Lock() - f.history = append(f.history, r0) - f.mutex.Unlock() -} - -// History returns a sequence of ClientResolveRevisionsFuncCall objects -// describing the invocations of this function. -func (f *ClientResolveRevisionsFunc) History() []ClientResolveRevisionsFuncCall { - f.mutex.Lock() - history := make([]ClientResolveRevisionsFuncCall, len(f.history)) - copy(history, f.history) - f.mutex.Unlock() - - return history -} - -// ClientResolveRevisionsFuncCall is an object that describes an invocation -// of method ResolveRevisions on an instance of MockClient. -type ClientResolveRevisionsFuncCall struct { - // Arg0 is the value of the 1st argument passed to this method - // invocation. - Arg0 context.Context - // Arg1 is the value of the 2nd argument passed to this method - // invocation. - Arg1 api.RepoName - // Arg2 is the value of the 3rd argument passed to this method - // invocation. - Arg2 []protocol.RevisionSpecifier - // Result0 is the value of the 1st result returned from this method - // invocation. - Result0 []string - // 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. -func (c ClientResolveRevisionsFuncCall) Args() []interface{} { - return []interface{}{c.Arg0, c.Arg1, c.Arg2} -} - -// Results returns an interface slice containing the results of this -// invocation. -func (c ClientResolveRevisionsFuncCall) Results() []interface{} { - return []interface{}{c.Result0, c.Result1} -} - // ClientRevListFunc describes the behavior when the RevList method of the // parent MockClient instance is invoked. type ClientRevListFunc struct { diff --git a/internal/gitserver/observability.go b/internal/gitserver/observability.go index 7c606810bb2..8196c30d774 100644 --- a/internal/gitserver/observability.go +++ b/internal/gitserver/observability.go @@ -50,7 +50,6 @@ type operations struct { perforceGetChangelist *observation.Operation createCommitFromPatch *observation.Operation getObject *observation.Operation - resolveRevisions *observation.Operation commitGraph *observation.Operation refDescriptions *observation.Operation branchesContaining *observation.Operation @@ -148,7 +147,6 @@ func newOperations(observationCtx *observation.Context) *operations { perforceGetChangelist: op("PerforceGetChangelist"), createCommitFromPatch: op("CreateCommitFromPatch"), getObject: op("GetObject"), - resolveRevisions: op("ResolveRevisions"), commitGraph: op("CommitGraph"), refDescriptions: op("RefDescriptions"), branchesContaining: op("BranchesContaining"), diff --git a/internal/gitserver/protocol/gitserver.go b/internal/gitserver/protocol/gitserver.go index a746a332073..ab5fe315e5f 100644 --- a/internal/gitserver/protocol/gitserver.go +++ b/internal/gitserver/protocol/gitserver.go @@ -16,7 +16,7 @@ import ( type SearchRequest struct { Repo api.RepoName - Revisions []RevisionSpecifier + Revisions []string Query Node IncludeDiff bool Limit int @@ -26,7 +26,7 @@ type SearchRequest struct { func (r *SearchRequest) ToProto() *proto.SearchRequest { revs := make([]*proto.RevisionSpecifier, 0, len(r.Revisions)) for _, rev := range r.Revisions { - revs = append(revs, rev.ToProto()) + revs = append(revs, &proto.RevisionSpecifier{RevSpec: rev}) } return &proto.SearchRequest{ Repo: string(r.Repo), @@ -44,9 +44,9 @@ func SearchRequestFromProto(p *proto.SearchRequest) (*SearchRequest, error) { return nil, err } - revisions := make([]RevisionSpecifier, 0, len(p.GetRevisions())) + revisions := make([]string, 0, len(p.GetRevisions())) for _, rev := range p.GetRevisions() { - revisions = append(revisions, RevisionSpecifierFromProto(rev)) + revisions = append(revisions, rev.GetRevSpec()) } return &SearchRequest{ @@ -59,24 +59,6 @@ func SearchRequestFromProto(p *proto.SearchRequest) (*SearchRequest, error) { }, nil } -type RevisionSpecifier struct { - // RevSpec is a revision range specifier suitable for passing to git. See - // the manpage gitrevisions(7). - RevSpec string -} - -func (r *RevisionSpecifier) ToProto() *proto.RevisionSpecifier { - return &proto.RevisionSpecifier{ - RevSpec: r.RevSpec, - } -} - -func RevisionSpecifierFromProto(p *proto.RevisionSpecifier) RevisionSpecifier { - return RevisionSpecifier{ - RevSpec: p.GetRevSpec(), - } -} - type SearchEventMatches []CommitMatch type SearchEventDone struct { diff --git a/internal/gitserver/protocol/gitserver_test.go b/internal/gitserver/protocol/gitserver_test.go index b00a9083913..2e4556ae302 100644 --- a/internal/gitserver/protocol/gitserver_test.go +++ b/internal/gitserver/protocol/gitserver_test.go @@ -13,7 +13,7 @@ import ( func TestSearchRequestProtoRoundtrip(t *testing.T) { req := &SearchRequest{ Repo: "test1", - Revisions: []RevisionSpecifier{{RevSpec: "ABC"}}, + Revisions: []string{"ABC"}, Query: &Operator{ Kind: And, Operands: []Node{ diff --git a/internal/gitserver/search/search.go b/internal/gitserver/search/search.go index f4c1b50a0b7..aa09290b5fe 100644 --- a/internal/gitserver/search/search.go +++ b/internal/gitserver/search/search.go @@ -85,7 +85,7 @@ type CommitSearcher struct { Logger log.Logger RepoDir string Query MatchTree - Revisions []protocol.RevisionSpecifier + Revisions []string IncludeDiff bool IncludeModifiedFiles bool RepoName api.RepoName @@ -143,6 +143,18 @@ func (cs *CommitSearcher) gitArgs() []string { return args } +func revsToGitArgs(revs []string) []string { + revArgs := make([]string, 0, len(revs)) + for _, rev := range revs { + if rev != "" { + revArgs = append(revArgs, rev) + } else { + revArgs = append(revArgs, "HEAD") + } + } + return revArgs +} + func (cs *CommitSearcher) feedBatches(ctx context.Context, jobs chan job, resultChans chan chan *protocol.CommitMatch) (err error) { cmd := exec.CommandContext(ctx, "git", cs.gitArgs()...) cmd.Dir = cs.RepoDir @@ -263,18 +275,6 @@ func (cs *CommitSearcher) runJobs(ctx context.Context, jobs chan job) error { return errs } -func revsToGitArgs(revs []protocol.RevisionSpecifier) []string { - revArgs := make([]string, 0, len(revs)) - for _, rev := range revs { - if rev.RevSpec != "" { - revArgs = append(revArgs, rev.RevSpec) - } else { - revArgs = append(revArgs, "HEAD") - } - } - return revArgs -} - // RawCommit is a shallow parse of the output of git log type RawCommit struct { Hash []byte diff --git a/internal/gitserver/search/search_test.go b/internal/gitserver/search/search_test.go index 1e09a27252e..9d8823f8a9f 100644 --- a/internal/gitserver/search/search_test.go +++ b/internal/gitserver/search/search_test.go @@ -756,19 +756,17 @@ func (authorNameGenerator) Generate(rand *rand.Rand, size int) reflect.Value { func Test_revsToGitArgs(t *testing.T) { cases := []struct { name string - revSpecs []protocol.RevisionSpecifier + revSpecs []string expected []string }{ { - name: "explicit HEAD", - revSpecs: []protocol.RevisionSpecifier{{ - RevSpec: "HEAD", - }}, + name: "explicit HEAD", + revSpecs: []string{"HEAD"}, expected: []string{"HEAD"}, }, { name: "implicit HEAD", - revSpecs: []protocol.RevisionSpecifier{{}}, + revSpecs: []string{""}, expected: []string{"HEAD"}, }, } diff --git a/internal/search/commit/BUILD.bazel b/internal/search/commit/BUILD.bazel index 696ce719145..b5a626910fd 100644 --- a/internal/search/commit/BUILD.bazel +++ b/internal/search/commit/BUILD.bazel @@ -10,6 +10,7 @@ go_library( "//internal/api", "//internal/database", "//internal/errcode", + "//internal/gitserver", "//internal/gitserver/gitdomain", "//internal/gitserver/protocol", "//internal/search", diff --git a/internal/search/commit/commit.go b/internal/search/commit/commit.go index bea2f3c04e2..f0ebf68c230 100644 --- a/internal/search/commit/commit.go +++ b/internal/search/commit/commit.go @@ -12,6 +12,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/errcode" + "github.com/sourcegraph/sourcegraph/internal/gitserver" "github.com/sourcegraph/sourcegraph/internal/gitserver/gitdomain" "github.com/sourcegraph/sourcegraph/internal/gitserver/protocol" gitprotocol "github.com/sourcegraph/sourcegraph/internal/gitserver/protocol" @@ -42,7 +43,7 @@ type CodeMonitorHook func(context.Context, database.DB, GitserverClient, *gitpro type GitserverClient interface { Search(_ context.Context, _ *protocol.SearchRequest, onMatches func([]protocol.CommitMatch)) (limitHit bool, _ error) - ResolveRevisions(context.Context, api.RepoName, []gitprotocol.RevisionSpecifier) ([]string, error) + ResolveRevision(context.Context, api.RepoName, string, gitserver.ResolveRevisionOptions) (api.CommitID, error) } func (j *SearchJob) Run(ctx context.Context, clients job.RuntimeClients, stream streaming.Sender) (alert *search.Alert, err error) { @@ -61,7 +62,7 @@ func (j *SearchJob) Run(ctx context.Context, clients job.RuntimeClients, stream args := &protocol.SearchRequest{ Repo: repoRev.Repo.Name, - Revisions: searchRevsToGitserverRevs(repoRev.Revs), + Revisions: repoRev.Revs, Query: j.Query, IncludeDiff: j.Diff, Limit: j.Limit, @@ -248,16 +249,6 @@ func QueryToGitQuery(b query.Basic, diff bool) gitprotocol.Node { return gitprotocol.Reduce(gitprotocol.NewAnd(res...)) } -func searchRevsToGitserverRevs(in []string) []gitprotocol.RevisionSpecifier { - out := make([]gitprotocol.RevisionSpecifier, 0, len(in)) - for _, rev := range in { - out = append(out, gitprotocol.RevisionSpecifier{ - RevSpec: rev, - }) - } - return out -} - func queryPatternToPredicate(node query.Node, caseSensitive, diff bool) gitprotocol.Node { switch v := node.(type) { case query.Operator: