Merge branch 'pks-operations-drop-git2go-merge-gob-ff' into 'master'

operations: Always return structured errors from Git2Go merges

Closes #3756

See merge request gitlab-org/gitaly!3896

2 files changed, 51 insertions, 104 deletions

diff --git a/cmd/gitaly-git2go/merge.go b/cmd/gitaly-git2go/merge.go
index 8d57cb16d..316e49714 100644
--- a/cmd/gitaly-git2go/merge.go
+++ b/cmd/gitaly-git2go/merge.go
@@ -18,27 +18,16 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git2go"
 )
 
-type mergeSubcommand struct {
-	request string
-}
+type mergeSubcommand struct{}
 
 func (cmd *mergeSubcommand) Flags() *flag.FlagSet {
 	flags := flag.NewFlagSet("merge", flag.ExitOnError)
-	flags.StringVar(&cmd.request, "request", "", "git2go.MergeCommand")
 	return flags
 }
 
 func (cmd *mergeSubcommand) Run(_ context.Context, r io.Reader, w io.Writer) error {
-	useGob := cmd.request == ""
-
-	var err error
 	var request git2go.MergeCommand
-	if useGob {
-		err = gob.NewDecoder(r).Decode(&request)
-	} else {
-		request, err = git2go.MergeCommandFromSerialized(cmd.request)
-	}
-	if err != nil {
+	if err := gob.NewDecoder(r).Decode(&request); err != nil {
 		return err
 	}
 
@@ -46,30 +35,15 @@ func (cmd *mergeSubcommand) Run(_ context.Context, r io.Reader, w io.Writer) err
 		request.AuthorDate = time.Now()
 	}
 
-	commitID, err := merge(request, useGob)
-	if useGob {
-		return gob.NewEncoder(w).Encode(git2go.Result{
-			CommitID: commitID,
-			Error:    git2go.SerializableError(err),
-		})
-	}
+	commitID, err := merge(request)
 
-	if err != nil {
-		return err
-	}
-
-	response := git2go.MergeResult{
+	return gob.NewEncoder(w).Encode(git2go.Result{
 		CommitID: commitID,
-	}
-
-	if err := response.SerializeTo(w); err != nil {
-		return err
-	}
-
-	return nil
+		Error:    git2go.SerializableError(err),
+	})
 }
 
-func merge(request git2go.MergeCommand, useStructuredErrors bool) (string, error) {
+func merge(request git2go.MergeCommand) (string, error) {
 	repo, err := git2goutil.OpenRepository(request.Repository)
 	if err != nil {
 		return "", fmt.Errorf("could not open repository: %w", err)
@@ -100,21 +74,17 @@ func merge(request git2go.MergeCommand, useStructuredErrors bool) (string, error
 
 	if index.HasConflicts() {
 		if !request.AllowConflicts {
-			if useStructuredErrors {
-				conflictingFiles, err := getConflictingFiles(index)
-				if err != nil {
-					return "", fmt.Errorf("getting conflicting files: %w", err)
-				}
-
-				return "", git2go.ConflictingFilesError{
-					ConflictingFiles: conflictingFiles,
-				}
+			conflictingFiles, err := getConflictingFiles(index)
+			if err != nil {
+				return "", fmt.Errorf("getting conflicting files: %w", err)
 			}
 
-			return "", errors.New("could not auto-merge due to conflicts")
+			return "", git2go.ConflictingFilesError{
+				ConflictingFiles: conflictingFiles,
+			}
 		}
 
-		if err := resolveConflicts(repo, index, useStructuredErrors); err != nil {
+		if err := resolveConflicts(repo, index); err != nil {
 			return "", fmt.Errorf("could not resolve conflicts: %w", err)
 		}
 	}
@@ -133,7 +103,7 @@ func merge(request git2go.MergeCommand, useStructuredErrors bool) (string, error
 	return commit.String(), nil
 }
 
-func resolveConflicts(repo *git.Repository, index *git.Index, useStructuredErrors bool) error {
+func resolveConflicts(repo *git.Repository, index *git.Index) error {
 	// We need to get all conflicts up front as resolving conflicts as we
 	// iterate breaks the iterator.
 	indexConflicts, err := getConflicts(index)
@@ -194,18 +164,14 @@ func resolveConflicts(repo *git.Repository, index *git.Index, useStructuredError
 	}
 
 	if index.HasConflicts() {
-		if useStructuredErrors {
-			conflictingFiles, err := getConflictingFiles(index)
-			if err != nil {
-				return fmt.Errorf("getting conflicting files: %w", err)
-			}
-
-			return git2go.ConflictingFilesError{
-				ConflictingFiles: conflictingFiles,
-			}
+		conflictingFiles, err := getConflictingFiles(index)
+		if err != nil {
+			return fmt.Errorf("getting conflicting files: %w", err)
 		}
 
-		return errors.New("index still has conflicts")
+		return git2go.ConflictingFilesError{
+			ConflictingFiles: conflictingFiles,
+		}
 	}
 
 	return nil
diff --git a/cmd/gitaly-git2go/merge_test.go b/cmd/gitaly-git2go/merge_test.go
index 99aca86d4..d12937e98 100644
--- a/cmd/gitaly-git2go/merge_test.go
+++ b/cmd/gitaly-git2go/merge_test.go
@@ -4,8 +4,6 @@
 package main
 
 import (
-	"context"
-	"errors"
 	"fmt"
 	"testing"
 	"time"
@@ -18,18 +16,16 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
 )
 
-func TestMergeFailsWithMissingArguments(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.Git2GoMergeGob,
-	}).Run(t, testMergeFailsWithMissingArguments)
-}
+func TestMerge_missingArguments(t *testing.T) {
+	t.Parallel()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testMergeFailsWithMissingArguments(t *testing.T, ctx context.Context) {
 	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
 	executor := git2go.NewExecutor(cfg, config.NewLocator(cfg))
 
@@ -83,13 +79,12 @@ func testMergeFailsWithMissingArguments(t *testing.T, ctx context.Context) {
 	}
 }
 
-func TestMergeFailsWithInvalidRepositoryPath(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.Git2GoMergeGob,
-	}).Run(t, testMergeFailsWithInvalidRepositoryPath)
-}
+func TestMerge_invalidRepositoryPath(t *testing.T) {
+	t.Parallel()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testMergeFailsWithInvalidRepositoryPath(t *testing.T, ctx context.Context) {
 	cfg, repo, _ := testcfg.BuildWithRepo(t)
 	testhelper.BuildGitalyGit2Go(t, cfg)
 	executor := git2go.NewExecutor(cfg, config.NewLocator(cfg))
@@ -101,22 +96,20 @@ func testMergeFailsWithInvalidRepositoryPath(t *testing.T, ctx context.Context)
 	require.Contains(t, err.Error(), "merge: could not open repository")
 }
 
-func TestMergeTrees(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.Git2GoMergeGob,
-	}).Run(t, testMergeTrees)
-}
+func TestMerge_trees(t *testing.T) {
+	t.Parallel()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testMergeTrees(t *testing.T, ctx context.Context) {
 	testcases := []struct {
-		desc                  string
-		base                  map[string]string
-		ours                  map[string]string
-		theirs                map[string]string
-		expected              map[string]string
-		expectedResponse      git2go.MergeResult
-		expectedErr           error
-		expectedErrWithoutGob error
+		desc             string
+		base             map[string]string
+		ours             map[string]string
+		theirs           map[string]string
+		expected         map[string]string
+		expectedResponse git2go.MergeResult
+		expectedErr      error
 	}{
 		{
 			desc: "trivial merge succeeds",
@@ -194,8 +187,6 @@ func testMergeTrees(t *testing.T, ctx context.Context) {
 			expectedErr: fmt.Errorf("merge: %w", git2go.ConflictingFilesError{
 				ConflictingFiles: []string{"1"},
 			}),
-			//nolint:revive
-			expectedErrWithoutGob: errors.New("merge: could not auto-merge due to conflicts\n"),
 		},
 	}
 
@@ -222,11 +213,7 @@ func testMergeTrees(t *testing.T, ctx context.Context) {
 			})
 
 			if tc.expectedErr != nil {
-				if featureflag.Git2GoMergeGob.IsEnabled(ctx) {
-					require.Equal(t, tc.expectedErr, err)
-				} else {
-					require.Equal(t, tc.expectedErrWithoutGob, err)
-				}
+				require.Equal(t, tc.expectedErr, err)
 				return
 			}
 
@@ -260,12 +247,11 @@ func testMergeTrees(t *testing.T, ctx context.Context) {
 }
 
 func TestMerge_recursive(t *testing.T) {
-	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
-		featureflag.Git2GoMergeGob,
-	}).Run(t, testMergeRecursive)
-}
+	t.Parallel()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
 
-func testMergeRecursive(t *testing.T, ctx context.Context) {
 	cfg := testcfg.Build(t)
 	testhelper.BuildGitalyGit2Go(t, cfg)
 	executor := git2go.NewExecutor(cfg, config.NewLocator(cfg))
@@ -332,14 +318,9 @@ func testMergeRecursive(t *testing.T, ctx context.Context) {
 		Ours:       ours[len(ours)-1].String(),
 		Theirs:     theirs[len(theirs)-1].String(),
 	})
-	if featureflag.Git2GoMergeGob.IsEnabled(ctx) {
-		require.Equal(t, fmt.Errorf("merge: %w", git2go.ConflictingFilesError{
-			ConflictingFiles: []string{"theirs"},
-		}), err)
-	} else {
-		//nolint:revive
-		require.Equal(t, errors.New("merge: could not auto-merge due to conflicts\n"), err)
-	}
+	require.Equal(t, fmt.Errorf("merge: %w", git2go.ConflictingFilesError{
+		ConflictingFiles: []string{"theirs"},
+	}), err)
 
 	// Otherwise, if we're merging an earlier criss-cross merge which has
 	// half of the limit many criss-cross patterns, we exactly hit the