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

operations: Squash via patches if commits are not directly related

See merge request gitlab-org/gitaly!3685

2 files changed, 39 insertions, 33 deletions

diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go
index 96415f29b..ca8b531d9 100644
--- a/internal/gitaly/service/operations/squash.go
+++ b/internal/gitaly/service/operations/squash.go
@@ -106,24 +106,22 @@ func (er gitError) Error() string {
 }
 
 func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest, env []string, repoPath string) (string, error) {
-	if featureflag.UserSquashWithoutWorktree.IsEnabled(ctx) {
-		repo := s.localrepo(req.GetRepository())
+	// 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(),
+		},
+	})
 
-		var stderr bytes.Buffer
-		if 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(),
-			},
-		}, git.WithStderr(&stderr)); err != nil {
-			err := errorWithStderr(fmt.Errorf("%s is not an ancestor of %s",
-				req.GetStartSha(), req.GetEndSha()), &stderr)
-			return "", helper.ErrPreconditionFailed(err)
-		}
+	if err == nil && featureflag.UserSquashWithoutWorktree.IsEnabled(ctx) {
+		repo := s.localrepo(req.GetRepository())
 
 		// We need to retrieve the start commit such that we can create the new commit with
 		// all parents of the start commit.
@@ -173,9 +171,7 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 			},
 		}
 
-		var stdout bytes.Buffer
-		stderr.Reset()
-
+		var stdout, stderr bytes.Buffer
 		if err := repo.ExecAndWait(ctx, git.SubCmd{
 			Name:  "commit-tree",
 			Flags: flags,
diff --git a/internal/gitaly/service/operations/squash_test.go b/internal/gitaly/service/operations/squash_test.go
index a99bed140..1c69b7882 100644
--- a/internal/gitaly/service/operations/squash_test.go
+++ b/internal/gitaly/service/operations/squash_test.go
@@ -469,12 +469,19 @@ func TestUserSquash_ancestry(t *testing.T) {
 
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
-	// We create two new commits which both branch off from the current HEAD commit. As a
-	// result, they are not ancestors of each other.
-	commit1 := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("1"))
-	commit2 := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("2"))
+	// We create an empty parent commit and two commits which both branch off from it. As a
+	// result, they are not direct ancestors of each other.
+	parent := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("p"), gittest.WithTreeEntries(), gittest.WithParents())
+	commit1 := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("1"),
+		gittest.WithTreeEntries(gittest.TreeEntry{Path: "a", Mode: "100644", Content: "a-content"}),
+		gittest.WithParents(parent),
+	)
+	commit2 := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("2"),
+		gittest.WithTreeEntries(gittest.TreeEntry{Path: "b", Mode: "100644", Content: "b-content"}),
+		gittest.WithParents(parent),
+	)
 
-	_, err := client.UserSquash(ctx, &gitalypb.UserSquashRequest{
+	response, err := client.UserSquash(ctx, &gitalypb.UserSquashRequest{
 		Repository:    repo,
 		SquashId:      "1",
 		User:          gittest.TestUser,
@@ -482,12 +489,13 @@ func TestUserSquash_ancestry(t *testing.T) {
 		CommitMessage: commitMessage,
 		StartSha:      commit1.String(),
 		EndSha:        commit2.String(),
+		Timestamp:     &timestamp.Timestamp{Seconds: 1234512345},
 	})
 
-	expectedErr := fmt.Sprintf("rpc error: code = FailedPrecondition desc = %s is not an ancestor of %s",
-		commit1.String(), commit2.String())
-	require.NotNil(t, err)
-	require.Equal(t, expectedErr, err.Error())
+	require.Nil(t, err)
+	testassert.ProtoEqual(t, &gitalypb.UserSquashResponse{
+		SquashSha: "b277ddc0aafcba53f23f3d4d4a46dde42c9e7ad2",
+	}, response)
 }
 
 func TestUserSquashWithGitError(t *testing.T) {
@@ -514,8 +522,9 @@ func TestUserSquashWithGitError(t *testing.T) {
 				StartSha:      "doesntexisting",
 				EndSha:        endSha,
 			},
-			expectedErr: fmt.Errorf("rpc error: code = FailedPrecondition desc = %s is not an ancestor of %s, stderr: %q",
-				"doesntexisting", endSha, "fatal: Not a valid object name doesntexisting\n"),
+			expectedResponse: &gitalypb.UserSquashResponse{
+				GitError: "fatal: ambiguous argument 'doesntexisting...54cec5282aa9f21856362fe321c800c236a61615': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n",
+			},
 		},
 		{
 			desc: "not existing end SHA",
@@ -528,8 +537,9 @@ func TestUserSquashWithGitError(t *testing.T) {
 				StartSha:      startSha,
 				EndSha:        "doesntexisting",
 			},
-			expectedErr: fmt.Errorf("rpc error: code = FailedPrecondition desc = %s is not an ancestor of %s, stderr: %q",
-				startSha, "doesntexisting", "fatal: Not a valid object name doesntexisting\n"),
+			expectedResponse: &gitalypb.UserSquashResponse{
+				GitError: "fatal: ambiguous argument 'b83d6e391c22777fca1ed3012fce84f633d7fed0...doesntexisting': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git <command> [<revision>...] -- [<file>...]'\n",
+			},
 		},
 		{
 			desc: "user has no name set",