diff options
author | Paul Okstad <pokstad@gitlab.com> | 2020-09-15 21:56:24 +0300 |
---|---|---|
committer | Paul Okstad <pokstad@gitlab.com> | 2020-09-15 21:56:24 +0300 |
commit | 0cc72a02229617905e560886227b3743527cd197 (patch) | |
tree | 46006a4e10123496359f773e3d0526e501e2e281 | |
parent | 7de73fe9bdced68051087ea6f33dff61c4ed3573 (diff) | |
parent | f7bb67439a8e6a1bcd0b2a3e632407dab6479dcf (diff) |
Merge branch 'pks-git2go-merge-operation-service' into 'master'
Port OperationService.UserMergeBranch to Go
Closes #3069
See merge request gitlab-org/gitaly!2540
-rw-r--r-- | changelogs/unreleased/pks-git2go-merge-operation-service.yml | 5 | ||||
-rw-r--r-- | cmd/gitaly-git2go/merge.go | 18 | ||||
-rw-r--r-- | internal/gitaly/service/operations/merge.go | 218 | ||||
-rw-r--r-- | internal/gitaly/service/operations/merge_test.go | 40 | ||||
-rw-r--r-- | internal/gitaly/service/operations/server.go | 17 | ||||
-rw-r--r-- | internal/gitaly/service/operations/testhelper_test.go | 8 | ||||
-rw-r--r-- | internal/gitaly/service/register.go | 2 | ||||
-rw-r--r-- | internal/metadata/featureflag/feature_flags.go | 3 |
8 files changed, 292 insertions, 19 deletions
diff --git a/changelogs/unreleased/pks-git2go-merge-operation-service.yml b/changelogs/unreleased/pks-git2go-merge-operation-service.yml new file mode 100644 index 000000000..430fd1573 --- /dev/null +++ b/changelogs/unreleased/pks-git2go-merge-operation-service.yml @@ -0,0 +1,5 @@ +--- +title: Port OperationService.UserMergeBranch to Go +merge_request: 2540 +author: +type: performance diff --git a/cmd/gitaly-git2go/merge.go b/cmd/gitaly-git2go/merge.go index 1e5c13aa4..7eb6c691a 100644 --- a/cmd/gitaly-git2go/merge.go +++ b/cmd/gitaly-git2go/merge.go @@ -6,6 +6,7 @@ import ( "errors" "flag" "fmt" + "strings" "time" git "github.com/libgit2/git2go/v30" @@ -74,12 +75,23 @@ func lookupCommit(repo *git.Repository, ref string) (*git.Commit, error) { return commit, nil } +func sanitizeSignatureInfo(info string) string { + return strings.Map(func(r rune) rune { + switch r { + case '<', '>', '\n': + return -1 + default: + return r + } + }, info) +} + func (cmd *mergeSubcommand) Run() error { if err := cmd.verifyOptions(); err != nil { return fmt.Errorf("invalid options: %w", err) } - var date time.Time + var date time.Time = time.Now() if cmd.authorDate != "" { var err error date, err = time.Parse("Mon Jan 2 15:04:05 2006 -0700", cmd.authorDate) @@ -118,8 +130,8 @@ func (cmd *mergeSubcommand) Run() error { } committer := git.Signature{ - Name: cmd.authorName, - Email: cmd.authorMail, + Name: sanitizeSignatureInfo(cmd.authorName), + Email: sanitizeSignatureInfo(cmd.authorMail), When: date, } diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go index 4cdc43d5d..4ff11fea6 100644 --- a/internal/gitaly/service/operations/merge.go +++ b/internal/gitaly/service/operations/merge.go @@ -1,22 +1,55 @@ package operations import ( + "bytes" "context" + "errors" "fmt" + "io" + "os/exec" + "path" "strings" + "gitlab.com/gitlab-org/gitaly/internal/command" + "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/updateref" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" + "gitlab.com/gitlab-org/gitaly/internal/gitlabshell" "gitlab.com/gitlab-org/gitaly/internal/helper" + "gitlab.com/gitlab-org/gitaly/internal/helper/text" + "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" ) +type preReceiveError struct { + message string +} + +func (e preReceiveError) Error() string { + return e.message +} + +type updateRefError struct { + reference string +} + +func (e updateRefError) Error() string { + return fmt.Sprintf("Could not update %s. Please refresh and try again.", e.reference) +} + func (s *server) UserMergeBranch(bidi gitalypb.OperationService_UserMergeBranchServer) error { + ctx := bidi.Context() + + if featureflag.IsEnabled(ctx, featureflag.GoUserMergeBranch) { + return s.userMergeBranch(bidi) + } + firstRequest, err := bidi.Recv() if err != nil { return err } - ctx := bidi.Context() client, err := s.ruby.OperationServiceClient(ctx) if err != nil { return err @@ -57,6 +90,189 @@ func (s *server) UserMergeBranch(bidi gitalypb.OperationService_UserMergeBranchS ) } +func validateMergeBranchRequest(request *gitalypb.UserMergeBranchRequest) error { + if request.User == nil { + return fmt.Errorf("empty user") + } + + if len(request.Branch) == 0 { + return fmt.Errorf("empty branch name") + } + + if request.CommitId == "" { + return fmt.Errorf("empty commit ID") + } + + if len(request.Message) == 0 { + return fmt.Errorf("empty message") + } + + return nil +} + +func (s *server) updateReferenceWithHooks(ctx context.Context, repo *gitalypb.Repository, user *gitalypb.User, reference, newrev, oldrev string) error { + gitlabshellEnv, err := gitlabshell.Env() + if err != nil { + return err + } + + env := append([]string{ + fmt.Sprintf("GL_ID=%s", user.GetGlId()), + fmt.Sprintf("GL_USERNAME=%s", user.GetGlUsername()), + fmt.Sprintf("GL_REPOSITORY=%s", repo.GetGlRepository()), + fmt.Sprintf("GL_PROJECT_PATH=%s", repo.GetGlProjectPath()), + fmt.Sprintf("GITALY_SOCKET=" + config.GitalyInternalSocketPath()), + fmt.Sprintf("GITALY_REPO=%s", repo), + fmt.Sprintf("GITALY_TOKEN=%s", s.cfg.Auth.Token), + }, gitlabshellEnv...) + + stdin := bytes.NewBufferString(fmt.Sprintf("%s %s %s\n", oldrev, newrev, reference)) + var stdout, stderr bytes.Buffer + + if err := s.hookManager.PreReceiveHook(ctx, repo, env, stdin, &stdout, &stderr); err != nil { + return preReceiveError{message: stdout.String()} + } + if err := s.hookManager.UpdateHook(ctx, repo, reference, oldrev, newrev, env, &stdout, &stderr); err != nil { + return preReceiveError{message: stdout.String()} + } + + updater, err := updateref.New(ctx, repo) + if err != nil { + return err + } + + if err := updater.Update(reference, newrev, oldrev); err != nil { + return err + } + + if err := updater.Wait(); err != nil { + return updateRefError{reference: reference} + } + + if err := s.hookManager.PostReceiveHook(ctx, repo, nil, env, stdin, &stdout, &stderr); err != nil { + return err + } + + return nil +} + +// parseRevision parses a Git revision and returns its OID. +func parseRevision(ctx context.Context, repo *gitalypb.Repository, revision string) (string, error) { + revParse, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ + Name: "rev-parse", + Flags: []git.Option{git.Flag{"--verify"}}, + Args: []string{revision}, + }) + if err != nil { + return "", err + } + + var stdout bytes.Buffer + if _, err := io.Copy(&stdout, revParse); err != nil { + return "", err + } + + if err := revParse.Wait(); err != nil { + return "", err + } + + return text.ChompBytes(stdout.Bytes()), nil +} + +func (s *server) userMergeBranch(stream gitalypb.OperationService_UserMergeBranchServer) error { + ctx := stream.Context() + + firstRequest, err := stream.Recv() + if err != nil { + return err + } + + if err := validateMergeBranchRequest(firstRequest); err != nil { + return helper.ErrInvalidArgument(err) + } + + revision, err := parseRevision(ctx, firstRequest.Repository, string(firstRequest.Branch)) + if err != nil { + return err + } + + repoPath, err := s.locator.GetPath(firstRequest.Repository) + if err != nil { + return err + } + + binary := path.Join(s.cfg.BinDir, "gitaly-git2go") + args := []string{ + "merge", + "-repository", repoPath, + "-author-name", string(firstRequest.User.Name), + "-author-mail", string(firstRequest.User.Email), + "-message", string(firstRequest.Message), + "-ours", firstRequest.CommitId, + "-theirs", revision, + } + + var stderr, stdout bytes.Buffer + mergeCommand, err := command.New(ctx, exec.Command(binary, args...), nil, &stdout, &stderr) + if err != nil { + return err + } + + if err := mergeCommand.Wait(); err != nil { + if _, ok := err.(*exec.ExitError); ok { + return fmt.Errorf("%w: %s", err, stderr.String()) + } + return err + } + + mergeCommit := text.ChompBytes(stdout.Bytes()) + + if err := stream.Send(&gitalypb.UserMergeBranchResponse{ + CommitId: mergeCommit, + }); err != nil { + return err + } + + secondRequest, err := stream.Recv() + if err != nil { + return err + } + if !secondRequest.Apply { + return helper.ErrPreconditionFailedf("merge aborted by client") + } + + branch := "refs/heads/" + text.ChompBytes(firstRequest.Branch) + if err := s.updateReferenceWithHooks(ctx, firstRequest.Repository, firstRequest.User, branch, mergeCommit, revision); err != nil { + var preReceiveError preReceiveError + var updateRefError updateRefError + + if errors.As(err, &preReceiveError) { + err = stream.Send(&gitalypb.UserMergeBranchResponse{ + PreReceiveError: preReceiveError.message, + }) + } else if errors.As(err, &updateRefError) { + // When an error happens updating the reference, e.g. because of a race + // with another update, then Ruby code didn't send an error but just an + // empty response. + err = stream.Send(&gitalypb.UserMergeBranchResponse{}) + } + + return err + } + + if err := stream.Send(&gitalypb.UserMergeBranchResponse{ + BranchUpdate: &gitalypb.OperationBranchUpdate{ + CommitId: mergeCommit, + RepoCreated: false, + BranchCreated: false, + }, + }); err != nil { + return err + } + + return nil +} + func validateFFRequest(in *gitalypb.UserFFBranchRequest) error { if len(in.Branch) == 0 { return fmt.Errorf("empty branch name") diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go index 5a9ad356e..691e07a2e 100644 --- a/internal/gitaly/service/operations/merge_test.go +++ b/internal/gitaly/service/operations/merge_test.go @@ -13,6 +13,7 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/command" gitlog "gitlab.com/gitlab-org/gitaly/internal/git/log" "gitlab.com/gitlab-org/gitaly/internal/helper/text" + "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" @@ -24,11 +25,26 @@ var ( mergeBranchHeadBefore = "281d3a76f31c812dbf48abce82ccf6860adedd81" ) -func TestSuccessfulMerge(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() +func testUserMergeBranch(t *testing.T, testcase func(*testing.T, context.Context)) { + featureSets, err := testhelper.NewFeatureSets([]featureflag.FeatureFlag{ + featureflag.GoUserMergeBranch, + }) + require.NoError(t, err) + + for _, featureSet := range featureSets { + t.Run("disabled "+featureSet.String(), func(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() + + ctx = featureSet.Disable(ctx) - testSuccessfulMerge(t, ctx) + testcase(t, ctx) + }) + } +} + +func TestSuccessfulMerge(t *testing.T) { + testUserMergeBranch(t, testSuccessfulMerge) } func testSuccessfulMerge(t *testing.T, ctx context.Context) { @@ -105,9 +121,10 @@ func testSuccessfulMerge(t *testing.T, ctx context.Context) { } func TestAbortedMerge(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() + testUserMergeBranch(t, testAbortedMerge) +} +func testAbortedMerge(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -173,9 +190,10 @@ func TestAbortedMerge(t *testing.T) { } func TestFailedMergeConcurrentUpdate(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() + testUserMergeBranch(t, testFailedMergeConcurrentUpdate) +} +func testFailedMergeConcurrentUpdate(t *testing.T, ctx context.Context) { testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() @@ -220,6 +238,10 @@ func TestFailedMergeConcurrentUpdate(t *testing.T) { } func TestFailedMergeDueToHooks(t *testing.T) { + testUserMergeBranch(t, testFailedMergeDueToHooks) +} + +func testFailedMergeDueToHooks(t *testing.T, ctx context.Context) { testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() @@ -239,7 +261,7 @@ func TestFailedMergeDueToHooks(t *testing.T) { require.NoError(t, err) defer remove() - ctx, cancel := testhelper.Context() + ctx, cancel := context.WithCancel(ctx) defer cancel() mergeBidi, err := client.UserMergeBranch(ctx) diff --git a/internal/gitaly/service/operations/server.go b/internal/gitaly/service/operations/server.go index 56224fa68..f6592ee20 100644 --- a/internal/gitaly/service/operations/server.go +++ b/internal/gitaly/service/operations/server.go @@ -1,16 +1,27 @@ package operations import ( + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/hook" "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" + "gitlab.com/gitlab-org/gitaly/internal/storage" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" ) type server struct { - ruby *rubyserver.Server + cfg config.Cfg + ruby *rubyserver.Server + hookManager hook.Manager + locator storage.Locator gitalypb.UnimplementedOperationServiceServer } // NewServer creates a new instance of a grpc OperationServiceServer -func NewServer(rs *rubyserver.Server) gitalypb.OperationServiceServer { - return &server{ruby: rs} +func NewServer(cfg config.Cfg, rs *rubyserver.Server, hookManager hook.Manager, locator storage.Locator) gitalypb.OperationServiceServer { + return &server{ + ruby: rs, + cfg: cfg, + hookManager: hookManager, + locator: locator, + } } diff --git a/internal/gitaly/service/operations/testhelper_test.go b/internal/gitaly/service/operations/testhelper_test.go index 6e10b42e6..98f124d8b 100644 --- a/internal/gitaly/service/operations/testhelper_test.go +++ b/internal/gitaly/service/operations/testhelper_test.go @@ -58,6 +58,7 @@ func testMain(m *testing.M) int { config.Config.GitlabShell.Dir = filepath.Join(cwd, "testdata", "gitlab-shell") testhelper.ConfigureGitalySSH() + testhelper.ConfigureGitalyGit2Go() testhelper.ConfigureGitalyHooksBinary() defer func(token string) { @@ -87,9 +88,12 @@ func runOperationServiceServerWithRubyServer(t *testing.T, ruby *rubyserver.Serv internalListener, err := net.Listen("unix", internalSocket) require.NoError(t, err) + hookManager := gitalyhook.NewManager(gitalyhook.GitlabAPIStub, config.Config) locator := config.NewLocator(config.Config) - gitalypb.RegisterOperationServiceServer(srv.GrpcServer(), &server{ruby: ruby}) - gitalypb.RegisterHookServiceServer(srv.GrpcServer(), hook.NewServer(gitalyhook.NewManager(gitalyhook.GitlabAPIStub, config.Config))) + server := NewServer(config.Config, ruby, hookManager, locator) + + gitalypb.RegisterOperationServiceServer(srv.GrpcServer(), server) + gitalypb.RegisterHookServiceServer(srv.GrpcServer(), hook.NewServer(hookManager)) gitalypb.RegisterRepositoryServiceServer(srv.GrpcServer(), repository.NewServer(ruby, locator, internalSocket)) gitalypb.RegisterRefServiceServer(srv.GrpcServer(), ref.NewServer()) gitalypb.RegisterCommitServiceServer(srv.GrpcServer(), commit.NewServer(locator)) diff --git a/internal/gitaly/service/register.go b/internal/gitaly/service/register.go index d0efe3166..cd9de4afa 100644 --- a/internal/gitaly/service/register.go +++ b/internal/gitaly/service/register.go @@ -73,7 +73,7 @@ func RegisterAll(grpcServer *grpc.Server, cfg config.Cfg, rubyServer *rubyserver gitalypb.RegisterCommitServiceServer(grpcServer, commit.NewServer(locator)) gitalypb.RegisterDiffServiceServer(grpcServer, diff.NewServer()) gitalypb.RegisterNamespaceServiceServer(grpcServer, namespace.NewServer()) - gitalypb.RegisterOperationServiceServer(grpcServer, operations.NewServer(rubyServer)) + gitalypb.RegisterOperationServiceServer(grpcServer, operations.NewServer(cfg, rubyServer, hookManager, locator)) gitalypb.RegisterRefServiceServer(grpcServer, ref.NewServer()) gitalypb.RegisterRepositoryServiceServer(grpcServer, repository.NewServer(rubyServer, locator, config.GitalyInternalSocketPath())) gitalypb.RegisterSSHServiceServer(grpcServer, ssh.NewServer( diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 65b57aa81..e4309db49 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -31,6 +31,8 @@ var ( ReferenceTransactionHook = FeatureFlag{Name: "reference_transaction_hook", OnByDefault: true} // LogCommandStats will log additional rusage stats for commands LogCommandStats = FeatureFlag{Name: "log_command_stats", OnByDefault: false} + // GoUserMergeBranch enables the Go implementation of UserMergeBranch + GoUserMergeBranch = FeatureFlag{Name: "go_user_merge_branch", OnByDefault: false} ) // All includes all feature flags. @@ -44,6 +46,7 @@ var All = []FeatureFlag{ ReferenceTransactionsSSHService, ReferenceTransactionsPrimaryWins, ReferenceTransactionHook, + GoUserMergeBranch, } const ( |