Merge branch 'sh-fix-squash-renamed-files-target-branch' into 'master'

Fix squash when target branch has a renamed file

Closes #2395

See merge request gitlab-org/gitaly!1786

2 files changed, 65 insertions, 33 deletions

diff --git a/internal/service/operations/squash.go b/internal/service/operations/squash.go
index 56480fc60..b51572a4a 100644
--- a/internal/service/operations/squash.go
+++ b/internal/service/operations/squash.go
@@ -41,10 +41,6 @@ func validateUserSquashRequest(req *gitalypb.UserSquashRequest) error {
 		return fmt.Errorf("empty SquashId")
 	}
 
-	if len(req.GetBranch()) == 0 {
-		return fmt.Errorf("empty Branch")
-	}
-
 	if req.GetStartSha() == "" {
 		return fmt.Errorf("empty StartSha")
 	}
diff --git a/internal/service/operations/squash_test.go b/internal/service/operations/squash_test.go
index 25e4936e0..7115f7977 100644
--- a/internal/service/operations/squash_test.go
+++ b/internal/service/operations/squash_test.go
@@ -43,7 +43,6 @@ func TestSuccessfulUserSquashRequest(t *testing.T) {
 		Repository:    testRepo,
 		User:          user,
 		SquashId:      "1",
-		Branch:        []byte(branchName),
 		Author:        author,
 		CommitMessage: commitMessage,
 		StartSha:      startSha,
@@ -94,7 +93,6 @@ func TestSuccessfulUserSquashRequestWith3wayMerge(t *testing.T) {
 		Repository:    testRepo,
 		User:          user,
 		SquashId:      "1",
-		Branch:        []byte("gitaly/squash-test"),
 		Author:        author,
 		CommitMessage: commitMessage,
 		// The diff between two of these commits results in some changes to files/ruby/popen.rb
@@ -108,7 +106,7 @@ func TestSuccessfulUserSquashRequestWith3wayMerge(t *testing.T) {
 
 	commit, err := log.GetCommit(ctx, testRepo, response.SquashSha)
 	require.NoError(t, err)
-	require.Equal(t, []string{"3d05a143ac193c1a6fe4d046a6e3fe71e825258a"}, commit.ParentIds)
+	require.Equal(t, []string{"6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9"}, commit.ParentIds)
 	require.Equal(t, author.Name, commit.Author.Name)
 	require.Equal(t, author.Email, commit.Author.Email)
 	require.Equal(t, user.Name, commit.Committer.Name)
@@ -145,7 +143,6 @@ func TestSplitIndex(t *testing.T) {
 		Repository:    testRepo,
 		User:          user,
 		SquashId:      "1",
-		Branch:        []byte(branchName),
 		Author:        author,
 		CommitMessage: commitMessage,
 		StartSha:      startSha,
@@ -158,7 +155,69 @@ func TestSplitIndex(t *testing.T) {
 	require.False(t, ensureSplitIndexExists(t, testRepoPath))
 }
 
-func TestFailedUserSquashRequestDueToGitError(t *testing.T) {
+func TestSquashRequestWithRenamedFiles(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	server, serverSocketPath := runOperationServiceServer(t)
+	defer server.Stop()
+
+	client, conn := NewOperationClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepoWithWorktree(t)
+	defer cleanupFn()
+
+	originalFilename := "original-file.txt"
+	renamedFilename := "renamed-file.txt"
+
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "config", "user.name", string(author.Name))
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "config", "user.email", string(author.Email))
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "checkout", "-b", "squash-rename-test", "master")
+	require.NoError(t, ioutil.WriteFile(filepath.Join(testRepoPath, originalFilename), []byte("This is a test"), 0644))
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "add", ".")
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "commit", "-m", "test file")
+
+	startCommitID := text.ChompBytes(testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", "HEAD"))
+
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "mv", originalFilename, renamedFilename)
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "commit", "-a", "-m", "renamed test file")
+
+	// Modify the original file in another branch
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "checkout", "-b", "squash-rename-branch", startCommitID)
+	require.NoError(t, ioutil.WriteFile(filepath.Join(testRepoPath, originalFilename), []byte("This is a change"), 0644))
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "commit", "-a", "-m", "test")
+
+	require.NoError(t, ioutil.WriteFile(filepath.Join(testRepoPath, originalFilename), []byte("This is another change"), 0644))
+	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "commit", "-a", "-m", "test")
+
+	endCommitID := text.ChompBytes(testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", "HEAD"))
+
+	request := &gitalypb.UserSquashRequest{
+		Repository:    testRepo,
+		User:          user,
+		SquashId:      "1",
+		Author:        author,
+		CommitMessage: commitMessage,
+		StartSha:      startCommitID,
+		EndSha:        endCommitID,
+	}
+
+	response, err := client.UserSquash(ctx, request)
+	require.NoError(t, err)
+	require.Empty(t, response.GetGitError())
+
+	commit, err := log.GetCommit(ctx, testRepo, response.SquashSha)
+	require.NoError(t, err)
+	require.Equal(t, []string{startCommitID}, commit.ParentIds)
+	require.Equal(t, author.Name, commit.Author.Name)
+	require.Equal(t, author.Email, commit.Author.Email)
+	require.Equal(t, user.Name, commit.Committer.Name)
+	require.Equal(t, user.Email, commit.Committer.Email)
+	require.Equal(t, commitMessage, commit.Subject)
+}
+
+func TestSuccessfulUserSquashRequestWithMissingFileOnTargetBranch(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
@@ -172,13 +231,11 @@ func TestFailedUserSquashRequestDueToGitError(t *testing.T) {
 	defer cleanup()
 
 	conflictingStartSha := "bbd36ad238d14e1c03ece0f3358f545092dc9ca3"
-	branchName := "gitaly-stuff"
 
 	request := &gitalypb.UserSquashRequest{
 		Repository:    testRepo,
 		User:          user,
 		SquashId:      "1",
-		Branch:        []byte(branchName),
 		Author:        author,
 		CommitMessage: commitMessage,
 		StartSha:      conflictingStartSha,
@@ -187,7 +244,7 @@ func TestFailedUserSquashRequestDueToGitError(t *testing.T) {
 
 	response, err := client.UserSquash(ctx, request)
 	require.NoError(t, err)
-	require.Contains(t, response.GitError, "error: large_diff_old_name.md: does not exist in index")
+	require.Empty(t, response.GetGitError())
 }
 
 func TestFailedUserSquashRequestDueToValidations(t *testing.T) {
@@ -211,7 +268,6 @@ func TestFailedUserSquashRequestDueToValidations(t *testing.T) {
 				Repository:    nil,
 				User:          user,
 				SquashId:      "1",
-				Branch:        []byte("some-branch"),
 				Author:        user,
 				CommitMessage: commitMessage,
 				StartSha:      startSha,
@@ -225,7 +281,6 @@ func TestFailedUserSquashRequestDueToValidations(t *testing.T) {
 				Repository:    testRepo,
 				User:          nil,
 				SquashId:      "1",
-				Branch:        []byte("some-branch"),
 				Author:        user,
 				CommitMessage: commitMessage,
 				StartSha:      startSha,
@@ -239,21 +294,6 @@ func TestFailedUserSquashRequestDueToValidations(t *testing.T) {
 				Repository:    testRepo,
 				User:          user,
 				SquashId:      "",
-				Branch:        []byte("some-branch"),
-				Author:        user,
-				CommitMessage: commitMessage,
-				StartSha:      startSha,
-				EndSha:        endSha,
-			},
-			code: codes.InvalidArgument,
-		},
-		{
-			desc: "empty Branch",
-			request: &gitalypb.UserSquashRequest{
-				Repository:    testRepo,
-				User:          user,
-				SquashId:      "1",
-				Branch:        nil,
 				Author:        user,
 				CommitMessage: commitMessage,
 				StartSha:      startSha,
@@ -267,7 +307,6 @@ func TestFailedUserSquashRequestDueToValidations(t *testing.T) {
 				Repository:    testRepo,
 				User:          user,
 				SquashId:      "1",
-				Branch:        []byte("some-branch"),
 				Author:        user,
 				CommitMessage: commitMessage,
 				StartSha:      "",
@@ -281,7 +320,6 @@ func TestFailedUserSquashRequestDueToValidations(t *testing.T) {
 				Repository:    testRepo,
 				User:          user,
 				SquashId:      "1",
-				Branch:        []byte("some-branch"),
 				Author:        user,
 				CommitMessage: commitMessage,
 				StartSha:      startSha,
@@ -295,7 +333,6 @@ func TestFailedUserSquashRequestDueToValidations(t *testing.T) {
 				Repository:    testRepo,
 				User:          user,
 				SquashId:      "1",
-				Branch:        []byte("some-branch"),
 				Author:        nil,
 				CommitMessage: commitMessage,
 				StartSha:      startSha,
@@ -309,7 +346,6 @@ func TestFailedUserSquashRequestDueToValidations(t *testing.T) {
 				Repository:    testRepo,
 				User:          user,
 				SquashId:      "1",
-				Branch:        []byte("some-branch"),
 				Author:        user,
 				CommitMessage: nil,
 				StartSha:      startSha,