Support binary diffs (#887)

Patches can contain non UTF-8 characters (who knew). Since we JSON-encode the patch as a string field, we lose that encoding over the wire to Sourcegraph. This PR adds support for a byte slice encoded (so base64 over the wire) mode from Sourcegraph 4.4 onwards, which retains original encoding.

Author: eseliger

.tool-versions

@@ -1,3 +1,3 @@
-golang 1.18.1
+golang 1.19.3
 shfmt 3.2.0
 shellcheck 0.7.1

CHANGELOG.md

@@ -13,6 +13,16 @@ All notable changes to `src-cli` are documented in this file.
 
 ### Added
 
+### Changed
+
+### Fixed
+
+### Removed
+
+## 4.2.1
+
+### Added
+
 - Batch specs being run locally with `src batch preview` or `src batch apply` can now be run with the `-run-as-root` flag, which will run all step containers as root instead of the default user for the image. This is off by default. [#886](https://github.com/sourcegraph/src-cli/pull/886)
 
 ### Changed
@@ -21,7 +31,7 @@ All notable changes to `src-cli` are documented in this file.
 
 ### Fixed
 
-### Removed
+- Batch changes: Git patches are now binary encoded instead of UTF-8 over the wire, fixing support for non-UTF-8 files. [#887](https://github.com/sourcegraph/src-cli/pull/887)
 
 ## 4.2.0
 

cmd/src/batch_apply.go

@@ -5,10 +5,7 @@ import (
 	"flag"
 	"fmt"
 
-	"github.com/sourcegraph/src-cli/internal/batches/ui"
 	"github.com/sourcegraph/src-cli/internal/cmderrors"
-
-	"github.com/sourcegraph/sourcegraph/lib/output"
 )
 
 func init() {
@@ -47,15 +44,7 @@ Examples:
 		ctx, cancel := contextCancelOnInterrupt(context.Background())
 		defer cancel()
 
-		var execUI ui.ExecUI
-		if flags.textOnly {
-			execUI = &ui.JSONLines{}
-		} else {
-			out := output.NewOutput(flagSet.Output(), output.OutputOpts{Verbose: *verbose})
-			execUI = &ui.TUI{Out: out}
-		}
-
-		if err = executeBatchSpec(ctx, execUI, executeBatchSpecOpts{
+		if err = executeBatchSpec(ctx, executeBatchSpecOpts{
 			flags:  flags,
 			client: cfg.apiClient(flags.api, flagSet.Output()),
 			file:   file,

cmd/src/batch_common.go

@@ -18,6 +18,7 @@ import (
 	"github.com/mattn/go-isatty"
 
 	"github.com/sourcegraph/sourcegraph/lib/errors"
+	"github.com/sourcegraph/sourcegraph/lib/output"
 
 	batcheslib "github.com/sourcegraph/sourcegraph/lib/batches"
 	"github.com/sourcegraph/sourcegraph/lib/batches/template"
@@ -253,20 +254,38 @@ type executeBatchSpecOpts struct {
 // executeBatchSpec performs all the steps required to upload the batch spec to
 // Sourcegraph, including execution as needed and applying the resulting batch
 // spec if specified.
-func executeBatchSpec(ctx context.Context, ui ui.ExecUI, opts executeBatchSpecOpts) (err error) {
+func executeBatchSpec(ctx context.Context, opts executeBatchSpecOpts) (err error) {
+	var execUI ui.ExecUI
+	if opts.flags.textOnly {
+		execUI = &ui.JSONLines{}
+	} else {
+		out := output.NewOutput(os.Stderr, output.OutputOpts{Verbose: *verbose})
+		execUI = &ui.TUI{Out: out}
+	}
+
 	defer func() {
 		if err != nil {
-			ui.ExecutionError(err)
+			execUI.ExecutionError(err)
 		}
 	}()
 
 	svc := service.New(&service.Opts{
 		Client: opts.client,
 	})
 
+	ffs, err := svc.DetermineFeatureFlags(ctx)
+	if err != nil {
+		return err
+	}
+
+	// Once we know about feature flags, reconfigure the UI if needed.
+	if opts.flags.textOnly && ffs.BinaryDiffs {
+		execUI = &ui.JSONLines{BinaryDiffs: true}
+	}
+
 	imageCache := docker.NewImageCache()
 
-	if err := validateSourcegraphVersionConstraint(ctx, svc); err != nil {
+	if err := validateSourcegraphVersionConstraint(ctx, ffs); err != nil {
 		return err
 	}
 
@@ -309,45 +328,45 @@ func executeBatchSpec(ctx context.Context, ui ui.ExecUI, opts executeBatchSpecOp
 	}
 
 	// Parse flags and build up our service and executor options.
-	ui.ParsingBatchSpec()
+	execUI.ParsingBatchSpec()
 	batchSpec, batchSpecDir, rawSpec, err := parseBatchSpec(ctx, opts.file, svc)
 	if err != nil {
 		var multiErr errors.MultiError
 		if errors.As(err, &multiErr) {
-			ui.ParsingBatchSpecFailure(multiErr)
+			execUI.ParsingBatchSpecFailure(multiErr)
 			return cmderrors.ExitCode(2, nil)
 		} else {
 			// This shouldn't happen; let's just punt and let the normal
 			// rendering occur.
 			return err
 		}
 	}
-	ui.ParsingBatchSpecSuccess()
+	execUI.ParsingBatchSpecSuccess()
 
-	ui.ResolvingNamespace()
+	execUI.ResolvingNamespace()
 	namespace, err := svc.ResolveNamespace(ctx, opts.flags.namespace)
 	if err != nil {
 		return err
 	}
-	ui.ResolvingNamespaceSuccess(namespace.ID)
+	execUI.ResolvingNamespaceSuccess(namespace.ID)
 
 	var workspaceCreator workspace.Creator
 
 	if len(batchSpec.Steps) > 0 {
-		ui.PreparingContainerImages()
+		execUI.PreparingContainerImages()
 		images, err := svc.EnsureDockerImages(
 			ctx,
 			imageCache,
 			batchSpec.Steps,
 			parallelism,
-			ui.PreparingContainerImagesProgress,
+			execUI.PreparingContainerImagesProgress,
 		)
 		if err != nil {
 			return err
 		}
-		ui.PreparingContainerImagesSuccess()
+		execUI.PreparingContainerImagesSuccess()
 
-		ui.DeterminingWorkspaceCreatorType()
+		execUI.DeterminingWorkspaceCreatorType()
 		var typ workspace.CreatorType
 		workspaceCreator, typ = workspace.NewCreator(ctx, opts.flags.workspace, opts.flags.cacheDir, opts.flags.tempDir, images)
 		if typ == workspace.CreatorTypeVolume {
@@ -357,21 +376,21 @@ func executeBatchSpec(ctx context.Context, ui ui.ExecUI, opts executeBatchSpecOp
 				return err
 			}
 		}
-		ui.DeterminingWorkspaceCreatorTypeSuccess(typ)
+		execUI.DeterminingWorkspaceCreatorTypeSuccess(typ)
 	}
 
-	ui.DeterminingWorkspaces()
+	execUI.DeterminingWorkspaces()
 	workspaces, repos, err := svc.ResolveWorkspacesForBatchSpec(ctx, batchSpec, opts.flags.allowUnsupported, opts.flags.allowIgnored)
 	if err != nil {
 		if repoSet, ok := err.(batches.UnsupportedRepoSet); ok {
-			ui.DeterminingWorkspacesSuccess(len(workspaces), len(repos), repoSet, nil)
+			execUI.DeterminingWorkspacesSuccess(len(workspaces), len(repos), repoSet, nil)
 		} else if repoSet, ok := err.(batches.IgnoredRepoSet); ok {
-			ui.DeterminingWorkspacesSuccess(len(workspaces), len(repos), nil, repoSet)
+			execUI.DeterminingWorkspacesSuccess(len(workspaces), len(repos), nil, repoSet)
 		} else {
 			return errors.Wrap(err, "resolving repositories")
 		}
 	} else {
-		ui.DeterminingWorkspacesSuccess(len(workspaces), len(repos), nil, nil)
+		execUI.DeterminingWorkspacesSuccess(len(workspaces), len(repos), nil, nil)
 	}
 
 	archiveRegistry := repozip.NewArchiveRegistry(opts.client, opts.flags.cacheDir, opts.flags.cleanArchives)
@@ -389,14 +408,16 @@ func executeBatchSpec(ctx context.Context, ui ui.ExecUI, opts executeBatchSpecOp
 				TempDir:             opts.flags.tempDir,
 				GlobalEnv:           os.Environ(),
 				ForceRoot:           opts.flags.runAsRoot,
+				BinaryDiffs:         ffs.BinaryDiffs,
 			},
-			Logger:    logManager,
-			Cache:     executor.NewDiskCache(opts.flags.cacheDir),
-			GlobalEnv: os.Environ(),
+			Logger:      logManager,
+			Cache:       executor.NewDiskCache(opts.flags.cacheDir),
+			BinaryDiffs: ffs.BinaryDiffs,
+			GlobalEnv:   os.Environ(),
 		},
 	)
 
-	ui.CheckingCache()
+	execUI.CheckingCache()
 	tasks := svc.BuildTasks(
 		&template.BatchChangeAttributes{
 			Name:        batchSpec.Name,
@@ -421,9 +442,9 @@ func executeBatchSpec(ctx context.Context, ui ui.ExecUI, opts executeBatchSpecOp
 			return err
 		}
 	}
-	ui.CheckingCacheSuccess(len(specs), len(uncachedTasks))
+	execUI.CheckingCacheSuccess(len(specs), len(uncachedTasks))
 
-	taskExecUI := ui.ExecutingTasks(*verbose, parallelism)
+	taskExecUI := execUI.ExecutingTasks(*verbose, parallelism)
 	freshSpecs, logFiles, execErr := coord.ExecuteAndBuildSpecs(ctx, batchSpec, uncachedTasks, taskExecUI)
 	// Add external changeset specs.
 	importedSpecs, importErr := svc.CreateImportChangesetSpecs(ctx, batchSpec)
@@ -440,7 +461,7 @@ func executeBatchSpec(ctx context.Context, ui ui.ExecUI, opts executeBatchSpecOp
 		if err == nil {
 			taskExecUI.Success()
 		} else {
-			ui.ExecutingTasksSkippingErrors(err)
+			execUI.ExecutingTasksSkippingErrors(err)
 		}
 	} else {
 		if err != nil {
@@ -450,7 +471,7 @@ func executeBatchSpec(ctx context.Context, ui ui.ExecUI, opts executeBatchSpecOp
 	}
 
 	if len(logFiles) > 0 && opts.flags.keepLogs {
-		ui.LogFilesKept(logFiles)
+		execUI.LogFilesKept(logFiles)
 	}
 
 	specs = append(specs, freshSpecs...)
@@ -464,29 +485,29 @@ func executeBatchSpec(ctx context.Context, ui ui.ExecUI, opts executeBatchSpecOp
 	ids := make([]graphql.ChangesetSpecID, len(specs))
 
 	if len(specs) > 0 {
-		ui.UploadingChangesetSpecs(len(specs))
+		execUI.UploadingChangesetSpecs(len(specs))
 
 		for i, spec := range specs {
 			id, err := svc.CreateChangesetSpec(ctx, spec)
 			if err != nil {
 				return err
 			}
 			ids[i] = id
-			ui.UploadingChangesetSpecsProgress(i+1, len(specs))
+			execUI.UploadingChangesetSpecsProgress(i+1, len(specs))
 		}
 
-		ui.UploadingChangesetSpecsSuccess(ids)
+		execUI.UploadingChangesetSpecsSuccess(ids)
 	} else if len(repos) == 0 {
-		ui.NoChangesetSpecs()
+		execUI.NoChangesetSpecs()
 	}
 
-	ui.CreatingBatchSpec()
+	execUI.CreatingBatchSpec()
 	id, url, err := svc.CreateBatchSpec(ctx, namespace.ID, rawSpec, ids)
 	if err != nil {
-		return ui.CreatingBatchSpecError(err)
+		return execUI.CreatingBatchSpecError(err)
 	}
 	previewURL := cfg.Endpoint + url
-	ui.CreatingBatchSpecSuccess(previewURL)
+	execUI.CreatingBatchSpecSuccess(previewURL)
 
 	hasWorkspaceFiles := false
 	for _, step := range batchSpec.Steps {
@@ -496,26 +517,26 @@ func executeBatchSpec(ctx context.Context, ui ui.ExecUI, opts executeBatchSpecOp
 		}
 	}
 	if hasWorkspaceFiles {
-		ui.UploadingWorkspaceFiles()
+		execUI.UploadingWorkspaceFiles()
 		if err = svc.UploadBatchSpecWorkspaceFiles(ctx, batchSpecDir, string(id), batchSpec.Steps); err != nil {
 			// Since failing to upload workspace files should not stop processing, just warn
-			ui.UploadingWorkspaceFilesWarning(errors.Wrap(err, "uploading workspace files"))
+			execUI.UploadingWorkspaceFilesWarning(errors.Wrap(err, "uploading workspace files"))
 		} else {
-			ui.UploadingWorkspaceFilesSuccess()
+			execUI.UploadingWorkspaceFilesSuccess()
 		}
 	}
 
 	if !opts.applyBatchSpec {
-		ui.PreviewBatchSpec(previewURL)
+		execUI.PreviewBatchSpec(previewURL)
 		return
 	}
 
-	ui.ApplyingBatchSpec()
+	execUI.ApplyingBatchSpec()
 	batch, err := svc.ApplyBatchChange(ctx, id)
 	if err != nil {
 		return err
 	}
-	ui.ApplyingBatchSpecSuccess(cfg.Endpoint + batch.URL)
+	execUI.ApplyingBatchSpecSuccess(cfg.Endpoint + batch.URL)
 
 	return nil
 }
@@ -617,11 +638,7 @@ func getBatchParallelism(ctx context.Context, flag int) (int, error) {
 	return docker.NCPU(ctx)
 }
 
-func validateSourcegraphVersionConstraint(ctx context.Context, svc *service.Service) error {
-	ffs, err := svc.DetermineFeatureFlags(ctx)
-	if err != nil {
-		return err
-	}
+func validateSourcegraphVersionConstraint(ctx context.Context, ffs *batches.FeatureFlags) error {
 	if ffs.Sourcegraph40 {
 		return nil
 	}

cmd/src/batch_exec.go

@@ -37,6 +37,7 @@ type executorModeFlags struct {
 	tempDir           string
 	repoDir           string
 	workspaceFilesDir string
+	binaryDiffs       bool
 }
 
 func newExecutorModeFlags(flagSet *flag.FlagSet) (f *executorModeFlags) {
@@ -47,6 +48,7 @@ func newExecutorModeFlags(flagSet *flag.FlagSet) (f *executorModeFlags) {
 	flagSet.StringVar(&f.tempDir, "tmp", "", "Directory for storing temporary data.")
 	flagSet.StringVar(&f.repoDir, "repo", "", "Path of the checked out repo on disk.")
 	flagSet.StringVar(&f.workspaceFilesDir, "workspaceFiles", "", "Path of workspace files on disk.")
+	flagSet.BoolVar(&f.binaryDiffs, "binaryDiffs", false, "Whether to encode diffs as base64.")
 
 	return f
 }
@@ -121,7 +123,7 @@ Examples:
 }
 
 func executeBatchSpecInWorkspaces(ctx context.Context, flags *executorModeFlags) (err error) {
-	ui := &ui.JSONLines{}
+	ui := &ui.JSONLines{BinaryDiffs: flags.binaryDiffs}
 
 	// Ensure the temp dir exists.
 	tempDir := flags.tempDir
@@ -211,6 +213,7 @@ func executeBatchSpecInWorkspaces(ctx context.Context, flags *executorModeFlags)
 		RepoArchive:      &repozip.NoopArchive{},
 		UI:               taskExecUI.StepsExecutionUI(task),
 		ForceRoot:        !flags.runAsImageUser,
+		BinaryDiffs:      flags.binaryDiffs,
 	}
 	results, err := executor.RunSteps(ctx, opts)
 

cmd/src/batch_new.go

@@ -48,7 +48,12 @@ Examples:
 			Client: cfg.apiClient(apiFlags, flagSet.Output()),
 		})
 
-		if err := validateSourcegraphVersionConstraint(ctx, svc); err != nil {
+		ffs, err := svc.DetermineFeatureFlags(ctx)
+		if err != nil {
+			return err
+		}
+
+		if err := validateSourcegraphVersionConstraint(ctx, ffs); err != nil {
 			return err
 		}
 

cmd/src/batch_preview.go

@@ -5,10 +5,7 @@ import (
 	"flag"
 	"fmt"
 
-	"github.com/sourcegraph/src-cli/internal/batches/ui"
 	"github.com/sourcegraph/src-cli/internal/cmderrors"
-
-	"github.com/sourcegraph/sourcegraph/lib/output"
 )
 
 func init() {
@@ -45,15 +42,7 @@ Examples:
 		ctx, cancel := contextCancelOnInterrupt(context.Background())
 		defer cancel()
 
-		var execUI ui.ExecUI
-		if flags.textOnly {
-			execUI = &ui.JSONLines{}
-		} else {
-			out := output.NewOutput(flagSet.Output(), output.OutputOpts{Verbose: *verbose})
-			execUI = &ui.TUI{Out: out}
-		}
-
-		if err = executeBatchSpec(ctx, execUI, executeBatchSpecOpts{
+		if err = executeBatchSpec(ctx, executeBatchSpecOpts{
 			flags:  flags,
 			client: cfg.apiClient(flags.api, flagSet.Output()),
 			file:   file,