Check for branch duplicates after creating changeset specs (#448)

* Check for branch duplicates after creating changeset specs

This is a follow-up to #442 and ensures that changeset specs are not
getting silently lost by validating that multiple changeset specs in the
same repository have different branches.

I decided to make this a separate step _after_ the execution of the
steps so that users can leverage the cache. That allows them to change
the campaign spec and then rerun the command after they get this error,
vs. the execution being aborted after running into this error (if we'd
do the check inside executor).

* Fix naming in duplicateBranchesErr

Author: mrnugget

cmd/src/campaigns_common.go

@@ -272,6 +272,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 {
@@ -366,7 +371,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

@@ -315,9 +315,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
 		}
@@ -391,10 +391,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
 }
 
@@ -408,6 +408,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

@@ -401,13 +401,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)
+				}
+			}
+		})
+	}
+}