Merge branch 'smh-is-ancestor' into 'master'zj-test

Add IsAncestor to localrepo

See merge request gitlab-org/gitaly!3244

5 files changed, 110 insertions, 30 deletions

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)
+		})
+	}
+}
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
 		}