operations: Call git plumbing implementation of commit files

Start calling the pure git plumbing implementation of UserCommitFiles
gated by a feature flag.

2 files changed, 387 insertions, 41 deletions

diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go
index 9cc254d6d..2f69130c4 100644
--- a/internal/gitaly/service/operations/commit_files.go
+++ b/internal/gitaly/service/operations/commit_files.go
@@ -7,10 +7,12 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"path/filepath"
 	"strings"
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"github.com/sirupsen/logrus"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/remoterepo"
@@ -125,6 +127,290 @@ func validatePath(rootPath, relPath string) (string, error) {
 	return path, nil
 }
 
+// applyAction applies an action to an TreeEntry.
+func applyAction(
+	ctx context.Context,
+	action git2go.Action,
+	root *localrepo.TreeEntry,
+	repo *localrepo.Repo,
+) error {
+	switch action := action.(type) {
+	case git2go.ChangeFileMode:
+		if err := root.Modify(
+			action.Path,
+			func(entry *localrepo.TreeEntry) error {
+				if action.ExecutableMode {
+					if entry.Mode != "100755" {
+						entry.Mode = "100755"
+					}
+				} else {
+					if entry.Mode == "100755" {
+						entry.Mode = "100644"
+					}
+				}
+
+				return nil
+			}); err != nil {
+			return translateToGit2GoError(err, action.Path)
+		}
+	case git2go.UpdateFile:
+		if err := root.Modify(
+			action.Path,
+			func(entry *localrepo.TreeEntry) error {
+				entry.OID = git.ObjectID(action.OID)
+				return nil
+			}); err != nil {
+			return translateToGit2GoError(err, action.Path)
+		}
+	case git2go.MoveFile:
+		entry, err := root.Get(action.Path)
+		if err != nil {
+			return translateToGit2GoError(err, action.Path)
+		}
+
+		if entry.Type != localrepo.Blob {
+			return git2go.IndexError{
+				Path: action.Path,
+				Type: git2go.ErrFileNotFound,
+			}
+		}
+
+		mode := entry.Mode
+
+		if action.OID == "" {
+			action.OID = string(entry.OID)
+		}
+
+		if err := root.Delete(action.Path); err != nil {
+			return translateToGit2GoError(err, action.Path)
+		}
+
+		if err := root.Add(
+			action.NewPath,
+			localrepo.TreeEntry{
+				OID:  git.ObjectID(action.OID),
+				Mode: mode,
+				Path: filepath.Base(action.NewPath),
+			},
+			localrepo.WithOverwriteDirectory(),
+		); err != nil {
+			return translateToGit2GoError(err, action.NewPath)
+		}
+	case git2go.CreateDirectory:
+		if entry, err := root.Get(action.Path); err != nil && !errors.Is(err, localrepo.ErrEntryNotFound) {
+			return translateToGit2GoError(err, action.Path)
+		} else if entry != nil {
+			switch entry.Type {
+			case localrepo.Tree, localrepo.Submodule:
+				return git2go.IndexError{
+					Path: action.Path,
+					Type: git2go.ErrDirectoryExists,
+				}
+			default:
+				return git2go.IndexError{
+					Path: action.Path,
+					Type: git2go.ErrFileExists,
+				}
+			}
+		}
+
+		blobID, err := repo.WriteBlob(ctx, filepath.Join(action.Path, ".gitkeep"), strings.NewReader(""))
+		if err != nil {
+			return err
+		}
+
+		if err := root.Add(
+			filepath.Join(action.Path, ".gitkeep"),
+			localrepo.TreeEntry{
+				Mode: "100644",
+				Path: ".gitkeep",
+				Type: localrepo.Blob,
+				OID:  blobID,
+			},
+		); err != nil {
+			if errors.Is(err, localrepo.ErrEntryExists) {
+				return git2go.IndexError{
+					Path: action.Path,
+					Type: git2go.ErrDirectoryExists,
+				}
+			}
+
+			return translateToGit2GoError(err, action.Path)
+		}
+	case git2go.CreateFile:
+		mode := "100644"
+		if action.ExecutableMode {
+			mode = "100755"
+		}
+
+		if err := root.Add(
+			action.Path,
+			localrepo.TreeEntry{
+				OID:  git.ObjectID(action.OID),
+				Path: filepath.Base(action.Path),
+				Type: localrepo.Blob,
+				Mode: mode,
+			},
+			localrepo.WithOverwriteDirectory(),
+		); err != nil {
+			return translateToGit2GoError(err, action.Path)
+		}
+	case git2go.DeleteFile:
+		if err := root.Delete(
+			action.Path,
+		); err != nil {
+			return translateToGit2GoError(err, action.Path)
+		}
+	default:
+		return errors.New("unsupported action")
+	}
+
+	return nil
+}
+
+// translateToGit2GoError maintains backwards compatibility with existing git2go
+// errors.
+func translateToGit2GoError(err error, path string) error {
+	switch err {
+	case localrepo.ErrEntryNotFound, localrepo.ErrObjectNotFound:
+		return git2go.IndexError{
+			Path: path,
+			Type: git2go.ErrFileNotFound,
+		}
+	case localrepo.ErrEmptyPath,
+		localrepo.ErrPathTraversal,
+		localrepo.ErrAbsolutePath,
+		localrepo.ErrInvalidPath:
+		//The error coming back from git2go has the path in single
+		//quotes. This is to match the git2go error for now.
+		//nolint:gitaly-linters
+		return git2go.UnknownIndexError(
+			fmt.Sprintf("invalid path: '%s'", path),
+		)
+	case localrepo.ErrPathTraversal:
+		return git2go.IndexError{
+			Path: path,
+			Type: git2go.ErrDirectoryTraversal,
+		}
+	case localrepo.ErrEntryExists:
+		return git2go.IndexError{
+			Path: path,
+			Type: git2go.ErrFileExists,
+		}
+	}
+	return err
+}
+
+// ErrSignatureMissingNameOrEmail matches the git2go error
+var ErrSignatureMissingNameOrEmail = errors.New(
+	"commit: failed to parse signature - Signature cannot have an empty name or email",
+)
+
+func (s *Server) userCommitFilesGit(
+	ctx context.Context,
+	header *gitalypb.UserCommitFilesRequestHeader,
+	parentCommitOID git.ObjectID,
+	quarantineRepo *localrepo.Repo,
+	repoPath string,
+	actions []git2go.Action,
+) (git.ObjectID, error) {
+	now, err := dateFromProto(header)
+	if err != nil {
+		return "", structerr.NewInvalidArgument("%w", err)
+	}
+
+	var treeish git.ObjectID
+
+	if parentCommitOID != "" {
+		treeish, err = quarantineRepo.ResolveRevision(
+			ctx,
+			git.Revision(fmt.Sprintf("%s^{tree}", parentCommitOID)),
+		)
+		if err != nil {
+			return "", err
+		}
+	}
+
+	treeEntry := &localrepo.TreeEntry{
+		Mode: "040000",
+		Type: localrepo.Tree,
+	}
+
+	if treeish != "" {
+		treeEntry, err = quarantineRepo.ReadTree(
+			ctx,
+			git.Revision(treeish),
+			localrepo.WithRecursive(),
+		)
+		if err != nil {
+			return "", err
+		}
+	}
+
+	for _, action := range actions {
+		if err = applyAction(
+			ctx,
+			action,
+			treeEntry,
+			quarantineRepo,
+		); err != nil {
+			return "", err
+		}
+	}
+
+	if len(actions) > 0 {
+		if err := treeEntry.Write(
+			ctx,
+			quarantineRepo,
+		); err != nil {
+			return "", err
+		}
+
+		treeish = treeEntry.OID
+	}
+
+	if treeish == "" {
+		objectHash, err := quarantineRepo.ObjectHash(ctx)
+		if err != nil {
+			return "", fmt.Errorf("getting object hash: %w", err)
+		}
+
+		treeish = objectHash.EmptyTreeOID
+	}
+
+	cfg := localrepo.WriteCommitConfig{
+		AuthorDate:     now,
+		AuthorName:     strings.TrimSpace(string(header.CommitAuthorName)),
+		AuthorEmail:    strings.TrimSpace(string(header.CommitAuthorEmail)),
+		CommitterDate:  now,
+		CommitterName:  strings.TrimSpace(string(header.User.Name)),
+		CommitterEmail: strings.TrimSpace(string(header.User.Email)),
+		Message:        string(header.CommitMessage),
+		TreeID:         treeish,
+	}
+
+	if cfg.AuthorName == "" {
+		cfg.AuthorName = cfg.CommitterName
+	}
+
+	if cfg.AuthorEmail == "" {
+		cfg.AuthorEmail = cfg.CommitterEmail
+	}
+
+	if cfg.AuthorName == "" || cfg.AuthorEmail == "" {
+		return "", structerr.NewInvalidArgument("%w", ErrSignatureMissingNameOrEmail)
+	}
+
+	if parentCommitOID != "" {
+		cfg.Parents = []git.ObjectID{parentCommitOID}
+	}
+
+	return quarantineRepo.WriteCommit(
+		ctx,
+		cfg,
+	)
+}
+
 func (s *Server) userCommitFilesGit2Go(
 	ctx context.Context,
 	header *gitalypb.UserCommitFilesRequestHeader,
@@ -153,7 +439,7 @@ func (s *Server) userCommitFilesGit2Go(
 		Actions:    actions,
 	})
 	if err != nil {
-		return "", fmt.Errorf("commit: %w", err)
+		return "", fmt.Errorf("%w", err)
 	}
 
 	return commitID, nil
@@ -295,7 +581,6 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi
 					return err
 				}
 			}
-
 			actions = append(actions, git2go.MoveFile{
 				Path:    prevPath,
 				NewPath: path,
@@ -322,14 +607,28 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi
 		}
 	}
 
-	commitID, err := s.userCommitFilesGit2Go(
-		ctx,
-		header,
-		parentCommitOID,
-		quarantineRepo,
-		repoPath,
-		actions,
-	)
+	var commitID git.ObjectID
+
+	if featureflag.CommitFilesInGit.IsEnabled(ctx) {
+		commitID, err = s.userCommitFilesGit(
+			ctx,
+			header,
+			parentCommitOID,
+			quarantineRepo,
+			repoPath,
+			actions,
+		)
+	} else {
+		commitID, err = s.userCommitFilesGit2Go(
+			ctx,
+			header,
+			parentCommitOID,
+			quarantineRepo,
+			repoPath,
+			actions,
+		)
+	}
+
 	if err != nil {
 		return err
 	}
diff --git a/internal/gitaly/service/operations/commit_files_test.go b/internal/gitaly/service/operations/commit_files_test.go
index 5f8e3e979..98dd586a2 100644
--- a/internal/gitaly/service/operations/commit_files_test.go
+++ b/internal/gitaly/service/operations/commit_files_test.go
@@ -12,6 +12,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
@@ -31,8 +32,12 @@ var commitFilesMessage = []byte("Change files")
 
 func TestUserCommitFiles(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
+	testhelper.NewFeatureSets(featureflag.CommitFilesInGit).
+		Run(t, testUserCommitFiles)
+}
+
+func testUserCommitFiles(t *testing.T, ctx context.Context) {
 	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx)
 
 	const (
@@ -965,8 +970,12 @@ func TestUserCommitFiles(t *testing.T) {
 
 func TestUserCommitFilesStableCommitID(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
+	testhelper.NewFeatureSets(featureflag.CommitFilesInGit).
+		Run(t, testUserCommitFilesStableCommitID)
+}
+
+func testUserCommitFilesStableCommitID(t *testing.T, ctx context.Context) {
 	ctx, cfg, _, _, client := setupOperationsService(t, ctx)
 
 	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
@@ -1023,8 +1032,12 @@ func TestUserCommitFilesStableCommitID(t *testing.T) {
 
 func TestUserCommitFilesQuarantine(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
+	testhelper.NewFeatureSets(featureflag.CommitFilesInGit).
+		Run(t, testUserCommitFilesQuarantine)
+}
+
+func testUserCommitFilesQuarantine(t *testing.T, ctx context.Context) {
 	ctx, cfg, _, _, client := setupOperationsService(t, ctx)
 
 	repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
@@ -1073,10 +1086,14 @@ func TestUserCommitFilesQuarantine(t *testing.T) {
 	require.False(t, exists, "quarantined commit should have been discarded")
 }
 
-func TestSuccessfulUserCommitFilesFilesRequest(t *testing.T) {
+func TestSuccessfulUserCommitFilesRequest(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
+	testhelper.NewFeatureSets(featureflag.CommitFilesInGit).
+		Run(t, testSuccessfulUserCommitFilesRequest)
+}
+
+func testSuccessfulUserCommitFilesRequest(t *testing.T, ctx context.Context) {
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	newRepo, newRepoPath := gittest.CreateRepository(t, ctx, cfg)
@@ -1224,10 +1241,14 @@ func TestSuccessfulUserCommitFilesFilesRequest(t *testing.T) {
 	}
 }
 
-func TestSuccessUserCommitFilesRequestMove(t *testing.T) {
+func TestSuccessfulUserCommitFilesRequestMove(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
+	testhelper.NewFeatureSets(featureflag.CommitFilesInGit).
+		Run(t, testSuccessfulUserCommitFilesRequestMove)
+}
+
+func testSuccessfulUserCommitFilesRequestMove(t *testing.T, ctx context.Context) {
 	ctx, cfg, _, _, client := setupOperationsService(t, ctx)
 
 	branchName := "master"
@@ -1282,10 +1303,14 @@ func TestSuccessUserCommitFilesRequestMove(t *testing.T) {
 	}
 }
 
-func TestSuccessUserCommitFilesRequestForceCommit(t *testing.T) {
+func TestSuccessfulUserCommitFilesRequestForceCommit(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.CommitFilesInGit).
+		Run(t, testSuccessUserCommitFilesRequestForceCommit)
+}
+
+func testSuccessUserCommitFilesRequestForceCommit(t *testing.T, ctx context.Context) {
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -1327,10 +1352,14 @@ func TestSuccessUserCommitFilesRequestForceCommit(t *testing.T) {
 	require.Equal(t, newTargetBranchCommit.ParentIds, []string{startBranchCommit.Id})
 }
 
-func TestSuccessUserCommitFilesRequestStartSha(t *testing.T) {
+func TestSuccessUserCommitFilesRequestForceCommit(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.CommitFilesInGit).
+		Run(t, testSuccessUserCommitFilesRequestStartSha)
+}
+
+func testSuccessUserCommitFilesRequestStartSha(t *testing.T, ctx context.Context) {
 	ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
@@ -1363,17 +1392,19 @@ func TestSuccessUserCommitFilesRequestStartSha(t *testing.T) {
 func TestSuccessUserCommitFilesRequestStartShaRemoteRepository(t *testing.T) {
 	t.Parallel()
 
-	testSuccessfulUserCommitFilesRemoteRepositoryRequest(func(header *gitalypb.UserCommitFilesRequest) {
-		setStartSha(header, "1e292f8fedd741b75372e19097c76d327140c312")
-	})
+	testhelper.NewFeatureSets(featureflag.CommitFilesInGit).
+		Run(t, testSuccessfulUserCommitFilesRemoteRepositoryRequest(func(header *gitalypb.UserCommitFilesRequest) {
+			setStartSha(header, "1e292f8fedd741b75372e19097c76d327140c312")
+		}))
 }
 
 func TestSuccessUserCommitFilesRequestStartBranchRemoteRepository(t *testing.T) {
 	t.Parallel()
 
-	testSuccessfulUserCommitFilesRemoteRepositoryRequest(func(header *gitalypb.UserCommitFilesRequest) {
-		setStartBranchName(header, []byte("master"))
-	})
+	testhelper.NewFeatureSets(featureflag.CommitFilesInGit).
+		Run(t, testSuccessfulUserCommitFilesRemoteRepositoryRequest(func(header *gitalypb.UserCommitFilesRequest) {
+			setStartBranchName(header, []byte("master"))
+		}))
 }
 
 func testSuccessfulUserCommitFilesRemoteRepositoryRequest(setHeader func(header *gitalypb.UserCommitFilesRequest)) func(*testing.T, context.Context) {
@@ -1417,7 +1448,11 @@ func testSuccessfulUserCommitFilesRemoteRepositoryRequest(setHeader func(header
 func TestSuccessfulUserCommitFilesRequestWithSpecialCharactersInSignature(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.CommitFilesInGit).
+		Run(t, testSuccessfulUserCommitFilesRequestWithSpecialCharactersInSignature)
+}
+
+func testSuccessfulUserCommitFilesRequestWithSpecialCharactersInSignature(t *testing.T, ctx context.Context) {
 	ctx, cfg, _, _, client := setupOperationsService(t, ctx)
 
 	repoProto, _ := gittest.CreateRepository(t, ctx, cfg)
@@ -1468,7 +1503,11 @@ func TestSuccessfulUserCommitFilesRequestWithSpecialCharactersInSignature(t *tes
 func TestFailedUserCommitFilesRequestDueToHooks(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.CommitFilesInGit).
+		Run(t, testFailedUserCommitFilesRequestDueToHooks)
+}
+
+func testFailedUserCommitFilesRequestDueToHooks(t *testing.T, ctx context.Context) {
 	ctx, _, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	branchName := "feature"
@@ -1517,7 +1556,11 @@ func TestFailedUserCommitFilesRequestDueToHooks(t *testing.T) {
 func TestFailedUserCommitFilesRequestDueToIndexError(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.CommitFilesInGit).
+		Run(t, testFailedUserCommitFilesRequestDueToIndexError)
+}
+
+func testFailedUserCommitFilesRequestDueToIndexError(t *testing.T, ctx context.Context) {
 	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
 	testCases := []struct {
@@ -1590,7 +1633,11 @@ func TestFailedUserCommitFilesRequestDueToIndexError(t *testing.T) {
 func TestFailedUserCommitFilesRequest(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.CommitFilesInGit).
+		Run(t, testFailedUserCommitFilesRequest)
+}
+
+func testFailedUserCommitFilesRequest(t *testing.T, ctx context.Context) {
 	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
 	branchName := "feature"
@@ -1629,29 +1676,29 @@ func TestFailedUserCommitFilesRequest(t *testing.T) {
 			expectedErr: status.Error(codes.InvalidArgument, fmt.Sprintf(`invalid object ID: "foobar", expected length %v, got 6`, gittest.DefaultObjectHash.EncodedLen())),
 		},
 		{
-			desc:        "failed to parse signature - Signature cannot have an empty name or email",
+			desc:        "failed to parse signature - Signature cannot have an empty name or email 1",
 			req:         headerRequest(repo, &gitalypb.User{}, branchName, commitFilesMessage, "", ""),
-			expectedErr: status.Error(codes.InvalidArgument, "commit: commit: failed to parse signature - Signature cannot have an empty name or email"),
+			expectedErr: status.Error(codes.InvalidArgument, "commit: failed to parse signature - Signature cannot have an empty name or email"),
 		},
 		{
-			desc:        "failed to parse signature - Signature cannot have an empty name or email",
+			desc:        "failed to parse signature - Signature cannot have an empty name or email 2",
 			req:         headerRequest(repo, &gitalypb.User{Name: []byte(""), Email: []byte("")}, branchName, commitFilesMessage, "", ""),
-			expectedErr: status.Error(codes.InvalidArgument, "commit: commit: failed to parse signature - Signature cannot have an empty name or email"),
+			expectedErr: status.Error(codes.InvalidArgument, "commit: failed to parse signature - Signature cannot have an empty name or email"),
 		},
 		{
-			desc:        "failed to parse signature - Signature cannot have an empty name or email",
+			desc:        "failed to parse signature - Signature cannot have an empty name or email 3",
 			req:         headerRequest(repo, &gitalypb.User{Name: []byte(" "), Email: []byte(" ")}, branchName, commitFilesMessage, "", ""),
-			expectedErr: status.Error(codes.InvalidArgument, "commit: commit: failed to parse signature - Signature cannot have an empty name or email"),
+			expectedErr: status.Error(codes.InvalidArgument, "commit: failed to parse signature - Signature cannot have an empty name or email"),
 		},
 		{
-			desc:        "failed to parse signature - Signature cannot have an empty name or email",
+			desc:        "failed to parse signature - Signature cannot have an empty name or email 4",
 			req:         headerRequest(repo, &gitalypb.User{Name: []byte("Jane Doe"), Email: []byte("")}, branchName, commitFilesMessage, "", ""),
-			expectedErr: status.Error(codes.InvalidArgument, "commit: commit: failed to parse signature - Signature cannot have an empty name or email"),
+			expectedErr: status.Error(codes.InvalidArgument, "commit: failed to parse signature - Signature cannot have an empty name or email"),
 		},
 		{
-			desc:        "failed to parse signature - Signature cannot have an empty name or email",
+			desc:        "failed to parse signature - Signature cannot have an empty name or email 5",
 			req:         headerRequest(repo, &gitalypb.User{Name: []byte(""), Email: []byte("janedoe@gitlab.com")}, branchName, commitFilesMessage, "", ""),
-			expectedErr: status.Error(codes.InvalidArgument, "commit: commit: failed to parse signature - Signature cannot have an empty name or email"),
+			expectedErr: status.Error(codes.InvalidArgument, "commit: failed to parse signature - Signature cannot have an empty name or email"),
 		},
 	}