From df0bf6e1f1c9fdd554cafb5ba75244195dfbbf1d Mon Sep 17 00:00:00 2001 From: Paul Okstad Date: Tue, 10 Nov 2020 07:42:15 +0000 Subject: Port UserUpdateSubmodule to Go --- internal/git2go/submodule.go | 110 ++++++++++++ internal/git2go/submodule_test.go | 186 +++++++++++++++++++++ internal/gitaly/service/operations/submodules.go | 116 ++++++++++++- .../gitaly/service/operations/submodules_test.go | 44 +++-- internal/helper/fstype/detect_unix.go | 2 +- internal/metadata/featureflag/feature_flags.go | 4 + 6 files changed, 447 insertions(+), 15 deletions(-) create mode 100644 internal/git2go/submodule.go create mode 100644 internal/git2go/submodule_test.go (limited to 'internal') diff --git a/internal/git2go/submodule.go b/internal/git2go/submodule.go new file mode 100644 index 000000000..c4e8fe078 --- /dev/null +++ b/internal/git2go/submodule.go @@ -0,0 +1,110 @@ +package git2go + +import ( + "context" + "fmt" + "io" + "time" + + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" +) + +// Error strings present in the legacy Ruby implementation +const ( + LegacyErrPrefixInvalidBranch = "Invalid branch" + LegacyErrPrefixInvalidSubmodulePath = "Invalid submodule path" + LegacyErrPrefixFailedCommit = "Failed to create commit" +) + +// SubmoduleCommand instructs how to commit a submodule update to a repo +type SubmoduleCommand struct { + // Repository is the path to commit the submodule change + Repository string `json:"repository"` + + // AuthorName is the author name of submodule commit. + AuthorName string `json:"author_name"` + // AuthorMail is the author mail of submodule commit. + AuthorMail string `json:"author_mail"` + // AuthorDate is the auithor date of submodule commit. + AuthorDate time.Time `json:"author_date"` + // Message is the message to be used for the submodule commit. + Message string `json:"message"` + + // CommitSHA is where the submodule should point + CommitSHA string `json:"commit_sha"` + // Submodule is the actual submodule string to commit to the tree + Submodule string `json:"submodule"` + // Branch where to commit submodule update + Branch string `json:"branch"` +} + +// SubmoduleResult contains results from a committing a submodule update +type SubmoduleResult struct { + // CommitID is the object ID of the generated submodule commit. + CommitID string `json:"commit_id"` +} + +// SubmoduleCommandFromSerialized deserializes the submodule request from its JSON representation encoded with base64. +func SubmoduleCommandFromSerialized(serialized string) (SubmoduleCommand, error) { + var request SubmoduleCommand + if err := deserialize(serialized, &request); err != nil { + return SubmoduleCommand{}, err + } + + if err := request.verify(); err != nil { + return SubmoduleCommand{}, fmt.Errorf("submodule: %w", err) + } + + return request, nil +} + +// SerializeTo serializes the submodule result and writes it into the writer. +func (s SubmoduleResult) SerializeTo(w io.Writer) error { + return serializeTo(w, s) +} + +// Run attempts to commit the request submodule change +func (s SubmoduleCommand) Run(ctx context.Context, cfg config.Cfg) (SubmoduleResult, error) { + if err := s.verify(); err != nil { + return SubmoduleResult{}, fmt.Errorf("submodule: %w", err) + } + + serialized, err := serialize(s) + if err != nil { + return SubmoduleResult{}, err + } + + stdout, err := run(ctx, binaryPathFromCfg(cfg), nil, "submodule", "-request", serialized) + if err != nil { + return SubmoduleResult{}, err + } + + var response SubmoduleResult + if err := deserialize(stdout.String(), &response); err != nil { + return SubmoduleResult{}, err + } + + return response, nil +} + +func (s SubmoduleCommand) verify() (err error) { + if s.Repository == "" { + return InvalidArgumentError("missing repository") + } + if s.AuthorName == "" { + return InvalidArgumentError("missing author name") + } + if s.AuthorMail == "" { + return InvalidArgumentError("missing author mail") + } + if s.CommitSHA == "" { + return InvalidArgumentError("missing commit SHA") + } + if s.Branch == "" { + return InvalidArgumentError("missing branch name") + } + if s.Submodule == "" { + return InvalidArgumentError("missing submodule") + } + return nil +} diff --git a/internal/git2go/submodule_test.go b/internal/git2go/submodule_test.go new file mode 100644 index 000000000..9327025bd --- /dev/null +++ b/internal/git2go/submodule_test.go @@ -0,0 +1,186 @@ +package git2go + +import ( + "bytes" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestGit2Go_SubmoduleCommandSerialization(t *testing.T) { + testcases := []struct { + desc string + cmd SubmoduleCommand + err string + }{ + { + desc: "missing repository", + cmd: SubmoduleCommand{}, + err: "missing repository", + }, + { + desc: "missing author name", + cmd: SubmoduleCommand{ + Repository: "foo", + }, + err: "missing author name", + }, + { + desc: "missing author mail", + cmd: SubmoduleCommand{ + Repository: "foo", + AuthorName: "Au Thor", + }, + err: "missing author mail", + }, + { + desc: "missing commit SHA", + cmd: SubmoduleCommand{ + Repository: "foo", + AuthorName: "Au Thor", + AuthorMail: "au@thor.com", + }, + err: "missing commit SHA", + }, + { + desc: "missing branch", + cmd: SubmoduleCommand{ + Repository: "foo", + AuthorName: "Au Thor", + AuthorMail: "au@thor.com", + CommitSHA: "deadbeef1010", + }, + err: "missing branch name", + }, + { + desc: "missing submodule path", + cmd: SubmoduleCommand{ + Repository: "foo", + AuthorName: "Au Thor", + AuthorMail: "au@thor.com", + CommitSHA: "deadbeef1010", + Branch: "master", + }, + err: "missing submodule", + }, + { + desc: "valid command", + cmd: SubmoduleCommand{ + Repository: "foo", + AuthorName: "Au Thor", + AuthorMail: "au@thor.com", + CommitSHA: "deadbeef1010", + Branch: "master", + Submodule: "path/to/my/subby", + }, + }, + { + desc: "valid command with message", + cmd: SubmoduleCommand{ + Repository: "foo", + AuthorName: "Au Thor", + AuthorMail: "au@thor.com", + Message: "meow to you my friend", + CommitSHA: "deadbeef1010", + Branch: "master", + Submodule: "path/to/my/subby", + }, + }, + { + desc: "valid command with date", + cmd: SubmoduleCommand{ + Repository: "foo", + AuthorName: "Au Thor", + AuthorMail: "au@thor.com", + AuthorDate: time.Now().UTC(), + Message: "Message", + CommitSHA: "deadbeef1010", + Branch: "master", + Submodule: "path/to/my/subby", + }, + }, + { + desc: "valid command with message and date", + cmd: SubmoduleCommand{ + Repository: "foo", + AuthorName: "Au Thor", + AuthorMail: "au@thor.com", + AuthorDate: time.Now().UTC(), + Message: "woof for dayz", + CommitSHA: "deadbeef1010", + Branch: "master", + Submodule: "path/to/my/subby", + }, + }, + } + + for _, tc := range testcases { + t.Run(tc.desc, func(t *testing.T) { + serialized, err := serialize(tc.cmd) + require.NoError(t, err) + + deserialized, err := SubmoduleCommandFromSerialized(serialized) + + if tc.err != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.err) + } else { + require.NoError(t, err) + require.Equal(t, deserialized, tc.cmd) + } + }) + } +} + +func TestGit2Go_SubmoduleResultSerialization(t *testing.T) { + serializeResult := func(t *testing.T, result SubmoduleResult) string { + t.Helper() + var buf bytes.Buffer + err := result.SerializeTo(&buf) + require.NoError(t, err) + return buf.String() + } + + testcases := []struct { + desc string + serialized string + expected SubmoduleResult + err string + }{ + { + desc: "empty merge result", + serialized: serializeResult(t, SubmoduleResult{}), + expected: SubmoduleResult{}, + }, + { + desc: "merge result with commit", + serialized: serializeResult(t, SubmoduleResult{ + CommitID: "1234", + }), + expected: SubmoduleResult{ + CommitID: "1234", + }, + }, + { + desc: "invalid serialized representation", + serialized: "xvlc", + err: "invalid character", + }, + } + + for _, tc := range testcases { + t.Run(tc.desc, func(t *testing.T) { + var deserialized SubmoduleResult + err := deserialize(tc.serialized, &deserialized) + + if tc.err != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.err) + } else { + require.NoError(t, err) + require.Equal(t, deserialized, tc.expected) + } + }) + } +} diff --git a/internal/gitaly/service/operations/submodules.go b/internal/gitaly/service/operations/submodules.go index 3a65fef65..b415bf2b6 100644 --- a/internal/gitaly/service/operations/submodules.go +++ b/internal/gitaly/service/operations/submodules.go @@ -2,18 +2,32 @@ package operations import ( "context" + "errors" "fmt" "regexp" + "strings" + "time" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" + "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git2go" "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" + "gitlab.com/gitlab-org/gitaly/internal/helper" + "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) +const userUpdateSubmoduleName = "UserUpdateSubmodule" + func (s *server) UserUpdateSubmodule(ctx context.Context, req *gitalypb.UserUpdateSubmoduleRequest) (*gitalypb.UserUpdateSubmoduleResponse, error) { if err := validateUserUpdateSubmoduleRequest(req); err != nil { - return nil, status.Errorf(codes.InvalidArgument, "UserUpdateSubmodule: %v", err) + return nil, status.Errorf(codes.InvalidArgument, userUpdateSubmoduleName+": %v", err) + } + + if featureflag.IsEnabled(ctx, featureflag.GoUserUpdateSubmodule) { + return s.userUpdateSubmodule(ctx, req) } client, err := s.ruby.OperationServiceClient(ctx) @@ -60,3 +74,103 @@ func validateUserUpdateSubmoduleRequest(req *gitalypb.UserUpdateSubmoduleRequest return nil } + +func (s *server) userUpdateSubmodule(ctx context.Context, req *gitalypb.UserUpdateSubmoduleRequest) (*gitalypb.UserUpdateSubmoduleResponse, error) { + repo := git.NewRepository(req.GetRepository()) + branches, err := repo.GetBranches(ctx) + if err != nil { + return nil, fmt.Errorf("%s: get branches: %w", userUpdateSubmoduleName, err) + } + if len(branches) == 0 { + return &gitalypb.UserUpdateSubmoduleResponse{ + CommitError: "Repository is empty", + }, nil + } + + branchRef, err := repo.GetBranch(ctx, string(req.GetBranch())) + if err != nil { + if errors.Is(err, git.ErrReferenceNotFound) { + return nil, helper.ErrInvalidArgumentf("Cannot find branch") //nolint + } + return nil, fmt.Errorf("%s: get branch: %w", userUpdateSubmoduleName, err) + } + + repoPath, err := s.locator.GetRepoPath(req.GetRepository()) + if err != nil { + return nil, fmt.Errorf("%s: locate repo: %w", userUpdateSubmoduleName, err) + } + + result, err := git2go.SubmoduleCommand{ + Repository: repoPath, + AuthorMail: string(req.GetUser().GetEmail()), + AuthorName: string(req.GetUser().GetName()), + AuthorDate: time.Now(), + Branch: string(req.GetBranch()), + CommitSHA: req.GetCommitSha(), + Submodule: string(req.GetSubmodule()), + Message: string(req.GetCommitMessage()), + }.Run(ctx, s.cfg) + if err != nil { + errStr := strings.TrimPrefix(err.Error(), "submodule: ") + errStr = strings.TrimSpace(errStr) + + var resp *gitalypb.UserUpdateSubmoduleResponse + for _, legacyErr := range []string{ + git2go.LegacyErrPrefixInvalidBranch, + git2go.LegacyErrPrefixInvalidSubmodulePath, + git2go.LegacyErrPrefixFailedCommit, + } { + if strings.HasPrefix(errStr, legacyErr) { + resp = &gitalypb.UserUpdateSubmoduleResponse{ + CommitError: legacyErr, + } + ctxlogrus. + Extract(ctx). + WithError(err). + Error(userUpdateSubmoduleName + ": git2go subcommand failure") + break + } + } + if strings.Contains(errStr, "is already at") { + resp = &gitalypb.UserUpdateSubmoduleResponse{ + CommitError: errStr, + } + } + if resp != nil { + return resp, nil + } + return nil, fmt.Errorf("%s: submodule subcommand: %w", userUpdateSubmoduleName, err) + } + + if err := s.updateReferenceWithHooks( + ctx, + req.GetRepository(), + req.GetUser(), + "refs/heads/"+string(req.GetBranch()), + result.CommitID, + branchRef.Target, + ); err != nil { + var preReceiveError preReceiveError + if errors.As(err, &preReceiveError) { + return &gitalypb.UserUpdateSubmoduleResponse{ + PreReceiveError: preReceiveError.Error(), + }, nil + } + + var updateRefError updateRefError + 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. + return &gitalypb.UserUpdateSubmoduleResponse{}, nil + } + } + + return &gitalypb.UserUpdateSubmoduleResponse{ + BranchUpdate: &gitalypb.OperationBranchUpdate{ + CommitId: result.CommitID, + BranchCreated: false, + RepoCreated: false, + }, + }, nil +} diff --git a/internal/gitaly/service/operations/submodules_test.go b/internal/gitaly/service/operations/submodules_test.go index e3de793b0..779d9ca4f 100644 --- a/internal/gitaly/service/operations/submodules_test.go +++ b/internal/gitaly/service/operations/submodules_test.go @@ -9,16 +9,16 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/internal/git/log" "gitlab.com/gitlab-org/gitaly/internal/git/lstree" + "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" ) func TestSuccessfulUserUpdateSubmoduleRequest(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() - - testSuccessfulUserUpdateSubmoduleRequest(t, ctx) + testhelper.NewFeatureSets( + []featureflag.FeatureFlag{featureflag.GoUserUpdateSubmodule}, + ).Run(t, testSuccessfulUserUpdateSubmoduleRequest) } func testSuccessfulUserUpdateSubmoduleRequest(t *testing.T, ctx context.Context) { @@ -86,6 +86,12 @@ func testSuccessfulUserUpdateSubmoduleRequest(t *testing.T, ctx context.Context) } func TestFailedUserUpdateSubmoduleRequestDueToValidations(t *testing.T) { + testhelper.NewFeatureSets( + []featureflag.FeatureFlag{featureflag.GoUserUpdateSubmodule}, + ).Run(t, testFailedUserUpdateSubmoduleRequestDueToValidations) +} + +func testFailedUserUpdateSubmoduleRequestDueToValidations(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -200,7 +206,7 @@ func TestFailedUserUpdateSubmoduleRequestDueToValidations(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.desc, func(t *testing.T) { - ctx, cancel := testhelper.Context() + ctx, cancel := context.WithCancel(ctx) defer cancel() _, err := client.UserUpdateSubmodule(ctx, testCase.request) @@ -211,9 +217,12 @@ func TestFailedUserUpdateSubmoduleRequestDueToValidations(t *testing.T) { } func TestFailedUserUpdateSubmoduleRequestDueToInvalidBranch(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() + testhelper.NewFeatureSets( + []featureflag.FeatureFlag{featureflag.GoUserUpdateSubmodule}, + ).Run(t, testFailedUserUpdateSubmoduleRequestDueToInvalidBranch) +} +func testFailedUserUpdateSubmoduleRequestDueToInvalidBranch(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -238,9 +247,12 @@ func TestFailedUserUpdateSubmoduleRequestDueToInvalidBranch(t *testing.T) { } func TestFailedUserUpdateSubmoduleRequestDueToInvalidSubmodule(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() + testhelper.NewFeatureSets( + []featureflag.FeatureFlag{featureflag.GoUserUpdateSubmodule}, + ).Run(t, testFailedUserUpdateSubmoduleRequestDueToInvalidSubmodule) +} +func testFailedUserUpdateSubmoduleRequestDueToInvalidSubmodule(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -265,9 +277,12 @@ func TestFailedUserUpdateSubmoduleRequestDueToInvalidSubmodule(t *testing.T) { } func TestFailedUserUpdateSubmoduleRequestDueToSameReference(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() + testhelper.NewFeatureSets( + []featureflag.FeatureFlag{featureflag.GoUserUpdateSubmodule}, + ).Run(t, testFailedUserUpdateSubmoduleRequestDueToSameReference) +} +func testFailedUserUpdateSubmoduleRequestDueToSameReference(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -295,9 +310,12 @@ func TestFailedUserUpdateSubmoduleRequestDueToSameReference(t *testing.T) { } func TestFailedUserUpdateSubmoduleRequestDueToRepositoryEmpty(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() + testhelper.NewFeatureSets( + []featureflag.FeatureFlag{featureflag.GoUserUpdateSubmodule}, + ).Run(t, testFailedUserUpdateSubmoduleRequestDueToRepositoryEmpty) +} +func testFailedUserUpdateSubmoduleRequestDueToRepositoryEmpty(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() diff --git a/internal/helper/fstype/detect_unix.go b/internal/helper/fstype/detect_unix.go index f946002e0..e5665dae5 100644 --- a/internal/helper/fstype/detect_unix.go +++ b/internal/helper/fstype/detect_unix.go @@ -15,7 +15,7 @@ func detectFileSystem(path string) string { if c == 0 { break } - buf = append(buf, byte(c)) + buf = append(buf, c) } if len(buf) == 0 { diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 94ecf25f4..381bfe57c 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -36,6 +36,9 @@ var ( GoUserCommitFiles = FeatureFlag{Name: "go_user_commit_files", OnByDefault: false} // GoResolveConflicts enables the Go implementation of ResolveConflicts GoResolveConflicts = FeatureFlag{Name: "go_resolve_conflicts", OnByDefault: false} + // GoUserUpdateSubmodule enables the Go implementation of + // UserUpdateSubmodules + GoUserUpdateSubmodule = FeatureFlag{Name: "go_user_update_submodule", OnByDefault: false} ) // All includes all feature flags. @@ -52,6 +55,7 @@ var All = []FeatureFlag{ GoListConflictFiles, GoUserCommitFiles, GoResolveConflicts, + GoUserUpdateSubmodule, } const ( -- cgit v1.2.3