From 83fc926987b86b692a0acacc2797b210fceb0c93 Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Tue, 20 Oct 2020 09:47:32 +0200 Subject: gitaly-git2go apply subcommand Implements apply subcommand for gitaly-git2go that allows for applying patches from diffs. Similar to 'git am', three-way merge is attempted as a fallback if the patch does not apply cleanly. --- internal/git2go/apply.go | 112 +++++++++++++++++++++ internal/git2go/apply_test.go | 217 +++++++++++++++++++++++++++++++++++++++++ internal/git2go/commit.go | 2 + internal/git2go/commit_test.go | 7 +- internal/git2go/executor.go | 10 +- 5 files changed, 343 insertions(+), 5 deletions(-) create mode 100644 internal/git2go/apply.go create mode 100644 internal/git2go/apply_test.go (limited to 'internal/git2go') diff --git a/internal/git2go/apply.go b/internal/git2go/apply.go new file mode 100644 index 000000000..447e92530 --- /dev/null +++ b/internal/git2go/apply.go @@ -0,0 +1,112 @@ +package git2go + +import ( + "context" + "encoding/gob" + "fmt" + "io" +) + +// ErrMergeConflict is returned when there is a merge conflict. +var ErrMergeConflict = wrapError{Message: "merge conflict"} + +// Patch represents a single patch. +type Patch struct { + // Author is the author of the patch. + Author Signature + // Message is used as the commit message when applying the patch. + Message string + // Diff contains the diff of the patch. + Diff []byte +} + +// ApplyParams are the parameters for Apply. +type ApplyParams struct { + // Repository is the path to the repository. + Repository string + // Committer is the committer applying the patch. + Committer Signature + // ParentCommit is the OID of the commit to apply the patches against. + ParentCommit string + // Patches iterates over all the patches to be applied. + Patches PatchIterator +} + +// PatchIterator iterates over a stream of patches. +type PatchIterator interface { + // Next returns whether there is a next patch. + Next() bool + // Value returns the patch being currently iterated upon. + Value() Patch + // Err returns the iteration error. Err should + // be always checked after Next returns false. + Err() error +} + +type slicePatchIterator struct { + value Patch + patches []Patch +} + +// NewSlicePatchIterator returns a PatchIterator that iterates over the slice +// of patches. +func NewSlicePatchIterator(patches []Patch) PatchIterator { + return &slicePatchIterator{patches: patches} +} + +func (iter *slicePatchIterator) Next() bool { + if len(iter.patches) == 0 { + return false + } + + iter.value = iter.patches[0] + iter.patches = iter.patches[1:] + return true +} + +func (iter *slicePatchIterator) Value() Patch { return iter.value } + +func (iter *slicePatchIterator) Err() error { return nil } + +// Apply applies the provided patches and returns the OID of the commit with the patches +// applied. +func (b Executor) Apply(ctx context.Context, params ApplyParams) (string, error) { + reader, writer := io.Pipe() + defer writer.Close() + + go func() { + writer.CloseWithError(func() error { + patches := params.Patches + params.Patches = nil + + encoder := gob.NewEncoder(writer) + if err := encoder.Encode(params); err != nil { + return fmt.Errorf("encode header: %w", err) + } + + for patches.Next() { + if err := encoder.Encode(patches.Value()); err != nil { + return fmt.Errorf("encode patch: %w", err) + } + } + + if err := patches.Err(); err != nil { + return fmt.Errorf("patch iterator: %w", err) + } + + return nil + }()) + }() + + var result Result + output, err := run(ctx, b.binaryPath, reader, "apply", "-git-binary-path", b.gitBinaryPath) + if err != nil { + return "", fmt.Errorf("run: %w", err) + } + + if err := gob.NewDecoder(output).Decode(&result); err != nil { + return "", fmt.Errorf("decode: %w", err) + } + + return result.CommitID, result.Error +} diff --git a/internal/git2go/apply_test.go b/internal/git2go/apply_test.go new file mode 100644 index 000000000..1744432b6 --- /dev/null +++ b/internal/git2go/apply_test.go @@ -0,0 +1,217 @@ +package git2go + +import ( + "errors" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/internal/command" + "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/internal/testhelper" +) + +func TestExecutor_Apply(t *testing.T) { + pbRepo, repoPath, clean := testhelper.InitBareRepo(t) + defer clean() + + repo := git.NewRepository(pbRepo) + executor := New(filepath.Join(config.Config.BinDir, "gitaly-git2go"), command.GitPath()) + + ctx, cancel := testhelper.Context() + defer cancel() + + oidBase, err := repo.WriteBlob(ctx, "file", strings.NewReader("base")) + require.NoError(t, err) + + oidA, err := repo.WriteBlob(ctx, "file", strings.NewReader("a")) + require.NoError(t, err) + + oidB, err := repo.WriteBlob(ctx, "file", strings.NewReader("b")) + require.NoError(t, err) + + author := NewSignature("Test Author", "test.author@example.com", time.Now()) + committer := NewSignature("Test Committer", "test.committer@example.com", time.Now()) + + parentCommitSHA, err := executor.Commit(ctx, CommitParams{ + Repository: repoPath, + Author: author, + Committer: committer, + Message: "base commit", + Actions: []Action{CreateFile{Path: "file", OID: oidBase}}, + }) + require.NoError(t, err) + + noCommonAncestor, err := executor.Commit(ctx, CommitParams{ + Repository: repoPath, + Author: author, + Committer: committer, + Message: "commit with ab", + Actions: []Action{CreateFile{Path: "file", OID: oidA}}, + }) + require.NoError(t, err) + + updateToA, err := executor.Commit(ctx, CommitParams{ + Repository: repoPath, + Author: author, + Committer: committer, + Message: "commit with a", + Parent: parentCommitSHA, + Actions: []Action{UpdateFile{Path: "file", OID: oidA}}, + }) + require.NoError(t, err) + + updateToB, err := executor.Commit(ctx, CommitParams{ + Repository: repoPath, + Author: author, + Committer: committer, + Message: "commit to b", + Parent: parentCommitSHA, + Actions: []Action{UpdateFile{Path: "file", OID: oidB}}, + }) + require.NoError(t, err) + + updateFromAToB, err := executor.Commit(ctx, CommitParams{ + Repository: repoPath, + Author: author, + Committer: committer, + Message: "commit a -> b", + Parent: updateToA, + Actions: []Action{UpdateFile{Path: "file", OID: oidB}}, + }) + require.NoError(t, err) + + otherFile, err := executor.Commit(ctx, CommitParams{ + Repository: repoPath, + Author: author, + Committer: committer, + Message: "commit with other-file", + Actions: []Action{CreateFile{Path: "other-file", OID: oidA}}, + }) + require.NoError(t, err) + + diffBetween := func(t testing.TB, fromCommit, toCommit string) []byte { + t.Helper() + return testhelper.MustRunCommand(t, nil, + "git", "-C", repoPath, "format-patch", "--stdout", fromCommit+".."+toCommit) + } + + for _, tc := range []struct { + desc string + patches []Patch + parentCommit string + tree []testhelper.TreeEntry + error error + }{ + { + desc: "patch applies cleanly", + patches: []Patch{ + { + Author: author, + Message: "test commit message", + Diff: diffBetween(t, parentCommitSHA, updateToA), + }, + }, + parentCommit: parentCommitSHA, + tree: []testhelper.TreeEntry{ + {Path: "file", Mode: "100644", Content: "a"}, + }, + }, + { + desc: "multiple patches apply cleanly", + patches: []Patch{ + { + Author: author, + Message: "commit with a", + Diff: diffBetween(t, parentCommitSHA, updateToA), + }, + { + Author: author, + Message: "commit with a -> b", + Diff: diffBetween(t, updateToA, updateFromAToB), + }, + }, + parentCommit: updateToA, + tree: []testhelper.TreeEntry{ + {Path: "file", Mode: "100644", Content: "b"}, + }, + }, + { + desc: "three way merged", + patches: []Patch{ + { + Author: author, + Message: "three-way merged files", + Diff: diffBetween(t, parentCommitSHA, otherFile), + }, + }, + parentCommit: parentCommitSHA, + tree: []testhelper.TreeEntry{ + {Path: "file", Mode: "100644", Content: "base"}, + {Path: "other-file", Mode: "100644", Content: "a"}, + }, + }, + { + // This test asserts incorrect behavior due to a bug in libgit2's + // git_apply_to_tree function. The correct behavior would be to + // return an error about merge conflict but we currently concatenate + // the blobs of the two trees together. This test will fail once the + // issue is fixed. + // + // See: https://gitlab.com/gitlab-org/gitaly/-/issues/3325 + desc: "no common ancestor", + patches: []Patch{ + { + Author: author, + Message: "three-way merged file", + Diff: diffBetween(t, parentCommitSHA, noCommonAncestor), + }, + }, + parentCommit: parentCommitSHA, + // error: ErrMergeConflict, <- correct output + tree: []testhelper.TreeEntry{ + {Path: "file", Mode: "100644", Content: "abase"}, + }, + }, + { + desc: "merge conflict", + patches: []Patch{ + { + Author: author, + Message: "applies cleanly", + Diff: diffBetween(t, parentCommitSHA, updateToA), + }, + { + Author: author, + Message: "conflicts", + Diff: diffBetween(t, parentCommitSHA, updateToB), + }, + }, + error: ErrMergeConflict, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + commitID, err := executor.Apply(ctx, ApplyParams{ + Repository: repoPath, + Committer: committer, + ParentCommit: parentCommitSHA, + Patches: NewSlicePatchIterator(tc.patches), + }) + if tc.error != nil { + require.True(t, errors.Is(err, tc.error), err) + return + } + + require.Equal(t, commit{ + Parent: tc.parentCommit, + Author: author, + Committer: committer, + Message: tc.patches[len(tc.patches)-1].Message, + }, getCommit(t, ctx, repo, commitID)) + testhelper.RequireTree(t, repoPath, commitID, tc.tree) + }) + } +} diff --git a/internal/git2go/commit.go b/internal/git2go/commit.go index ee24862d9..2a9478609 100644 --- a/internal/git2go/commit.go +++ b/internal/git2go/commit.go @@ -39,6 +39,8 @@ type CommitParams struct { Repository string // Author is the author of the commit. Author Signature + // Committer is the committer of the commit. + Committer Signature // Message is message of the commit. Message string // Parent is the OID of the commit to use as the parent of this commit. diff --git a/internal/git2go/commit_test.go b/internal/git2go/commit_test.go index 218c5b51d..0fbe24bd7 100644 --- a/internal/git2go/commit_test.go +++ b/internal/git2go/commit_test.go @@ -13,6 +13,7 @@ import ( "time" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/internal/testhelper" @@ -63,7 +64,7 @@ func TestExecutor_Commit(t *testing.T) { updatedFile, err := repo.WriteBlob(ctx, "file", bytes.NewBufferString("updated")) require.NoError(t, err) - executor := New(filepath.Join(config.Config.BinDir, "gitaly-git2go")) + executor := New(filepath.Join(config.Config.BinDir, "gitaly-git2go"), command.GitPath()) for _, tc := range []struct { desc string @@ -461,12 +462,14 @@ func TestExecutor_Commit(t *testing.T) { } { t.Run(tc.desc, func(t *testing.T) { author := NewSignature("Author Name", "author.email@example.com", time.Now()) + committer := NewSignature("Committer Name", "committer.email@example.com", time.Now()) var parentCommit string for i, step := range tc.steps { message := fmt.Sprintf("commit %d", i+1) commitID, err := executor.Commit(ctx, CommitParams{ Repository: repoPath, Author: author, + Committer: committer, Message: message, Parent: parentCommit, Actions: step.actions, @@ -482,7 +485,7 @@ func TestExecutor_Commit(t *testing.T) { require.Equal(t, commit{ Parent: parentCommit, Author: author, - Committer: author, + Committer: committer, Message: message, }, getCommit(t, ctx, repo, commitID)) diff --git a/internal/git2go/executor.go b/internal/git2go/executor.go index 0007ab7e5..65058d2d2 100644 --- a/internal/git2go/executor.go +++ b/internal/git2go/executor.go @@ -2,10 +2,14 @@ package git2go // Executor executes gitaly-git2go. type Executor struct { - binaryPath string + binaryPath string + gitBinaryPath string } // New returns a new gitaly-git2go executor using the provided binary. -func New(binaryPath string) Executor { - return Executor{binaryPath: binaryPath} +func New(binaryPath, gitBinaryPath string) Executor { + return Executor{ + binaryPath: binaryPath, + gitBinaryPath: gitBinaryPath, + } } -- cgit v1.2.3