Merge branch 'kn-4407-expected-old-oid-7' into 'master'

operations: Use `ExpectedOldOid` in `UserFFBranch`

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5173

Merged-by: Toon Claes <toon@gitlab.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>
Reviewed-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>

2 files changed, 295 insertions, 89 deletions

diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index 901a37b32..8c47c8340 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -258,9 +258,31 @@ func (s *Server) UserFFBranch(ctx context.Context, in *gitalypb.UserFFBranchRequ
 		return nil, err
 	}
 
-	revision, err := quarantineRepo.ResolveRevision(ctx, referenceName.Revision())
-	if err != nil {
-		return nil, structerr.NewInvalidArgument("%w", err)
+	var revision git.ObjectID
+	if expectedOldOID := in.GetExpectedOldOid(); expectedOldOID != "" {
+		objectHash, err := quarantineRepo.ObjectHash(ctx)
+		if err != nil {
+			return nil, structerr.NewInternal("detecting object hash: %w", err)
+		}
+
+		revision, err = objectHash.FromHex(expectedOldOID)
+		if err != nil {
+			return nil, structerr.NewInvalidArgument("invalid expected old object ID: %w", err).
+				WithMetadata("old_object_id", expectedOldOID)
+		}
+
+		revision, err = quarantineRepo.ResolveRevision(
+			ctx, git.Revision(fmt.Sprintf("%s^{object}", revision)),
+		)
+		if err != nil {
+			return nil, structerr.NewInvalidArgument("cannot resolve expected old object ID: %w", err).
+				WithMetadata("old_object_id", expectedOldOID)
+		}
+	} else {
+		revision, err = quarantineRepo.ResolveRevision(ctx, referenceName.Revision())
+		if err != nil {
+			return nil, structerr.NewInvalidArgument("%w", err)
+		}
 	}
 
 	commitID, err := git.ObjectHashSHA1.FromHex(in.CommitId)
diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index a821c1f58..80a4b8f65 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -1029,120 +1029,304 @@ func errWithDetails(tb testing.TB, err error, details ...proto.Message) error {
 	return detailedErr
 }
 
-func TestUserFFBranch_successful(t *testing.T) {
+func TestUserFFBranch(t *testing.T) {
 	t.Parallel()
-	ctx := testhelper.Context(t)
 
-	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
+	ctx := testhelper.Context(t)
+	ctx, cfg, client := setupOperationsServiceWithoutRepo(t, ctx)
 
-	commitID := "cfe32cf61b73a0d5e9f13e774abde7ff789b1660"
-	branchName := "test-ff-target-branch"
-	request := &gitalypb.UserFFBranchRequest{
-		Repository: repo,
-		CommitId:   commitID,
-		Branch:     []byte(branchName),
-		User:       gittest.TestUser,
-	}
-	expectedResponse := &gitalypb.UserFFBranchResponse{
-		BranchUpdate: &gitalypb.OperationBranchUpdate{
-			RepoCreated:   false,
-			BranchCreated: false,
-			CommitId:      commitID,
-		},
+	type setupData struct {
+		repoPath         string
+		request          *gitalypb.UserFFBranchRequest
+		expectedResponse *gitalypb.UserFFBranchResponse
 	}
 
-	gittest.Exec(t, cfg, "-C", repoPath, "branch", "-f", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f")
+	testCases := []struct {
+		desc        string
+		setup       func(t *testing.T, ctx context.Context) setupData
+		expectedErr error
+	}{
+		{
+			desc: "successful",
+			setup: func(t *testing.T, ctx context.Context) setupData {
+				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
 
-	resp, err := client.UserFFBranch(ctx, request)
-	require.NoError(t, err)
-	testhelper.ProtoEqual(t, expectedResponse, resp)
-	newBranchHead := gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", branchName)
-	require.Equal(t, commitID, text.ChompBytes(newBranchHead), "branch head not updated")
-}
+				firstCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
+				commitToMerge := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(firstCommit))
 
-func TestUserFFBranch_failure(t *testing.T) {
-	t.Parallel()
-	ctx := testhelper.Context(t)
+				return setupData{
+					repoPath: repoPath,
+					request: &gitalypb.UserFFBranchRequest{
+						Repository: repoProto,
+						User:       gittest.TestUser,
+						CommitId:   commitToMerge.String(),
+						Branch:     []byte("master"),
+					},
+					expectedResponse: &gitalypb.UserFFBranchResponse{
+						BranchUpdate: &gitalypb.OperationBranchUpdate{
+							CommitId: commitToMerge.String(),
+						},
+					},
+				}
+			},
+			expectedErr: nil,
+		},
+		{
+			desc: "successful + expectedOldOID",
+			setup: func(t *testing.T, ctx context.Context) setupData {
+				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
 
-	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
+				firstCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
+				commitToMerge := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(firstCommit))
 
-	commitID := "cfe32cf61b73a0d5e9f13e774abde7ff789b1660"
-	branchName := "test-ff-target-branch"
+				return setupData{
+					repoPath: repoPath,
+					request: &gitalypb.UserFFBranchRequest{
+						Repository:     repoProto,
+						User:           gittest.TestUser,
+						CommitId:       commitToMerge.String(),
+						Branch:         []byte("master"),
+						ExpectedOldOid: string(firstCommit),
+					},
+					expectedResponse: &gitalypb.UserFFBranchResponse{
+						BranchUpdate: &gitalypb.OperationBranchUpdate{
+							CommitId: commitToMerge.String(),
+						},
+					},
+				}
+			},
+			expectedErr: nil,
+		},
+		{
+			desc: "empty repository",
+			setup: func(t *testing.T, ctx context.Context) setupData {
+				_, repoPath := gittest.CreateRepository(t, ctx, cfg)
+
+				firstCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
+				commitToMerge := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(firstCommit))
+
+				return setupData{
+					repoPath: repoPath,
+					request: &gitalypb.UserFFBranchRequest{
+						User:     gittest.TestUser,
+						CommitId: commitToMerge.String(),
+						Branch:   []byte("master"),
+					},
+				}
+			},
+			expectedErr: structerr.NewInvalidArgument(testhelper.GitalyOrPraefect("empty Repository", "repo scoped: empty Repository")),
+		},
+		{
+			desc: "empty user",
+			setup: func(t *testing.T, ctx context.Context) setupData {
+				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
+
+				firstCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
+				commitToMerge := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(firstCommit))
+
+				return setupData{
+					repoPath: repoPath,
+					request: &gitalypb.UserFFBranchRequest{
+						Repository: repoProto,
+						CommitId:   commitToMerge.String(),
+						Branch:     []byte("master"),
+					},
+				}
+			},
+			expectedErr: structerr.NewInvalidArgument("empty user"),
+		},
+		{
+			desc: "empty commit",
+			setup: func(t *testing.T, ctx context.Context) setupData {
+				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
 
-	gittest.Exec(t, cfg, "-C", repoPath, "branch", "-f", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f")
+				firstCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
+				gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(firstCommit))
 
-	testCases := []struct {
-		desc     string
-		user     *gitalypb.User
-		branch   []byte
-		commitID string
-		repo     *gitalypb.Repository
-		code     codes.Code
-	}{
+				return setupData{
+					repoPath: repoPath,
+					request: &gitalypb.UserFFBranchRequest{
+						Repository: repoProto,
+						User:       gittest.TestUser,
+						Branch:     []byte("master"),
+					},
+				}
+			},
+			expectedErr: structerr.NewInvalidArgument("empty commit id"),
+		},
 		{
-			desc:     "empty repository",
-			user:     gittest.TestUser,
-			branch:   []byte(branchName),
-			commitID: commitID,
-			code:     codes.InvalidArgument,
+			desc: "non-existing commit",
+			setup: func(t *testing.T, ctx context.Context) setupData {
+				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
+
+				firstCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
+				gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(firstCommit))
+
+				return setupData{
+					repoPath: repoPath,
+					request: &gitalypb.UserFFBranchRequest{
+						Repository: repoProto,
+						User:       gittest.TestUser,
+						CommitId:   gittest.DefaultObjectHash.ZeroOID.String(),
+						Branch:     []byte("master"),
+					},
+				}
+			},
+			expectedErr: structerr.NewInternal(`checking for ancestry: invalid commit: "%s"`, gittest.DefaultObjectHash.ZeroOID),
 		},
 		{
-			desc:     "empty user",
-			repo:     repo,
-			branch:   []byte(branchName),
-			commitID: commitID,
-			code:     codes.InvalidArgument,
+			desc: "empty branch",
+			setup: func(t *testing.T, ctx context.Context) setupData {
+				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
+
+				firstCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
+				commitToMerge := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(firstCommit))
+
+				return setupData{
+					repoPath: repoPath,
+					request: &gitalypb.UserFFBranchRequest{
+						Repository: repoProto,
+						CommitId:   commitToMerge.String(),
+						User:       gittest.TestUser,
+					},
+				}
+			},
+			expectedErr: structerr.NewInvalidArgument("empty branch name"),
 		},
 		{
-			desc:   "empty commit",
-			repo:   repo,
-			user:   gittest.TestUser,
-			branch: []byte(branchName),
-			code:   codes.InvalidArgument,
+			desc: "non-existing branch",
+			setup: func(t *testing.T, ctx context.Context) setupData {
+				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
+
+				firstCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
+				commitToMerge := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(firstCommit))
+
+				return setupData{
+					repoPath: repoPath,
+					request: &gitalypb.UserFFBranchRequest{
+						Repository: repoProto,
+						CommitId:   commitToMerge.String(),
+						User:       gittest.TestUser,
+						Branch:     []byte("main"),
+					},
+				}
+			},
+			expectedErr: structerr.NewInvalidArgument("reference not found"),
 		},
 		{
-			desc:     "non-existing commit",
-			repo:     repo,
-			user:     gittest.TestUser,
-			branch:   []byte(branchName),
-			commitID: "f001",
-			code:     codes.InvalidArgument,
+			desc: "commit is not a descendant of branch head",
+			setup: func(t *testing.T, ctx context.Context) setupData {
+				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
+
+				gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
+				commitToMerge := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries(
+					gittest.TreeEntry{Path: "file", Mode: "100644", Content: "something"},
+				))
+
+				return setupData{
+					repoPath: repoPath,
+					request: &gitalypb.UserFFBranchRequest{
+						Repository: repoProto,
+						CommitId:   commitToMerge.String(),
+						User:       gittest.TestUser,
+						Branch:     []byte("master"),
+					},
+				}
+			},
+			expectedErr: structerr.NewFailedPrecondition("not fast forward"),
 		},
 		{
-			desc:     "empty branch",
-			repo:     repo,
-			user:     gittest.TestUser,
-			commitID: commitID,
-			code:     codes.InvalidArgument,
+			desc: "invalid expectedOldOID",
+			setup: func(t *testing.T, ctx context.Context) setupData {
+				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
+
+				firstCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
+				commitToMerge := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(firstCommit))
+
+				return setupData{
+					repoPath: repoPath,
+					request: &gitalypb.UserFFBranchRequest{
+						Repository:     repoProto,
+						CommitId:       commitToMerge.String(),
+						User:           gittest.TestUser,
+						Branch:         []byte("master"),
+						ExpectedOldOid: "foobar",
+					},
+				}
+			},
+			expectedErr: structerr.NewInvalidArgument(`invalid expected old object ID: invalid object ID: "foobar"`),
 		},
 		{
-			desc:     "non-existing branch",
-			repo:     repo,
-			user:     gittest.TestUser,
-			branch:   []byte("this-isnt-real"),
-			commitID: commitID,
-			code:     codes.InvalidArgument,
+			desc: "valid SHA, but not existing expectedOldOID",
+			setup: func(t *testing.T, ctx context.Context) setupData {
+				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
+
+				firstCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"))
+				commitToMerge := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(firstCommit))
+
+				return setupData{
+					repoPath: repoPath,
+					request: &gitalypb.UserFFBranchRequest{
+						Repository:     repoProto,
+						CommitId:       commitToMerge.String(),
+						User:           gittest.TestUser,
+						Branch:         []byte("master"),
+						ExpectedOldOid: gittest.DefaultObjectHash.ZeroOID.String(),
+					},
+				}
+			},
+			expectedErr: structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found"),
 		},
 		{
-			desc:     "commit is not a descendant of branch head",
-			repo:     repo,
-			user:     gittest.TestUser,
-			branch:   []byte(branchName),
-			commitID: "1a0b36b3cdad1d2ee32457c102a8c0b7056fa863",
-			code:     codes.FailedPrecondition,
+			desc: "expectedOldOID pointing to old commit",
+			setup: func(t *testing.T, ctx context.Context) setupData {
+				repoProto, repoPath := gittest.CreateRepository(t, ctx, cfg)
+
+				firstCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries(
+					gittest.TreeEntry{Path: "bar", Mode: "100644", Content: "something"},
+				))
+				secondCommit := gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("master"), gittest.WithParents(firstCommit),
+					gittest.WithTreeEntries(
+						gittest.TreeEntry{Path: "foo", Mode: "100644", Content: "something"},
+					),
+				)
+				commitToMerge := gittest.WriteCommit(t, cfg, repoPath, gittest.WithParents(secondCommit), gittest.WithTreeEntries(
+					gittest.TreeEntry{Path: "goo", Mode: "100644", Content: "something"},
+				))
+
+				return setupData{
+					repoPath: repoPath,
+					request: &gitalypb.UserFFBranchRequest{
+						Repository:     repoProto,
+						CommitId:       commitToMerge.String(),
+						User:           gittest.TestUser,
+						Branch:         []byte("master"),
+						ExpectedOldOid: firstCommit.String(),
+					},
+					// empty response is the expected (legacy) behavior when we fail to
+					// update the ref.
+					expectedResponse: &gitalypb.UserFFBranchResponse{},
+				}
+			},
 		},
 	}
 
-	for _, testCase := range testCases {
-		t.Run(testCase.desc, func(t *testing.T) {
-			request := &gitalypb.UserFFBranchRequest{
-				Repository: testCase.repo,
-				User:       testCase.user,
-				Branch:     testCase.branch,
-				CommitId:   testCase.commitID,
+	for _, tc := range testCases {
+		tc := tc
+
+		t.Run(tc.desc, func(t *testing.T) {
+			t.Parallel()
+
+			data := tc.setup(t, ctx)
+
+			resp, err := client.UserFFBranch(ctx, data.request)
+			testhelper.RequireGrpcError(t, tc.expectedErr, err)
+			testhelper.ProtoEqual(t, data.expectedResponse, resp)
+
+			if data.expectedResponse != nil && data.expectedResponse.BranchUpdate != nil {
+				newBranchHead := text.ChompBytes(gittest.Exec(t, cfg, "-C", data.repoPath, "rev-parse", string(data.request.Branch)))
+				require.Equal(t, data.request.CommitId, newBranchHead, "branch head not updated")
 			}
-			_, err := client.UserFFBranch(ctx, request)
-			testhelper.RequireGrpcCode(t, err, testCase.code)
 		})
 	}
 }