diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2021-09-21 12:03:08 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2021-09-21 12:03:08 +0300 |
commit | c0e4ef911e65635f215da10344c1900029f2e318 (patch) | |
tree | eb2463292360dd357db21c16a82c147feb917019 /cmd | |
parent | 5b7382e0251176923f258aa8f8c23a8084f6cae9 (diff) |
operations: Always return structured errors from Git2Go merges
In commit 84bd2721e (gitaly-git2go: Return structured conflict error on
merge conflict, 2021-08-26), we have changed gitaly-git2go's merge
subcommand to support structured errors. This allows us to return more
information why a merge failed, e.g. by returning the set of conflicting
files to the caller.
Drop the feature flag which guards this code and thus always enable the
use of structured errors.
Diffstat (limited to 'cmd')
-rw-r--r-- | cmd/gitaly-git2go/merge.go | 76 | ||||
-rw-r--r-- | cmd/gitaly-git2go/merge_test.go | 79 |
2 files changed, 51 insertions, 104 deletions
diff --git a/cmd/gitaly-git2go/merge.go b/cmd/gitaly-git2go/merge.go index 8d57cb16d..316e49714 100644 --- a/cmd/gitaly-git2go/merge.go +++ b/cmd/gitaly-git2go/merge.go @@ -18,27 +18,16 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/git2go" ) -type mergeSubcommand struct { - request string -} +type mergeSubcommand struct{} func (cmd *mergeSubcommand) Flags() *flag.FlagSet { flags := flag.NewFlagSet("merge", flag.ExitOnError) - flags.StringVar(&cmd.request, "request", "", "git2go.MergeCommand") return flags } func (cmd *mergeSubcommand) Run(_ context.Context, r io.Reader, w io.Writer) error { - useGob := cmd.request == "" - - var err error var request git2go.MergeCommand - if useGob { - err = gob.NewDecoder(r).Decode(&request) - } else { - request, err = git2go.MergeCommandFromSerialized(cmd.request) - } - if err != nil { + if err := gob.NewDecoder(r).Decode(&request); err != nil { return err } @@ -46,30 +35,15 @@ func (cmd *mergeSubcommand) Run(_ context.Context, r io.Reader, w io.Writer) err request.AuthorDate = time.Now() } - commitID, err := merge(request, useGob) - if useGob { - return gob.NewEncoder(w).Encode(git2go.Result{ - CommitID: commitID, - Error: git2go.SerializableError(err), - }) - } + commitID, err := merge(request) - if err != nil { - return err - } - - response := git2go.MergeResult{ + return gob.NewEncoder(w).Encode(git2go.Result{ CommitID: commitID, - } - - if err := response.SerializeTo(w); err != nil { - return err - } - - return nil + Error: git2go.SerializableError(err), + }) } -func merge(request git2go.MergeCommand, useStructuredErrors bool) (string, error) { +func merge(request git2go.MergeCommand) (string, error) { repo, err := git2goutil.OpenRepository(request.Repository) if err != nil { return "", fmt.Errorf("could not open repository: %w", err) @@ -100,21 +74,17 @@ func merge(request git2go.MergeCommand, useStructuredErrors bool) (string, error if index.HasConflicts() { if !request.AllowConflicts { - if useStructuredErrors { - conflictingFiles, err := getConflictingFiles(index) - if err != nil { - return "", fmt.Errorf("getting conflicting files: %w", err) - } - - return "", git2go.ConflictingFilesError{ - ConflictingFiles: conflictingFiles, - } + conflictingFiles, err := getConflictingFiles(index) + if err != nil { + return "", fmt.Errorf("getting conflicting files: %w", err) } - return "", errors.New("could not auto-merge due to conflicts") + return "", git2go.ConflictingFilesError{ + ConflictingFiles: conflictingFiles, + } } - if err := resolveConflicts(repo, index, useStructuredErrors); err != nil { + if err := resolveConflicts(repo, index); err != nil { return "", fmt.Errorf("could not resolve conflicts: %w", err) } } @@ -133,7 +103,7 @@ func merge(request git2go.MergeCommand, useStructuredErrors bool) (string, error return commit.String(), nil } -func resolveConflicts(repo *git.Repository, index *git.Index, useStructuredErrors bool) error { +func resolveConflicts(repo *git.Repository, index *git.Index) error { // We need to get all conflicts up front as resolving conflicts as we // iterate breaks the iterator. indexConflicts, err := getConflicts(index) @@ -194,18 +164,14 @@ func resolveConflicts(repo *git.Repository, index *git.Index, useStructuredError } if index.HasConflicts() { - if useStructuredErrors { - conflictingFiles, err := getConflictingFiles(index) - if err != nil { - return fmt.Errorf("getting conflicting files: %w", err) - } - - return git2go.ConflictingFilesError{ - ConflictingFiles: conflictingFiles, - } + conflictingFiles, err := getConflictingFiles(index) + if err != nil { + return fmt.Errorf("getting conflicting files: %w", err) } - return errors.New("index still has conflicts") + return git2go.ConflictingFilesError{ + ConflictingFiles: conflictingFiles, + } } return nil diff --git a/cmd/gitaly-git2go/merge_test.go b/cmd/gitaly-git2go/merge_test.go index 99aca86d4..d12937e98 100644 --- a/cmd/gitaly-git2go/merge_test.go +++ b/cmd/gitaly-git2go/merge_test.go @@ -4,8 +4,6 @@ package main import ( - "context" - "errors" "fmt" "testing" "time" @@ -18,18 +16,16 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest" "gitlab.com/gitlab-org/gitaly/v14/internal/git2go" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config" - "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/testcfg" ) -func TestMergeFailsWithMissingArguments(t *testing.T) { - testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.Git2GoMergeGob, - }).Run(t, testMergeFailsWithMissingArguments) -} +func TestMerge_missingArguments(t *testing.T) { + t.Parallel() + + ctx, cancel := testhelper.Context() + defer cancel() -func testMergeFailsWithMissingArguments(t *testing.T, ctx context.Context) { cfg, repo, repoPath := testcfg.BuildWithRepo(t) executor := git2go.NewExecutor(cfg, config.NewLocator(cfg)) @@ -83,13 +79,12 @@ func testMergeFailsWithMissingArguments(t *testing.T, ctx context.Context) { } } -func TestMergeFailsWithInvalidRepositoryPath(t *testing.T) { - testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.Git2GoMergeGob, - }).Run(t, testMergeFailsWithInvalidRepositoryPath) -} +func TestMerge_invalidRepositoryPath(t *testing.T) { + t.Parallel() + + ctx, cancel := testhelper.Context() + defer cancel() -func testMergeFailsWithInvalidRepositoryPath(t *testing.T, ctx context.Context) { cfg, repo, _ := testcfg.BuildWithRepo(t) testhelper.BuildGitalyGit2Go(t, cfg) executor := git2go.NewExecutor(cfg, config.NewLocator(cfg)) @@ -101,22 +96,20 @@ func testMergeFailsWithInvalidRepositoryPath(t *testing.T, ctx context.Context) require.Contains(t, err.Error(), "merge: could not open repository") } -func TestMergeTrees(t *testing.T) { - testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.Git2GoMergeGob, - }).Run(t, testMergeTrees) -} +func TestMerge_trees(t *testing.T) { + t.Parallel() + + ctx, cancel := testhelper.Context() + defer cancel() -func testMergeTrees(t *testing.T, ctx context.Context) { testcases := []struct { - desc string - base map[string]string - ours map[string]string - theirs map[string]string - expected map[string]string - expectedResponse git2go.MergeResult - expectedErr error - expectedErrWithoutGob error + desc string + base map[string]string + ours map[string]string + theirs map[string]string + expected map[string]string + expectedResponse git2go.MergeResult + expectedErr error }{ { desc: "trivial merge succeeds", @@ -194,8 +187,6 @@ func testMergeTrees(t *testing.T, ctx context.Context) { expectedErr: fmt.Errorf("merge: %w", git2go.ConflictingFilesError{ ConflictingFiles: []string{"1"}, }), - //nolint:revive - expectedErrWithoutGob: errors.New("merge: could not auto-merge due to conflicts\n"), }, } @@ -222,11 +213,7 @@ func testMergeTrees(t *testing.T, ctx context.Context) { }) if tc.expectedErr != nil { - if featureflag.Git2GoMergeGob.IsEnabled(ctx) { - require.Equal(t, tc.expectedErr, err) - } else { - require.Equal(t, tc.expectedErrWithoutGob, err) - } + require.Equal(t, tc.expectedErr, err) return } @@ -260,12 +247,11 @@ func testMergeTrees(t *testing.T, ctx context.Context) { } func TestMerge_recursive(t *testing.T) { - testhelper.NewFeatureSets([]featureflag.FeatureFlag{ - featureflag.Git2GoMergeGob, - }).Run(t, testMergeRecursive) -} + t.Parallel() + + ctx, cancel := testhelper.Context() + defer cancel() -func testMergeRecursive(t *testing.T, ctx context.Context) { cfg := testcfg.Build(t) testhelper.BuildGitalyGit2Go(t, cfg) executor := git2go.NewExecutor(cfg, config.NewLocator(cfg)) @@ -332,14 +318,9 @@ func testMergeRecursive(t *testing.T, ctx context.Context) { Ours: ours[len(ours)-1].String(), Theirs: theirs[len(theirs)-1].String(), }) - if featureflag.Git2GoMergeGob.IsEnabled(ctx) { - require.Equal(t, fmt.Errorf("merge: %w", git2go.ConflictingFilesError{ - ConflictingFiles: []string{"theirs"}, - }), err) - } else { - //nolint:revive - require.Equal(t, errors.New("merge: could not auto-merge due to conflicts\n"), err) - } + require.Equal(t, fmt.Errorf("merge: %w", git2go.ConflictingFilesError{ + ConflictingFiles: []string{"theirs"}, + }), err) // Otherwise, if we're merging an earlier criss-cross merge which has // half of the limit many criss-cross patterns, we exactly hit the |