diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2021-03-10 15:36:45 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2021-03-10 16:25:07 +0300 |
commit | 842d86ff6bd91d9826b613851472ae1defdd9b00 (patch) | |
tree | ac681e9b9954f70331d77e5fc4dd63a8dd7d2a36 /internal | |
parent | cff707acc7afdaa82f13edc3ae3c2bdcd9357c1d (diff) |
git2go: Convert commands to return OIDs
This commit converts commands in our `internal/git2go` wrappers to
return properly typed `git.ObjectID`s instead of plain strings.
Diffstat (limited to 'internal')
-rw-r--r-- | internal/git2go/apply.go | 15 | ||||
-rw-r--r-- | internal/git2go/apply_test.go | 16 | ||||
-rw-r--r-- | internal/git2go/cherry_pick.go | 3 | ||||
-rw-r--r-- | internal/git2go/commit.go | 15 | ||||
-rw-r--r-- | internal/git2go/commit_test.go | 14 | ||||
-rw-r--r-- | internal/git2go/gob.go | 10 | ||||
-rw-r--r-- | internal/git2go/revert.go | 3 | ||||
-rw-r--r-- | internal/gitaly/service/operations/cherry_pick.go | 8 | ||||
-rw-r--r-- | internal/gitaly/service/operations/commit_files.go | 4 | ||||
-rw-r--r-- | internal/gitaly/service/operations/revert.go | 8 |
10 files changed, 63 insertions, 33 deletions
diff --git a/internal/git2go/apply.go b/internal/git2go/apply.go index 447e92530..93c74925b 100644 --- a/internal/git2go/apply.go +++ b/internal/git2go/apply.go @@ -5,6 +5,8 @@ import ( "encoding/gob" "fmt" "io" + + "gitlab.com/gitlab-org/gitaly/internal/git" ) // ErrMergeConflict is returned when there is a merge conflict. @@ -70,7 +72,7 @@ func (iter *slicePatchIterator) Err() error { return nil } // Apply applies the provided patches and returns the OID of the commit with the patches // applied. -func (b Executor) Apply(ctx context.Context, params ApplyParams) (string, error) { +func (b Executor) Apply(ctx context.Context, params ApplyParams) (git.ObjectID, error) { reader, writer := io.Pipe() defer writer.Close() @@ -108,5 +110,14 @@ func (b Executor) Apply(ctx context.Context, params ApplyParams) (string, error) return "", fmt.Errorf("decode: %w", err) } - return result.CommitID, result.Error + if result.Error != nil { + return "", result.Error + } + + commitID, err := git.NewObjectIDFromHex(result.CommitID) + if err != nil { + return "", fmt.Errorf("could not parse commit ID: %w", err) + } + + return commitID, nil } diff --git a/internal/git2go/apply_test.go b/internal/git2go/apply_test.go index 6f303257c..ca140f395 100644 --- a/internal/git2go/apply_test.go +++ b/internal/git2go/apply_test.go @@ -60,7 +60,7 @@ func TestExecutor_Apply(t *testing.T) { Author: author, Committer: committer, Message: "commit with a", - Parent: parentCommitSHA, + Parent: parentCommitSHA.String(), Actions: []Action{UpdateFile{Path: "file", OID: oidA.String()}}, }) require.NoError(t, err) @@ -70,7 +70,7 @@ func TestExecutor_Apply(t *testing.T) { Author: author, Committer: committer, Message: "commit to b", - Parent: parentCommitSHA, + Parent: parentCommitSHA.String(), Actions: []Action{UpdateFile{Path: "file", OID: oidB.String()}}, }) require.NoError(t, err) @@ -80,7 +80,7 @@ func TestExecutor_Apply(t *testing.T) { Author: author, Committer: committer, Message: "commit a -> b", - Parent: updateToA, + Parent: updateToA.String(), Actions: []Action{UpdateFile{Path: "file", OID: oidB.String()}}, }) require.NoError(t, err) @@ -94,16 +94,16 @@ func TestExecutor_Apply(t *testing.T) { }) require.NoError(t, err) - diffBetween := func(t testing.TB, fromCommit, toCommit string) []byte { + diffBetween := func(t testing.TB, fromCommit, toCommit git.ObjectID) []byte { t.Helper() return testhelper.MustRunCommand(t, nil, - "git", "-C", repoPath, "format-patch", "--stdout", fromCommit+".."+toCommit) + "git", "-C", repoPath, "format-patch", "--stdout", fromCommit.String()+".."+toCommit.String()) } for _, tc := range []struct { desc string patches []Patch - parentCommit string + parentCommit git.ObjectID tree []gittest.TreeEntry error error }{ @@ -198,7 +198,7 @@ func TestExecutor_Apply(t *testing.T) { commitID, err := executor.Apply(ctx, ApplyParams{ Repository: repoPath, Committer: committer, - ParentCommit: parentCommitSHA, + ParentCommit: parentCommitSHA.String(), Patches: NewSlicePatchIterator(tc.patches), }) if tc.error != nil { @@ -212,7 +212,7 @@ func TestExecutor_Apply(t *testing.T) { Committer: committer, Message: tc.patches[len(tc.patches)-1].Message, }, getCommit(t, ctx, repo, commitID)) - gittest.RequireTree(t, config.Config.Git.BinPath, repoPath, commitID, tc.tree) + gittest.RequireTree(t, config.Config.Git.BinPath, repoPath, commitID.String(), tc.tree) }) } } diff --git a/internal/git2go/cherry_pick.go b/internal/git2go/cherry_pick.go index 71c7c6f06..fb5b9de4f 100644 --- a/internal/git2go/cherry_pick.go +++ b/internal/git2go/cherry_pick.go @@ -4,6 +4,7 @@ import ( "context" "time" + "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" ) @@ -28,6 +29,6 @@ type CherryPickCommand struct { } // Run performs a cherry pick via gitaly-git2go. -func (m CherryPickCommand) Run(ctx context.Context, cfg config.Cfg) (string, error) { +func (m CherryPickCommand) Run(ctx context.Context, cfg config.Cfg) (git.ObjectID, error) { return runWithGob(ctx, cfg, "cherry-pick", m) } diff --git a/internal/git2go/commit.go b/internal/git2go/commit.go index 558c82bd9..8f0f03256 100644 --- a/internal/git2go/commit.go +++ b/internal/git2go/commit.go @@ -5,6 +5,8 @@ import ( "context" "encoding/gob" "fmt" + + "gitlab.com/gitlab-org/gitaly/internal/git" ) // IndexError is an error that was produced by performing an invalid operation on the index. @@ -57,7 +59,7 @@ type CommitParams struct { // Commit builds a commit from the actions, writes it to the object database and // returns its object id. -func (b Executor) Commit(ctx context.Context, params CommitParams) (string, error) { +func (b Executor) Commit(ctx context.Context, params CommitParams) (git.ObjectID, error) { input := &bytes.Buffer{} if err := gob.NewEncoder(input).Encode(params); err != nil { return "", err @@ -73,5 +75,14 @@ func (b Executor) Commit(ctx context.Context, params CommitParams) (string, erro return "", err } - return result.CommitID, result.Error + if result.Error != nil { + return "", result.Error + } + + commitID, err := git.NewObjectIDFromHex(result.CommitID) + if err != nil { + return "", fmt.Errorf("could not parse commit ID: %w", err) + } + + return commitID, nil } diff --git a/internal/git2go/commit_test.go b/internal/git2go/commit_test.go index c4c8427f7..ea6ecc5c2 100644 --- a/internal/git2go/commit_test.go +++ b/internal/git2go/commit_test.go @@ -33,7 +33,7 @@ func testMain(m *testing.M) int { } type commit struct { - Parent string + Parent git.ObjectID Author Signature Committer Signature Message string @@ -464,7 +464,7 @@ func TestExecutor_Commit(t *testing.T) { t.Run(tc.desc, func(t *testing.T) { author := NewSignature("Author Name", "author.email@example.com", time.Now()) committer := NewSignature("Committer Name", "committer.email@example.com", time.Now()) - var parentCommit string + var parentCommit git.ObjectID for i, step := range tc.steps { message := fmt.Sprintf("commit %d", i+1) commitID, err := executor.Commit(ctx, CommitParams{ @@ -472,7 +472,7 @@ func TestExecutor_Commit(t *testing.T) { Author: author, Committer: committer, Message: message, - Parent: parentCommit, + Parent: parentCommit.String(), Actions: step.actions, }) @@ -490,17 +490,17 @@ func TestExecutor_Commit(t *testing.T) { Message: message, }, getCommit(t, ctx, repo, commitID)) - gittest.RequireTree(t, config.Config.Git.BinPath, repoPath, commitID, step.treeEntries) + gittest.RequireTree(t, config.Config.Git.BinPath, repoPath, commitID.String(), step.treeEntries) parentCommit = commitID } }) } } -func getCommit(t testing.TB, ctx context.Context, repo *localrepo.Repo, oid string) commit { +func getCommit(t testing.TB, ctx context.Context, repo *localrepo.Repo, oid git.ObjectID) commit { t.Helper() - data, err := repo.ReadObject(ctx, git.ObjectID(oid)) + data, err := repo.ReadObject(ctx, oid) require.NoError(t, err) var commit commit @@ -518,7 +518,7 @@ func getCommit(t testing.TB, ctx context.Context, repo *localrepo.Repo, oid stri switch field { case "parent": require.Empty(t, commit.Parent, "multi parent parsing not implemented") - commit.Parent = value + commit.Parent = git.ObjectID(value) case "author": require.Empty(t, commit.Author, "commit contained multiple authors") commit.Author = unmarshalSignature(t, value) diff --git a/internal/git2go/gob.go b/internal/git2go/gob.go index 4e760b437..7c4ef0554 100644 --- a/internal/git2go/gob.go +++ b/internal/git2go/gob.go @@ -8,6 +8,7 @@ import ( "fmt" "reflect" + "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" ) @@ -86,7 +87,7 @@ func SerializableError(err error) error { // runWithGob runs the specified gitaly-git2go cmd with the request gob-encoded // as input and returns the commit ID as string or an error. -func runWithGob(ctx context.Context, cfg config.Cfg, cmd string, request interface{}) (string, error) { +func runWithGob(ctx context.Context, cfg config.Cfg, cmd string, request interface{}) (git.ObjectID, error) { input := &bytes.Buffer{} if err := gob.NewEncoder(input).Encode(request); err != nil { return "", fmt.Errorf("%s: %w", cmd, err) @@ -106,5 +107,10 @@ func runWithGob(ctx context.Context, cfg config.Cfg, cmd string, request interfa return "", fmt.Errorf("%s: %w", cmd, result.Error) } - return result.CommitID, nil + commitID, err := git.NewObjectIDFromHex(result.CommitID) + if err != nil { + return "", fmt.Errorf("could not parse commit ID: %w", err) + } + + return commitID, nil } diff --git a/internal/git2go/revert.go b/internal/git2go/revert.go index 8e7af30ba..7dd1d7420 100644 --- a/internal/git2go/revert.go +++ b/internal/git2go/revert.go @@ -4,6 +4,7 @@ import ( "context" "time" + "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" ) @@ -26,6 +27,6 @@ type RevertCommand struct { Mainline uint `json:"mainline"` } -func (r RevertCommand) Run(ctx context.Context, cfg config.Cfg) (string, error) { +func (r RevertCommand) Run(ctx context.Context, cfg config.Cfg) (git.ObjectID, error) { return runWithGob(ctx, cfg, "revert", r) } diff --git a/internal/gitaly/service/operations/cherry_pick.go b/internal/gitaly/service/operations/cherry_pick.go index eaf2f5015..3818fd240 100644 --- a/internal/gitaly/service/operations/cherry_pick.go +++ b/internal/gitaly/service/operations/cherry_pick.go @@ -96,11 +96,11 @@ func (s *Server) userCherryPick(ctx context.Context, req *gitalypb.UserCherryPic } if req.DryRun { - newrev = startRevision.String() + newrev = startRevision } if !branchCreated { - ancestor, err := s.isAncestor(ctx, req.Repository, oldrev.String(), newrev) + ancestor, err := s.isAncestor(ctx, req.Repository, oldrev.String(), newrev.String()) if err != nil { return nil, err } @@ -111,7 +111,7 @@ func (s *Server) userCherryPick(ctx context.Context, req *gitalypb.UserCherryPic } } - if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, branch, newrev, oldrev.String()); err != nil { + if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, branch, newrev.String(), oldrev.String()); err != nil { if errors.As(err, &preReceiveError{}) { return &gitalypb.UserCherryPickResponse{ PreReceiveError: err.Error(), @@ -123,7 +123,7 @@ func (s *Server) userCherryPick(ctx context.Context, req *gitalypb.UserCherryPic return &gitalypb.UserCherryPickResponse{ BranchUpdate: &gitalypb.OperationBranchUpdate{ - CommitId: newrev, + CommitId: newrev.String(), BranchCreated: branchCreated, RepoCreated: !repoHadBranches, }, diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index 5b7698fab..8e855131f 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -363,7 +363,7 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi oldRevision = targetBranchCommit } - if err := s.updateReferenceWithHooks(ctx, header.Repository, header.User, targetBranchName.String(), commitID, oldRevision.String()); err != nil { + if err := s.updateReferenceWithHooks(ctx, header.Repository, header.User, targetBranchName.String(), commitID.String(), oldRevision.String()); err != nil { if errors.As(err, &updateRefError{}) { return status.Errorf(codes.FailedPrecondition, err.Error()) } @@ -372,7 +372,7 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi } return stream.SendAndClose(&gitalypb.UserCommitFilesResponse{BranchUpdate: &gitalypb.OperationBranchUpdate{ - CommitId: commitID, + CommitId: commitID.String(), RepoCreated: !hasBranches, BranchCreated: parentCommitOID == "", }}) diff --git a/internal/gitaly/service/operations/revert.go b/internal/gitaly/service/operations/revert.go index bebee71ba..31b815b0c 100644 --- a/internal/gitaly/service/operations/revert.go +++ b/internal/gitaly/service/operations/revert.go @@ -89,11 +89,11 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest } if req.DryRun { - newrev = startRevision.String() + newrev = startRevision } if !branchCreated { - ancestor, err := s.isAncestor(ctx, req.Repository, oldrev.String(), newrev) + ancestor, err := s.isAncestor(ctx, req.Repository, oldrev.String(), newrev.String()) if err != nil { return nil, err } @@ -104,7 +104,7 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest } } - if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, branch, newrev, oldrev.String()); err != nil { + if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, branch, newrev.String(), oldrev.String()); err != nil { var preReceiveError preReceiveError if errors.As(err, &preReceiveError) { return &gitalypb.UserRevertResponse{ @@ -117,7 +117,7 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest return &gitalypb.UserRevertResponse{ BranchUpdate: &gitalypb.OperationBranchUpdate{ - CommitId: newrev, + CommitId: newrev.String(), BranchCreated: branchCreated, RepoCreated: !repoHadBranches, }, |