Check for branch duplicates after creating changeset specs

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())
 				}
 			}
 		}

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()

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
 				}
 

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 {

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)
+				}
+			}
+		})
+	}
+}