git2go: rebase: Allow passing object IDs

The git2go rebase command currently only accepts a branch name and
upstream revision as inputs. While the latter is fine, requiring a
branch name is overly restrictive: all we want to have is a commit which
shall be rebased onto the upstream revision, and we may not necessarily
have a branch available. Updating references never happens inside of
gitaly-git2go either, but instead in the calling context.

Amend the command struct to also support a commit ID as input,
deprecating the old branch name parameter. For backwards compatibility
reasons, the branch name needs to stay around for at least one release.
To align types, this also introduces a new `UpstreamCommitID` field
which deprecates the previous `UpstreamRevision` field.

3 files changed, 111 insertions, 42 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