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

Commit ec90be4

Browse files
varungandhi-srcvarungandhi-src
authored
chore: Refactor and document GitTreeTranslator (#63390)
Previously, the code had several problems: - There was code which seemed like it was handling path renames across commits, but it was just a no-op. It was not documented why it was implemented that way. - The Position adjustment API and Range adjustment APIs were inconsistent. - The Position and Range adjustment APIs suggested that they could handle file renames, but they don't/cannot. - The mocking logic was just returning wrong outputs (commits instead of paths in one place). This patch documents the rationale behind the lack of rename handling, as well as adds a slow-but-correct form of Document checking.
1 parent fc1b06a commit ec90be4

12 files changed

+213
-352
lines changed

‎internal/codeintel/codenav/gittree_translator.go

Lines changed: 53 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ 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"
1413
"github.com/sourcegraph/sourcegraph/internal/gitserver"
1514
sgtypes "github.com/sourcegraph/sourcegraph/internal/types"
1615
"github.com/sourcegraph/sourcegraph/lib/errors"
@@ -19,39 +18,58 @@ import (
1918
// GitTreeTranslator translates a position within a git tree at a source commit into the
2019
// equivalent position in a target commit. The git tree translator instance carries
2120
// along with it the source commit.
21+
//
22+
// NOTE(id: codenav-file-rename-detection) At the moment, this code cannot handle positions/ranges
23+
// going from one file to another (notice that the return values don't contain any updated
24+
// path), because there is no way in gitserver to get rename detection without requesting
25+
// a diff of the full repo (which may be quite large).
26+
//
27+
// Additionally, it's not clear if reusing the Document at path P1 in an older commit,
28+
// at a different path P2 in a newer commit is even reliably useful, since symbol names
29+
// may change based on the file name or directory name depending on the language.
30+
//
31+
// TODO(id: GitTreeTranslator-cleanup): Instead of storing the translationBase, we should
32+
// take that as an argument. Specifically, use a struct with two fields, AncestorCommit
33+
// and DescendantCommit, and avoid Source/Target terminology (which becomes confusing to
34+
// understand with the reverse parameter). Instead, we can use an enum MappingDirection
35+
// FromDescendantToAncestor | FromAncestorToDescendant if really needed (to avoid
36+
// inconsistency when modifying the APIs below, as they take different values for 'reverse'
37+
// in production).
2238
type GitTreeTranslator interface {
23-
// GetTargetCommitPathFromSourcePath translates the given path from the source commit into the given target
24-
// commit. If revese is true, then the source and target commits are swapped.
25-
GetTargetCommitPathFromSourcePath(ctx context.Context, commit, path string, reverse bool) (string, bool, error)
26-
// AdjustPath
27-
2839
// GetTargetCommitPositionFromSourcePosition translates the given position from the source commit into the given
29-
// target commit. The target commit's path and position are returned, along with a boolean flag
30-
// indicating that the translation was successful. If revese is true, then the source and
40+
// target commit. The target commit's position is returned, along with a boolean flag
41+
// indicating that the translation was successful. If reverse is true, then the source and
3142
// target commits are swapped.
32-
GetTargetCommitPositionFromSourcePosition(ctx context.Context, commit string, px shared.Position, reverse bool) (string, shared.Position, bool, error)
33-
// AdjustPosition
43+
//
44+
// TODO(id: GitTreeTranslator-cleanup): The reverse parameter is always false in production,
45+
// let's remove the extra parameter.
46+
GetTargetCommitPositionFromSourcePosition(ctx context.Context, commit string, path string, px shared.Position, reverse bool) (shared.Position, bool, error)
3447

3548
// GetTargetCommitRangeFromSourceRange translates the given range from the source commit into the given target
36-
// commit. The target commit's path and range are returned, along with a boolean flag indicating
37-
// that the translation was successful. If revese is true, then the source and target commits
49+
// commit. The target commit's range is returned, along with a boolean flag indicating
50+
// that the translation was successful. If reverse is true, then the source and target commits
3851
// are swapped.
39-
GetTargetCommitRangeFromSourceRange(ctx context.Context, commit, path string, rx shared.Range, reverse bool) (string, shared.Range, bool, error)
52+
//
53+
// TODO(id: GitTreeTranslator-cleanup): The reverse parameter is always true in production,
54+
// let's remove the extra parameter.
55+
GetTargetCommitRangeFromSourceRange(ctx context.Context, commit string, path string, rx shared.Range, reverse bool) (shared.Range, bool, error)
4056
}
4157

4258
type gitTreeTranslator struct {
43-
client gitserver.Client
44-
localRequestArgs *requestArgs
45-
hunkCache HunkCache
59+
client gitserver.Client
60+
base *translationBase
61+
hunkCache HunkCache
4662
}
4763

48-
type requestArgs struct {
64+
// TODO(id: GitTreeTranslator-cleanup): Strictly speaking, calling this translationBase is not
65+
// quite correct as things can flip around based on the reverse parameter. So get rid
66+
// of the commit field and pass that as a parameter for increased clarity at call-sites.
67+
type translationBase struct {
4968
repo *sgtypes.Repo
5069
commit string
51-
path core.RepoRelPath
5270
}
5371

54-
func (r *requestArgs) GetRepoID() int {
72+
func (r *translationBase) GetRepoID() int {
5573
return int(r.repo.ID)
5674
}
5775

@@ -76,47 +94,40 @@ func NewHunkCache(size int) (HunkCache, error) {
7694
}
7795

7896
// NewGitTreeTranslator creates a new GitTreeTranslator with the given repository and source commit.
79-
func NewGitTreeTranslator(client gitserver.Client, args *requestArgs, hunkCache HunkCache) GitTreeTranslator {
97+
func NewGitTreeTranslator(client gitserver.Client, base *translationBase, hunkCache HunkCache) GitTreeTranslator {
8098
return &gitTreeTranslator{
81-
client: client,
82-
hunkCache: hunkCache,
83-
localRequestArgs: args,
99+
client: client,
100+
hunkCache: hunkCache,
101+
base: base,
84102
}
85103
}
86104

87-
// GetTargetCommitPathFromSourcePath translates the given path from the source commit into the given target
88-
// commit. If revese is true, then the source and target commits are swapped.
89-
func (g *gitTreeTranslator) GetTargetCommitPathFromSourcePath(ctx context.Context, commit, path string, reverse bool) (string, bool, error) {
90-
return path, true, nil
91-
}
92-
93105
// GetTargetCommitPositionFromSourcePosition translates the given position from the source commit into the given
94-
// target commit. The target commit path and position are returned, along with a boolean flag
95-
// indicating that the translation was successful. If revese is true, then the source and
106+
// target commit. The target commit position is returned, along with a boolean flag
107+
// indicating that the translation was successful. If reverse is true, then the source and
96108
// target commits are swapped.
97-
// TODO: No todo just letting me know that I updated path just on this one. Need to do it like that.
98-
func (g *gitTreeTranslator) GetTargetCommitPositionFromSourcePosition(ctx context.Context, commit string, px shared.Position, reverse bool) (string, shared.Position, bool, error) {
99-
hunks, err := g.readCachedHunks(ctx, g.localRequestArgs.repo, g.localRequestArgs.commit, commit, g.localRequestArgs.path.RawValue(), reverse)
109+
func (g *gitTreeTranslator) GetTargetCommitPositionFromSourcePosition(ctx context.Context, commit string, path string, px shared.Position, reverse bool) (shared.Position, bool, error) {
110+
hunks, err := g.readCachedHunks(ctx, g.base.repo, g.base.commit, commit, path, reverse)
100111
if err != nil {
101-
return "", shared.Position{}, false, err
112+
return shared.Position{}, false, err
102113
}
103114

104115
commitPosition, ok := translatePosition(hunks, px)
105-
return g.localRequestArgs.path.RawValue(), commitPosition, ok, nil
116+
return commitPosition, ok, nil
106117
}
107118

108119
// GetTargetCommitRangeFromSourceRange translates the given range from the source commit into the given target
109-
// commit. The target commit path and range are returned, along with a boolean flag indicating
110-
// that the translation was successful. If revese is true, then the source and target commits
120+
// commit. The target commit range is returned, along with a boolean flag indicating
121+
// that the translation was successful. If reverse is true, then the source and target commits
111122
// are swapped.
112-
func (g *gitTreeTranslator) GetTargetCommitRangeFromSourceRange(ctx context.Context, commit, path string, rx shared.Range, reverse bool) (string, shared.Range, bool, error) {
113-
hunks, err := g.readCachedHunks(ctx, g.localRequestArgs.repo, g.localRequestArgs.commit, commit, path, reverse)
123+
func (g *gitTreeTranslator) GetTargetCommitRangeFromSourceRange(ctx context.Context, commit string, path string, rx shared.Range, reverse bool) (shared.Range, bool, error) {
124+
hunks, err := g.readCachedHunks(ctx, g.base.repo, g.base.commit, commit, path, reverse)
114125
if err != nil {
115-
return "", shared.Range{}, false, err
126+
return shared.Range{}, false, err
116127
}
117128

118129
commitRange, ok := translateRange(hunks, rx)
119-
return path, commitRange, ok, nil
130+
return commitRange, ok, nil
120131
}
121132

122133
// readCachedHunks returns a position-ordered slice of changes (additions or deletions) of

‎internal/codeintel/codenav/gittree_translator_test.go

Lines changed: 17 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -12,38 +12,18 @@ import (
1212

1313
"github.com/sourcegraph/sourcegraph/internal/api"
1414
"github.com/sourcegraph/sourcegraph/internal/codeintel/codenav/shared"
15-
"github.com/sourcegraph/sourcegraph/internal/codeintel/core"
1615
"github.com/sourcegraph/sourcegraph/internal/gitserver"
1716
sgtypes "github.com/sourcegraph/sourcegraph/internal/types"
1817
)
1918

20-
var mockRequestArgs = requestArgs{
19+
var mockTranslationBase = translationBase{
2120
repo: &sgtypes.Repo{ID: 50},
2221
commit: "deadbeef1",
23-
path: core.NewRepoRelPathUnchecked("foo/bar.go"),
24-
}
25-
26-
func TestGetTargetCommitPathFromSourcePath(t *testing.T) {
27-
client := gitserver.NewMockClient()
28-
29-
args := &mockRequestArgs
30-
adjuster := NewGitTreeTranslator(client, args, nil)
31-
path, ok, err := adjuster.GetTargetCommitPathFromSourcePath(context.Background(), "deadbeef2", args.path.RawValue(), false)
32-
if err != nil {
33-
t.Fatalf("unexpected error: %s", err)
34-
}
35-
36-
if !ok {
37-
t.Errorf("expected translation to succeed")
38-
}
39-
if path != "foo/bar.go" {
40-
t.Errorf("unexpected path. want=%s have=%s", "foo/bar.go", path)
41-
}
4222
}
4323

4424
func TestGetTargetCommitPositionFromSourcePosition(t *testing.T) {
4525
client := gitserver.NewMockClientWithExecReader(nil, func(_ context.Context, _ api.RepoName, args []string) (reader io.ReadCloser, err error) {
46-
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef1..deadbeef2", "--", mockRequestArgs.path.RawValue()}
26+
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef1..deadbeef2", "--", "foo/bar.go"}
4727
if diff := cmp.Diff(expectedArgs, args); diff != "" {
4828
t.Errorf("unexpected exec reader args (-want +got):\n%s", diff)
4929
}
@@ -53,19 +33,16 @@ func TestGetTargetCommitPositionFromSourcePosition(t *testing.T) {
5333

5434
posIn := shared.Position{Line: 302, Character: 15}
5535

56-
args := &mockRequestArgs
36+
args := &mockTranslationBase
5737
adjuster := NewGitTreeTranslator(client, args, nil)
58-
path, posOut, ok, err := adjuster.GetTargetCommitPositionFromSourcePosition(context.Background(), "deadbeef2", posIn, false)
38+
posOut, ok, err := adjuster.GetTargetCommitPositionFromSourcePosition(context.Background(), "deadbeef2", "foo/bar.go", posIn, false)
5939
if err != nil {
6040
t.Fatalf("unexpected error: %s", err)
6141
}
6242

6343
if !ok {
6444
t.Errorf("expected translation to succeed")
6545
}
66-
if path != "foo/bar.go" {
67-
t.Errorf("unexpected path. want=%s have=%s", "foo/bar.go", path)
68-
}
6946

7047
expectedPos := shared.Position{Line: 294, Character: 15}
7148
if diff := cmp.Diff(expectedPos, posOut); diff != "" {
@@ -80,27 +57,24 @@ func TestGetTargetCommitPositionFromSourcePositionEmptyDiff(t *testing.T) {
8057

8158
posIn := shared.Position{Line: 10, Character: 15}
8259

83-
args := &mockRequestArgs
60+
args := &mockTranslationBase
8461
adjuster := NewGitTreeTranslator(client, args, nil)
85-
path, posOut, ok, err := adjuster.GetTargetCommitPositionFromSourcePosition(context.Background(), "deadbeef2", posIn, false)
62+
posOut, ok, err := adjuster.GetTargetCommitPositionFromSourcePosition(context.Background(), "deadbeef2", "foo/bar.go", posIn, false)
8663
if err != nil {
8764
t.Fatalf("unexpected error: %s", err)
8865
}
8966

9067
if !ok {
9168
t.Errorf("expected translation to succeed")
9269
}
93-
if path != "foo/bar.go" {
94-
t.Errorf("unexpected path. want=%s have=%s", "foo/bar.go", path)
95-
}
9670
if diff := cmp.Diff(posOut, posIn); diff != "" {
9771
t.Errorf("unexpected position (-want +got):\n%s", diff)
9872
}
9973
}
10074

10175
func TestGetTargetCommitPositionFromSourcePositionReverse(t *testing.T) {
10276
client := gitserver.NewMockClientWithExecReader(nil, func(_ context.Context, _ api.RepoName, args []string) (reader io.ReadCloser, err error) {
103-
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef2..deadbeef1", "--", mockRequestArgs.path.RawValue()}
77+
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef2..deadbeef1", "--", "foo/bar.go"}
10478
if diff := cmp.Diff(expectedArgs, args); diff != "" {
10579
t.Errorf("unexpected exec reader args (-want +got):\n%s", diff)
10680
}
@@ -110,19 +84,16 @@ func TestGetTargetCommitPositionFromSourcePositionReverse(t *testing.T) {
11084

11185
posIn := shared.Position{Line: 302, Character: 15}
11286

113-
args := &mockRequestArgs
87+
args := &mockTranslationBase
11488
adjuster := NewGitTreeTranslator(client, args, nil)
115-
path, posOut, ok, err := adjuster.GetTargetCommitPositionFromSourcePosition(context.Background(), "deadbeef2", posIn, true)
89+
posOut, ok, err := adjuster.GetTargetCommitPositionFromSourcePosition(context.Background(), "deadbeef2", "foo/bar.go", posIn, true)
11690
if err != nil {
11791
t.Fatalf("unexpected error: %s", err)
11892
}
11993

12094
if !ok {
12195
t.Errorf("expected translation to succeed")
12296
}
123-
if path != "foo/bar.go" {
124-
t.Errorf("unexpected path. want=%s have=%s", "foo/bar.go", path)
125-
}
12697

12798
expectedPos := shared.Position{Line: 294, Character: 15}
12899
if diff := cmp.Diff(expectedPos, posOut); diff != "" {
@@ -132,7 +103,7 @@ func TestGetTargetCommitPositionFromSourcePositionReverse(t *testing.T) {
132103

133104
func TestGetTargetCommitRangeFromSourceRange(t *testing.T) {
134105
client := gitserver.NewMockClientWithExecReader(nil, func(_ context.Context, _ api.RepoName, args []string) (reader io.ReadCloser, err error) {
135-
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef1..deadbeef2", "--", mockRequestArgs.path.RawValue()}
106+
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef1..deadbeef2", "--", "foo/bar.go"}
136107
if diff := cmp.Diff(expectedArgs, args); diff != "" {
137108
t.Errorf("unexpected exec reader args (-want +got):\n%s", diff)
138109
}
@@ -145,19 +116,16 @@ func TestGetTargetCommitRangeFromSourceRange(t *testing.T) {
145116
End: shared.Position{Line: 305, Character: 20},
146117
}
147118

148-
args := &mockRequestArgs
119+
args := &mockTranslationBase
149120
adjuster := NewGitTreeTranslator(client, args, nil)
150-
path, rOut, ok, err := adjuster.GetTargetCommitRangeFromSourceRange(context.Background(), "deadbeef2", mockRequestArgs.path.RawValue(), rIn, false)
121+
rOut, ok, err := adjuster.GetTargetCommitRangeFromSourceRange(context.Background(), "deadbeef2", "foo/bar.go", rIn, false)
151122
if err != nil {
152123
t.Fatalf("unexpected error: %s", err)
153124
}
154125

155126
if !ok {
156127
t.Errorf("expected translation to succeed")
157128
}
158-
if path != "foo/bar.go" {
159-
t.Errorf("unexpected path. want=%s have=%s", "foo/bar.go", path)
160-
}
161129

162130
expectedRange := shared.Range{
163131
Start: shared.Position{Line: 294, Character: 15},
@@ -178,27 +146,24 @@ func TestGetTargetCommitRangeFromSourceRangeEmptyDiff(t *testing.T) {
178146
End: shared.Position{Line: 305, Character: 20},
179147
}
180148

181-
args := &mockRequestArgs
149+
args := &mockTranslationBase
182150
adjuster := NewGitTreeTranslator(client, args, nil)
183-
path, rOut, ok, err := adjuster.GetTargetCommitRangeFromSourceRange(context.Background(), "deadbeef2", mockRequestArgs.path.RawValue(), rIn, false)
151+
rOut, ok, err := adjuster.GetTargetCommitRangeFromSourceRange(context.Background(), "deadbeef2", "foo/bar.go", rIn, false)
184152
if err != nil {
185153
t.Fatalf("unexpected error: %s", err)
186154
}
187155

188156
if !ok {
189157
t.Errorf("expected translation to succeed")
190158
}
191-
if path != "foo/bar.go" {
192-
t.Errorf("unexpected path. want=%s have=%s", "foo/bar.go", path)
193-
}
194159
if diff := cmp.Diff(rOut, rIn); diff != "" {
195160
t.Errorf("unexpected position (-want +got):\n%s", diff)
196161
}
197162
}
198163

199164
func TestGetTargetCommitRangeFromSourceRangeReverse(t *testing.T) {
200165
client := gitserver.NewMockClientWithExecReader(nil, func(_ context.Context, _ api.RepoName, args []string) (reader io.ReadCloser, err error) {
201-
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef2..deadbeef1", "--", mockRequestArgs.path.RawValue()}
166+
expectedArgs := []string{"diff", "--find-renames", "--full-index", "--inter-hunk-context=3", "--no-prefix", "deadbeef2..deadbeef1", "--", "foo/bar.go"}
202167
if diff := cmp.Diff(expectedArgs, args); diff != "" {
203168
t.Errorf("unexpected exec reader args (-want +got):\n%s", diff)
204169
}
@@ -211,19 +176,15 @@ func TestGetTargetCommitRangeFromSourceRangeReverse(t *testing.T) {
211176
End: shared.Position{Line: 305, Character: 20},
212177
}
213178

214-
args := &mockRequestArgs
179+
args := &mockTranslationBase
215180
adjuster := NewGitTreeTranslator(client, args, nil)
216-
path, rOut, ok, err := adjuster.GetTargetCommitRangeFromSourceRange(context.Background(), "deadbeef2", args.path.RawValue(), rIn, true)
181+
rOut, ok, err := adjuster.GetTargetCommitRangeFromSourceRange(context.Background(), "deadbeef2", "foo/bar.go", rIn, true)
217182
if err != nil {
218183
t.Fatalf("unexpected error: %s", err)
219184
}
220-
221185
if !ok {
222186
t.Errorf("expected translation to succeed")
223187
}
224-
if path != "foo/bar.go" {
225-
t.Errorf("unexpected path. want=%s have=%s", "foo/bar.go", path)
226-
}
227188

228189
expectedRange := shared.Range{
229190
Start: shared.Position{Line: 294, Character: 15},

0 commit comments

Comments
 (0)