Refactor zipfetcher and consistently use rev, not baseref (#598)

Before, sometimes we used Rev, sometimes BaseRef, which could in the worst case lead to inconsistencies. Search results might not match the fetched zip archive, or detected workspaces might differ from what's on disk in the end.

Author: eseliger

internal/batches/errors.go

@@ -77,9 +77,3 @@ func (e IgnoredRepoSet) Append(repo *graphql.Repository) {
 func (e IgnoredRepoSet) HasIgnored() bool {
 	return len(e) > 0
 }
-
-type ValidationError struct{ Reason string }
-
-func (e ValidationError) Error() string {
-	return fmt.Sprintf("validation failed: %s", e.Reason)
-}

internal/batches/executor/changeset_specs.go

@@ -13,19 +13,17 @@ import (
 	"github.com/sourcegraph/src-cli/internal/batches/util"
 )
 
-var errOptionalPublishedUnsupported = errors.New(`This Sourcegraph version requires the "published" field to be specified in the batch spec; upgrade to version 3.30.0 or later to be able to omit the published field and control publication from the UI.`)
+var errOptionalPublishedUnsupported = batcheslib.NewValidationError(errors.New(`This Sourcegraph version requires the "published" field to be specified in the batch spec; upgrade to version 3.30.0 or later to be able to omit the published field and control publication from the UI.`))
 
 func createChangesetSpecs(task *Task, result executionResult, features batches.FeatureFlags) ([]*batcheslib.ChangesetSpec, error) {
-	repo := task.Repository.Name
-
 	tmplCtx := &template.ChangesetTemplateContext{
 		BatchChangeAttributes: *task.BatchChangeAttributes,
 		Steps: template.StepsContext{
 			Changes: result.ChangedFiles,
 			Path:    result.Path,
 		},
 		Outputs:    result.Outputs,
-		Repository: util.GraphQLRepoToTemplatingRepo(task.Repository),
+		Repository: util.NewTemplatingRepo(task.Repository.Name, task.Repository.FileMatches),
 	}
 
 	var authorName string
@@ -75,7 +73,7 @@ func createChangesetSpecs(task *Task, result executionResult, features batches.F
 	newSpec := func(branch, diff string) (*batcheslib.ChangesetSpec, error) {
 		var published interface{} = nil
 		if task.Template.Published != nil {
-			published = task.Template.Published.ValueWithSuffix(repo, branch)
+			published = task.Template.Published.ValueWithSuffix(task.Repository.Name, branch)
 
 			// Backward compatibility: before optional published fields were
 			// allowed, ValueWithSuffix() would fall back to false, not nil. We
@@ -93,7 +91,7 @@ func createChangesetSpecs(task *Task, result executionResult, features batches.F
 			BaseRef:        task.Repository.BaseRef(),
 			BaseRev:        task.Repository.Rev(),
 			HeadRepository: task.Repository.ID,
-			HeadRef:        "refs/heads/" + branch,
+			HeadRef:        util.EnsureRefPrefix(branch),
 			Title:          title,
 			Body:           body,
 			Commits: []batcheslib.GitCommitDescription{
@@ -141,16 +139,16 @@ func createChangesetSpecs(task *Task, result executionResult, features batches.F
 	return specs, nil
 }
 
-func groupsForRepository(repo string, transform *batcheslib.TransformChanges) []batcheslib.Group {
-	var groups []batcheslib.Group
+func groupsForRepository(repoName string, transform *batcheslib.TransformChanges) []batcheslib.Group {
+	groups := []batcheslib.Group{}
 
 	if transform == nil {
 		return groups
 	}
 
 	for _, g := range transform.Group {
 		if g.Repository != "" {
-			if g.Repository == repo {
+			if g.Repository == repoName {
 				groups = append(groups, g)
 			}
 		} else {
@@ -161,18 +159,18 @@ func groupsForRepository(repo string, transform *batcheslib.TransformChanges) []
 	return groups
 }
 
-func validateGroups(repo, defaultBranch string, groups []batcheslib.Group) error {
+func validateGroups(repoName, defaultBranch string, groups []batcheslib.Group) error {
 	uniqueBranches := make(map[string]struct{}, len(groups))
 
 	for _, g := range groups {
 		if _, ok := uniqueBranches[g.Branch]; ok {
-			return fmt.Errorf("transformChanges would lead to multiple changesets in repository %s to have the same branch %q", repo, g.Branch)
+			return batcheslib.NewValidationError(fmt.Errorf("transformChanges would lead to multiple changesets in repository %s to have the same branch %q", repoName, g.Branch))
 		} else {
 			uniqueBranches[g.Branch] = struct{}{}
 		}
 
 		if g.Branch == defaultBranch {
-			return fmt.Errorf("transformChanges group branch for repository %s is the same as branch %q in changesetTemplate", repo, defaultBranch)
+			return batcheslib.NewValidationError(fmt.Errorf("transformChanges group branch for repository %s is the same as branch %q in changesetTemplate", repoName, defaultBranch))
 		}
 	}
 

internal/batches/executor/changeset_specs_test.go

@@ -17,8 +17,8 @@ func TestCreateChangesetSpecs(t *testing.T) {
 	defaultChangesetSpec := &batcheslib.ChangesetSpec{
 		BaseRepository: testRepo1.ID,
 
-		BaseRef:        testRepo1.DefaultBranch.Name,
-		BaseRev:        testRepo1.DefaultBranch.Target.OID,
+		BaseRef:        testRepo1.BaseRef(),
+		BaseRev:        testRepo1.Rev(),
 		HeadRepository: testRepo1.ID,
 		HeadRef:        "refs/heads/my-branch",
 		Title:          "The title",

internal/batches/executor/coordinator.go

@@ -10,11 +10,13 @@ import (
 	"github.com/cockroachdb/errors"
 	"github.com/hashicorp/go-multierror"
 	batcheslib "github.com/sourcegraph/sourcegraph/lib/batches"
+
 	"github.com/sourcegraph/src-cli/internal/api"
 	"github.com/sourcegraph/src-cli/internal/batches"
 	"github.com/sourcegraph/src-cli/internal/batches/docker"
 	"github.com/sourcegraph/src-cli/internal/batches/graphql"
 	"github.com/sourcegraph/src-cli/internal/batches/log"
+	"github.com/sourcegraph/src-cli/internal/batches/repozip"
 	"github.com/sourcegraph/src-cli/internal/batches/workspace"
 )
 
@@ -67,15 +69,14 @@ func NewCoordinator(opts NewCoordinatorOpts) *Coordinator {
 	logManager := log.NewManager(opts.TempDir, opts.KeepLogs)
 
 	exec := newExecutor(newExecutorOpts{
-		Fetcher:     batches.NewRepoFetcher(opts.Client, opts.CacheDir, opts.CleanArchives),
-		EnsureImage: opts.EnsureImage,
-		Creator:     opts.Creator,
-		Logger:      logManager,
-
-		AutoAuthorDetails: opts.Features.IncludeAutoAuthorDetails,
-		Parallelism:       opts.Parallelism,
-		Timeout:           opts.Timeout,
-		TempDir:           opts.TempDir,
+		RepoArchiveRegistry: repozip.NewArchiveRegistry(opts.Client, opts.CacheDir, opts.CleanArchives),
+		EnsureImage:         opts.EnsureImage,
+		Creator:             opts.Creator,
+		Logger:              logManager,
+
+		Parallelism: opts.Parallelism,
+		Timeout:     opts.Timeout,
+		TempDir:     opts.TempDir,
 	})
 
 	return &Coordinator{
@@ -290,7 +291,7 @@ func (c *Coordinator) Execute(ctx context.Context, tasks []*Task, spec *batchesl
 			case float64:
 				sid = strconv.FormatFloat(tid, 'f', -1, 64)
 			default:
-				return nil, nil, errors.Errorf("cannot convert value of type %T into a valid external ID: expected string or int", id)
+				return nil, nil, batcheslib.NewValidationError(errors.Errorf("cannot convert value of type %T into a valid external ID: expected string or int", id))
 			}
 
 			specs = append(specs, &batcheslib.ChangesetSpec{

internal/batches/executor/coordinator_test.go

@@ -17,6 +17,7 @@ import (
 
 	"github.com/sourcegraph/src-cli/internal/batches/graphql"
 	"github.com/sourcegraph/src-cli/internal/batches/mock"
+	"github.com/sourcegraph/src-cli/internal/batches/util"
 )
 
 func TestCoordinator_Execute(t *testing.T) {
@@ -31,7 +32,7 @@ func TestCoordinator_Execute(t *testing.T) {
 			BaseRef:        repo.BaseRef(),
 			BaseRev:        repo.Rev(),
 			HeadRepository: repo.ID,
-			HeadRef:        "refs/heads/" + testChangesetTemplate.Branch,
+			HeadRef:        util.EnsureRefPrefix(testChangesetTemplate.Branch),
 			Title:          testChangesetTemplate.Title,
 			Body:           testChangesetTemplate.Body,
 			Commits: []batcheslib.GitCommitDescription{
@@ -217,7 +218,7 @@ func TestCoordinator_Execute(t *testing.T) {
 					spec.Commits[0].Diff = nestedChangesDiffSubdirC
 				}),
 				buildSpecFor(testRepo2, func(spec *batcheslib.ChangesetSpec) {
-					spec.HeadRef = "refs/heads/" + testChangesetTemplate.Branch
+					spec.HeadRef = util.EnsureRefPrefix(testChangesetTemplate.Branch)
 					spec.Commits[0].Diff = nestedChangesDiffSubdirA
 				}),
 			},

internal/batches/executor/execution_cache.go

@@ -12,6 +12,7 @@ import (
 
 	"github.com/cockroachdb/errors"
 	batcheslib "github.com/sourcegraph/sourcegraph/lib/batches"
+	"github.com/sourcegraph/src-cli/internal/batches/util"
 )
 
 func UserCacheDir() (string, error) {
@@ -101,7 +102,9 @@ func (key StepsCacheKey) Key() (string, error) {
 	return fmt.Sprintf("%s-step-%d", hash, key.StepIndex), err
 }
 
-func (key StepsCacheKey) Slug() string { return key.Repository.Slug() }
+func (key StepsCacheKey) Slug() string {
+	return util.SlugForRepo(key.Repository.Name, key.Repository.Rev())
+}
 
 // TaskCacheKey implements the CacheKeyer interface for a Task and all its
 // Steps.
@@ -132,7 +135,9 @@ func (key TaskCacheKey) Key() (string, error) {
 	return base64.RawURLEncoding.EncodeToString(hash[:16]), nil
 }
 
-func (key TaskCacheKey) Slug() string { return key.Repository.Slug() }
+func (key TaskCacheKey) Slug() string {
+	return util.SlugForRepo(key.Repository.Name, key.Repository.Rev())
+}
 
 type ExecutionCache interface {
 	Get(ctx context.Context, key CacheKeyer) (result executionResult, found bool, err error)

internal/batches/executor/executor.go

@@ -10,8 +10,9 @@ import (
 	"github.com/cockroachdb/errors"
 	"github.com/neelance/parallel"
 
-	"github.com/sourcegraph/src-cli/internal/batches"
 	"github.com/sourcegraph/src-cli/internal/batches/log"
+	"github.com/sourcegraph/src-cli/internal/batches/repozip"
+	"github.com/sourcegraph/src-cli/internal/batches/util"
 	"github.com/sourcegraph/src-cli/internal/batches/workspace"
 )
 
@@ -50,16 +51,15 @@ type taskResult struct {
 
 type newExecutorOpts struct {
 	// Dependencies
-	Creator     workspace.Creator
-	Fetcher     batches.RepoFetcher
-	EnsureImage imageEnsurer
-	Logger      log.LogManager
+	Creator             workspace.Creator
+	RepoArchiveRegistry repozip.ArchiveRegistry
+	EnsureImage         imageEnsurer
+	Logger              log.LogManager
 
 	// Config
-	AutoAuthorDetails bool
-	Parallelism       int
-	Timeout           time.Duration
-	TempDir           string
+	Parallelism int
+	Timeout     time.Duration
+	TempDir     string
 }
 
 type executor struct {
@@ -144,7 +144,7 @@ func (x *executor) do(ctx context.Context, task *Task, ui TaskExecutionUI) (err
 	ui.TaskStarted(task)
 
 	// Let's set up our logging.
-	log, err := x.opts.Logger.AddTask(task.Repository.SlugForPath(task.Path))
+	log, err := x.opts.Logger.AddTask(util.SlugForPathInRepo(task.Repository.Name, task.Repository.Rev(), task.Path))
 	if err != nil {
 		return errors.Wrap(err, "creating log file")
 	}
@@ -160,8 +160,8 @@ func (x *executor) do(ctx context.Context, task *Task, ui TaskExecutionUI) (err
 		log.Close()
 	}()
 
-	// Now checkout the archive
-	task.Archive = x.opts.Fetcher.Checkout(task.Repository, task.ArchivePathToFetch())
+	// Now checkout the archive.
+	task.Archive = x.opts.RepoArchiveRegistry.Checkout(repozip.RepoRevision{RepoName: task.Repository.Name, Commit: task.Repository.Rev()}, task.ArchivePathToFetch())
 
 	// Set up our timeout.
 	runCtx, cancel := context.WithTimeout(ctx, x.opts.Timeout)