Skip to content
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

symbols: replace usage of gitserver client's DiffSymbols method with new ChangedFiles method #62355

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/symbols/fetcher/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ go_test(
deps = [
"//cmd/symbols/gitserver",
"//internal/api",
"//internal/gitserver",
"//internal/gitserver/gitdomain",
"//internal/observation",
"//internal/search",
Expand Down
166 changes: 84 additions & 82 deletions cmd/symbols/fetcher/mocks_test.go

Large diffs are not rendered by default.

1 change: 0 additions & 1 deletion cmd/symbols/gitserver/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ go_test(
"//internal/gitserver",
"//internal/gitserver/gitdomain",
"//internal/observation",
"@com_github_google_go_cmp//cmp",
"@com_github_stretchr_testify//require",
],
)
15 changes: 4 additions & 11 deletions cmd/symbols/gitserver/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ type GitserverClient interface {
// determine if the error is a bad request (eg invalid repo).
FetchTar(context.Context, api.RepoName, api.CommitID, []string) (io.ReadCloser, error)

// GitDiff returns the paths that have changed between two commits.
GitDiff(context.Context, api.RepoName, api.CommitID, api.CommitID) (Changes, error)
// ChangedFiles returns an iterator that yields the paths that have changed between two commits.
ChangedFiles(context.Context, api.RepoName, api.CommitID, api.CommitID) (gitserver.ChangedFilesIterator, error)

// NewFileReader returns an io.ReadCloser reading from the named file at commit.
// The caller should always close the reader after use.
Expand Down Expand Up @@ -75,22 +75,15 @@ func (c *gitserverClient) FetchTar(ctx context.Context, repo api.RepoName, commi
return c.innerClient.ArchiveReader(ctx, repo, opts)
}

func (c *gitserverClient) GitDiff(ctx context.Context, repo api.RepoName, commitA, commitB api.CommitID) (_ Changes, err error) {
func (c *gitserverClient) ChangedFiles(ctx context.Context, repo api.RepoName, commitA, commitB api.CommitID) (iterator gitserver.ChangedFilesIterator, err error) {
ctx, _, endObservation := c.operations.gitDiff.With(ctx, &err, observation.Args{Attrs: []attribute.KeyValue{
repo.Attr(),
attribute.String("commitA", string(commitA)),
attribute.String("commitB", string(commitB)),
}})
defer endObservation(1, observation.Args{})

output, err := c.innerClient.DiffSymbols(ctx, repo, commitA, commitB)

changes, err := parseGitDiffOutput(output)
if err != nil {
return Changes{}, errors.Wrap(err, "failed to parse git diff output")
}

return changes, nil
return c.innerClient.ChangedFiles(ctx, repo, string(commitA), string(commitB))
}

func (c *gitserverClient) NewFileReader(ctx context.Context, repoCommitPath types.RepoCommitPath) (io.ReadCloser, error) {
Expand Down
62 changes: 0 additions & 62 deletions cmd/symbols/gitserver/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/require"

"github.com/sourcegraph/sourcegraph/internal/api"
Expand All @@ -13,67 +12,6 @@ import (
"github.com/sourcegraph/sourcegraph/internal/observation"
)

func TestParseGitDiffOutput(t *testing.T) {
testCases := []struct {
output []byte
expectedChanges Changes
shouldError bool
}{
{
output: combineBytes(
[]byte("A"), NUL, []byte("added1.json"), NUL,
[]byte("M"), NUL, []byte("modified1.json"), NUL,
[]byte("D"), NUL, []byte("deleted1.json"), NUL,
[]byte("A"), NUL, []byte("added2.json"), NUL,
[]byte("M"), NUL, []byte("modified2.json"), NUL,
[]byte("D"), NUL, []byte("deleted2.json"), NUL,
[]byte("A"), NUL, []byte("added3.json"), NUL,
[]byte("M"), NUL, []byte("modified3.json"), NUL,
[]byte("D"), NUL, []byte("deleted3.json"), NUL,
),
expectedChanges: Changes{
Added: []string{"added1.json", "added2.json", "added3.json"},
Modified: []string{"modified1.json", "modified2.json", "modified3.json"},
Deleted: []string{"deleted1.json", "deleted2.json", "deleted3.json"},
},
},
{
output: combineBytes(
[]byte("A"), NUL, []byte("added1.json"), NUL,
[]byte("M"), NUL, []byte("modified1.json"), NUL,
[]byte("D"), NUL,
),
shouldError: true,
},
{
output: []byte{},
},
}

for _, testCase := range testCases {
changes, err := parseGitDiffOutput(testCase.output)
if err != nil {
if !testCase.shouldError {
t.Fatalf("unexpected error parsing git diff output: %s", err)
}
} else if testCase.shouldError {
t.Fatalf("expected error, got none")
}

if diff := cmp.Diff(testCase.expectedChanges, changes); diff != "" {
t.Errorf("unexpected changes (-want +got):\n%s", diff)
}
}
}

func combineBytes(bss ...[]byte) (combined []byte) {
for _, bs := range bss {
combined = append(combined, bs...)
}

return combined
}

func TestGitserverClient_PaginatedRevList(t *testing.T) {
allCommits := []*gitdomain.Commit{
{ID: "4ac04f2761285633cd35188c696a6e08de03c00c"},
Expand Down
2 changes: 1 addition & 1 deletion cmd/symbols/gitserver/observability.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,6 @@ func newOperations(observationCtx *observation.Context) *operations {

return &operations{
fetchTar: op("FetchTar"),
gitDiff: op("GitDiff"),
gitDiff: op("ChangedFiles"),
}
}
1 change: 1 addition & 0 deletions cmd/symbols/internal/api/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ go_test(
"//internal/database/dbmocks",
"//internal/diskcache",
"//internal/endpoint",
"//internal/gitserver",
"//internal/gitserver/gitdomain",
"//internal/grpc/defaults",
"//internal/observation",
Expand Down