From 44f9bfba28da3e883ceceae0fa49a698467ba7eb Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Thu, 11 Mar 2021 10:48:13 +0100 Subject: add IsAncestor to localrepo This commit adds IsAncestor to localrepo for checking whether a commit is the ancestor of another commit. The method captures not valid commit errors as the error needs to be handled in Go port of UpdateRemoteMirror later. --- internal/git/localrepo/objects.go | 36 ++++++++++++++++++ internal/git/localrepo/objects_test.go | 69 ++++++++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+) diff --git a/internal/git/localrepo/objects.go b/internal/git/localrepo/objects.go index b4b25eef9..312b3f593 100644 --- a/internal/git/localrepo/objects.go +++ b/internal/git/localrepo/objects.go @@ -9,6 +9,7 @@ import ( "strings" "time" + "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/internal/git/log" @@ -247,3 +248,38 @@ func (repo *Repo) ReadCommit(ctx context.Context, revision git.Revision, opts .. return commit, nil } + +// InvalidCommitError is returned when the revision does not point to a valid commit object. +type InvalidCommitError git.Revision + +func (err InvalidCommitError) Error() string { + return fmt.Sprintf("invalid commit: %q", string(err)) +} + +// IsAncestor returns whether the parent is an ancestor of the child. InvalidCommitError is returned +// if either revision does not point to a commit in the repository. +func (repo *Repo) IsAncestor(ctx context.Context, parent, child git.Revision) (bool, error) { + const notValidCommitName = "fatal: Not a valid commit name" + + stderr := &bytes.Buffer{} + if err := repo.ExecAndWait(ctx, + git.SubCmd{ + Name: "merge-base", + Flags: []git.Option{git.Flag{Name: "--is-ancestor"}}, + Args: []string{parent.String(), child.String()}, + }, + git.WithStderr(stderr), + ); err != nil { + status, ok := command.ExitStatus(err) + if ok && status == 1 { + return false, nil + } else if ok && strings.HasPrefix(stderr.String(), notValidCommitName) { + commitOID := strings.TrimSpace(strings.TrimPrefix(stderr.String(), notValidCommitName)) + return false, InvalidCommitError(commitOID) + } + + return false, fmt.Errorf("determine ancestry: %w, stderr: %q", err, stderr) + } + + return true, nil +} diff --git a/internal/git/localrepo/objects_test.go b/internal/git/localrepo/objects_test.go index d2926835f..ef293ef93 100644 --- a/internal/git/localrepo/objects_test.go +++ b/internal/git/localrepo/objects_test.go @@ -393,3 +393,72 @@ func TestRepo_ReadCommit(t *testing.T) { }) } } + +func TestRepo_IsAncestor(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() + + repo, _, cleanup := setupRepo(t, false) + defer cleanup() + + for _, tc := range []struct { + desc string + parent git.Revision + child git.Revision + isAncestor bool + errorMatcher func(testing.TB, error) + }{ + { + desc: "parent is ancestor", + parent: "HEAD~1", + child: "HEAD", + isAncestor: true, + }, + { + desc: "parent is not ancestor", + parent: "HEAD", + child: "HEAD~1", + isAncestor: false, + }, + { + desc: "parent is not valid commit", + parent: git.ZeroOID.Revision(), + child: "HEAD", + errorMatcher: func(t testing.TB, err error) { + require.Equal(t, InvalidCommitError(git.ZeroOID), err) + }, + }, + { + desc: "child is not valid commit", + parent: "HEAD", + child: git.ZeroOID.Revision(), + errorMatcher: func(t testing.TB, err error) { + require.Equal(t, InvalidCommitError(git.ZeroOID), err) + }, + }, + { + desc: "child points to a tree", + parent: "HEAD", + child: "HEAD^{tree}", + errorMatcher: func(t testing.TB, actualErr error) { + treeOID, err := repo.ResolveRevision(ctx, "HEAD^{tree}") + require.NoError(t, err) + require.EqualError(t, actualErr, fmt.Sprintf( + `determine ancestry: exit status 128, stderr: "error: object %s is a tree, not a commit\nfatal: Not a valid commit name HEAD^{tree}\n"`, + treeOID, + )) + }, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + isAncestor, err := repo.IsAncestor(ctx, tc.parent, tc.child) + if tc.errorMatcher != nil { + tc.errorMatcher(t, err) + return + } + + require.NoError(t, err) + require.Equal(t, tc.isAncestor, isAncestor) + }) + } +} -- cgit v1.2.3 From 53ea4846e77086bf6766a1dca6c8949b39048f3b Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Thu, 11 Mar 2021 12:42:21 +0100 Subject: replace existing isAncestor implementations with local repo's one Operations service contains a method for checking ancestry of a commit. This commit replaces the implementation with the one that was now added to localrepo. --- internal/gitaly/service/operations/cherry_pick.go | 2 +- internal/gitaly/service/operations/merge.go | 31 +++-------------------- internal/gitaly/service/operations/revert.go | 2 +- 3 files changed, 5 insertions(+), 30 deletions(-) diff --git a/internal/gitaly/service/operations/cherry_pick.go b/internal/gitaly/service/operations/cherry_pick.go index a38448c84..22205ba87 100644 --- a/internal/gitaly/service/operations/cherry_pick.go +++ b/internal/gitaly/service/operations/cherry_pick.go @@ -99,7 +99,7 @@ func (s *Server) userCherryPick(ctx context.Context, req *gitalypb.UserCherryPic } if !branchCreated { - ancestor, err := s.isAncestor(ctx, req.Repository, oldrev, newrev) + ancestor, err := localRepo.IsAncestor(ctx, oldrev.Revision(), newrev.Revision()) if err != nil { return nil, err } diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go index 98f10a709..603b4706f 100644 --- a/internal/gitaly/service/operations/merge.go +++ b/internal/gitaly/service/operations/merge.go @@ -8,10 +8,8 @@ import ( "time" "github.com/golang/protobuf/ptypes" - "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/localrepo" - "gitlab.com/gitlab-org/gitaly/internal/git/repository" "gitlab.com/gitlab-org/gitaly/internal/git2go" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -159,7 +157,8 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ referenceName := git.NewReferenceNameFromBranchName(string(in.Branch)) - revision, err := localrepo.New(s.gitCmdFactory, in.Repository, s.cfg).ResolveRevision(ctx, referenceName.Revision()) + repo := localrepo.New(s.gitCmdFactory, in.Repository, s.cfg) + revision, err := repo.ResolveRevision(ctx, referenceName.Revision()) if err != nil { return nil, helper.ErrInvalidArgument(err) } @@ -169,7 +168,7 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ return nil, helper.ErrInvalidArgumentf("cannot parse commit ID: %w", err) } - ancestor, err := s.isAncestor(ctx, in.Repository, revision, commitID) + ancestor, err := repo.IsAncestor(ctx, revision.Revision(), commitID.Revision()) if err != nil { return nil, err } @@ -306,27 +305,3 @@ func (s *Server) UserMergeToRef(ctx context.Context, request *gitalypb.UserMerge CommitId: mergeOID.String(), }, nil } - -func (s *Server) isAncestor(ctx context.Context, repo repository.GitRepo, ancestor, descendant git.ObjectID) (bool, error) { - cmd, err := s.gitCmdFactory.New(ctx, repo, git.SubCmd{ - Name: "merge-base", - Flags: []git.Option{git.Flag{Name: "--is-ancestor"}}, - Args: []string{ancestor.String(), descendant.String()}, - }) - if err != nil { - return false, helper.ErrInternalf("isAncestor: %w", err) - } - if err := cmd.Wait(); err != nil { - status, ok := command.ExitStatus(err) - if !ok { - return false, helper.ErrInternalf("isAncestor: %w", err) - } - // --is-ancestor errors are signaled by a non-zero status that is not 1. - // https://git-scm.com/docs/git-merge-base#Documentation/git-merge-base.txt---is-ancestor - if status != 1 { - return false, helper.ErrInvalidArgumentf("isAncestor: %w", err) - } - return false, nil - } - return true, nil -} diff --git a/internal/gitaly/service/operations/revert.go b/internal/gitaly/service/operations/revert.go index 8192e5363..fce7ebdfc 100644 --- a/internal/gitaly/service/operations/revert.go +++ b/internal/gitaly/service/operations/revert.go @@ -93,7 +93,7 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest } if !branchCreated { - ancestor, err := s.isAncestor(ctx, req.Repository, oldrev, newrev) + ancestor, err := localRepo.IsAncestor(ctx, oldrev.Revision(), newrev.Revision()) if err != nil { return nil, err } -- cgit v1.2.3