From 05be57947d5775c509272ea46c4211b8c952e234 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Tue, 1 Nov 2022 16:14:15 -0400 Subject: operations: Implement merge using git-merge-tree(1) git-merge-tree(1) is a recent Git command that was introduced to do in-memory merges without a worktree. Implement merge() with git-merge-tree(1) Co-authored-by: John Cai --- internal/gitaly/service/operations/merge.go | 112 ++++++++++++++++++++++- internal/gitaly/service/operations/merge_test.go | 49 ++++++---- 2 files changed, 139 insertions(+), 22 deletions(-) diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go index 6eef9c304..5047b16b3 100644 --- a/internal/gitaly/service/operations/merge.go +++ b/internal/gitaly/service/operations/merge.go @@ -1,6 +1,7 @@ package operations import ( + "bytes" "context" "errors" "fmt" @@ -9,12 +10,14 @@ import ( "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "github.com/sirupsen/logrus" + "gitlab.com/gitlab-org/gitaly/v15/internal/command" "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v15/internal/git/updateref" "gitlab.com/gitlab-org/gitaly/v15/internal/git2go" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/hook" "gitlab.com/gitlab-org/gitaly/v15/internal/helper" + "gitlab.com/gitlab-org/gitaly/v15/internal/helper/text" "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" ) @@ -47,6 +50,58 @@ func validateMergeBranchRequest(request *gitalypb.UserMergeBranchRequest) error return nil } +func createCommitFromTree( + ctx context.Context, + quarantineRepo *localrepo.Repo, + resultTree []byte, + authorName string, + authorMail string, + authorDate time.Time, + message string, + ours string, + theirs string, +) (string, error) { + commitEnv := []string{ + "GIT_COMMITTER_NAME=" + authorName, + "GIT_COMMITTER_EMAIL=" + authorMail, + "GIT_COMMITTER_DATE=" + authorDate.String(), + "GIT_AUTHOR_NAME=" + authorName, + "GIT_AUTHOR_EMAIL=" + authorMail, + "GIT_AUTHOR_DATE=" + authorDate.String(), + } + + flags := []git.Option{ + git.ValueFlag{ + Name: "-m", + Value: message, + }, + git.ValueFlag{ + Name: "-p", + Value: ours, + }, + git.ValueFlag{ + Name: "-p", + Value: theirs, + }, + } + + var stdout, stderr bytes.Buffer + if err := quarantineRepo.ExecAndWait(ctx, git.SubCmd{ + Name: "commit-tree", + Flags: flags, + Args: []string{ + string(resultTree), + }, + }, git.WithStdout(&stdout), git.WithStderr(&stderr), git.WithEnv(commitEnv...)); err != nil { + return "", fmt.Errorf("creating commit: %w", gitError{ + Err: err, + ErrMsg: stderr.String(), + }) + } + + return text.ChompBytes(stdout.Bytes()), nil +} + func (s *Server) merge( ctx context.Context, repoPath string, @@ -58,7 +113,44 @@ func (s *Server) merge( ours string, theirs string, ) (string, error) { - return "", helper.ErrInternalf("UserMergeBranch using Git is not yet implemented") + var mergeTreeStderr bytes.Buffer + var mergeTreeStdout bytes.Buffer + cmdMergeTree, err := s.gitCmdFactory.New(ctx, quarantineRepo, + git.SubCmd{ + Name: "merge-tree", + Flags: []git.Option{ + git.Flag{Name: "--write-tree"}, + git.Flag{Name: "--name-only"}, + }, + Args: []string{ours, theirs}, + }, + git.WithStderr(&mergeTreeStderr), + git.WithStdout(&mergeTreeStdout), + ) + if err != nil { + return "", fmt.Errorf("merge-tree: %w", gitError{ErrMsg: mergeTreeStderr.String(), Err: err}) + } + + cmdErr := cmdMergeTree.Wait() + if exitCode, success := command.ExitStatus(cmdErr); success && exitCode > 1 { + return "", fmt.Errorf("merge-tree: %w", gitError{ErrMsg: mergeTreeStderr.String(), Err: err}) + } + + mergeTreeResult, err := git.NewMergeTreeResult(mergeTreeStdout.String()) + if err != nil { + return "", err + } + + var commit string + if commit, err = createCommitFromTree( + ctx, quarantineRepo, []byte(mergeTreeResult.TreeOid), + authorName, authorMail, authorDate, + message, ours, theirs, + ); err != nil { + return "", fmt.Errorf("create commit from tree: %w", err) + } + + return commit, nil } func (s *Server) mergeWithGit2Go( @@ -82,6 +174,13 @@ func (s *Server) mergeWithGit2Go( Theirs: theirs, }) if err != nil { + var conflictErr git2go.ConflictingFilesError + if errors.As(err, &conflictErr) { + return "", &git.ConflictingFilesError{ + ConflictingFiles: conflictErr.ConflictingFiles, + } + } + if errors.Is(err, git2go.ErrInvalidArgument) { return "", helper.ErrInvalidArgument(err) } @@ -130,7 +229,12 @@ func (s *Server) UserMergeBranch(stream gitalypb.OperationService_UserMergeBranc var mergeCommitID string var mergeErr error - if featureflag.MergeTreeMerge.IsEnabled(ctx) { + version, err := s.gitCmdFactory.GitVersion(ctx) + if err != nil { + return helper.ErrInternalf("getting git version: %w", err) + } + + if featureflag.MergeTreeMerge.IsEnabled(ctx) && version.IsMergeTreeSupported() { mergeCommitID, mergeErr = s.merge(ctx, repoPath, quarantineRepo, string(firstRequest.User.Name), string(firstRequest.User.Email), @@ -149,7 +253,7 @@ func (s *Server) UserMergeBranch(stream gitalypb.OperationService_UserMergeBranc } if mergeErr != nil { - var conflictErr git2go.ConflictingFilesError + var conflictErr *git.ConflictingFilesError if errors.As(mergeErr, &conflictErr) { conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles)) for _, conflictingFile := range conflictErr.ConflictingFiles { @@ -157,7 +261,7 @@ func (s *Server) UserMergeBranch(stream gitalypb.OperationService_UserMergeBranc } detailedErr, err := helper.ErrWithDetails( - helper.ErrFailedPreconditionf("merging commits: %w", err), + helper.ErrFailedPreconditionf("merging commits: %w", mergeErr), &gitalypb.UserMergeBranchError{ Error: &gitalypb.UserMergeBranchError_MergeConflict{ MergeConflict: &gitalypb.MergeConflictError{ diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go index 0e835f555..035c14287 100644 --- a/internal/gitaly/service/operations/merge_test.go +++ b/internal/gitaly/service/operations/merge_test.go @@ -43,7 +43,7 @@ var ( func TestUserMergeBranch_successful(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.MergeTreeMerge).Run( + testhelper.NewFeatureSets(featureflag.MergeTreeMerge, featureflag.GitV238).Run( t, testUserMergeBranchSuccessful, ) @@ -231,7 +231,7 @@ func TestUserMergeBranch_failure(t *testing.T) { func TestUserMergeBranch_quarantine(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.MergeTreeMerge).Run( + testhelper.NewFeatureSets(featureflag.MergeTreeMerge, featureflag.GitV238).Run( t, testUserMergeBranchQuarantine, ) @@ -293,7 +293,7 @@ func testUserMergeBranchQuarantine(t *testing.T, ctx context.Context) { func TestUserMergeBranch_stableMergeIDs(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.MergeTreeMerge).Run( + testhelper.NewFeatureSets(featureflag.MergeTreeMerge, featureflag.GitV238).Run( t, testUserMergeBranchStableMergeIDs, ) @@ -367,7 +367,7 @@ func testUserMergeBranchStableMergeIDs(t *testing.T, ctx context.Context) { func TestUserMergeBranch_abort(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.MergeTreeMerge).Run( + testhelper.NewFeatureSets(featureflag.MergeTreeMerge, featureflag.GitV238).Run( t, testUserMergeBranchAbort, ) @@ -437,7 +437,7 @@ func testUserMergeBranchAbort(t *testing.T, ctx context.Context) { func TestUserMergeBranch_concurrentUpdate(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.MergeTreeMerge).Run( + testhelper.NewFeatureSets(featureflag.MergeTreeMerge, featureflag.GitV238).Run( t, testUserMergeBranchConcurrentUpdate, ) @@ -499,7 +499,7 @@ func testUserMergeBranchConcurrentUpdate(t *testing.T, ctx context.Context) { func TestUserMergeBranch_ambiguousReference(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.MergeTreeMerge).Run( + testhelper.NewFeatureSets(featureflag.MergeTreeMerge, featureflag.GitV238).Run( t, testUserMergeBranchAmbiguousReference, ) @@ -567,7 +567,7 @@ func testUserMergeBranchAmbiguousReference(t *testing.T, ctx context.Context) { func TestUserMergeBranch_failingHooks(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.MergeTreeMerge).Run( + testhelper.NewFeatureSets(featureflag.MergeTreeMerge, featureflag.GitV238).Run( t, testUserMergeBranchFailingHooks, ) @@ -664,7 +664,7 @@ func testUserMergeBranchFailingHooks(t *testing.T, ctx context.Context) { func TestUserMergeBranch_conflict(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.MergeTreeMerge).Run( + testhelper.NewFeatureSets(featureflag.MergeTreeMerge, featureflag.GitV238).Run( t, testUserMergeBranchConflict, ) @@ -675,23 +675,36 @@ func testUserMergeBranchConflict(t *testing.T, ctx context.Context) { ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx) + const baseBranch = "base" const mergeIntoBranch = "mergeIntoBranch" const mergeFromBranch = "mergeFromBranch" const conflictingFile = "file" - baseCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch(mergeIntoBranch), gittest.WithTreeEntries(gittest.TreeEntry{ + baseCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch(baseBranch), gittest.WithTreeEntries(gittest.TreeEntry{ Mode: "100644", Path: conflictingFile, Content: "data", })) - gittest.Exec(t, cfg, "-C", repoPath, "branch", mergeFromBranch, baseCommit.String()) - - divergedInto := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch(mergeIntoBranch), gittest.WithTreeEntries(gittest.TreeEntry{ - Mode: "100644", Path: conflictingFile, Content: "data-1", - })) + divergedInto := gittest.WriteCommit( + t, + cfg, + repoPath, + gittest.WithBranch(mergeIntoBranch), + gittest.WithParents(baseCommit), + gittest.WithTreeEntries(gittest.TreeEntry{ + Mode: "100644", Path: conflictingFile, Content: "data-1", + }), + ) - divergedFrom := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch(mergeFromBranch), gittest.WithTreeEntries(gittest.TreeEntry{ - Mode: "100644", Path: conflictingFile, Content: "data-2", - })) + divergedFrom := gittest.WriteCommit( + t, + cfg, + repoPath, + gittest.WithBranch(mergeFromBranch), + gittest.WithParents(baseCommit), + gittest.WithTreeEntries(gittest.TreeEntry{ + Mode: "100644", Path: conflictingFile, Content: "data-2", + }), + ) mergeBidi, err := client.UserMergeBranch(ctx) require.NoError(t, err) @@ -726,7 +739,7 @@ func testUserMergeBranchConflict(t *testing.T, ctx context.Context) { func TestUserMergeBranch_allowed(t *testing.T) { t.Parallel() - testhelper.NewFeatureSets(featureflag.MergeTreeMerge).Run( + testhelper.NewFeatureSets(featureflag.MergeTreeMerge, featureflag.GitV238).Run( t, testUserMergeBranchAllowed, ) -- cgit v1.2.3