Merge branch 'squash-using-merge' into 'master'

Use git2go merge for squashing

Closes gitlab#352581

See merge request gitlab-org/gitaly!4514

5 files changed, 293 insertions, 77 deletions

diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go
index 9b3adaba9..16bc61e1c 100644
--- a/internal/gitaly/service/operations/squash.go
+++ b/internal/gitaly/service/operations/squash.go
@@ -5,12 +5,14 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git2go"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/voting"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
@@ -137,84 +139,138 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 		return "", helper.ErrInvalidArgument(err)
 	}
 
-	// We're now rebasing the end commit on top of the start commit. The resulting tree
-	// is then going to be the tree of the squashed commit.
-	rebasedCommitID, err := s.git2goExecutor.Rebase(ctx, quarantineRepo, git2go.RebaseCommand{
-		Repository: quarantineRepoPath,
-		Committer: git2go.NewSignature(
-			string(req.GetUser().Name), string(req.GetUser().Email), commitDate,
-		),
-		CommitID:         endCommit,
-		UpstreamCommitID: startCommit,
-		SkipEmptyCommits: true,
-	})
-	if err != nil {
-		var conflictErr git2go.ConflictingFilesError
+	var commitID string
+	if featureflag.SquashUsingMerge.IsEnabled(ctx) {
+		message := string(req.GetCommitMessage())
+		// In previous implementation, we've used git commit-tree to create commit.
+		// When message wasn't empty and didn't end in a new line,
+		// git commit-tree would add a trailing new line to the commit message.
+		// Let's keep that behaviour for compatibility.
+		if len(message) > 0 && !strings.HasSuffix(message, "\n") {
+			message += "\n"
+		}
+
+		merge, err := s.git2goExecutor.Merge(ctx, quarantineRepo, git2go.MergeCommand{
+			Repository:    quarantineRepoPath,
+			AuthorName:    string(req.GetAuthor().GetName()),
+			AuthorMail:    string(req.GetAuthor().GetEmail()),
+			AuthorDate:    commitDate,
+			CommitterName: string(req.GetUser().Name),
+			CommitterMail: string(req.GetUser().Email),
+			CommitterDate: commitDate,
+			Message:       message,
+			Ours:          startCommit.String(),
+			Theirs:        endCommit.String(),
+			Squash:        true,
+		})
+		if err != nil {
+			var conflictErr git2go.ConflictingFilesError
+
+			if errors.As(err, &conflictErr) {
+				conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles))
+				for _, conflictingFile := range conflictErr.ConflictingFiles {
+					conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
+				}
+
+				detailedErr, err := helper.ErrWithDetails(
+					helper.ErrFailedPreconditionf("squashing commits: %w", err),
+					&gitalypb.UserSquashError{
+						Error: &gitalypb.UserSquashError_RebaseConflict{
+							RebaseConflict: &gitalypb.MergeConflictError{
+								ConflictingFiles: conflictingFiles,
+							},
+						},
+					},
+				)
+				if err != nil {
+					return "", helper.ErrInternalf("error details: %w", err)
+				}
 
-		if errors.As(err, &conflictErr) {
-			conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles))
-			for _, conflictingFile := range conflictErr.ConflictingFiles {
-				conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
+				return "", detailedErr
 			}
+		}
 
-			detailedErr, err := helper.ErrWithDetails(
-				helper.ErrFailedPreconditionf("rebasing commits: %w", err),
-				&gitalypb.UserSquashError{
-					Error: &gitalypb.UserSquashError_RebaseConflict{
-						RebaseConflict: &gitalypb.MergeConflictError{
-							ConflictingFiles: conflictingFiles,
+		commitID = merge.CommitID
+	} else {
+		// We're now rebasing the end commit on top of the start commit. The resulting tree
+		// is then going to be the tree of the squashed commit.
+		rebasedCommitID, err := s.git2goExecutor.Rebase(ctx, quarantineRepo, git2go.RebaseCommand{
+			Repository: quarantineRepoPath,
+			Committer: git2go.NewSignature(
+				string(req.GetUser().Name), string(req.GetUser().Email), commitDate,
+			),
+			CommitID:         endCommit,
+			UpstreamCommitID: startCommit,
+			SkipEmptyCommits: true,
+		})
+		if err != nil {
+			var conflictErr git2go.ConflictingFilesError
+
+			if errors.As(err, &conflictErr) {
+				conflictingFiles := make([][]byte, 0, len(conflictErr.ConflictingFiles))
+				for _, conflictingFile := range conflictErr.ConflictingFiles {
+					conflictingFiles = append(conflictingFiles, []byte(conflictingFile))
+				}
+
+				detailedErr, err := helper.ErrWithDetails(
+					helper.ErrFailedPreconditionf("rebasing commits: %w", err),
+					&gitalypb.UserSquashError{
+						Error: &gitalypb.UserSquashError_RebaseConflict{
+							RebaseConflict: &gitalypb.MergeConflictError{
+								ConflictingFiles: conflictingFiles,
+							},
 						},
 					},
-				},
-			)
-			if err != nil {
-				return "", helper.ErrInternalf("error details: %w", err)
+				)
+				if err != nil {
+					return "", helper.ErrInternalf("error details: %w", err)
+				}
+
+				return "", detailedErr
 			}
 
-			return "", detailedErr
+			return "", helper.ErrInternalf("rebasing commits: %w", err)
 		}
 
-		return "", helper.ErrInternalf("rebasing commits: %w", err)
-	}
+		treeID, err := quarantineRepo.ResolveRevision(ctx, rebasedCommitID.Revision()+"^{tree}")
+		if err != nil {
+			return "", fmt.Errorf("cannot resolve rebased tree: %w", err)
+		}
 
-	treeID, err := quarantineRepo.ResolveRevision(ctx, rebasedCommitID.Revision()+"^{tree}")
-	if err != nil {
-		return "", fmt.Errorf("cannot resolve rebased tree: %w", err)
-	}
+		commitEnv := []string{
+			"GIT_COMMITTER_NAME=" + string(req.GetUser().Name),
+			"GIT_COMMITTER_EMAIL=" + string(req.GetUser().Email),
+			fmt.Sprintf("GIT_COMMITTER_DATE=%d %s", commitDate.Unix(), commitDate.Format("-0700")),
+			"GIT_AUTHOR_NAME=" + string(req.GetAuthor().Name),
+			"GIT_AUTHOR_EMAIL=" + string(req.GetAuthor().Email),
+			fmt.Sprintf("GIT_AUTHOR_DATE=%d %s", commitDate.Unix(), commitDate.Format("-0700")),
+		}
 
-	commitEnv := []string{
-		"GIT_COMMITTER_NAME=" + string(req.GetUser().Name),
-		"GIT_COMMITTER_EMAIL=" + string(req.GetUser().Email),
-		fmt.Sprintf("GIT_COMMITTER_DATE=%d %s", commitDate.Unix(), commitDate.Format("-0700")),
-		"GIT_AUTHOR_NAME=" + string(req.GetAuthor().Name),
-		"GIT_AUTHOR_EMAIL=" + string(req.GetAuthor().Email),
-		fmt.Sprintf("GIT_AUTHOR_DATE=%d %s", commitDate.Unix(), commitDate.Format("-0700")),
-	}
+		flags := []git.Option{
+			git.ValueFlag{
+				Name:  "-m",
+				Value: string(req.GetCommitMessage()),
+			},
+			git.ValueFlag{
+				Name:  "-p",
+				Value: startCommit.String(),
+			},
+		}
 
-	flags := []git.Option{
-		git.ValueFlag{
-			Name:  "-m",
-			Value: string(req.GetCommitMessage()),
-		},
-		git.ValueFlag{
-			Name:  "-p",
-			Value: startCommit.String(),
-		},
-	}
+		var stdout, stderr bytes.Buffer
+		if err := quarantineRepo.ExecAndWait(ctx, git.SubCmd{
+			Name:  "commit-tree",
+			Flags: flags,
+			Args: []string{
+				treeID.String(),
+			},
+		}, git.WithStdout(&stdout), git.WithStderr(&stderr), git.WithEnv(commitEnv...)); err != nil {
+			return "", helper.ErrInternalf("creating squashed commit: %w", err)
+		}
 
-	var stdout, stderr bytes.Buffer
-	if err := quarantineRepo.ExecAndWait(ctx, git.SubCmd{
-		Name:  "commit-tree",
-		Flags: flags,
-		Args: []string{
-			treeID.String(),
-		},
-	}, git.WithStdout(&stdout), git.WithStderr(&stderr), git.WithEnv(commitEnv...)); err != nil {
-		return "", helper.ErrInternalf("creating squashed commit: %w", err)
+		commitID = text.ChompBytes(stdout.Bytes())
 	}
 
-	commitID := text.ChompBytes(stdout.Bytes())
-
 	// The RPC is badly designed in that it never updates any references, but only creates the
 	// objects and writes them to disk. We still use a quarantine directory to stage the new
 	// objects, vote on them and migrate them into the main directory if quorum was reached so
diff --git a/internal/gitaly/service/operations/squash_test.go b/internal/gitaly/service/operations/squash_test.go
index 57cb362fe..e39b9edb2 100644
--- a/internal/gitaly/service/operations/squash_test.go
+++ b/internal/gitaly/service/operations/squash_test.go
@@ -17,6 +17,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
@@ -41,7 +42,11 @@ var (
 func TestUserSquash_successful(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashSuccessful)
+}
+
+func testUserSquashSuccessful(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	for _, tc := range []struct {
 		desc             string
@@ -98,7 +103,11 @@ func TestUserSquash_successful(t *testing.T) {
 func TestUserSquash_transactional(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashTransactional)
+}
+
+func testUserSquashTransactional(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	txManager := transaction.MockManager{}
 
@@ -212,7 +221,11 @@ func TestUserSquash_transactional(t *testing.T) {
 func TestUserSquash_stableID(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashStableID)
+}
+
+func testUserSquashStableID(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx)
 
@@ -272,7 +285,11 @@ func ensureSplitIndexExists(t *testing.T, cfg config.Cfg, repoDir string) bool {
 func TestUserSquash_threeWayMerge(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashThreeWayMerge)
+}
+
+func testUserSquashThreeWayMerge(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
 	ctx, cfg, repoProto, _, client := setupOperationsService(t, ctx)
 
@@ -307,8 +324,12 @@ func TestUserSquash_threeWayMerge(t *testing.T) {
 
 func TestUserSquash_splitIndex(t *testing.T) {
 	t.Parallel()
+	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashSplitIndex)
+}
+
+func testUserSquashSplitIndex(t *testing.T, ctx context.Context) {
+	t.Parallel()
 
-	ctx := testhelper.Context(t)
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	require.False(t, ensureSplitIndexExists(t, cfg, repoPath))
@@ -332,7 +353,10 @@ func TestUserSquash_splitIndex(t *testing.T) {
 func TestUserSquash_renames(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashRenames)
+}
+
+func testUserSquashRenames(t *testing.T, ctx context.Context) {
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	gittest.AddWorktree(t, cfg, repoPath, "worktree")
@@ -392,7 +416,12 @@ func TestUserSquash_renames(t *testing.T) {
 func TestUserSquash_missingFileOnTargetBranch(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashMissingFileOnTargetBranch)
+}
+
+func testUserSquashMissingFileOnTargetBranch(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
 	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
 	conflictingStartSha := "bbd36ad238d14e1c03ece0f3358f545092dc9ca3"
@@ -415,7 +444,12 @@ func TestUserSquash_missingFileOnTargetBranch(t *testing.T) {
 func TestUserSquash_emptyCommit(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashEmptyCommit)
+}
+
+func testUserSquashEmptyCommit(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
 
@@ -614,7 +648,11 @@ func TestUserSquash_validation(t *testing.T) {
 func TestUserSquash_conflicts(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashConflicts)
+}
+
+func testUserSquashConflicts(t *testing.T, ctx context.Context) {
+	t.Parallel()
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	base := gittest.WriteCommit(t, cfg, repoPath, gittest.WithTreeEntries(
@@ -641,8 +679,13 @@ func TestUserSquash_conflicts(t *testing.T) {
 		EndSha:        ours.String(),
 	})
 
+	expectedErr := helper.ErrFailedPreconditionf("rebasing commits: rebase: commit %q: there are conflicting files", ours)
+	if featureflag.SquashUsingMerge.IsEnabled(ctx) {
+		expectedErr = helper.ErrFailedPreconditionf("squashing commits: merge: there are conflicting files")
+	}
+
 	testhelper.RequireGrpcError(t, errWithDetails(t,
-		helper.ErrFailedPreconditionf("rebasing commits: rebase: commit %q: there are conflicting files", ours),
+		expectedErr,
 		&gitalypb.UserSquashError{
 			Error: &gitalypb.UserSquashError_RebaseConflict{
 				RebaseConflict: &gitalypb.MergeConflictError{
@@ -659,7 +702,12 @@ func TestUserSquash_conflicts(t *testing.T) {
 func TestUserSquash_ancestry(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashAncestry)
+}
+
+func testUserSquashAncestry(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
 	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
 
 	// We create an empty parent commit and two commits which both branch off from it. As a
@@ -693,7 +741,11 @@ func TestUserSquash_ancestry(t *testing.T) {
 func TestUserSquash_gitError(t *testing.T) {
 	t.Parallel()
 
-	ctx := testhelper.Context(t)
+	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashGitError)
+}
+
+func testUserSquashGitError(t *testing.T, ctx context.Context) {
+	t.Parallel()
 	ctx, _, repo, _, client := setupOperationsService(t, ctx)
 
 	testCases := []struct {
@@ -778,3 +830,104 @@ func TestUserSquash_gitError(t *testing.T) {
 		})
 	}
 }
+
+func TestUserSquash_squashingMerge(t *testing.T) {
+	t.Parallel()
+
+	testhelper.NewFeatureSets(featureflag.SquashUsingMerge).Run(t, testUserSquashingMerge)
+}
+
+func testUserSquashingMerge(t *testing.T, ctx context.Context) {
+	t.Parallel()
+
+	ctx, cfg, repo, repoPath, client := setupOperationsService(t, ctx)
+
+	base := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("base"),
+		gittest.WithTreeEntries(gittest.TreeEntry{Path: "a", Mode: "100644", Content: "base-content"}),
+		gittest.WithParents(),
+	)
+	ours := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("ours"),
+		gittest.WithTreeEntries(gittest.TreeEntry{Path: "a", Mode: "100644", Content: "ours-content"}),
+		gittest.WithParents(base),
+	)
+	theirs := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("theirs"),
+		gittest.WithTreeEntries(gittest.TreeEntry{Path: "a", Mode: "100644", Content: "theirs-content"}),
+		gittest.WithParents(base),
+	)
+	oursMergedIntoTheirs := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("merge ours into theirs"),
+		gittest.WithTreeEntries(gittest.TreeEntry{Path: "a", Mode: "100644", Content: "ours-content\ntheirs-content"}),
+		gittest.WithParents(theirs, ours),
+	)
+	ours2 := gittest.WriteCommit(t, cfg, repoPath, gittest.WithMessage("ours 2"),
+		gittest.WithTreeEntries(
+			gittest.TreeEntry{Path: "a", Mode: "100644", Content: "ours-content"},
+			gittest.TreeEntry{Path: "ours-file", Mode: "100644", Content: "new-content"},
+		),
+		gittest.WithParents(ours),
+	)
+
+	// We had conflicting commit on "ours" and on "theirs",
+	// then we have manually merged "ours into "theirs" resolving the conflict,
+	// and then we created one non-conflicting commit on branch "ours".
+	//
+	//        o-------o ours
+	//       / \
+	// base o   X
+	//       \   \
+	//        o---o     theirs
+	//
+	// We're now squashing both commits from "theirs" onto "ours".
+	response, err := client.UserSquash(ctx, &gitalypb.UserSquashRequest{
+		Repository:    repo,
+		User:          gittest.TestUser,
+		Author:        gittest.TestUser,
+		CommitMessage: commitMessage,
+		StartSha:      ours2.String(),
+		EndSha:        oursMergedIntoTheirs.String(),
+		Timestamp:     &timestamppb.Timestamp{Seconds: 1234512345},
+	})
+
+	if featureflag.SquashUsingMerge.IsEnabled(ctx) {
+		// With squashing using merge, we should successfully merge without any issues.
+		// The new detached commit history will look like this:
+		//
+		// HEAD o---o---o---o
+		//
+		// We have one commit from "base", two from "ours"
+		// and one squash commit that contains squashed changes from branch "theirs".
+		require.Nil(t, err)
+		testhelper.ProtoEqual(t, &gitalypb.UserSquashResponse{
+			SquashSha: "69d8db2439502c18b9c17c2d1bddb122a82bd448",
+		}, response)
+		gittest.RequireTree(t, cfg, repoPath, "69d8db2439502c18b9c17c2d1bddb122a82bd448", []gittest.TreeEntry{
+			{
+				// It should use the version from commit "oursMergedIntoTheirs",
+				// as it resolves the pre-existing conflict.
+				Content: "ours-content\ntheirs-content",
+				Mode:    "100644",
+				Path:    "a",
+			},
+			{
+				// This is the file that only existed on branch "ours".
+				Content: "new-content",
+				Mode:    "100644",
+				Path:    "ours-file",
+			},
+		})
+	} else {
+		// With the old method, in which we have to rebase first, we will re-encounter
+		// the already-resolved conflict and won't be able to perform the squash.
+		testhelper.RequireGrpcError(t, errWithDetails(t,
+			helper.ErrFailedPreconditionf("rebasing commits: rebase: commit %q: there are conflicting files", theirs),
+			&gitalypb.UserSquashError{
+				Error: &gitalypb.UserSquashError_RebaseConflict{
+					RebaseConflict: &gitalypb.MergeConflictError{
+						ConflictingFiles: [][]byte{
+							[]byte("a"),
+						},
+					},
+				},
+			},
+		), err)
+	}
+}
diff --git a/internal/metadata/featureflag/ff_squash_using_merge.go b/internal/metadata/featureflag/ff_squash_using_merge.go
new file mode 100644
index 000000000..2ef265d4d
--- /dev/null
+++ b/internal/metadata/featureflag/ff_squash_using_merge.go
@@ -0,0 +1,5 @@
+package featureflag
+
+// SquashUsingMerge uses merge to squash commits instead of rebase and commit-tree.
+// It should be better at handling conflicts.
+var SquashUsingMerge = NewFeatureFlag("squash_using_merge", false)
diff --git a/proto/go/gitalypb/operations.pb.go b/proto/go/gitalypb/operations.pb.go
index 5131a8573..67c8676e1 100644
--- a/proto/go/gitalypb/operations.pb.go
+++ b/proto/go/gitalypb/operations.pb.go
@@ -3003,7 +3003,8 @@ type isUserRebaseConfirmableError_Error interface {
 
 type UserRebaseConfirmableError_RebaseConflict struct {
 	// RebaseConflict is returned in case rebasing commits on top of the start
-	// commit fails with a merge conflict.
+	// commit fails with a merge conflict and in case merge squashing commits
+	// fails with a merge conflict.
 	RebaseConflict *MergeConflictError `protobuf:"bytes,1,opt,name=rebase_conflict,json=rebaseConflict,proto3,oneof"`
 }
 
diff --git a/proto/operations.proto b/proto/operations.proto
index 3db97dc30..b4afcd0bd 100644
--- a/proto/operations.proto
+++ b/proto/operations.proto
@@ -736,7 +736,8 @@ message UserSquashResponse {
 message UserRebaseConfirmableError {
   oneof error {
     // RebaseConflict is returned in case rebasing commits on top of the start
-    // commit fails with a merge conflict.
+    // commit fails with a merge conflict and in case merge squashing commits
+    // fails with a merge conflict.
     MergeConflictError rebase_conflict = 1;
     // AccessCheckError is returned in case GitLab's `/internal/allowed` endpoint rejected
     // the change.