diff --git a/cmd/src/campaigns_common.go b/cmd/src/campaigns_common.go index 2064946ffc..49a424c663 100644 --- a/cmd/src/campaigns_common.go +++ b/cmd/src/campaigns_common.go @@ -268,6 +268,11 @@ func campaignsExecute(ctx context.Context, out *output.Output, svc *campaigns.Se }() } + err = svc.ValidateChangesetSpecs(repos, specs) + if err != nil { + return "", "", err + } + ids := make([]campaigns.ChangesetSpecID, len(specs)) if len(specs) > 0 { @@ -362,7 +367,7 @@ func printExecutionError(out *output.Output, err error) { if err == context.Canceled { block.Writef("%sAborting", output.StyleBold) } else { - block.Writef("%s%s=%+v)", output.StyleBold, e.Error()) + block.Writef("%s%s", output.StyleBold, e.Error()) } } } diff --git a/internal/campaigns/executor.go b/internal/campaigns/executor.go index c71101df21..d9bcfb2bab 100644 --- a/internal/campaigns/executor.go +++ b/internal/campaigns/executor.go @@ -316,9 +316,9 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { }) // Add the spec to the executor's list of completed specs. - x.specsMu.Lock() - x.specs = append(x.specs, specs...) - x.specsMu.Unlock() + if err := x.addCompletedSpecs(task.Repository, specs); err != nil { + return err + } return } @@ -392,10 +392,10 @@ func (x *executor) do(ctx context.Context, task *Task) (err error) { status.ChangesetSpecs = specs }) - // Add the spec to the executor's list of completed specs. - x.specsMu.Lock() - x.specs = append(x.specs, specs...) - x.specsMu.Unlock() + if err := x.addCompletedSpecs(task.Repository, specs); err != nil { + return err + } + return } @@ -409,6 +409,14 @@ func (x *executor) updateTaskStatus(task *Task, update func(status *TaskStatus)) } } +func (x *executor) addCompletedSpecs(repository *graphql.Repository, specs []*ChangesetSpec) error { + x.specsMu.Lock() + defer x.specsMu.Unlock() + + x.specs = append(x.specs, specs...) + return nil +} + func (x *executor) LockedTaskStatuses(callback func([]*TaskStatus)) { x.statusesMu.RLock() defer x.statusesMu.RUnlock() diff --git a/internal/campaigns/executor_test.go b/internal/campaigns/executor_test.go index a8a8fea9ae..d7075253aa 100644 --- a/internal/campaigns/executor_test.go +++ b/internal/campaigns/executor_test.go @@ -400,13 +400,18 @@ repository_name=github.com/sourcegraph/src-cli`, executor.Start(context.Background()) specs, err := executor.Wait(context.Background()) - if tc.wantErrInclude == "" && err != nil { - t.Fatalf("execution failed: %s", err) - } - if err != nil && !strings.Contains(err.Error(), tc.wantErrInclude) { - t.Errorf("wrong error. have=%q want included=%q", err, tc.wantErrInclude) - } - if tc.wantErrInclude != "" { + if tc.wantErrInclude == "" { + if err != nil { + t.Fatalf("execution failed: %s", err) + } + } else { + if err == nil { + t.Fatalf("expected error to include %q, but got no error", tc.wantErrInclude) + } else { + if !strings.Contains(err.Error(), tc.wantErrInclude) { + t.Errorf("wrong error. have=%q want included=%q", err, tc.wantErrInclude) + } + } return } diff --git a/internal/campaigns/service.go b/internal/campaigns/service.go index 6f97714fe1..c2128a2202 100644 --- a/internal/campaigns/service.go +++ b/internal/campaigns/service.go @@ -415,6 +415,66 @@ func (svc *Service) ExecuteCampaignSpec(ctx context.Context, opts ExecutorOpts, return specs, x.LogFiles(), errs.ErrorOrNil() } +func (svc *Service) ValidateChangesetSpecs(repos []*graphql.Repository, specs []*ChangesetSpec) error { + repoByID := make(map[string]*graphql.Repository, len(repos)) + for _, repo := range repos { + repoByID[repo.ID] = repo + } + + byRepoAndBranch := make(map[string]map[string][]*ChangesetSpec) + for _, spec := range specs { + _, ok := byRepoAndBranch[spec.HeadRepository] + if !ok { + byRepoAndBranch[spec.HeadRepository] = make(map[string][]*ChangesetSpec) + } + + byRepoAndBranch[spec.HeadRepository][spec.HeadRef] = append(byRepoAndBranch[spec.HeadRepository][spec.HeadRef], spec) + } + + duplicates := make(map[*graphql.Repository]map[string]int) + for repoID, specsByBranch := range byRepoAndBranch { + for branch, specs := range specsByBranch { + if len(specs) < 2 { + continue + } + + r := repoByID[repoID] + if _, ok := duplicates[r]; !ok { + duplicates[r] = make(map[string]int) + } + + duplicates[r][branch] = len(specs) + } + } + + if len(duplicates) > 0 { + return &duplicateBranchesErr{duplicates: duplicates} + } + + return nil +} + +type duplicateBranchesErr struct { + duplicates map[*graphql.Repository]map[string]int +} + +func (e *duplicateBranchesErr) Error() string { + var out strings.Builder + + fmt.Fprintf(&out, "Multiple changeset specs have the same branch:\n\n") + + for repo, branches := range e.duplicates { + for branch, duplicates := range branches { + branch = strings.TrimPrefix(branch, "refs/heads/") + fmt.Fprintf(&out, "\t* %s: %d changeset specs have the branch %q\n", repo.Name, duplicates, branch) + } + } + + fmt.Fprint(&out, "\nMake sure that the changesetTemplate.branch field in the campaign spec produces unique values for each changeset in a single repository and rerun this command.") + + return out.String() +} + func (svc *Service) ParseCampaignSpec(in io.Reader) (*CampaignSpec, string, error) { data, err := ioutil.ReadAll(in) if err != nil { diff --git a/internal/campaigns/service_test.go b/internal/campaigns/service_test.go index b86e9a320e..bb3b42cda2 100644 --- a/internal/campaigns/service_test.go +++ b/internal/campaigns/service_test.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "strings" "testing" "github.com/google/go-cmp/cmp" @@ -455,3 +456,72 @@ func mockDirectoriesInReposResults(t *testing.T, filesPerRepo filesInRepos) (cli return mockGraphQLClient(string(rawRes)) } + +func TestService_ValidateChangesetSpecs(t *testing.T) { + repo1 := &graphql.Repository{ID: "repo-graphql-id-1", Name: "github.com/sourcegraph/src-cli"} + repo2 := &graphql.Repository{ID: "repo-graphql-id-2", Name: "github.com/sourcegraph/sourcegraph"} + + tests := map[string]struct { + repos []*graphql.Repository + specs []*ChangesetSpec + + wantErrInclude string + }{ + "no errors": { + repos: []*graphql.Repository{repo1, repo2}, + specs: []*ChangesetSpec{ + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo1.ID, HeadRef: "refs/heads/branch-1"}, + }, + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo1.ID, HeadRef: "refs/heads/branch-2"}, + }, + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo2.ID, HeadRef: "refs/heads/branch-1"}, + }, + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo2.ID, HeadRef: "refs/heads/branch-2"}, + }, + }, + }, + + "duplicate branches": { + repos: []*graphql.Repository{repo1, repo2}, + specs: []*ChangesetSpec{ + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo1.ID, HeadRef: "refs/heads/branch-1"}, + }, + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo1.ID, HeadRef: "refs/heads/branch-2"}, + }, + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo2.ID, HeadRef: "refs/heads/branch-1"}, + }, + {CreatedChangeset: &CreatedChangeset{ + HeadRepository: repo2.ID, HeadRef: "refs/heads/branch-1"}, + }, + }, + wantErrInclude: `github.com/sourcegraph/sourcegraph: 2 changeset specs have the branch "branch-1"`, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + svc := &Service{} + haveErr := svc.ValidateChangesetSpecs(tt.repos, tt.specs) + if tt.wantErrInclude != "" { + if haveErr == nil { + t.Fatalf("expected %q to be included in error, but got none", tt.wantErrInclude) + } else { + if !strings.Contains(haveErr.Error(), tt.wantErrInclude) { + t.Fatalf("expected %q to be included in error, but was not. error=%q", tt.wantErrInclude, haveErr.Error()) + } + } + } else { + if haveErr != nil { + t.Fatalf("unexpected error: %s", haveErr) + } + } + }) + } +}