Merge branch 'pks-operations-squash-rebase' into 'master'

operations: Support worktreeless squashing for non-fast-forward merges

Closes #3690

See merge request gitlab-org/gitaly!3714

5 files changed, 168 insertions, 83 deletions

diff --git a/cmd/gitaly-git2go/rebase.go b/cmd/gitaly-git2go/rebase.go
index 632a780bb..8a6a66e12 100644
--- a/cmd/gitaly-git2go/rebase.go
+++ b/cmd/gitaly-git2go/rebase.go
@@ -44,12 +44,18 @@ func (cmd *rebaseSubcommand) verify(ctx context.Context, r *git2go.RebaseCommand
 	if r.Committer.Email == "" {
 		return errors.New("missing committer email")
 	}
-	if r.BranchName == "" {
+	if r.BranchName == "" && r.CommitID == "" {
 		return errors.New("missing branch name")
 	}
-	if r.UpstreamRevision == "" {
+	if r.BranchName != "" && r.CommitID != "" {
+		return errors.New("both branch name and commit ID")
+	}
+	if r.UpstreamRevision == "" && r.UpstreamCommitID == "" {
 		return errors.New("missing upstream revision")
 	}
+	if r.UpstreamRevision != "" && r.UpstreamCommitID != "" {
+		return errors.New("both upstream revision and upstream commit ID")
+	}
 	return nil
 }
 
@@ -69,34 +75,52 @@ func (cmd *rebaseSubcommand) rebase(ctx context.Context, request *git2go.RebaseC
 	}
 	opts.InMemory = 1
 
-	branch, err := repo.AnnotatedCommitFromRevspec(fmt.Sprintf("refs/heads/%s", request.BranchName))
-	if err != nil {
-		return "", fmt.Errorf("look up branch %q: %w", request.BranchName, err)
+	var commit *git.AnnotatedCommit
+	if request.BranchName != "" {
+		commit, err = repo.AnnotatedCommitFromRevspec(fmt.Sprintf("refs/heads/%s", request.BranchName))
+		if err != nil {
+			return "", fmt.Errorf("look up branch %q: %w", request.BranchName, err)
+		}
+	} else {
+		commitOid, err := git.NewOid(request.CommitID.String())
+		if err != nil {
+			return "", fmt.Errorf("parse commit %q: %w", request.CommitID, err)
+		}
+
+		commit, err = repo.LookupAnnotatedCommit(commitOid)
+		if err != nil {
+			return "", fmt.Errorf("look up commit %q: %w", request.CommitID, err)
+		}
+	}
+
+	upstreamCommitParam := request.UpstreamRevision
+	if upstreamCommitParam == "" {
+		upstreamCommitParam = request.UpstreamCommitID.String()
 	}
 
-	ontoOid, err := git.NewOid(request.UpstreamRevision)
+	upstreamCommitOID, err := git.NewOid(upstreamCommitParam)
 	if err != nil {
-		return "", fmt.Errorf("parse upstream revision %q: %w", request.UpstreamRevision, err)
+		return "", fmt.Errorf("parse upstream revision %q: %w", upstreamCommitParam, err)
 	}
 
-	onto, err := repo.LookupAnnotatedCommit(ontoOid)
+	upstreamCommit, err := repo.LookupAnnotatedCommit(upstreamCommitOID)
 	if err != nil {
-		return "", fmt.Errorf("look up upstream revision %q: %w", request.UpstreamRevision, err)
+		return "", fmt.Errorf("look up upstream revision %q: %w", upstreamCommitParam, err)
 	}
 
-	mergeBase, err := repo.MergeBase(onto.Id(), branch.Id())
+	mergeBase, err := repo.MergeBase(upstreamCommit.Id(), commit.Id())
 	if err != nil {
 		return "", fmt.Errorf("find merge base: %w", err)
 	}
 
-	if mergeBase.Equal(onto.Id()) {
+	if mergeBase.Equal(upstreamCommit.Id()) {
 		// Branch is zero commits behind, so do not rebase
-		return branch.Id().String(), nil
+		return commit.Id().String(), nil
 	}
 
-	if mergeBase.Equal(branch.Id()) {
+	if mergeBase.Equal(commit.Id()) {
 		// Branch is merged, so fast-forward to upstream
-		return onto.Id().String(), nil
+		return upstreamCommit.Id().String(), nil
 	}
 
 	mergeCommit, err := repo.LookupAnnotatedCommit(mergeBase)
@@ -104,7 +128,7 @@ func (cmd *rebaseSubcommand) rebase(ctx context.Context, request *git2go.RebaseC
 		return "", fmt.Errorf("look up merge base: %w", err)
 	}
 
-	rebase, err := repo.InitRebase(branch, mergeCommit, onto, &opts)
+	rebase, err := repo.InitRebase(commit, mergeCommit, upstreamCommit, &opts)
 	if err != nil {
 		return "", fmt.Errorf("initiate rebase: %w", err)
 	}
@@ -132,7 +156,7 @@ func (cmd *rebaseSubcommand) rebase(ctx context.Context, request *git2go.RebaseC
 	}
 
 	if oid == nil {
-		return branch.Id().String(), nil
+		return commit.Id().String(), nil
 	}
 
 	if err = rebase.Finish(); err != nil {
diff --git a/cmd/gitaly-git2go/rebase_test.go b/cmd/gitaly-git2go/rebase_test.go
index b8995d3bd..275adc757 100644
--- a/cmd/gitaly-git2go/rebase_test.go
+++ b/cmd/gitaly-git2go/rebase_test.go
@@ -10,6 +10,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v14/cmd/gitaly-git2go/git2goutil"
 	cmdtesthelper "gitlab.com/gitlab-org/gitaly/v14/cmd/gitaly-git2go/testhelper"
+	gitalygit "gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
@@ -61,6 +62,16 @@ func TestRebase_validation(t *testing.T) {
 			request:     git2go.RebaseCommand{Repository: repoPath, Committer: committer, BranchName: "feature"},
 			expectedErr: "rebase: missing upstream revision",
 		},
+		{
+			desc:        "both branch name and commit ID",
+			request:     git2go.RebaseCommand{Repository: repoPath, Committer: committer, BranchName: "feature", CommitID: "a"},
+			expectedErr: "rebase: both branch name and commit ID",
+		},
+		{
+			desc:        "both upstream revision and upstream commit ID",
+			request:     git2go.RebaseCommand{Repository: repoPath, Committer: committer, BranchName: "feature", UpstreamRevision: "a", UpstreamCommitID: "a"},
+			expectedErr: "rebase: both upstream revision and upstream commit ID",
+		},
 	}
 	for _, tc := range testcases {
 		t.Run(tc.desc, func(t *testing.T) {
@@ -176,31 +187,56 @@ func TestRebase_rebase(t *testing.T) {
 				tc.setupRepo(t, repo)
 			}
 
-			request := git2go.RebaseCommand{
-				Repository:       repoPath,
-				Committer:        committer,
-				BranchName:       tc.branch,
-				UpstreamRevision: masterRevision,
-			}
-
-			response, err := executor.Rebase(ctx, repoProto, request)
-			if tc.expectedErr != "" {
-				require.EqualError(t, err, tc.expectedErr)
-			} else {
-				require.NoError(t, err)
-
-				result := response.String()
-				require.Equal(t, tc.expected, result)
-
-				commit, err := lookupCommit(repo, result)
-				require.NoError(t, err)
+			branchCommit, err := lookupCommit(repo, tc.branch)
+			require.NoError(t, err)
 
-				for i := tc.commitsAhead; i > 0; i-- {
-					commit = commit.Parent(0)
-				}
-				masterCommit, err := lookupCommit(repo, masterRevision)
-				require.NoError(t, err)
-				require.Equal(t, masterCommit, commit)
+			for desc, request := range map[string]git2go.RebaseCommand{
+				"with branch and upstream": {
+					Repository:       repoPath,
+					Committer:        committer,
+					BranchName:       tc.branch,
+					UpstreamRevision: masterRevision,
+				},
+				"with branch and upstream commit ID": {
+					Repository:       repoPath,
+					Committer:        committer,
+					BranchName:       tc.branch,
+					UpstreamCommitID: gitalygit.ObjectID(masterRevision),
+				},
+				"with commit ID and upstream": {
+					Repository:       repoPath,
+					Committer:        committer,
+					BranchName:       tc.branch,
+					UpstreamRevision: masterRevision,
+				},
+				"with commit ID and upstream commit ID": {
+					Repository:       repoPath,
+					Committer:        committer,
+					CommitID:         gitalygit.ObjectID(branchCommit.Id().String()),
+					UpstreamCommitID: gitalygit.ObjectID(masterRevision),
+				},
+			} {
+				t.Run(desc, func(t *testing.T) {
+					response, err := executor.Rebase(ctx, repoProto, request)
+					if tc.expectedErr != "" {
+						require.EqualError(t, err, tc.expectedErr)
+					} else {
+						require.NoError(t, err)
+
+						result := response.String()
+						require.Equal(t, tc.expected, result)
+
+						commit, err := lookupCommit(repo, result)
+						require.NoError(t, err)
+
+						for i := tc.commitsAhead; i > 0; i-- {
+							commit = commit.Parent(0)
+						}
+						masterCommit, err := lookupCommit(repo, masterRevision)
+						require.NoError(t, err)
+						require.Equal(t, masterCommit, commit)
+					}
+				})
 			}
 		})
 	}
diff --git a/internal/git2go/rebase.go b/internal/git2go/rebase.go
index 32051fd03..906d1bf79 100644
--- a/internal/git2go/rebase.go
+++ b/internal/git2go/rebase.go
@@ -13,10 +13,19 @@ type RebaseCommand struct {
 	Repository string
 	// Committer contains the the committer signature.
 	Committer Signature
-	// BranchName is the branch that is rebased.
+	// BranchName is the branch that is rebased. Deprecated, can be removed in the next release.
 	BranchName string
-	// UpstreamRevision is the revision where the branch is rebased onto.
+	// UpstreamRevision is the revision where the branch is rebased onto. Deprecated, can be
+	// removed in the next release.
 	UpstreamRevision string
+	// CommitID is the object ID of the commit that shall be rebased. Deprecates BranchName.
+	CommitID git.ObjectID
+	// UpstreamCommitID is the object ID of the commit which is considered to be the
+	// upstream branch. This parameter determines both the commit onto which we're
+	// about to rebase, which is the merge base of the upstream commit and rebased
+	// commit, and which commits should be rebased, which is the commit range
+	// upstream..commit. Deprecates the UpstreamRevision.
+	UpstreamCommitID git.ObjectID
 }
 
 // Rebase performs the rebase via gitaly-git2go
diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go
index 9bedd6e44..6b9470fb1 100644
--- a/internal/gitaly/service/operations/squash.go
+++ b/internal/gitaly/service/operations/squash.go
@@ -16,22 +16,22 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/command"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/alternates"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 const (
 	squashWorktreePrefix  = "squash"
 	gitlabWorktreesSubDir = "gitlab-worktree"
+	ambiguousArgumentFmt  = "fatal: ambiguous argument '%s...%s': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n"
 )
 
 func (s *Server) UserSquash(ctx context.Context, req *gitalypb.UserSquashRequest) (*gitalypb.UserSquashResponse, error) {
 	if err := validateUserSquashRequest(req); err != nil {
-		return nil, status.Errorf(codes.InvalidArgument, "UserSquash: %v", err)
+		return nil, helper.ErrInvalidArgumentf("UserSquash: %v", err)
 	}
 
 	if strings.Contains(req.GetSquashId(), "/") {
@@ -64,31 +64,47 @@ func (s *Server) UserSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 
 func validateUserSquashRequest(req *gitalypb.UserSquashRequest) error {
 	if req.GetRepository() == nil {
-		return fmt.Errorf("empty Repository")
+		return errors.New("empty Repository")
 	}
 
 	if req.GetUser() == nil {
-		return fmt.Errorf("empty User")
+		return errors.New("empty User")
+	}
+
+	if len(req.GetUser().GetName()) == 0 {
+		return errors.New("empty user name")
+	}
+
+	if len(req.GetUser().GetEmail()) == 0 {
+		return errors.New("empty user email")
 	}
 
 	if req.GetSquashId() == "" {
-		return fmt.Errorf("empty SquashId")
+		return errors.New("empty SquashId")
 	}
 
 	if req.GetStartSha() == "" {
-		return fmt.Errorf("empty StartSha")
+		return errors.New("empty StartSha")
 	}
 
 	if req.GetEndSha() == "" {
-		return fmt.Errorf("empty EndSha")
+		return errors.New("empty EndSha")
 	}
 
 	if len(req.GetCommitMessage()) == 0 {
-		return fmt.Errorf("empty CommitMessage")
+		return errors.New("empty CommitMessage")
 	}
 
 	if req.GetAuthor() == nil {
-		return fmt.Errorf("empty Author")
+		return errors.New("empty Author")
+	}
+
+	if len(req.GetAuthor().GetName()) == 0 {
+		return errors.New("empty author name")
+	}
+
+	if len(req.GetAuthor().GetEmail()) == 0 {
+		return errors.New("empty auithor email")
 	}
 
 	return nil
@@ -106,22 +122,12 @@ func (er gitError) Error() string {
 }
 
 func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest, env []string, repoPath string) (string, error) {
-	// In case there is an error, we silently ignore it and assume that the commits do not have
-	// a simple parent-child relationship. Ignoring this error is fine igven that we then fall
-	// back to doing a squash with a worktree.
-	err := s.localrepo(req.GetRepository()).ExecAndWait(ctx, git.SubCmd{
-		Name: "merge-base",
-		Flags: []git.Option{
-			git.Flag{Name: "--is-ancestor"},
-		},
-		Args: []string{
-			req.GetStartSha(),
-			req.GetEndSha(),
-		},
-	})
-
-	if err == nil && featureflag.UserSquashWithoutWorktree.IsEnabled(ctx) {
+	if featureflag.UserSquashWithoutWorktree.IsEnabled(ctx) {
 		repo := s.localrepo(req.GetRepository())
+		repoPath, err := s.locator.GetRepoPath(repo)
+		if err != nil {
+			return "", helper.ErrInternalf("cannot resolve repo path: %w", err)
+		}
 
 		// We need to retrieve the start commit such that we can create the new commit with
 		// all parents of the start commit.
@@ -131,18 +137,18 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 				// This error is simply for backwards compatibility. We should just
 				// return the plain error eventually.
 				Err:    err,
-				ErrMsg: fmt.Sprintf("fatal: ambiguous argument '%s...%s'", req.GetStartSha(), req.GetEndSha()),
+				ErrMsg: fmt.Sprintf(ambiguousArgumentFmt, req.GetStartSha(), req.GetEndSha()),
 			})
 		}
 
 		// And we need to take the tree of the end commit. This tree already is the result
-		treeID, err := repo.ResolveRevision(ctx, git.Revision(req.GetEndSha()+"^{tree}"))
+		endCommit, err := repo.ResolveRevision(ctx, git.Revision(req.GetEndSha()+"^{commit}"))
 		if err != nil {
 			return "", fmt.Errorf("cannot resolve end commit's tree: %w", gitError{
 				// This error is simply for backwards compatibility. We should just
 				// return the plain error eventually.
 				Err:    err,
-				ErrMsg: fmt.Sprintf("fatal: ambiguous argument '%s...%s'", req.GetStartSha(), req.GetEndSha()),
+				ErrMsg: fmt.Sprintf(ambiguousArgumentFmt, req.GetStartSha(), req.GetEndSha()),
 			})
 		}
 
@@ -151,6 +157,25 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 			return "", helper.ErrInvalidArgument(err)
 		}
 
+		// We're now rebasing the end commit on top of the start commit. The resulting tree
+		// is then going to be the tree of the squashed commit.
+		rebasedCommitID, err := s.git2go.Rebase(ctx, repo, git2go.RebaseCommand{
+			Repository: repoPath,
+			Committer: git2go.NewSignature(
+				string(req.GetUser().Name), string(req.GetUser().Email), commitDate,
+			),
+			CommitID:         endCommit,
+			UpstreamCommitID: startCommit,
+		})
+		if err != nil {
+			return "", fmt.Errorf("rebasing end onto start commit: %w", err)
+		}
+
+		treeID, err := repo.ResolveRevision(ctx, rebasedCommitID.Revision()+"^{tree}")
+		if err != nil {
+			return "", fmt.Errorf("cannot resolve rebased tree: %w", err)
+		}
+
 		commitEnv := []string{
 			"GIT_COMMITTER_NAME=" + string(req.GetUser().Name),
 			"GIT_COMMITTER_EMAIL=" + string(req.GetUser().Email),
diff --git a/internal/gitaly/service/operations/squash_test.go b/internal/gitaly/service/operations/squash_test.go
index 05a3e192f..6ced274a2 100644
--- a/internal/gitaly/service/operations/squash_test.go
+++ b/internal/gitaly/service/operations/squash_test.go
@@ -2,7 +2,6 @@ package operations
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"io/ioutil"
 	"path/filepath"
@@ -14,6 +13,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
@@ -552,9 +552,7 @@ func TestUserSquashWithGitError(t *testing.T) {
 				StartSha:      startSha,
 				EndSha:        endSha,
 			},
-			expectedResponse: &gitalypb.UserSquashResponse{
-				GitError: "fatal: empty ident name (for <janedoe@gitlab.com>) not allowed\n",
-			},
+			expectedErr: helper.ErrInvalidArgumentf("UserSquash: empty user name"),
 		},
 		{
 			desc: "author has no name set",
@@ -567,21 +565,14 @@ func TestUserSquashWithGitError(t *testing.T) {
 				StartSha:      startSha,
 				EndSha:        endSha,
 			},
-			expectedResponse: &gitalypb.UserSquashResponse{
-				GitError: "fatal: empty ident name (for <janedoe@gitlab.com>) not allowed\n",
-			},
+			expectedErr: helper.ErrInvalidArgumentf("UserSquash: empty author name"),
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
 			response, err := client.UserSquash(ctx, tc.request)
-			if err != nil {
-				// Flatten the error to make it easier to compare.
-				err = errors.New(err.Error())
-			}
-
-			require.Equal(t, tc.expectedErr, err)
+			testassert.GrpcEqualErr(t, tc.expectedErr, err)
 			testassert.ProtoEqual(t, tc.expectedResponse, response)
 		})
 	}