diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index d213c449c3b54aaabcfc9459945f7ebea00dba26..c92e7bd6517b045d45d5fe1ac408fd9624ba6c15 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -337,11 +337,27 @@ func (s *Server) userCommitFiles(ctx context.Context, header *gitalypb.UserCommi return fmt.Errorf("was repo created: %w", err) } - oldRevision := parentCommitOID - if targetBranchCommit == "" { - oldRevision = git.ObjectHashSHA1.ZeroOID - } else if header.Force { - oldRevision = targetBranchCommit + var oldRevision git.ObjectID + if expectedOldOID := header.GetExpectedOldOid(); expectedOldOID != "" { + oldRevision, err = git.ObjectHashSHA1.FromHex(expectedOldOID) + if err != nil { + return structerr.NewInvalidArgument("invalid expected old object ID: %w", err).WithMetadata("old_object_id", expectedOldOID) + } + + oldRevision, err = s.localrepo(header.GetRepository()).ResolveRevision( + ctx, git.Revision(fmt.Sprintf("%s^{object}", oldRevision)), + ) + if err != nil { + return structerr.NewInvalidArgument("cannot resolve expected old object ID: %w", err). + WithMetadata("old_object_id", expectedOldOID) + } + } else { + oldRevision = parentCommitOID + if targetBranchCommit == "" { + oldRevision = git.ObjectHashSHA1.ZeroOID + } else if header.Force { + oldRevision = targetBranchCommit + } } if err := s.updateReferenceWithHooks(ctx, header.GetRepository(), header.User, quarantineDir, targetBranchName, commitID, oldRevision); err != nil { diff --git a/internal/gitaly/service/operations/commit_files_test.go b/internal/gitaly/service/operations/commit_files_test.go index 16fe18a8d9bb8c41029643e2355685460d70a5cc..ccd85753ad950bcf59233775b662a39dc2ff16e7 100644 --- a/internal/gitaly/service/operations/commit_files_test.go +++ b/internal/gitaly/service/operations/commit_files_test.go @@ -19,6 +19,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/helper" "gitlab.com/gitlab-org/gitaly/v15/internal/helper/text" "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" + "gitlab.com/gitlab-org/gitaly/v15/internal/structerr" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper" "gitlab.com/gitlab-org/gitaly/v15/internal/testhelper/testcfg" "gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb" @@ -912,6 +913,7 @@ func testUserCommitFiles(t *testing.T, ctx context.Context) { branch, []byte("commit message"), "", + "", ) setAuthorAndEmail(headerRequest, []byte("Author Name"), []byte("author.email@example.com")) @@ -1001,7 +1003,7 @@ func testUserCommitFilesStableCommitID(t *testing.T, ctx context.Context) { stream, err := client.UserCommitFiles(ctx) require.NoError(t, err) - headerRequest := headerRequest(repoProto, gittest.TestUser, "master", []byte("commit message"), "") + headerRequest := headerRequest(repoProto, gittest.TestUser, "master", []byte("commit message"), "", "") setAuthorAndEmail(headerRequest, []byte("Author Name"), []byte("author.email@example.com")) setTimestamp(headerRequest, time.Unix(12345, 0)) require.NoError(t, stream.Send(headerRequest)) @@ -1070,7 +1072,7 @@ func testUserCommitFilesQuarantine(t *testing.T, ctx context.Context) { stream, err := client.UserCommitFiles(ctx) require.NoError(t, err) - headerRequest := headerRequest(repoProto, gittest.TestUser, "master", []byte("commit message"), "") + headerRequest := headerRequest(repoProto, gittest.TestUser, "master", []byte("commit message"), "", "") setAuthorAndEmail(headerRequest, []byte("Author Name"), []byte("author.email@example.com")) setTimestamp(headerRequest, time.Unix(12345, 0)) require.NoError(t, stream.Send(headerRequest)) @@ -1125,9 +1127,11 @@ func testSuccessfulUserCommitFilesRequest(t *testing.T, ctx context.Context) { repoPath string branchName string startBranchName string + expectedOldOID string repoCreated bool branchCreated bool executeFilemode bool + expectedError error }{ { desc: "existing repo and branch", @@ -1137,6 +1141,37 @@ func testSuccessfulUserCommitFilesRequest(t *testing.T, ctx context.Context) { repoCreated: false, branchCreated: false, }, + { + desc: "existing repo and branch + expectedOldOID", + repo: repo, + repoPath: repoPath, + branchName: "wip", + expectedOldOID: text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", "refs/heads/wip")), + }, + { + desc: "existing repo and branch + invalid expectedOldOID", + repo: repo, + repoPath: repoPath, + branchName: "few-commits", + expectedOldOID: "foobar", + expectedError: structerr.NewInvalidArgument(`invalid expected old object ID: invalid object ID: "foobar"`), + }, + { + desc: "existing repo and branch + valid expectedOldOID but invalid object", + repo: repo, + repoPath: repoPath, + branchName: "few-commits", + expectedOldOID: gittest.DefaultObjectHash.ZeroOID.String(), + expectedError: structerr.NewInvalidArgument("cannot resolve expected old object ID: reference not found"), + }, + { + desc: "existing repo and branch + incorrect expectedOldOID", + repo: repo, + repoPath: repoPath, + branchName: "few-commits", + expectedOldOID: text.ChompBytes(gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", "refs/heads/few-commits~1")), + expectedError: structerr.NewFailedPrecondition("Could not update refs/heads/few-commits. Please refresh and try again."), + }, { desc: "existing repo, new branch", repo: repo, @@ -1175,7 +1210,7 @@ func testSuccessfulUserCommitFilesRequest(t *testing.T, ctx context.Context) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - headerRequest := headerRequest(tc.repo, gittest.TestUser, tc.branchName, commitFilesMessage, tc.startBranchName) + headerRequest := headerRequest(tc.repo, gittest.TestUser, tc.branchName, commitFilesMessage, tc.startBranchName, tc.expectedOldOID) setAuthorAndEmail(headerRequest, authorName, authorEmail) actionsRequest1 := createFileHeaderRequest(filePath) @@ -1184,6 +1219,7 @@ func testSuccessfulUserCommitFilesRequest(t *testing.T, ctx context.Context) { actionsRequest4 := chmodFileHeaderRequest(filePath, tc.executeFilemode) stream, err := client.UserCommitFiles(ctx) + require.NoError(t, err) require.NoError(t, stream.Send(headerRequest)) require.NoError(t, stream.Send(actionsRequest1)) @@ -1192,6 +1228,10 @@ func testSuccessfulUserCommitFilesRequest(t *testing.T, ctx context.Context) { require.NoError(t, stream.Send(actionsRequest4)) resp, err := stream.CloseAndRecv() + if tc.expectedError != nil { + testhelper.RequireGrpcError(t, tc.expectedError, err) + return + } require.NoError(t, err) require.Equal(t, tc.repoCreated, resp.GetBranchUpdate().GetRepoCreated()) require.Equal(t, tc.branchCreated, resp.GetBranchUpdate().GetBranchCreated()) @@ -1249,7 +1289,7 @@ func testSuccessfulUserCommitFilesRequestMove(t *testing.T, ctx context.Context) }) origFileContent := gittest.Exec(t, cfg, "-C", testRepoPath, "show", branchName+":"+previousFilePath) - headerRequest := headerRequest(testRepo, gittest.TestUser, branchName, commitFilesMessage, "") + headerRequest := headerRequest(testRepo, gittest.TestUser, branchName, commitFilesMessage, "", "") setAuthorAndEmail(headerRequest, authorName, authorEmail) actionsRequest1 := moveFileHeaderRequest(previousFilePath, filePath, tc.infer) @@ -1308,7 +1348,7 @@ func testSuccessfulUserCommitFilesRequestForceCommit(t *testing.T, ctx context.C mergeBaseID := text.ChompBytes(mergeBaseOut) require.NotEqual(t, mergeBaseID, targetBranchCommit.Id, "expected %s not to be an ancestor of %s", targetBranchCommit.Id, startBranchCommit.Id) - headerRequest := headerRequest(repoProto, gittest.TestUser, targetBranchName, commitFilesMessage, "") + headerRequest := headerRequest(repoProto, gittest.TestUser, targetBranchName, commitFilesMessage, "", "") setAuthorAndEmail(headerRequest, authorName, authorEmail) setStartBranchName(headerRequest, startBranchName) setForce(headerRequest, true) @@ -1348,7 +1388,7 @@ func testSuccessfulUserCommitFilesRequestStartSha(t *testing.T, ctx context.Cont startCommit, err := repo.ReadCommit(ctx, "master") require.NoError(t, err) - headerRequest := headerRequest(repoProto, gittest.TestUser, targetBranchName, commitFilesMessage, "") + headerRequest := headerRequest(repoProto, gittest.TestUser, targetBranchName, commitFilesMessage, "", "") setStartSha(headerRequest, startCommit.Id) stream, err := client.UserCommitFiles(ctx) @@ -1412,7 +1452,7 @@ func testSuccessfulUserCommitFilesRemoteRepositoryRequest(setHeader func(header startCommit, err := repo.ReadCommit(ctx, "master") require.NoError(t, err) - headerRequest := headerRequest(newRepoProto, gittest.TestUser, targetBranchName, commitFilesMessage, "") + headerRequest := headerRequest(newRepoProto, gittest.TestUser, targetBranchName, commitFilesMessage, "", "") setHeader(headerRequest) setStartRepository(headerRequest, repoProto) @@ -1469,7 +1509,7 @@ func testSuccessfulUserCommitFilesRequestWithSpecialCharactersInSignature(t *tes for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - headerRequest := headerRequest(repoProto, tc.user, targetBranchName, commitFilesMessage, "") + headerRequest := headerRequest(repoProto, tc.user, targetBranchName, commitFilesMessage, "", "") setAuthorAndEmail(headerRequest, tc.user.Name, tc.user.Email) stream, err := client.UserCommitFiles(ctx) @@ -1503,7 +1543,7 @@ func testFailedUserCommitFilesRequestDueToHooks(t *testing.T, ctx context.Contex branchName := "feature" filePath := "my/file.txt" - headerRequest := headerRequest(repoProto, gittest.TestUser, branchName, commitFilesMessage, "") + headerRequest := headerRequest(repoProto, gittest.TestUser, branchName, commitFilesMessage, "", "") actionsRequest1 := createFileHeaderRequest(filePath) actionsRequest2 := actionContentRequest("My content") hookContent := []byte("#!/bin/sh\nprintenv | grep -e GL_ID -e GL_USERNAME | sort | paste -sd ' ' -\nexit 1") @@ -1570,7 +1610,7 @@ func testFailedUserCommitFilesRequestDueToIndexError(t *testing.T, ctx context.C { desc: "file already exists", requests: []*gitalypb.UserCommitFilesRequest{ - headerRequest(repo, gittest.TestUser, "feature", commitFilesMessage, ""), + headerRequest(repo, gittest.TestUser, "feature", commitFilesMessage, "", ""), createFileHeaderRequest("README.md"), actionContentRequest("This file already exists"), }, @@ -1580,7 +1620,7 @@ func testFailedUserCommitFilesRequestDueToIndexError(t *testing.T, ctx context.C { desc: "file doesn't exists", requests: []*gitalypb.UserCommitFilesRequest{ - headerRequest(repo, gittest.TestUser, "feature", commitFilesMessage, ""), + headerRequest(repo, gittest.TestUser, "feature", commitFilesMessage, "", ""), chmodFileHeaderRequest("documents/story.txt", true), }, indexError: "A file with this name doesn't exist", @@ -1589,7 +1629,7 @@ func testFailedUserCommitFilesRequestDueToIndexError(t *testing.T, ctx context.C { desc: "dir already exists", requests: []*gitalypb.UserCommitFilesRequest{ - headerRequest(repo, gittest.TestUser, "utf-dir", commitFilesMessage, ""), + headerRequest(repo, gittest.TestUser, "utf-dir", commitFilesMessage, "", ""), actionRequest(&gitalypb.UserCommitFilesAction{ UserCommitFilesActionPayload: &gitalypb.UserCommitFilesAction_Header{ Header: &gitalypb.UserCommitFilesActionHeader{ @@ -1653,7 +1693,7 @@ func testFailedUserCommitFilesRequest(t *testing.T, ctx context.Context) { }{ { desc: "empty Repository", - req: headerRequest(nil, gittest.TestUser, branchName, commitFilesMessage, ""), + req: headerRequest(nil, gittest.TestUser, branchName, commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, testhelper.GitalyOrPraefect( "empty Repository", "repo scoped: empty Repository", @@ -1661,47 +1701,47 @@ func testFailedUserCommitFilesRequest(t *testing.T, ctx context.Context) { }, { desc: "empty User", - req: headerRequest(repo, nil, branchName, commitFilesMessage, ""), + req: headerRequest(repo, nil, branchName, commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, "empty User"), }, { desc: "empty BranchName", - req: headerRequest(repo, gittest.TestUser, "", commitFilesMessage, ""), + req: headerRequest(repo, gittest.TestUser, "", commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, "empty BranchName"), }, { desc: "empty CommitMessage", - req: headerRequest(repo, gittest.TestUser, branchName, nil, ""), + req: headerRequest(repo, gittest.TestUser, branchName, nil, "", ""), expectedErr: status.Error(codes.InvalidArgument, "empty CommitMessage"), }, { desc: "invalid object ID: \"foobar\"", - req: setStartSha(headerRequest(repo, gittest.TestUser, branchName, commitFilesMessage, ""), "foobar"), + req: setStartSha(headerRequest(repo, gittest.TestUser, branchName, commitFilesMessage, "", ""), "foobar"), expectedErr: status.Error(codes.InvalidArgument, `invalid object ID: "foobar"`), }, { desc: "failed to parse signature - Signature cannot have an empty name or email", - req: headerRequest(repo, &gitalypb.User{}, branchName, commitFilesMessage, ""), + req: headerRequest(repo, &gitalypb.User{}, branchName, commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, "commit: commit: failed to parse signature - Signature cannot have an empty name or email"), }, { desc: "failed to parse signature - Signature cannot have an empty name or email", - req: headerRequest(repo, &gitalypb.User{Name: []byte(""), Email: []byte("")}, branchName, commitFilesMessage, ""), + req: headerRequest(repo, &gitalypb.User{Name: []byte(""), Email: []byte("")}, branchName, commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, "commit: commit: failed to parse signature - Signature cannot have an empty name or email"), }, { desc: "failed to parse signature - Signature cannot have an empty name or email", - req: headerRequest(repo, &gitalypb.User{Name: []byte(" "), Email: []byte(" ")}, branchName, commitFilesMessage, ""), + req: headerRequest(repo, &gitalypb.User{Name: []byte(" "), Email: []byte(" ")}, branchName, commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, "commit: commit: failed to parse signature - Signature cannot have an empty name or email"), }, { desc: "failed to parse signature - Signature cannot have an empty name or email", - req: headerRequest(repo, &gitalypb.User{Name: []byte("Jane Doe"), Email: []byte("")}, branchName, commitFilesMessage, ""), + req: headerRequest(repo, &gitalypb.User{Name: []byte("Jane Doe"), Email: []byte("")}, branchName, commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, "commit: commit: failed to parse signature - Signature cannot have an empty name or email"), }, { desc: "failed to parse signature - Signature cannot have an empty name or email", - req: headerRequest(repo, &gitalypb.User{Name: []byte(""), Email: []byte("janedoe@gitlab.com")}, branchName, commitFilesMessage, ""), + req: headerRequest(repo, &gitalypb.User{Name: []byte(""), Email: []byte("janedoe@gitlab.com")}, branchName, commitFilesMessage, "", ""), expectedErr: status.Error(codes.InvalidArgument, "commit: commit: failed to parse signature - Signature cannot have an empty name or email"), }, } @@ -1736,7 +1776,7 @@ func testUserCommitFilesFailsIfRepositoryMissing(t *testing.T, ctx context.Conte } branchName := "feature" - req := headerRequest(repo, gittest.TestUser, branchName, commitFilesMessage, "") + req := headerRequest(repo, gittest.TestUser, branchName, commitFilesMessage, "", "") stream, err := client.UserCommitFiles(ctx) require.NoError(t, err) @@ -1752,7 +1792,7 @@ func testUserCommitFilesFailsIfRepositoryMissing(t *testing.T, ctx context.Conte } } -func headerRequest(repo *gitalypb.Repository, user *gitalypb.User, branchName string, commitMessage []byte, startBranchName string) *gitalypb.UserCommitFilesRequest { +func headerRequest(repo *gitalypb.Repository, user *gitalypb.User, branchName string, commitMessage []byte, startBranchName string, expectedOldOID string) *gitalypb.UserCommitFilesRequest { return &gitalypb.UserCommitFilesRequest{ UserCommitFilesRequestPayload: &gitalypb.UserCommitFilesRequest_Header{ Header: &gitalypb.UserCommitFilesRequestHeader{ @@ -1762,6 +1802,7 @@ func headerRequest(repo *gitalypb.Repository, user *gitalypb.User, branchName st CommitMessage: commitMessage, StartBranchName: []byte(startBranchName), StartRepository: nil, + ExpectedOldOid: expectedOldOID, }, }, }