Merge branch 'remove-ff-squash-using-merge' into 'master'

operations: Remove SquashUsingMerge feature flag

See merge request gitlab-org/gitaly!4563

3 files changed, 86 insertions, 247 deletions

diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go
index 16bc61e1c..efa787026 100644
--- a/internal/gitaly/service/operations/squash.go
+++ b/internal/gitaly/service/operations/squash.go
@@ -1,18 +1,14 @@
 package operations
 
 import (
-	"bytes"
 	"context"
 	"errors"
-	"fmt"
 	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
@@ -139,138 +135,60 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 		return "", helper.ErrInvalidArgument(err)
 	}
 
-	var commitID string
-	if featureflag.SquashUsingMerge.IsEnabled(ctx) {
-		message := string(req.GetCommitMessage())
-		// In previous implementation, we've used git commit-tree to create commit.
-		// When message wasn't empty and didn't end in a new line,
-		// git commit-tree would add a trailing new line to the commit message.
-		// Let's keep that behaviour for compatibility.
-		if len(message) > 0 && !strings.HasSuffix(message, "\n") {
-			message += "\n"
-		}
-
-		merge, err := s.git2goExecutor.Merge(ctx, quarantineRepo, git2go.MergeCommand{
-			Repository:    quarantineRepoPath,
-			AuthorName:    string(req.GetAuthor().GetName()),
-			AuthorMail:    string(req.GetAuthor().GetEmail()),
-			AuthorDate:    commitDate,
-			CommitterName: string(req.GetUser().Name),
-			CommitterMail: string(req.GetUser().Email),
-			CommitterDate: commitDate,
-			Message:       message,
-			Ours:          startCommit.String(),
-			Theirs:        endCommit.String(),
-			Squash:        true,
-		})
-		if err != nil {
-			var conflictErr git2go.ConflictingFilesError
-
-			if errors.As(err, &conflictErr) {
-				conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles))
-				for _, conflictingFile := range conflictErr.ConflictingFiles {
-					conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
-				}
-
-				detailedErr, err := helper.ErrWithDetails(
-					helper.ErrFailedPreconditionf("squashing commits: %w", err),
-					&gitalypb.UserSquashError{
-						Error: &gitalypb.UserSquashError_RebaseConflict{
-							RebaseConflict: &gitalypb.MergeConflictError{
-								ConflictingFiles: conflictingFiles,
-							},
-						},
-					},
-				)
-				if err != nil {
-					return "", helper.ErrInternalf("error details: %w", err)
-				}
+	message := string(req.GetCommitMessage())
+	// In previous implementation, we've used git commit-tree to create commit.
+	// When message wasn't empty and didn't end in a new line,
+	// git commit-tree would add a trailing new line to the commit message.
+	// Let's keep that behaviour for compatibility.
+	if len(message) > 0 && !strings.HasSuffix(message, "\n") {
+		message += "\n"
+	}
+
+	// Perform squash merge onto endCommit using git2go merge.
+	merge, err := s.git2goExecutor.Merge(ctx, quarantineRepo, git2go.MergeCommand{
+		Repository:    quarantineRepoPath,
+		AuthorName:    string(req.GetAuthor().GetName()),
+		AuthorMail:    string(req.GetAuthor().GetEmail()),
+		AuthorDate:    commitDate,
+		CommitterName: string(req.GetUser().Name),
+		CommitterMail: string(req.GetUser().Email),
+		CommitterDate: commitDate,
+		Message:       message,
+		Ours:          startCommit.String(),
+		Theirs:        endCommit.String(),
+		Squash:        true,
+	})
+	if err != nil {
+		var conflictErr git2go.ConflictingFilesError
 
-				return "", detailedErr
+		if errors.As(err, &conflictErr) {
+			conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles))
+			for _, conflictingFile := range conflictErr.ConflictingFiles {
+				conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
 			}
-		}
 
-		commitID = merge.CommitID
-	} else {
-		// We're now rebasing the end commit on top of the start commit. The resulting tree
-		// is then going to be the tree of the squashed commit.
-		rebasedCommitID, err := s.git2goExecutor.Rebase(ctx, quarantineRepo, git2go.RebaseCommand{
-			Repository: quarantineRepoPath,
-			Committer: git2go.NewSignature(
-				string(req.GetUser().Name), string(req.GetUser().Email), commitDate,
-			),
-			CommitID:         endCommit,
-			UpstreamCommitID: startCommit,
-			SkipEmptyCommits: true,
-		})
-		if err != nil {
-			var conflictErr git2go.ConflictingFilesError
-
-			if errors.As(err, &conflictErr) {
-				conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles))
-				for _, conflictingFile := range conflictErr.ConflictingFiles {
-					conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
-				}
-
-				detailedErr, err := helper.ErrWithDetails(
-					helper.ErrFailedPreconditionf("rebasing commits: %w", err),
-					&gitalypb.UserSquashError{
-						Error: &gitalypb.UserSquashError_RebaseConflict{
-							RebaseConflict: &gitalypb.MergeConflictError{
-								ConflictingFiles: conflictingFiles,
-							},
+			detailedErr, err := helper.ErrWithDetails(
+				helper.ErrFailedPreconditionf("squashing commits: %w", err),
+				&gitalypb.UserSquashError{
+					// Note: this is actually a merge conflict, but we've kept
+					// the old "rebase" name for compatibility reasons.
+					Error: &gitalypb.UserSquashError_RebaseConflict{
+						RebaseConflict: &gitalypb.MergeConflictError{
+							ConflictingFiles: conflictingFiles,
 						},
 					},
-				)
-				if err != nil {
-					return "", helper.ErrInternalf("error details: %w", err)
-				}
-
-				return "", detailedErr
+				},
+			)
+			if err != nil {
+				return "", helper.ErrInternalf("error details: %w", err)
 			}
 
-			return "", helper.ErrInternalf("rebasing commits: %w", err)
-		}
-
-		treeID, err := quarantineRepo.ResolveRevision(ctx, rebasedCommitID.Revision()+"^{tree}")
-		if err != nil {
-			return "", fmt.Errorf("cannot resolve rebased tree: %w", err)
-		}
-
-		commitEnv := []string{
-			"GIT_COMMITTER_NAME=" + string(req.GetUser().Name),
-			"GIT_COMMITTER_EMAIL=" + string(req.GetUser().Email),
-			fmt.Sprintf("GIT_COMMITTER_DATE=%d %s", commitDate.Unix(), commitDate.Format("-0700")),
-			"GIT_AUTHOR_NAME=" + string(req.GetAuthor().Name),
-			"GIT_AUTHOR_EMAIL=" + string(req.GetAuthor().Email),
-			fmt.Sprintf("GIT_AUTHOR_DATE=%d %s", commitDate.Unix(), commitDate.Format("-0700")),
-		}
-
-		flags := []git.Option{
-			git.ValueFlag{
-				Name:  "-m",
-				Value: string(req.GetCommitMessage()),
-			},
-			git.ValueFlag{
-				Name:  "-p",
-				Value: startCommit.String(),
-			},
-		}
-
-		var stdout, stderr bytes.Buffer
-		if err := quarantineRepo.ExecAndWait(ctx, git.SubCmd{
-			Name:  "commit-tree",
-			Flags: flags,
-			Args: []string{
-				treeID.String(),
-			},
-		}, git.WithStdout(&stdout), git.WithStderr(&stderr), git.WithEnv(commitEnv...)); err != nil {
-			return "", helper.ErrInternalf("creating squashed commit: %w", err)
+			return "", detailedErr
 		}
-
-		commitID = text.ChompBytes(stdout.Bytes())
 	}
 
+	commitID := merge.CommitID
+
 	// The RPC is badly designed in that it never updates any references, but only creates the
 	// objects and writes them to disk. We still use a quarantine directory to stage the new
 	// objects, vote on them and migrate them into the main directory if quorum was reached so
diff --git a/internal/gitaly/service/operations/squash_test.go b/internal/gitaly/service/operations/squash_test.go
index e39b9edb2..7e6997562 100644
--- a/internal/gitaly/service/operations/squash_test.go
+++ b/internal/gitaly/service/operations/squash_test.go
@@ -17,7 +17,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata"
-	"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/testserver"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
@@ -42,11 +41,7 @@ var (
 func TestUserSquash_successful(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashSuccessful)
-}
-
-func testUserSquashSuccessful(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	for _, tc := range []struct {
 		desc             string
@@ -103,11 +98,7 @@ func testUserSquashSuccessful(t *testing.T, ctx context.Context) {
 func TestUserSquash_transactional(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashTransactional)
-}
-
-func testUserSquashTransactional(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 
 	txManager := transaction.MockManager{}
 
@@ -221,12 +212,7 @@ func testUserSquashTransactional(t *testing.T, ctx context.Context) {
 func TestUserSquash_stableID(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashStableID)
-}
-
-func testUserSquashStableID(t *testing.T, ctx context.Context) {
-	t.Parallel()
-
+	ctx := testhelper.Context(t)
 	ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -285,12 +271,7 @@ func ensureSplitIndexExists(t *testing.T, cfg config.Cfg, repoDir string) bool {
 func TestUserSquash_threeWayMerge(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashThreeWayMerge)
-}
-
-func testUserSquashThreeWayMerge(t *testing.T, ctx context.Context) {
-	t.Parallel()
-
+	ctx := testhelper.Context(t)
 	ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -324,12 +305,8 @@ func testUserSquashThreeWayMerge(t *testing.T, ctx context.Context) {
 
 func TestUserSquash_splitIndex(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashSplitIndex)
-}
-
-func testUserSquashSplitIndex(t *testing.T, ctx context.Context) {
-	t.Parallel()
 
+	ctx := testhelper.Context(t)
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	require.False(t, ensureSplitIndexExists(t, cfg, repoPath))
@@ -353,10 +330,7 @@ func testUserSquashSplitIndex(t *testing.T, ctx context.Context) {
 func TestUserSquash_renames(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashRenames)
-}
-
-func testUserSquashRenames(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	gittest.AddWorktree(t, cfg, repoPath, "worktree")
@@ -416,12 +390,7 @@ func testUserSquashRenames(t *testing.T, ctx context.Context) {
 func TestUserSquash_missingFileOnTargetBranch(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashMissingFileOnTargetBranch)
-}
-
-func testUserSquashMissingFileOnTargetBranch(t *testing.T, ctx context.Context) {
-	t.Parallel()
-
+	ctx := testhelper.Context(t)
 	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
 	conflictingStartSha := "bbd36ad238d14e1c03ece0f3358f545092dc9ca3"
@@ -444,17 +413,12 @@ func testUserSquashMissingFileOnTargetBranch(t *testing.T, ctx context.Context)
 func TestUserSquash_emptyCommit(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashEmptyCommit)
-}
-
-func testUserSquashEmptyCommit(t *testing.T, ctx context.Context) {
-	t.Parallel()
-
+	ctx := testhelper.Context(t)
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
 	// Set up history with two diverging lines of branches, where both sides have implemented
-	// the same changes. During rebase, the diff will thus become empty.
+	// the same changes. During merge, the diff will thus become empty.
 	base := gittest.WriteCommit(t, cfg, repoPath,
 		gittest.WithParents(), gittest.WithTreeEntries(
 			gittest.TreeEntry{Path: "a", Content: "base", Mode: "100644"},
@@ -648,11 +612,7 @@ func TestUserSquash_validation(t *testing.T) {
 func TestUserSquash_conflicts(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashConflicts)
-}
-
-func testUserSquashConflicts(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	base := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries(
@@ -679,13 +639,10 @@ func testUserSquashConflicts(t *testing.T, ctx context.Context) {
 		EndSha:        ours.String(),
 	})
 
-	expectedErr := helper.ErrFailedPreconditionf("rebasing commits: rebase: commit %q: there are conflicting files", ours)
-	if featureflag.SquashUsingMerge.IsEnabled(ctx) {
-		expectedErr = helper.ErrFailedPreconditionf("squashing commits: merge: there are conflicting files")
-	}
-
 	testhelper.RequireGrpcError(t, errWithDetails(t,
-		expectedErr,
+		helper.ErrFailedPreconditionf(
+			"squashing commits: merge: there are conflicting files",
+		),
 		&gitalypb.UserSquashError{
 			Error: &gitalypb.UserSquashError_RebaseConflict{
 				RebaseConflict: &gitalypb.MergeConflictError{
@@ -702,12 +659,7 @@ func testUserSquashConflicts(t *testing.T, ctx context.Context) {
 func TestUserSquash_ancestry(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashAncestry)
-}
-
-func testUserSquashAncestry(t *testing.T, ctx context.Context) {
-	t.Parallel()
-
+	ctx := testhelper.Context(t)
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	// We create an empty parent commit and two commits which both branch off from it. As a
@@ -741,11 +693,7 @@ func testUserSquashAncestry(t *testing.T, ctx context.Context) {
 func TestUserSquash_gitError(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashGitError)
-}
-
-func testUserSquashGitError(t *testing.T, ctx context.Context) {
-	t.Parallel()
+	ctx := testhelper.Context(t)
 	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
 	testCases := []struct {
@@ -834,12 +782,7 @@ func testUserSquashGitError(t *testing.T, ctx context.Context) {
 func TestUserSquash_squashingMerge(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashingMerge)
-}
-
-func testUserSquashingMerge(t *testing.T, ctx context.Context) {
-	t.Parallel()
-
+	ctx := testhelper.Context(t)
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	base := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("base"),
@@ -887,47 +830,30 @@ func testUserSquashingMerge(t *testing.T, ctx context.Context) {
 		Timestamp:     &timestamppb.Timestamp{Seconds: 1234512345},
 	})
 
-	if featureflag.SquashUsingMerge.IsEnabled(ctx) {
-		// With squashing using merge, we should successfully merge without any issues.
-		// The new detached commit history will look like this:
-		//
-		// HEAD o---o---o---o
-		//
-		// We have one commit from "base", two from "ours"
-		// and one squash commit that contains squashed changes from branch "theirs".
-		require.Nil(t, err)
-		testhelper.ProtoEqual(t, &gitalypb.UserSquashResponse{
-			SquashSha: "69d8db2439502c18b9c17c2d1bddb122a82bd448",
-		}, response)
-		gittest.RequireTree(t, cfg, repoPath, "69d8db2439502c18b9c17c2d1bddb122a82bd448", []gittest.TreeEntry{
-			{
-				// It should use the version from commit "oursMergedIntoTheirs",
-				// as it resolves the pre-existing conflict.
-				Content: "ours-content\ntheirs-content",
-				Mode:    "100644",
-				Path:    "a",
-			},
-			{
-				// This is the file that only existed on branch "ours".
-				Content: "new-content",
-				Mode:    "100644",
-				Path:    "ours-file",
-			},
-		})
-	} else {
-		// With the old method, in which we have to rebase first, we will re-encounter
-		// the already-resolved conflict and won't be able to perform the squash.
-		testhelper.RequireGrpcError(t, errWithDetails(t,
-			helper.ErrFailedPreconditionf("rebasing commits: rebase: commit %q: there are conflicting files", theirs),
-			&gitalypb.UserSquashError{
-				Error: &gitalypb.UserSquashError_RebaseConflict{
-					RebaseConflict: &gitalypb.MergeConflictError{
-						ConflictingFiles: [][]byte{
-							[]byte("a"),
-						},
-					},
-				},
-			},
-		), err)
-	}
+	// With squashing using merge, we should successfully merge without any issues.
+	// The new detached commit history will look like this:
+	//
+	// HEAD o---o---o---o
+	//
+	// We have one commit from "base", two from "ours"
+	// and one squash commit that contains squashed changes from branch "theirs".
+	require.Nil(t, err)
+	testhelper.ProtoEqual(t, &gitalypb.UserSquashResponse{
+		SquashSha: "69d8db2439502c18b9c17c2d1bddb122a82bd448",
+	}, response)
+	gittest.RequireTree(t, cfg, repoPath, "69d8db2439502c18b9c17c2d1bddb122a82bd448", []gittest.TreeEntry{
+		{
+			// It should use the version from commit "oursMergedIntoTheirs",
+			// as it resolves the pre-existing conflict.
+			Content: "ours-content\ntheirs-content",
+			Mode:    "100644",
+			Path:    "a",
+		},
+		{
+			// This is the file that only existed on branch "ours".
+			Content: "new-content",
+			Mode:    "100644",
+			Path:    "ours-file",
+		},
+	})
 }
diff --git a/internal/metadata/featureflag/ff_squash_using_merge.go b/internal/metadata/featureflag/ff_squash_using_merge.go
deleted file mode 100644
index 2ef265d4d..000000000
--- a/internal/metadata/featureflag/ff_squash_using_merge.go
+++ /dev/null
@@ -1,5 +0,0 @@
-package featureflag
-
-// SquashUsingMerge uses merge to squash commits instead of rebase and commit-tree.
-// It should be better at handling conflicts.
-var SquashUsingMerge = NewFeatureFlag("squash_using_merge", false)