Skip to content

Commit e791fe1

Browse files
authored
Make checking of cache explicit in TUI (#535)
* Only checkout repo archive if steps will be executed Since we use a "reference counting" mechanism when checking out archives we shouldn't bump the ref count if we never execute steps. Because only after executing the steps do we decrease the ref count and thus "free" the archive to be cleaned up. The problem is that if a `Task` has a cache hit then the ref will never be decreased and the archive never cleaned up. * WIP is working * More tiny refactoring * Working state * Move stuff around * More refactoring * Improve caching messages after feedback
1 parent 5ae7778 commit e791fe1

File tree

9 files changed

+485
-406
lines changed

9 files changed

+485
-406
lines changed

cmd/src/batch_common.go

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -295,19 +295,41 @@ func executeBatchSpec(ctx context.Context, opts executeBatchSpecOpts) error {
295295
}
296296
batchCompletePending(pending, fmt.Sprintf("Found %d workspaces with steps to execute", len(tasks)))
297297

298-
execOpts := executor.Opts{
298+
// EXECUTION OF TASKS
299+
300+
svc.InitCache(opts.flags.cacheDir)
301+
svc.InitExecutor(ctx, executor.NewExecutorOpts{
299302
CacheDir: opts.flags.cacheDir,
300-
ClearCache: opts.flags.clearCache,
301303
CleanArchives: opts.flags.cleanArchives,
302304
Creator: workspaceCreator,
303305
Parallelism: opts.flags.parallelism,
304306
Timeout: opts.flags.timeout,
305307
KeepLogs: opts.flags.keepLogs,
306308
TempDir: opts.flags.tempDir,
309+
})
310+
311+
pending = batchCreatePending(opts.out, "Checking cache for changeset specs")
312+
uncachedTasks, cachedSpecs, err := svc.CheckCache(ctx, tasks, opts.flags.clearCache)
313+
if err != nil {
314+
return err
315+
}
316+
var specsFoundMessage string
317+
if len(cachedSpecs) == 1 {
318+
specsFoundMessage = "Found 1 cached changeset spec"
319+
} else {
320+
specsFoundMessage = fmt.Sprintf("Found %d cached changeset specs", len(cachedSpecs))
321+
}
322+
switch len(uncachedTasks) {
323+
case 0:
324+
batchCompletePending(pending, fmt.Sprintf("%s; no tasks need to be executed", specsFoundMessage))
325+
case 1:
326+
batchCompletePending(pending, fmt.Sprintf("%s; %d task needs to be executed", specsFoundMessage, len(uncachedTasks)))
327+
default:
328+
batchCompletePending(pending, fmt.Sprintf("%s; %d tasks need to be executed", specsFoundMessage, len(uncachedTasks)))
307329
}
308330

309331
p := newBatchProgressPrinter(opts.out, *verbose, opts.flags.parallelism)
310-
specs, logFiles, err := svc.RunExecutor(ctx, execOpts, tasks, batchSpec, p.PrintStatuses, opts.flags.skipErrors)
332+
freshSpecs, logFiles, err := svc.RunExecutor(ctx, uncachedTasks, batchSpec, p.PrintStatuses, opts.flags.skipErrors)
311333
if err != nil && !opts.flags.skipErrors {
312334
return err
313335
}
@@ -328,6 +350,8 @@ func executeBatchSpec(ctx context.Context, opts executeBatchSpecOpts) error {
328350
}()
329351
}
330352

353+
specs := append(cachedSpecs, freshSpecs...)
354+
331355
err = svc.ValidateChangesetSpecs(repos, specs)
332356
if err != nil {
333357
return err
Lines changed: 215 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,215 @@
1+
package executor
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
7+
"github.com/pkg/errors"
8+
"github.com/sourcegraph/go-diff/diff"
9+
"github.com/sourcegraph/src-cli/internal/batches"
10+
)
11+
12+
func createChangesetSpecs(task *Task, result executionResult, features batches.FeatureFlags) ([]*batches.ChangesetSpec, error) {
13+
repo := task.Repository.Name
14+
15+
tmplCtx := &ChangesetTemplateContext{
16+
BatchChangeAttributes: *task.BatchChangeAttributes,
17+
Steps: StepsContext{
18+
Changes: result.ChangedFiles,
19+
Path: result.Path,
20+
},
21+
Outputs: result.Outputs,
22+
Repository: *task.Repository,
23+
}
24+
25+
var authorName string
26+
var authorEmail string
27+
28+
if task.Template.Commit.Author == nil {
29+
if features.IncludeAutoAuthorDetails {
30+
// user did not provide author info, so use defaults
31+
authorName = "Sourcegraph"
32+
authorEmail = "[email protected]"
33+
}
34+
} else {
35+
var err error
36+
authorName, err = renderChangesetTemplateField("authorName", task.Template.Commit.Author.Name, tmplCtx)
37+
if err != nil {
38+
return nil, err
39+
}
40+
authorEmail, err = renderChangesetTemplateField("authorEmail", task.Template.Commit.Author.Email, tmplCtx)
41+
if err != nil {
42+
return nil, err
43+
}
44+
}
45+
46+
title, err := renderChangesetTemplateField("title", task.Template.Title, tmplCtx)
47+
if err != nil {
48+
return nil, err
49+
}
50+
51+
body, err := renderChangesetTemplateField("body", task.Template.Body, tmplCtx)
52+
if err != nil {
53+
return nil, err
54+
}
55+
56+
message, err := renderChangesetTemplateField("message", task.Template.Commit.Message, tmplCtx)
57+
if err != nil {
58+
return nil, err
59+
}
60+
61+
// TODO: As a next step, we should extend the ChangesetTemplateContext to also include
62+
// TransformChanges.Group and then change validateGroups and groupFileDiffs to, for each group,
63+
// render the branch name *before* grouping the diffs.
64+
defaultBranch, err := renderChangesetTemplateField("branch", task.Template.Branch, tmplCtx)
65+
if err != nil {
66+
return nil, err
67+
}
68+
69+
newSpec := func(branch, diff string) *batches.ChangesetSpec {
70+
return &batches.ChangesetSpec{
71+
BaseRepository: task.Repository.ID,
72+
CreatedChangeset: &batches.CreatedChangeset{
73+
BaseRef: task.Repository.BaseRef(),
74+
BaseRev: task.Repository.Rev(),
75+
HeadRepository: task.Repository.ID,
76+
HeadRef: "refs/heads/" + branch,
77+
Title: title,
78+
Body: body,
79+
Commits: []batches.GitCommitDescription{
80+
{
81+
Message: message,
82+
AuthorName: authorName,
83+
AuthorEmail: authorEmail,
84+
Diff: diff,
85+
},
86+
},
87+
Published: task.Template.Published.ValueWithSuffix(repo, branch),
88+
},
89+
}
90+
}
91+
92+
var specs []*batches.ChangesetSpec
93+
94+
groups := groupsForRepository(task.Repository.Name, task.TransformChanges)
95+
if len(groups) != 0 {
96+
err := validateGroups(task.Repository.Name, task.Template.Branch, groups)
97+
if err != nil {
98+
return specs, err
99+
}
100+
101+
// TODO: Regarding 'defaultBranch', see comment above
102+
diffsByBranch, err := groupFileDiffs(result.Diff, defaultBranch, groups)
103+
if err != nil {
104+
return specs, errors.Wrap(err, "grouping diffs failed")
105+
}
106+
107+
for branch, diff := range diffsByBranch {
108+
specs = append(specs, newSpec(branch, diff))
109+
}
110+
} else {
111+
specs = append(specs, newSpec(defaultBranch, result.Diff))
112+
}
113+
114+
return specs, nil
115+
}
116+
117+
func groupsForRepository(repo string, transform *batches.TransformChanges) []batches.Group {
118+
var groups []batches.Group
119+
120+
if transform == nil {
121+
return groups
122+
}
123+
124+
for _, g := range transform.Group {
125+
if g.Repository != "" {
126+
if g.Repository == repo {
127+
groups = append(groups, g)
128+
}
129+
} else {
130+
groups = append(groups, g)
131+
}
132+
}
133+
134+
return groups
135+
}
136+
137+
func validateGroups(repo, defaultBranch string, groups []batches.Group) error {
138+
uniqueBranches := make(map[string]struct{}, len(groups))
139+
140+
for _, g := range groups {
141+
if _, ok := uniqueBranches[g.Branch]; ok {
142+
return fmt.Errorf("transformChanges would lead to multiple changesets in repository %s to have the same branch %q", repo, g.Branch)
143+
} else {
144+
uniqueBranches[g.Branch] = struct{}{}
145+
}
146+
147+
if g.Branch == defaultBranch {
148+
return fmt.Errorf("transformChanges group branch for repository %s is the same as branch %q in changesetTemplate", repo, defaultBranch)
149+
}
150+
}
151+
152+
return nil
153+
}
154+
155+
func groupFileDiffs(completeDiff, defaultBranch string, groups []batches.Group) (map[string]string, error) {
156+
fileDiffs, err := diff.ParseMultiFileDiff([]byte(completeDiff))
157+
if err != nil {
158+
return nil, err
159+
}
160+
161+
// Housekeeping: we setup these two datastructures so we can
162+
// - access the group.Branch by the directory for which they should be used
163+
// - check against the given directories, in order.
164+
branchesByDirectory := make(map[string]string, len(groups))
165+
dirs := make([]string, len(branchesByDirectory))
166+
for _, g := range groups {
167+
branchesByDirectory[g.Directory] = g.Branch
168+
dirs = append(dirs, g.Directory)
169+
}
170+
171+
byBranch := make(map[string][]*diff.FileDiff, len(groups))
172+
byBranch[defaultBranch] = []*diff.FileDiff{}
173+
174+
// For each file diff...
175+
for _, f := range fileDiffs {
176+
name := f.NewName
177+
if name == "/dev/null" {
178+
name = f.OrigName
179+
}
180+
181+
// .. we check whether it matches one of the given directories in the
182+
// group transformations, with the last match winning:
183+
var matchingDir string
184+
for _, d := range dirs {
185+
if strings.Contains(name, d) {
186+
matchingDir = d
187+
}
188+
}
189+
190+
// If the diff didn't match a rule, it goes into the default branch and
191+
// the default changeset.
192+
if matchingDir == "" {
193+
byBranch[defaultBranch] = append(byBranch[defaultBranch], f)
194+
continue
195+
}
196+
197+
// If it *did* match a directory, we look up which branch we should use:
198+
branch, ok := branchesByDirectory[matchingDir]
199+
if !ok {
200+
panic("this should not happen: " + matchingDir)
201+
}
202+
203+
byBranch[branch] = append(byBranch[branch], f)
204+
}
205+
206+
finalDiffsByBranch := make(map[string]string, len(byBranch))
207+
for branch, diffs := range byBranch {
208+
printed, err := diff.PrintMultiFileDiff(diffs)
209+
if err != nil {
210+
return nil, errors.Wrap(err, "printing multi file diff failed")
211+
}
212+
finalDiffsByBranch[branch] = string(printed)
213+
}
214+
return finalDiffsByBranch, nil
215+
}
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
package executor
2+
3+
import (
4+
"context"
5+
6+
"github.com/pkg/errors"
7+
"github.com/sourcegraph/src-cli/internal/batches"
8+
)
9+
10+
func CheckCache(ctx context.Context, cache ExecutionCache, clearCache bool, features batches.FeatureFlags, task *Task) (specs []*batches.ChangesetSpec, found bool, err error) {
11+
// Check if the task is cached.
12+
cacheKey := task.cacheKey()
13+
if clearCache {
14+
if err = cache.Clear(ctx, cacheKey); err != nil {
15+
return specs, false, errors.Wrapf(err, "clearing cache for %q", task.Repository.Name)
16+
}
17+
18+
return specs, false, nil
19+
}
20+
21+
var result executionResult
22+
result, found, err = cache.Get(ctx, cacheKey)
23+
if err != nil {
24+
return specs, false, errors.Wrapf(err, "checking cache for %q", task.Repository.Name)
25+
}
26+
27+
if !found {
28+
return specs, false, nil
29+
}
30+
31+
// If the cached result resulted in an empty diff, we don't need to
32+
// add it to the list of specs that are displayed to the user and
33+
// send to the server. Instead, we can just report that the task is
34+
// complete and move on.
35+
if result.Diff == "" {
36+
return specs, true, nil
37+
}
38+
39+
specs, err = createChangesetSpecs(task, result, features)
40+
if err != nil {
41+
return specs, false, err
42+
}
43+
44+
return specs, true, nil
45+
}

0 commit comments

Comments
 (0)