From 73486d5e71a7b9bc299e9f2f2dc2f879c9bb6a9d Mon Sep 17 00:00:00 2001 From: Enver Bisevac Date: Thu, 17 Aug 2023 17:00:39 +0200 Subject: [PATCH] errors are properly returned from diff-stats and merge-check api --- gitrpc/commit.go | 4 ++-- gitrpc/diff.go | 8 +++++--- gitrpc/errors.go | 3 +++ gitrpc/internal/gitea/diff.go | 4 ++-- gitrpc/internal/gitea/mapping.go | 11 ++++++++++- gitrpc/internal/gitea/merge.go | 4 ++-- gitrpc/internal/service/diff.go | 7 +++---- gitrpc/internal/service/errors.go | 10 +++++----- gitrpc/internal/service/merge.go | 2 +- 9 files changed, 33 insertions(+), 20 deletions(-) diff --git a/gitrpc/commit.go b/gitrpc/commit.go index 5408a560d..aca757a7e 100644 --- a/gitrpc/commit.go +++ b/gitrpc/commit.go @@ -219,7 +219,7 @@ func (c *Client) GetCommitDivergences(ctx context.Context, divergences := resp.GetDivergences() if divergences == nil { - return nil, fmt.Errorf("server response divergences were nil") + return nil, NewError(StatusInternal, "server response divergences were nil") } // build output @@ -228,7 +228,7 @@ func (c *Client) GetCommitDivergences(ctx context.Context, } for i := range divergences { if divergences[i] == nil { - return nil, fmt.Errorf("server returned nil divergence") + return nil, NewError(StatusInternal, "server returned nil divergence") } output.Divergences[i] = CommitDivergence{ diff --git a/gitrpc/diff.go b/gitrpc/diff.go index 97f3cbdc1..5397a2061 100644 --- a/gitrpc/diff.go +++ b/gitrpc/diff.go @@ -82,7 +82,8 @@ func (c *Client) DiffShortStat(ctx context.Context, params *DiffParams) (DiffSho MergeBase: params.MergeBase, }) if err != nil { - return DiffShortStatOutput{}, err + return DiffShortStatOutput{}, processRPCErrorf(err, "failed to get diff data between '%s' and '%s'", + params.BaseRef, params.HeadRef) } return DiffShortStatOutput{ Files: int(stat.GetFiles()), @@ -122,7 +123,8 @@ func (c *Client) DiffStats(ctx context.Context, params *DiffParams) (DiffStatsOu rpcOutput, err := c.GetCommitDivergences(groupCtx, options) if err != nil { - return fmt.Errorf("failed to count pull request commits: %w", err) + return processRPCErrorf(err, "failed to count pull request commits between '%s' and '%s'", + params.BaseRef, params.HeadRef) } if len(rpcOutput.Divergences) > 0 { totalCommits = int(rpcOutput.Divergences[0].Ahead) @@ -139,7 +141,7 @@ func (c *Client) DiffStats(ctx context.Context, params *DiffParams) (DiffStatsOu MergeBase: true, // must be true, because commitDivergences use tripple dot notation }) if err != nil { - return fmt.Errorf("failed to count pull request file changes: %w", err) + return err } totalFiles = stat.Files return nil diff --git a/gitrpc/errors.go b/gitrpc/errors.go index 26dc2822d..d97ef67bb 100644 --- a/gitrpc/errors.go +++ b/gitrpc/errors.go @@ -127,6 +127,9 @@ func ErrInvalidArgumentf(format string, args ...interface{}) *Error { } func processRPCErrorf(err error, format string, args ...interface{}) error { + if errors.Is(err, &Error{}) { + return err + } // create fallback error returned if we can't map it fallbackMsg := fmt.Sprintf(format, args...) fallbackErr := NewError(StatusInternal, fallbackMsg) diff --git a/gitrpc/internal/gitea/diff.go b/gitrpc/internal/gitea/diff.go index 272254d16..8f5fb27a0 100644 --- a/gitrpc/internal/gitea/diff.go +++ b/gitrpc/internal/gitea/diff.go @@ -64,8 +64,8 @@ func (g Adapter) DiffShortStat( } numFiles, totalAdditions, totalDeletions, err := git.GetDiffShortStat(ctx, repoPath, shortstatArgs...) if err != nil { - return types.DiffShortStat{}, fmt.Errorf("failed to get diff short stat between %s and %s with err: %w", - baseRef, headRef, err) + return types.DiffShortStat{}, processGiteaErrorf(err, "failed to get diff short stat between %s and %s", + baseRef, headRef) } return types.DiffShortStat{ Files: numFiles, diff --git a/gitrpc/internal/gitea/mapping.go b/gitrpc/internal/gitea/mapping.go index cb6435c43..c5a2e024b 100644 --- a/gitrpc/internal/gitea/mapping.go +++ b/gitrpc/internal/gitea/mapping.go @@ -63,7 +63,16 @@ func mapGiteaRunStdError(err gitea.RunStdError, fallback error) error { // exit status 128 - fatal: ambiguous argument 'branch1...branch2': unknown revision or path not in the working tree. case err.IsExitCode(128) && strings.Contains(err.Stderr(), "unknown revision"): - return types.ErrNotFound + msg := "unknown revision or path not in the working tree" + // parse the error response from git output + lines := strings.Split(err.Error(), "\n") + if len(lines) > 0 { + cols := strings.Split(lines[0], ": ") + if len(cols) >= 2 { + msg = cols[1] + ", " + cols[2] + } + } + return fmt.Errorf("%v err: %w", msg, types.ErrNotFound) // exit status 128 - fatal: couldn't find remote ref v1. case err.IsExitCode(128) && strings.Contains(err.Stderr(), "couldn't find"): diff --git a/gitrpc/internal/gitea/merge.go b/gitrpc/internal/gitea/merge.go index 73b31af01..3bee680c3 100644 --- a/gitrpc/internal/gitea/merge.go +++ b/gitrpc/internal/gitea/merge.go @@ -106,7 +106,7 @@ func (g Adapter) CreateTemporaryRepoForPR( // Fetch base branch baseCommit, err := g.GetCommit(ctx, pr.BaseRepoPath, pr.BaseBranch) if err != nil { - return types.TempRepository{}, fmt.Errorf("failed to get commit of %s branch: %w", baseBranch, err) + return types.TempRepository{}, fmt.Errorf("failed to get commit of base branch '%s', error: %w", pr.BaseBranch, err) } baseID := baseCommit.SHA if err = git.NewCommand(ctx, "fetch", "origin", "--no-tags", "--", @@ -162,7 +162,7 @@ func (g Adapter) CreateTemporaryRepoForPR( headCommit, err := g.GetCommit(ctx, pr.HeadRepoPath, pr.HeadBranch) if err != nil { - return types.TempRepository{}, fmt.Errorf("failed to get commit of %s branch: %w", trackingBranch, err) + return types.TempRepository{}, fmt.Errorf("failed to get commit of head branch '%s', error: %w", pr.HeadBranch, err) } headID := headCommit.SHA if err = git.NewCommand(ctx, "fetch", "--no-tags", remoteRepoName, headID+":"+trackingBranch). diff --git a/gitrpc/internal/service/diff.go b/gitrpc/internal/service/diff.go index 3b935eeb7..159eec171 100644 --- a/gitrpc/internal/service/diff.go +++ b/gitrpc/internal/service/diff.go @@ -77,8 +77,7 @@ func validateDiffRequest(in *rpc.DiffRequest) error { func (s DiffService) DiffShortStat(ctx context.Context, r *rpc.DiffRequest) (*rpc.DiffShortStatResponse, error) { err := validateDiffRequest(r) if err != nil { - return nil, fmt.Errorf("failed to validate request for short diff statistic "+ - "between %s and %s with err: %w", r.GetBaseRef(), r.GetHeadRef(), err) + return nil, fmt.Errorf("failed to validate request for short diff statistic, error: %v", err) } base := r.GetBase() @@ -91,8 +90,8 @@ func (s DiffService) DiffShortStat(ctx context.Context, r *rpc.DiffRequest) (*rp stat, err := s.adapter.DiffShortStat(ctx, repoPath, r.GetBaseRef(), r.GetHeadRef(), direct) if err != nil { - return nil, fmt.Errorf("failed to fetch short statistics "+ - "between %s and %s with err: %w", r.GetBaseRef(), r.GetHeadRef(), err) + return nil, processGitErrorf(err, "failed to fetch short statistics "+ + "between %s and %s", r.GetBaseRef(), r.GetHeadRef()) } return &rpc.DiffShortStatResponse{ diff --git a/gitrpc/internal/service/errors.go b/gitrpc/internal/service/errors.go index c26606774..e7180aa07 100644 --- a/gitrpc/internal/service/errors.go +++ b/gitrpc/internal/service/errors.go @@ -201,11 +201,11 @@ func processGitErrorf(err error, format string, args ...interface{}) error { errors.Is(err, types.ErrSHADoesNotMatch), errors.Is(err, types.ErrHunkNotFound), errors.Is(err, types.ErrPathNotFound): - return ErrNotFoundf(format, args...) + return ErrNotFound(err) case errors.Is(err, types.ErrAlreadyExists): - return ErrAlreadyExistsf(format, args...) + return ErrAlreadyExists(err) case errors.Is(err, types.ErrInvalidArgument): - return ErrInvalidArgumentf(format, args...) + return ErrInvalidArgument(err) case errors.As(err, &cferr): stdout := strings.Trim(cferr.StdOut, nl) conflictingFiles := strings.Split(stdout, nl) @@ -214,9 +214,9 @@ func processGitErrorf(err error, format string, args ...interface{}) error { } return ErrFailedPreconditionf("merging failed due to conflicting changes with the target branch", files, err) case types.IsMergeUnrelatedHistoriesError(err): - return ErrFailedPreconditionf(format, args...) + return ErrFailedPrecondition(err) case errors.Is(err, types.ErrFailedToConnect): - return ErrInvalidArgumentf(format, args...) + return ErrInvalidArgument(err) default: return ErrInternalf(format, args...) } diff --git a/gitrpc/internal/service/merge.go b/gitrpc/internal/service/merge.go index 4f199d05c..26a273c0b 100644 --- a/gitrpc/internal/service/merge.go +++ b/gitrpc/internal/service/merge.go @@ -64,7 +64,7 @@ func (s MergeService) Merge( // Clone base repo. tmpRepo, err := s.adapter.CreateTemporaryRepoForPR(ctx, s.reposTempDir, pr, baseBranch, trackingBranch) if err != nil { - return nil, err + return nil, processGitErrorf(err, "failed to initialize temporary repo") } defer func() { rmErr := tempdir.RemoveTemporaryPath(tmpRepo.Path)