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

Commit 34b54c9

Browse files
varungandhi-srcvarungandhi-src
authored
chore(codeintel): Differentiate between paths relative to upload root vs repo root (#63437)
The use of different types makes it clear which kind of path is needed in which place. This also makes the CodeNavService layering clearer; it has the responsibility of taking in RepoRelPaths and correctly interfacing with LsifStore, which deals in UploadRelPath values.
1 parent b47c376 commit 34b54c9

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

51 files changed

+615
-462
lines changed

‎cmd/frontend/graphqlbackend/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,7 @@ go_library(
252252
"//internal/authz/permssync",
253253
"//internal/binary",
254254
"//internal/cloud",
255+
"//internal/codeintel/core",
255256
"//internal/codeintel/dependencies",
256257
"//internal/codeintel/dependencies/shared",
257258
"//internal/codeintel/resolvers",

‎cmd/frontend/graphqlbackend/git_tree_entry.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/cloneurls"
2222
"github.com/sourcegraph/sourcegraph/internal/api"
2323
"github.com/sourcegraph/sourcegraph/internal/binary"
24+
"github.com/sourcegraph/sourcegraph/internal/codeintel/core"
2425
resolverstubs "github.com/sourcegraph/sourcegraph/internal/codeintel/resolvers"
2526
"github.com/sourcegraph/sourcegraph/internal/conf"
2627
"github.com/sourcegraph/sourcegraph/internal/database"
@@ -506,7 +507,7 @@ func (r *GitTreeEntryResolver) CodeGraphData(ctx context.Context, args *resolver
506507
Args: args,
507508
Repo: repo,
508509
Commit: api.CommitID(r.Commit().OID()),
509-
Path: r.Path(),
510+
Path: core.NewRepoRelPathUnchecked(r.Path()),
510511
})
511512
}
512513

‎internal/codeintel/codenav/BUILD.bazel

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ go_library(
2727
"//internal/authz",
2828
"//internal/codeintel/codenav/internal/lsifstore",
2929
"//internal/codeintel/codenav/shared",
30+
"//internal/codeintel/core",
3031
"//internal/codeintel/shared",
3132
"//internal/codeintel/uploads/shared",
3233
"//internal/collections",
@@ -78,6 +79,7 @@ go_test(
7879
"//internal/authz",
7980
"//internal/codeintel/codenav/internal/lsifstore",
8081
"//internal/codeintel/codenav/shared",
82+
"//internal/codeintel/core",
8183
"//internal/codeintel/uploads/shared",
8284
"//internal/collections",
8385
"//internal/database/dbmocks",

‎internal/codeintel/codenav/gittree_translator.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"github.com/sourcegraph/go-diff/diff"
1111

1212
"github.com/sourcegraph/sourcegraph/internal/codeintel/codenav/shared"
13+
"github.com/sourcegraph/sourcegraph/internal/codeintel/core"
1314
"github.com/sourcegraph/sourcegraph/internal/gitserver"
1415
sgtypes "github.com/sourcegraph/sourcegraph/internal/types"
1516
"github.com/sourcegraph/sourcegraph/lib/errors"
@@ -47,7 +48,7 @@ type gitTreeTranslator struct {
4748
type requestArgs struct {
4849
repo *sgtypes.Repo
4950
commit string
50-
path string
51+
path core.RepoRelPath
5152
}
5253

5354
func (r *requestArgs) GetRepoID() int {
@@ -95,13 +96,13 @@ func (g *gitTreeTranslator) GetTargetCommitPathFromSourcePath(ctx context.Contex
9596
// target commits are swapped.
9697
// TODO: No todo just letting me know that I updated path just on this one. Need to do it like that.
9798
func (g *gitTreeTranslator) GetTargetCommitPositionFromSourcePosition(ctx context.Context, commit string, px shared.Position, reverse bool) (string, shared.Position, bool, error) {
98-
hunks, err := g.readCachedHunks(ctx, g.localRequestArgs.repo, g.localRequestArgs.commit, commit, g.localRequestArgs.path, reverse)
99+
hunks, err := g.readCachedHunks(ctx, g.localRequestArgs.repo, g.localRequestArgs.commit, commit, g.localRequestArgs.path.RawValue(), reverse)
99100
if err != nil {
100101
return "", shared.Position{}, false, err
101102
}
102103

103104
commitPosition, ok := translatePosition(hunks, px)
104-
return g.localRequestArgs.path, commitPosition, ok, nil
105+
return g.localRequestArgs.path.RawValue(), commitPosition, ok, nil
105106
}
106107

107108
// GetTargetCommitRangeFromSourceRange translates the given range from the source commit into the given target

‎internal/codeintel/codenav/gittree_translator_test.go

Lines changed: 36 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -11,37 +11,39 @@ import (
1111
godiff "github.com/sourcegraph/go-diff/diff"
1212

1313
"github.com/sourcegraph/sourcegraph/internal/api"
14-
1514
"github.com/sourcegraph/sourcegraph/internal/codeintel/codenav/shared"
15+
"github.com/sourcegraph/sourcegraph/internal/codeintel/core"
1616
"github.com/sourcegraph/sourcegraph/internal/gitserver"
1717
sgtypes "github.com/sourcegraph/sourcegraph/internal/types"
1818
)
1919

20+
var mockRequestArgs = requestArgs{
21+
repo: &sgtypes.Repo{ID: 50},
22+
commit: "deadbeef1",
23+
path: core.NewRepoRelPathUnchecked("foo/bar.go"),
24+
}
25+
2026
func TestGetTargetCommitPathFromSourcePath(t *testing.T) {
2127
client := gitserver.NewMockClient()
2228

23-
args := &requestArgs{
24-
repo: &sgtypes.Repo{ID: 50},
25-
commit: "deadbeef1",
26-
path: "/foo/bar.go",
27-
}
29+
args := &mockRequestArgs
2830
adjuster := NewGitTreeTranslator(client, args, nil)
29-
path, ok, err := adjuster.GetTargetCommitPathFromSourcePath(context.Background(), "deadbeef2", "/foo/bar.go", false)
31+
path, ok, err := adjuster.GetTargetCommitPathFromSourcePath(context.Background(), "deadbeef2", args.path.RawValue(), false)
3032
if err != nil {
3133
t.Fatalf("unexpected error: %s", err)
3234
}
3335

3436
if !ok {
3537
t.Errorf("expected translation to succeed")
3638
}
37-
if path != "/foo/bar.go" {
38-
t.Errorf("unexpected path. want=%s have=%s", "/foo/bar.go", path)
39+
if path != "foo/bar.go" {
40+
t.Errorf("unexpected path. want=%s have=%s", "foo/bar.go", path)
3941
}
4042
}
4143

4244
func TestGetTargetCommitPositionFromSourcePosition(t *testing.T) {
4345
client := gitserver.NewMockClientWithExecReader(nil, func(_ context.Context, _ api.RepoName, args []string) (reader io.ReadCloser, err error) {
44-
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef1..deadbeef2", "--", "/foo/bar.go"}
46+
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef1..deadbeef2", "--", mockRequestArgs.path.RawValue()}
4547
if diff := cmp.Diff(expectedArgs, args); diff != "" {
4648
t.Errorf("unexpected exec reader args (-want +got):\n%s", diff)
4749
}
@@ -51,11 +53,7 @@ func TestGetTargetCommitPositionFromSourcePosition(t *testing.T) {
5153

5254
posIn := shared.Position{Line: 302, Character: 15}
5355

54-
args := &requestArgs{
55-
repo: &sgtypes.Repo{ID: 50},
56-
commit: "deadbeef1",
57-
path: "/foo/bar.go",
58-
}
56+
args := &mockRequestArgs
5957
adjuster := NewGitTreeTranslator(client, args, nil)
6058
path, posOut, ok, err := adjuster.GetTargetCommitPositionFromSourcePosition(context.Background(), "deadbeef2", posIn, false)
6159
if err != nil {
@@ -65,8 +63,8 @@ func TestGetTargetCommitPositionFromSourcePosition(t *testing.T) {
6563
if !ok {
6664
t.Errorf("expected translation to succeed")
6765
}
68-
if path != "/foo/bar.go" {
69-
t.Errorf("unexpected path. want=%s have=%s", "/foo/bar.go", path)
66+
if path != "foo/bar.go" {
67+
t.Errorf("unexpected path. want=%s have=%s", "foo/bar.go", path)
7068
}
7169

7270
expectedPos := shared.Position{Line: 294, Character: 15}
@@ -82,11 +80,7 @@ func TestGetTargetCommitPositionFromSourcePositionEmptyDiff(t *testing.T) {
8280

8381
posIn := shared.Position{Line: 10, Character: 15}
8482

85-
args := &requestArgs{
86-
repo: &sgtypes.Repo{ID: 50},
87-
commit: "deadbeef1",
88-
path: "/foo/bar.go",
89-
}
83+
args := &mockRequestArgs
9084
adjuster := NewGitTreeTranslator(client, args, nil)
9185
path, posOut, ok, err := adjuster.GetTargetCommitPositionFromSourcePosition(context.Background(), "deadbeef2", posIn, false)
9286
if err != nil {
@@ -96,8 +90,8 @@ func TestGetTargetCommitPositionFromSourcePositionEmptyDiff(t *testing.T) {
9690
if !ok {
9791
t.Errorf("expected translation to succeed")
9892
}
99-
if path != "/foo/bar.go" {
100-
t.Errorf("unexpected path. want=%s have=%s", "/foo/bar.go", path)
93+
if path != "foo/bar.go" {
94+
t.Errorf("unexpected path. want=%s have=%s", "foo/bar.go", path)
10195
}
10296
if diff := cmp.Diff(posOut, posIn); diff != "" {
10397
t.Errorf("unexpected position (-want +got):\n%s", diff)
@@ -106,7 +100,7 @@ func TestGetTargetCommitPositionFromSourcePositionEmptyDiff(t *testing.T) {
106100

107101
func TestGetTargetCommitPositionFromSourcePositionReverse(t *testing.T) {
108102
client := gitserver.NewMockClientWithExecReader(nil, func(_ context.Context, _ api.RepoName, args []string) (reader io.ReadCloser, err error) {
109-
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef2..deadbeef1", "--", "/foo/bar.go"}
103+
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef2..deadbeef1", "--", mockRequestArgs.path.RawValue()}
110104
if diff := cmp.Diff(expectedArgs, args); diff != "" {
111105
t.Errorf("unexpected exec reader args (-want +got):\n%s", diff)
112106
}
@@ -116,11 +110,7 @@ func TestGetTargetCommitPositionFromSourcePositionReverse(t *testing.T) {
116110

117111
posIn := shared.Position{Line: 302, Character: 15}
118112

119-
args := &requestArgs{
120-
repo: &sgtypes.Repo{ID: 50},
121-
commit: "deadbeef1",
122-
path: "/foo/bar.go",
123-
}
113+
args := &mockRequestArgs
124114
adjuster := NewGitTreeTranslator(client, args, nil)
125115
path, posOut, ok, err := adjuster.GetTargetCommitPositionFromSourcePosition(context.Background(), "deadbeef2", posIn, true)
126116
if err != nil {
@@ -130,8 +120,8 @@ func TestGetTargetCommitPositionFromSourcePositionReverse(t *testing.T) {
130120
if !ok {
131121
t.Errorf("expected translation to succeed")
132122
}
133-
if path != "/foo/bar.go" {
134-
t.Errorf("unexpected path. want=%s have=%s", "/foo/bar.go", path)
123+
if path != "foo/bar.go" {
124+
t.Errorf("unexpected path. want=%s have=%s", "foo/bar.go", path)
135125
}
136126

137127
expectedPos := shared.Position{Line: 294, Character: 15}
@@ -142,7 +132,7 @@ func TestGetTargetCommitPositionFromSourcePositionReverse(t *testing.T) {
142132

143133
func TestGetTargetCommitRangeFromSourceRange(t *testing.T) {
144134
client := gitserver.NewMockClientWithExecReader(nil, func(_ context.Context, _ api.RepoName, args []string) (reader io.ReadCloser, err error) {
145-
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef1..deadbeef2", "--", "/foo/bar.go"}
135+
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef1..deadbeef2", "--", mockRequestArgs.path.RawValue()}
146136
if diff := cmp.Diff(expectedArgs, args); diff != "" {
147137
t.Errorf("unexpected exec reader args (-want +got):\n%s", diff)
148138
}
@@ -155,22 +145,18 @@ func TestGetTargetCommitRangeFromSourceRange(t *testing.T) {
155145
End: shared.Position{Line: 305, Character: 20},
156146
}
157147

158-
args := &requestArgs{
159-
repo: &sgtypes.Repo{ID: 50},
160-
commit: "deadbeef1",
161-
path: "/foo/bar.go",
162-
}
148+
args := &mockRequestArgs
163149
adjuster := NewGitTreeTranslator(client, args, nil)
164-
path, rOut, ok, err := adjuster.GetTargetCommitRangeFromSourceRange(context.Background(), "deadbeef2", "/foo/bar.go", rIn, false)
150+
path, rOut, ok, err := adjuster.GetTargetCommitRangeFromSourceRange(context.Background(), "deadbeef2", mockRequestArgs.path.RawValue(), rIn, false)
165151
if err != nil {
166152
t.Fatalf("unexpected error: %s", err)
167153
}
168154

169155
if !ok {
170156
t.Errorf("expected translation to succeed")
171157
}
172-
if path != "/foo/bar.go" {
173-
t.Errorf("unexpected path. want=%s have=%s", "/foo/bar.go", path)
158+
if path != "foo/bar.go" {
159+
t.Errorf("unexpected path. want=%s have=%s", "foo/bar.go", path)
174160
}
175161

176162
expectedRange := shared.Range{
@@ -192,22 +178,18 @@ func TestGetTargetCommitRangeFromSourceRangeEmptyDiff(t *testing.T) {
192178
End: shared.Position{Line: 305, Character: 20},
193179
}
194180

195-
args := &requestArgs{
196-
repo: &sgtypes.Repo{ID: 50},
197-
commit: "deadbeef1",
198-
path: "/foo/bar.go",
199-
}
181+
args := &mockRequestArgs
200182
adjuster := NewGitTreeTranslator(client, args, nil)
201-
path, rOut, ok, err := adjuster.GetTargetCommitRangeFromSourceRange(context.Background(), "deadbeef2", "/foo/bar.go", rIn, false)
183+
path, rOut, ok, err := adjuster.GetTargetCommitRangeFromSourceRange(context.Background(), "deadbeef2", mockRequestArgs.path.RawValue(), rIn, false)
202184
if err != nil {
203185
t.Fatalf("unexpected error: %s", err)
204186
}
205187

206188
if !ok {
207189
t.Errorf("expected translation to succeed")
208190
}
209-
if path != "/foo/bar.go" {
210-
t.Errorf("unexpected path. want=%s have=%s", "/foo/bar.go", path)
191+
if path != "foo/bar.go" {
192+
t.Errorf("unexpected path. want=%s have=%s", "foo/bar.go", path)
211193
}
212194
if diff := cmp.Diff(rOut, rIn); diff != "" {
213195
t.Errorf("unexpected position (-want +got):\n%s", diff)
@@ -216,7 +198,7 @@ func TestGetTargetCommitRangeFromSourceRangeEmptyDiff(t *testing.T) {
216198

217199
func TestGetTargetCommitRangeFromSourceRangeReverse(t *testing.T) {
218200
client := gitserver.NewMockClientWithExecReader(nil, func(_ context.Context, _ api.RepoName, args []string) (reader io.ReadCloser, err error) {
219-
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef2..deadbeef1", "--", "/foo/bar.go"}
201+
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef2..deadbeef1", "--", mockRequestArgs.path.RawValue()}
220202
if diff := cmp.Diff(expectedArgs, args); diff != "" {
221203
t.Errorf("unexpected exec reader args (-want +got):\n%s", diff)
222204
}
@@ -229,22 +211,18 @@ func TestGetTargetCommitRangeFromSourceRangeReverse(t *testing.T) {
229211
End: shared.Position{Line: 305, Character: 20},
230212
}
231213

232-
args := &requestArgs{
233-
repo: &sgtypes.Repo{ID: 50},
234-
commit: "deadbeef1",
235-
path: "/foo/bar.go",
236-
}
214+
args := &mockRequestArgs
237215
adjuster := NewGitTreeTranslator(client, args, nil)
238-
path, rOut, ok, err := adjuster.GetTargetCommitRangeFromSourceRange(context.Background(), "deadbeef2", "/foo/bar.go", rIn, true)
216+
path, rOut, ok, err := adjuster.GetTargetCommitRangeFromSourceRange(context.Background(), "deadbeef2", args.path.RawValue(), rIn, true)
239217
if err != nil {
240218
t.Fatalf("unexpected error: %s", err)
241219
}
242220

243221
if !ok {
244222
t.Errorf("expected translation to succeed")
245223
}
246-
if path != "/foo/bar.go" {
247-
t.Errorf("unexpected path. want=%s have=%s", "/foo/bar.go", path)
224+
if path != "foo/bar.go" {
225+
t.Errorf("unexpected path. want=%s have=%s", "foo/bar.go", path)
248226
}
249227

250228
expectedRange := shared.Range{

‎internal/codeintel/codenav/internal/lsifstore/BUILD.bazel

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ go_library(
1818
visibility = ["//:__subpackages__"],
1919
deps = [
2020
"//internal/codeintel/codenav/shared",
21+
"//internal/codeintel/core",
2122
"//internal/codeintel/shared",
2223
"//internal/codeintel/shared/ranges",
2324
"//internal/codeintel/uploads/shared",
@@ -54,6 +55,7 @@ go_test(
5455
],
5556
deps = [
5657
"//internal/codeintel/codenav/shared",
58+
"//internal/codeintel/core",
5759
"//internal/codeintel/shared",
5860
"//internal/database/dbtest",
5961
"//internal/observation",

‎internal/codeintel/codenav/internal/lsifstore/document_metadata.go

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,22 +9,23 @@ import (
99
"go.opentelemetry.io/otel/attribute"
1010

1111
"github.com/sourcegraph/sourcegraph/internal/codeintel/codenav/shared"
12+
"github.com/sourcegraph/sourcegraph/internal/codeintel/core"
1213
"github.com/sourcegraph/sourcegraph/internal/database/basestore"
1314
"github.com/sourcegraph/sourcegraph/internal/observation"
1415
)
1516

1617
// GetPathExists determines if the path exists in the database.
17-
func (s *store) GetPathExists(ctx context.Context, bundleID int, path string) (_ bool, err error) {
18+
func (s *store) GetPathExists(ctx context.Context, bundleID int, path core.UploadRelPath) (_ bool, err error) {
1819
ctx, _, endObservation := s.operations.getPathExists.With(ctx, &err, observation.Args{Attrs: []attribute.KeyValue{
1920
attribute.Int("bundleID", bundleID),
20-
attribute.String("path", path),
21+
attribute.String("path", path.RawValue()),
2122
}})
2223
defer endObservation(1, observation.Args{})
2324

2425
exists, _, err := basestore.ScanFirstBool(s.db.Query(ctx, sqlf.Sprintf(
2526
existsQuery,
2627
bundleID,
27-
path,
28+
path.RawValue(),
2829
)))
2930
return exists, err
3031
}
@@ -40,17 +41,17 @@ SELECT EXISTS (
4041
`
4142

4243
// Stencil returns all ranges within a single document.
43-
func (s *store) GetStencil(ctx context.Context, bundleID int, path string) (_ []shared.Range, err error) {
44+
func (s *store) GetStencil(ctx context.Context, bundleID int, path core.UploadRelPath) (_ []shared.Range, err error) {
4445
ctx, trace, endObservation := s.operations.getStencil.With(ctx, &err, observation.Args{Attrs: []attribute.KeyValue{
4546
attribute.Int("bundleID", bundleID),
46-
attribute.String("path", path),
47+
attribute.String("path", path.RawValue()),
4748
}})
4849
defer endObservation(1, observation.Args{})
4950

5051
documentData, exists, err := s.scanFirstDocumentData(s.db.Query(ctx, sqlf.Sprintf(
5152
stencilQuery,
5253
bundleID,
53-
path,
54+
path.RawValue(),
5455
)))
5556
if err != nil || !exists {
5657
return nil, err
@@ -80,10 +81,10 @@ LIMIT 1
8081
`
8182

8283
// GetRanges returns definition, reference, implementation, and hover data for each range within the given span of lines.
83-
func (s *store) GetRanges(ctx context.Context, bundleID int, path string, startLine, endLine int) (_ []shared.CodeIntelligenceRange, err error) {
84+
func (s *store) GetRanges(ctx context.Context, bundleID int, path core.UploadRelPath, startLine, endLine int) (_ []shared.CodeIntelligenceRange, err error) {
8485
ctx, _, endObservation := s.operations.getRanges.With(ctx, &err, observation.Args{Attrs: []attribute.KeyValue{
8586
attribute.Int("bundleID", bundleID),
86-
attribute.String("path", path),
87+
attribute.String("path", path.RawValue()),
8788
attribute.Int("startLine", startLine),
8889
attribute.Int("endLine", endLine),
8990
}})
@@ -92,7 +93,7 @@ func (s *store) GetRanges(ctx context.Context, bundleID int, path string, startL
9293
documentData, exists, err := s.scanFirstDocumentData(s.db.Query(ctx, sqlf.Sprintf(
9394
rangesDocumentQuery,
9495
bundleID,
95-
path,
96+
path.RawValue(),
9697
)))
9798
if err != nil || !exists {
9899
return nil, err
@@ -132,7 +133,7 @@ WHERE
132133
LIMIT 1
133134
`
134135

135-
func convertSCIPRangesToLocations(ranges []scip.Range, uploadID int, path string) []shared.Location {
136+
func convertSCIPRangesToLocations(ranges []scip.Range, uploadID int, path core.UploadRelPath) []shared.Location {
136137
locations := make([]shared.Location, 0, len(ranges))
137138
for _, r := range ranges {
138139
locations = append(locations, shared.Location{

0 commit comments

Comments
 (0)