diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2021-01-18 17:16:51 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2021-01-19 17:33:03 +0300 |
commit | f1134accb38db6d3a6d38e672476383cde2f0b0a (patch) | |
tree | a9ddebcaeb9719a90f39f4be691331818d73c647 | |
parent | 36d22e0a6c4e3c148e070bb541b1e546382a061a (diff) |
git: Convert ResolveRevision to return an ObjectID
The `ResolveRevision()` function currently returns the resolved object
ID as a string, which makes it hard to see what it actually is. This
commit converts it to return an ObjectID instead and adjusts all
callers.
-rw-r--r-- | internal/git/remoterepo/repository.go | 11 | ||||
-rw-r--r-- | internal/git/repository.go | 11 | ||||
-rw-r--r-- | internal/git/repository_suite.go | 2 | ||||
-rw-r--r-- | internal/git/repository_test.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/conflicts/list_conflict_files.go | 4 | ||||
-rw-r--r-- | internal/gitaly/service/conflicts/list_conflict_files_test.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/conflicts/resolve_conflicts.go | 2 | ||||
-rw-r--r-- | internal/gitaly/service/operations/commit_files.go | 8 | ||||
-rw-r--r-- | internal/gitaly/service/operations/merge.go | 16 | ||||
-rw-r--r-- | internal/gitaly/service/operations/revert.go | 14 | ||||
-rw-r--r-- | internal/gitaly/service/repository/fetch.go | 10 |
11 files changed, 44 insertions, 38 deletions
diff --git a/internal/git/remoterepo/repository.go b/internal/git/remoterepo/repository.go index 08342b191..ce3230109 100644 --- a/internal/git/remoterepo/repository.go +++ b/internal/git/remoterepo/repository.go @@ -34,7 +34,7 @@ func New(ctx context.Context, repo *gitalypb.Repository, pool *client.Pool) (Rep // ResolveRevision will dial to the remote repository and attempt to resolve the // revision string via the gRPC interface. -func (rr Repository) ResolveRevision(ctx context.Context, revision git.Revision) (string, error) { +func (rr Repository) ResolveRevision(ctx context.Context, revision git.Revision) (git.ObjectID, error) { cli := gitalypb.NewCommitServiceClient(rr.conn) resp, err := cli.FindCommit(ctx, &gitalypb.FindCommitRequest{ Repository: rr.repo, @@ -44,11 +44,16 @@ func (rr Repository) ResolveRevision(ctx context.Context, revision git.Revision) return "", err } - oid := resp.GetCommit().GetId() - if oid == "" { + oidHex := resp.GetCommit().GetId() + if oidHex == "" { return "", git.ErrReferenceNotFound } + oid, err := git.NewObjectIDFromHex(oidHex) + if err != nil { + return "", err + } + return oid, nil } diff --git a/internal/git/repository.go b/internal/git/repository.go index 3d8941627..bdc1470db 100644 --- a/internal/git/repository.go +++ b/internal/git/repository.go @@ -107,7 +107,7 @@ type Repository interface { // for accepted syntax. This will not verify whether the object ID // exists. To do so, you can peel the reference to a given object type, // e.g. by passing `refs/heads/master^{commit}`. - ResolveRevision(ctx context.Context, revision Revision) (string, error) + ResolveRevision(ctx context.Context, revision Revision) (ObjectID, error) // HasBranches returns whether the repository has branches. HasBranches(ctx context.Context) (bool, error) } @@ -289,7 +289,7 @@ func (repo *LocalRepository) ReadObject(ctx context.Context, oid string) ([]byte // reference to a given object type, e.g. by passing // `refs/heads/master^{commit}`. Returns an ErrReferenceNotFound error in case // the revision does not exist. -func (repo *LocalRepository) ResolveRevision(ctx context.Context, revision Revision) (string, error) { +func (repo *LocalRepository) ResolveRevision(ctx context.Context, revision Revision) (ObjectID, error) { if revision.String() == "" { return "", errors.New("repository cannot contain empty reference name") } @@ -313,9 +313,10 @@ func (repo *LocalRepository) ResolveRevision(ctx context.Context, revision Revis return "", err } - oid := strings.TrimSpace(stdout.String()) - if len(oid) != 40 { - return "", fmt.Errorf("unsupported object hash %q", oid) + hex := strings.TrimSpace(stdout.String()) + oid, err := NewObjectIDFromHex(hex) + if err != nil { + return "", fmt.Errorf("unsupported object hash %q: %w", hex, err) } return oid, nil diff --git a/internal/git/repository_suite.go b/internal/git/repository_suite.go index 539cf63d1..0ed26ea65 100644 --- a/internal/git/repository_suite.go +++ b/internal/git/repository_suite.go @@ -40,7 +40,7 @@ func testRepositoryResolveRevision(t *testing.T, getRepository func(testing.TB, for _, tc := range []struct { desc string revision string - expected string + expected ObjectID }{ { desc: "unqualified master branch", diff --git a/internal/git/repository_test.go b/internal/git/repository_test.go index dce4a559f..341c2362c 100644 --- a/internal/git/repository_test.go +++ b/internal/git/repository_test.go @@ -574,7 +574,7 @@ func TestLocalRepository_FetchRemote(t *testing.T) { sha, err := repo.ResolveRevision(ctx, Revision("refs/remotes/origin/master^{commit}")) require.NoError(t, err, "the object from remote should exists in local after fetch done") - require.Equal(t, "1e292f8fedd741b75372e19097c76d327140c312", sha) + require.Equal(t, ObjectID("1e292f8fedd741b75372e19097c76d327140c312"), sha) }) t.Run("with env", func(t *testing.T) { diff --git a/internal/gitaly/service/conflicts/list_conflict_files.go b/internal/gitaly/service/conflicts/list_conflict_files.go index 996399912..20078ec0e 100644 --- a/internal/gitaly/service/conflicts/list_conflict_files.go +++ b/internal/gitaly/service/conflicts/list_conflict_files.go @@ -40,8 +40,8 @@ func (s *server) ListConflictFiles(request *gitalypb.ListConflictFilesRequest, s conflicts, err := git2go.ConflictsCommand{ Repository: repoPath, - Ours: ours, - Theirs: theirs, + Ours: ours.String(), + Theirs: theirs.String(), }.Run(ctx, s.cfg) if err != nil { if errors.Is(err, git2go.ErrInvalidArgument) { diff --git a/internal/gitaly/service/conflicts/list_conflict_files_test.go b/internal/gitaly/service/conflicts/list_conflict_files_test.go index c5c52c811..e33e8e1d1 100644 --- a/internal/gitaly/service/conflicts/list_conflict_files_test.go +++ b/internal/gitaly/service/conflicts/list_conflict_files_test.go @@ -213,7 +213,7 @@ func buildCommit(t *testing.T, ctx context.Context, repo *gitalypb.Repository, r testhelper.MustRunCommand(t, nil, "git", "-C", repoPath, "reset", "--hard", "HEAD~") - return oid + return oid.String() } func TestListConflictFilesFailedPrecondition(t *testing.T) { diff --git a/internal/gitaly/service/conflicts/resolve_conflicts.go b/internal/gitaly/service/conflicts/resolve_conflicts.go index 024e2df1a..ce026e194 100644 --- a/internal/gitaly/service/conflicts/resolve_conflicts.go +++ b/internal/gitaly/service/conflicts/resolve_conflicts.go @@ -288,7 +288,7 @@ func (s *server) repoWithBranchCommit(ctx context.Context, srcRepo, targetRepo * git.SubCmd{ Name: "fetch", Flags: []git.Option{git.Flag{Name: "--no-tags"}}, - Args: []string{gitalyssh.GitalyInternalURL, oid}, + Args: []string{gitalyssh.GitalyInternalURL, oid.String()}, }, git.WithEnv(env...), ) diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index e5b68c1af..449176f33 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -189,7 +189,7 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi localRepo, remoteRepo, targetBranchName, - targetBranchCommit, + targetBranchCommit.String(), string(header.StartBranchName), ) if err != nil { @@ -197,7 +197,7 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi } } - if parentCommitOID != targetBranchCommit { + if parentCommitOID != targetBranchCommit.String() { if err := s.fetchMissingCommit(ctx, header.Repository, remoteRepo, parentCommitOID); err != nil { return fmt.Errorf("fetch missing commit: %w", err) } @@ -343,7 +343,7 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi if targetBranchCommit == "" { oldRevision = git.ZeroOID.String() } else if header.Force { - oldRevision = targetBranchCommit + oldRevision = targetBranchCommit.String() } if err := s.updateReferenceWithHooks(ctx, header.Repository, header.User, targetBranchName, commitID, oldRevision); err != nil { @@ -411,7 +411,7 @@ func (s *Server) resolveParentCommit(ctx context.Context, local git.Repository, return "", fmt.Errorf("resolving refish %q in %T: %w", refish, repo, err) } - return commit, nil + return commit.String(), nil } func (s *Server) fetchMissingCommit(ctx context.Context, local, remote *gitalypb.Repository, commitID string) error { diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go index 947274a58..de440a181 100644 --- a/internal/gitaly/service/operations/merge.go +++ b/internal/gitaly/service/operations/merge.go @@ -118,7 +118,7 @@ func (s *Server) userMergeBranch(stream gitalypb.OperationService_UserMergeBranc AuthorName: string(firstRequest.User.Name), AuthorMail: string(firstRequest.User.Email), Message: string(firstRequest.Message), - Ours: revision, + Ours: revision.String(), Theirs: firstRequest.CommitId, }.Run(ctx, s.cfg) if err != nil { @@ -142,7 +142,7 @@ func (s *Server) userMergeBranch(stream gitalypb.OperationService_UserMergeBranc return helper.ErrPreconditionFailedf("merge aborted by client") } - if err := s.updateReferenceWithHooks(ctx, firstRequest.Repository, firstRequest.User, branch, merge.CommitID, revision); err != nil { + if err := s.updateReferenceWithHooks(ctx, firstRequest.Repository, firstRequest.User, branch, merge.CommitID, revision.String()); err != nil { var preReceiveError preReceiveError var updateRefError updateRefError @@ -204,7 +204,7 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ return nil, helper.ErrInvalidArgument(err) } - ancestor, err := isAncestor(ctx, in.Repository, revision, in.CommitId) + ancestor, err := isAncestor(ctx, in.Repository, revision.String(), in.CommitId) if err != nil { return nil, err } @@ -212,7 +212,7 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ return nil, helper.ErrPreconditionFailedf("not fast forward") } - if err := s.updateReferenceWithHooks(ctx, in.Repository, in.User, branch, in.CommitId, revision); err != nil { + if err := s.updateReferenceWithHooks(ctx, in.Repository, in.User, branch, in.CommitId, revision.String()); err != nil { var preReceiveError preReceiveError if errors.As(err, &preReceiveError) { return &gitalypb.UserFFBranchResponse{ @@ -305,7 +305,7 @@ func (s *Server) userMergeToRef(ctx context.Context, request *gitalypb.UserMerge } // First, overwrite the reference with the target reference. - if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), oid, ""); err != nil { + if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), oid.String(), ""); err != nil { return nil, updateRefError{reference: string(request.TargetRef)} } @@ -315,8 +315,8 @@ func (s *Server) userMergeToRef(ctx context.Context, request *gitalypb.UserMerge AuthorName: string(request.User.Name), AuthorMail: string(request.User.Email), Message: string(request.Message), - Ours: oid, - Theirs: sourceRef, + Ours: oid.String(), + Theirs: sourceRef.String(), }.Run(ctx, s.cfg) if err != nil { if errors.Is(err, git2go.ErrInvalidArgument) { @@ -328,7 +328,7 @@ func (s *Server) userMergeToRef(ctx context.Context, request *gitalypb.UserMerge // ... and move branch from target ref to the merge commit. The Ruby // implementation doesn't invoke hooks, so we don't either. - if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), merge.CommitID, oid); err != nil { + if err := repo.UpdateRef(ctx, git.ReferenceName(request.TargetRef), merge.CommitID, oid.String()); err != nil { //nolint:stylecheck return nil, helper.ErrPreconditionFailed(fmt.Errorf("Could not update %s. Please refresh and try again", string(request.TargetRef))) } diff --git a/internal/gitaly/service/operations/revert.go b/internal/gitaly/service/operations/revert.go index ea9b19ba6..021e75e7b 100644 --- a/internal/gitaly/service/operations/revert.go +++ b/internal/gitaly/service/operations/revert.go @@ -71,7 +71,7 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest oldrev, err := localRepo.ResolveRevision(ctx, git.Revision(fmt.Sprintf("%s^{commit}", branch))) if errors.Is(err, git.ErrReferenceNotFound) { branchCreated = true - oldrev = git.ZeroOID.String() + oldrev = git.ZeroOID } else if err != nil { return nil, helper.ErrInvalidArgumentf("resolve ref: %w", err) } @@ -81,7 +81,7 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest } if !branchCreated { - ancestor, err := isAncestor(ctx, req.Repository, oldrev, newrev) + ancestor, err := isAncestor(ctx, req.Repository, oldrev.String(), newrev) if err != nil { return nil, err } @@ -92,7 +92,7 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest } } - if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, branch, newrev, oldrev); err != nil { + if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, branch, newrev, oldrev.String()); err != nil { var preReceiveError preReceiveError if errors.As(err, &preReceiveError) { return &gitalypb.UserRevertResponse{ @@ -147,17 +147,17 @@ func (s *Server) fetchStartRevision(ctx context.Context, req *gitalypb.UserRever } if req.StartRepository == nil { - return startRevision, nil + return startRevision.String(), nil } - _, err = git.NewRepository(req.Repository).ResolveRevision(ctx, git.Revision(fmt.Sprintf("%s^{commit}", startRevision))) + _, err = git.NewRepository(req.Repository).ResolveRevision(ctx, startRevision.Revision()+"^{commit}") if errors.Is(err, git.ErrReferenceNotFound) { - if err := s.fetchRemoteObject(ctx, req.Repository, req.StartRepository, startRevision); err != nil { + if err := s.fetchRemoteObject(ctx, req.Repository, req.StartRepository, startRevision.String()); err != nil { return "", helper.ErrInternalf("fetch start: %w", err) } } else if err != nil { return "", helper.ErrInvalidArgumentf("resolve start: %w", err) } - return startRevision, nil + return startRevision.String(), nil } diff --git a/internal/gitaly/service/repository/fetch.go b/internal/gitaly/service/repository/fetch.go index 3d7d357a7..14c47238c 100644 --- a/internal/gitaly/service/repository/fetch.go +++ b/internal/gitaly/service/repository/fetch.go @@ -34,7 +34,7 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc return nil, helper.ErrInternal(err) } - var sourceOid string + var sourceOid git.ObjectID var containsObject bool // If source and target repository are the same, then we know that both @@ -67,7 +67,7 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc // Otherwise, if the source is a remote repository, we check // whether the target repo already contains the desired object. // If so, we can skip the fetch. - containsObject, err = targetRepo.HasRevision(ctx, git.Revision(sourceOid+"^{commit}")) + containsObject, err = targetRepo.HasRevision(ctx, sourceOid.Revision()+"^{commit}") if err != nil { return nil, helper.ErrInternal(err) } @@ -84,7 +84,7 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc cmd, err := git.NewCommand(ctx, req.Repository, nil, git.SubCmd{ Name: "fetch", - Args: []string{gitalyssh.GitalyInternalURL, sourceOid}, + Args: []string{gitalyssh.GitalyInternalURL, sourceOid.String()}, Flags: []git.Option{git.Flag{Name: "--no-tags"}, git.Flag{Name: "--quiet"}}, }, git.WithEnv(env...), @@ -96,13 +96,13 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc if err := cmd.Wait(); err != nil { // Design quirk: if the fetch fails, this RPC returns Result: false, but no error. ctxlogrus.Extract(ctx). - WithField("oid", sourceOid). + WithField("oid", sourceOid.String()). WithError(err).Warn("git fetch failed") return &gitalypb.FetchSourceBranchResponse{Result: false}, nil } } - if err := targetRepo.UpdateRef(ctx, git.ReferenceName(req.GetTargetRef()), sourceOid, ""); err != nil { + if err := targetRepo.UpdateRef(ctx, git.ReferenceName(req.GetTargetRef()), sourceOid.String(), ""); err != nil { return nil, err } |