diff options
author | James Fargher <proglottis@gmail.com> | 2021-09-23 03:27:13 +0300 |
---|---|---|
committer | James Fargher <proglottis@gmail.com> | 2021-09-23 03:27:13 +0300 |
commit | 2d8fbedec7f734d3953a6218325ce38c0457d27a (patch) | |
tree | 97572e6cf6a11022af9baa936d8baf8bbc8ee7b5 /cmd | |
parent | a47a975ef7d4ef51e0d68c5662d5cb3bb5b83b76 (diff) | |
parent | c0e4ef911e65635f215da10344c1900029f2e318 (diff) |
Merge branch 'pks-operations-drop-git2go-merge-gob-ff' into 'master'
operations: Always return structured errors from Git2Go merges
Closes #3756
See merge request gitlab-org/gitaly!3896
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 |