From 1d06026d0544f61892a7679ef85aa011c24bb719 Mon Sep 17 00:00:00 2001 From: Marko Gacesa Date: Tue, 28 Nov 2023 11:14:51 +0000 Subject: [PATCH] handle merge conflict not as an error (#823) --- app/api/controller/pullreq/merge.go | 89 ++++++---- app/api/controller/pullreq/pr_recheck.go | 25 +-- app/api/controller/repo/merge_check.go | 17 +- app/services/pullreq/handlers_mergeable.go | 29 +--- git/adapter.go | 2 +- git/adapter/merge.go | 184 ++++++++++++--------- git/merge.go | 58 +++++-- git/types/errors.go | 42 ----- git/types/types.go | 4 + 9 files changed, 251 insertions(+), 199 deletions(-) diff --git a/app/api/controller/pullreq/merge.go b/app/api/controller/pullreq/merge.go index 18e9ca356..286aa0f7f 100644 --- a/app/api/controller/pullreq/merge.go +++ b/app/api/controller/pullreq/merge.go @@ -29,7 +29,6 @@ import ( "github.com/harness/gitness/errors" "github.com/harness/gitness/git" gitenum "github.com/harness/gitness/git/enum" - gittypes "github.com/harness/gitness/git/types" "github.com/harness/gitness/types" "github.com/harness/gitness/types/enum" @@ -197,7 +196,49 @@ func (c *Controller) Merge( return nil, nil, fmt.Errorf("failed to verify protection rules: %w", err) } + //nolint:nestif if in.DryRun { + // As the merge API is always executed under a global lock, we use the opportunity of dry-running the merge + // to check the PR's mergeability status if it's currently "unchecked". This can happen if the target branch + // has advanced. It's possible that the merge base commit is different too. + // So, the next time the API gets called for the same PR the mergeability status will not be unchecked. + // Without dry-run the execution would proceed below and would either merge the PR or set the conflict status. + if pr.MergeCheckStatus == enum.MergeCheckStatusUnchecked { + mergeOutput, err := c.git.Merge(ctx, &git.MergeParams{ + WriteParams: targetWriteParams, + BaseBranch: pr.TargetBranch, + HeadRepoUID: sourceRepo.GitUID, + HeadBranch: pr.SourceBranch, + HeadExpectedSHA: in.SourceSHA, + }) + if err != nil { + return nil, nil, fmt.Errorf("merge check execution failed: %w", err) + } + + pr, err = c.pullreqStore.UpdateOptLock(ctx, pr, func(pr *types.PullReq) error { + if pr.SourceSHA != mergeOutput.HeadSHA { + return errors.New("source SHA has changed") + } + if mergeOutput.MergeSHA == "" || len(mergeOutput.ConflictFiles) > 0 { + pr.MergeCheckStatus = enum.MergeCheckStatusConflict + pr.MergeBaseSHA = mergeOutput.MergeBaseSHA + pr.MergeTargetSHA = &mergeOutput.BaseSHA + pr.MergeSHA = nil + pr.MergeConflicts = mergeOutput.ConflictFiles + } else { + pr.MergeCheckStatus = enum.MergeCheckStatusMergeable + pr.MergeBaseSHA = mergeOutput.MergeBaseSHA + pr.MergeTargetSHA = &mergeOutput.BaseSHA + pr.MergeSHA = &mergeOutput.MergeSHA + pr.MergeConflicts = nil + } + return nil + }) + if err != nil { + return nil, nil, fmt.Errorf("failed to update unchecked pull request: %w", err) + } + } + // With in.DryRun=true this function never returns types.MergeViolations out := &types.MergeResponse{ DryRun: true, @@ -207,26 +248,6 @@ func (c *Controller) Merge( RuleViolations: violations, } - // TODO: This is a temporary solution. The changes needed for the proper implementation: - // 1) Git: Change the merge method to return SHAs (source/target/merge base) even in case of conflicts. - // 2) Event handler: Update target and merge base SHA in the event handler even in case of merge conflicts. - // 3) Here: Update the pull request target and merge base SHA in the DB if merge check status is unchecked. - // 4) Remove the recheck API. - if pr.MergeCheckStatus == enum.MergeCheckStatusUnchecked { - _, err = c.git.Merge(ctx, &git.MergeParams{ - WriteParams: targetWriteParams, - BaseBranch: pr.TargetBranch, - HeadRepoUID: sourceRepo.GitUID, - HeadBranch: pr.SourceBranch, - HeadExpectedSHA: in.SourceSHA, - }) - if cferr := gittypes.AsMergeConflictsError(err); cferr != nil { - out.ConflictFiles = cferr.Files - } else if err != nil { - return nil, nil, fmt.Errorf("merge check execution failed: %w", err) - } - } - return out, nil, nil } @@ -260,15 +281,27 @@ func (c *Controller) Merge( Method: gitenum.MergeMethod(in.Method), }) if err != nil { - if cf := gittypes.AsMergeConflictsError(err); cf != nil { - //nolint: nilerr - return nil, &types.MergeViolations{ - ConflictFiles: cf.Files, - RuleViolations: violations, - }, nil - } return nil, nil, fmt.Errorf("merge check execution failed: %w", err) } + if mergeOutput.MergeSHA == "" || len(mergeOutput.ConflictFiles) > 0 { + _, err = c.pullreqStore.UpdateOptLock(ctx, pr, func(pr *types.PullReq) error { + // update all Merge specific information + pr.MergeCheckStatus = enum.MergeCheckStatusConflict + pr.MergeBaseSHA = mergeOutput.MergeBaseSHA + pr.MergeTargetSHA = &mergeOutput.BaseSHA + pr.MergeSHA = nil + pr.MergeConflicts = mergeOutput.ConflictFiles + return nil + }) + if err != nil { + return nil, nil, fmt.Errorf("failed to update pull request with conflict files: %w", err) + } + + return nil, &types.MergeViolations{ + ConflictFiles: mergeOutput.ConflictFiles, + RuleViolations: violations, + }, nil + } var activitySeqMerge, activitySeqBranchDeleted int64 pr, err = c.pullreqStore.UpdateOptLock(ctx, pr, func(pr *types.PullReq) error { diff --git a/app/api/controller/pullreq/pr_recheck.go b/app/api/controller/pullreq/pr_recheck.go index 4f37109db..3dfa73978 100644 --- a/app/api/controller/pullreq/pr_recheck.go +++ b/app/api/controller/pullreq/pr_recheck.go @@ -16,10 +16,8 @@ package pullreq import ( "context" - "fmt" "github.com/harness/gitness/app/auth" - "github.com/harness/gitness/types/enum" ) // Recheck re-checks all system PR checks (mergeability check, ...). @@ -29,15 +27,22 @@ func (c *Controller) Recheck( repoRef string, prNum int64, ) error { - repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoPush) - if err != nil { - return fmt.Errorf("failed to acquire access to repo: %w", err) - } + // TODO: Remove the API. + _ = ctx + _ = session + _ = repoRef + _ = prNum + /* + repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoPush) + if err != nil { + return fmt.Errorf("failed to acquire access to repo: %w", err) + } - err = c.pullreqService.UpdateMergeDataIfRequired(ctx, repo.ID, prNum) - if err != nil { - return fmt.Errorf("failed to refresh merge data: %w", err) - } + err = c.pullreqService.UpdateMergeDataIfRequired(ctx, repo.ID, prNum) + if err != nil { + return fmt.Errorf("failed to refresh merge data: %w", err) + } + */ return nil } diff --git a/app/api/controller/repo/merge_check.go b/app/api/controller/repo/merge_check.go index 3181008cd..7f5021b4e 100644 --- a/app/api/controller/repo/merge_check.go +++ b/app/api/controller/repo/merge_check.go @@ -16,13 +16,11 @@ package repo import ( "context" - "errors" "fmt" "github.com/harness/gitness/app/api/controller" "github.com/harness/gitness/app/auth" "github.com/harness/gitness/git" - gittypes "github.com/harness/gitness/git/types" "github.com/harness/gitness/types/enum" ) @@ -52,22 +50,21 @@ func (c *Controller) MergeCheck( return MergeCheck{}, fmt.Errorf("failed to create rpc write params: %w", err) } - _, err = c.git.Merge(ctx, &git.MergeParams{ + mergeOutput, err := c.git.Merge(ctx, &git.MergeParams{ WriteParams: writeParams, BaseBranch: info.BaseRef, HeadRepoUID: writeParams.RepoUID, // forks are not supported for now HeadBranch: info.HeadRef, }) if err != nil { - var cferr *gittypes.MergeConflictsError - if errors.As(err, &cferr) { - return MergeCheck{ - Mergeable: false, - ConflictFiles: cferr.Files, - }, nil - } return MergeCheck{}, fmt.Errorf("merge check execution failed: %w", err) } + if len(mergeOutput.ConflictFiles) > 0 { + return MergeCheck{ + Mergeable: false, + ConflictFiles: mergeOutput.ConflictFiles, + }, nil + } return MergeCheck{ Mergeable: true, diff --git a/app/services/pullreq/handlers_mergeable.go b/app/services/pullreq/handlers_mergeable.go index 5dae33bc4..5a80cb0b8 100644 --- a/app/services/pullreq/handlers_mergeable.go +++ b/app/services/pullreq/handlers_mergeable.go @@ -25,7 +25,6 @@ import ( "github.com/harness/gitness/events" "github.com/harness/gitness/git" gitenum "github.com/harness/gitness/git/enum" - gittypes "github.com/harness/gitness/git/types" "github.com/harness/gitness/pubsub" "github.com/harness/gitness/types" "github.com/harness/gitness/types/enum" @@ -221,8 +220,7 @@ func (s *Service) updateMergeDataInner( // call merge and store output in pr merge reference. now := time.Now() - var output git.MergeOutput - output, err = s.git.Merge(ctx, &git.MergeParams{ + mergeOutput, err := s.git.Merge(ctx, &git.MergeParams{ WriteParams: writeParams, BaseBranch: pr.TargetBranch, HeadRepoUID: sourceRepo.GitUID, @@ -240,35 +238,26 @@ func (s *Service) updateMergeDataInner( pr.SourceBranch, newSHA) } - var cferr *gittypes.MergeConflictsError - - isNotMergeableError := errors.As(err, &cferr) - if err != nil && !isNotMergeableError { - return fmt.Errorf("merge check failed for %d:%s and %d:%s with err: %w", - targetRepo.ID, pr.TargetBranch, - sourceRepo.ID, pr.SourceBranch, - err) - } - // Update DB in both cases (failure or success) _, err = s.pullreqStore.UpdateOptLock(ctx, pr, func(pr *types.PullReq) error { if pr.SourceSHA != newSHA { return events.NewDiscardEventErrorf("PR SHA %s is newer than %s", pr.SourceSHA, newSHA) } - if isNotMergeableError { - // TODO: should return sha's either way, and also conflicting files! + if mergeOutput.MergeSHA == "" || len(mergeOutput.ConflictFiles) > 0 { pr.MergeCheckStatus = enum.MergeCheckStatusConflict - // pr.MergeTargetSHA = &output.BaseSHA // TODO: Merge API doesn't return this when there are conflicts + pr.MergeBaseSHA = mergeOutput.MergeBaseSHA + pr.MergeTargetSHA = &mergeOutput.BaseSHA pr.MergeSHA = nil - pr.MergeConflicts = cferr.Files + pr.MergeConflicts = mergeOutput.ConflictFiles } else { pr.MergeCheckStatus = enum.MergeCheckStatusMergeable - pr.MergeTargetSHA = &output.BaseSHA - pr.MergeBaseSHA = output.MergeBaseSHA // TODO: Merge check should not update the merge base. - pr.MergeSHA = &output.MergeSHA + pr.MergeBaseSHA = mergeOutput.MergeBaseSHA + pr.MergeTargetSHA = &mergeOutput.BaseSHA + pr.MergeSHA = &mergeOutput.MergeSHA pr.MergeConflicts = nil } + return nil }) if err != nil { diff --git a/git/adapter.go b/git/adapter.go index a386ec247..8685e7f74 100644 --- a/git/adapter.go +++ b/git/adapter.go @@ -72,7 +72,7 @@ type Adapter interface { CreateTemporaryRepoForPR(ctx context.Context, reposTempPath string, pr *types.PullRequest, baseBranch, trackingBranch string) (types.TempRepository, error) Merge(ctx context.Context, pr *types.PullRequest, mergeMethod enum.MergeMethod, baseBranch, trackingBranch string, - tmpBasePath string, mergeMsg string, identity *types.Identity, env ...string) error + tmpBasePath string, mergeMsg string, identity *types.Identity, env ...string) (types.MergeResult, error) GetMergeBase(ctx context.Context, repoPath, remote, base, head string) (string, string, error) Blame(ctx context.Context, repoPath, rev, file string, lineFrom, lineTo int) types.BlameReader Sync(ctx context.Context, repoPath string, source string, refSpecs []string) error diff --git a/git/adapter/merge.go b/git/adapter/merge.go index c37b53c49..d7cf885e2 100644 --- a/git/adapter/merge.go +++ b/git/adapter/merge.go @@ -30,7 +30,6 @@ import ( "github.com/harness/gitness/git/types" "code.gitea.io/gitea/modules/git" - "github.com/rs/zerolog/log" ) // CreateTemporaryRepo creates a temporary repo with "base" for pr.BaseBranch and "tracking" for pr.HeadBranch @@ -197,6 +196,10 @@ func (a Adapter) CreateTemporaryRepoForPR( }, nil } +type runMergeResult struct { + conflictFiles []string +} + func runMergeCommand( ctx context.Context, pr *types.PullRequest, @@ -204,7 +207,7 @@ func runMergeCommand( cmd *git.Command, tmpBasePath string, env []string, -) error { +) (runMergeResult, error) { var outbuf, errbuf strings.Builder if err := cmd.Run(&git.RunOpts{ Dir: tmpBasePath, @@ -212,34 +215,33 @@ func runMergeCommand( Stderr: &errbuf, Env: env, }); err != nil { - // Merge will leave a MERGE_HEAD file in the .git folder if there is a conflict - if _, statErr := os.Stat(filepath.Join(tmpBasePath, ".git", "MERGE_HEAD")); statErr == nil { - // We have a merge conflict error - files, cferr := conflictFiles(ctx, pr, env, tmpBasePath) - if cferr != nil { - return cferr - } - return &types.MergeConflictsError{ - Method: mergeMethod, - StdOut: outbuf.String(), - StdErr: errbuf.String(), - Err: err, - Files: files, - } - } else if strings.Contains(errbuf.String(), "refusing to merge unrelated histories") { - return &types.MergeUnrelatedHistoriesError{ + if strings.Contains(errbuf.String(), "refusing to merge unrelated histories") { + return runMergeResult{}, &types.MergeUnrelatedHistoriesError{ Method: mergeMethod, StdOut: outbuf.String(), StdErr: errbuf.String(), Err: err, } } + + // Merge will leave a MERGE_HEAD file in the .git folder if there is a conflict + if _, statErr := os.Stat(filepath.Join(tmpBasePath, ".git", "MERGE_HEAD")); statErr == nil { + // We have a merge conflict error + files, cferr := conflictFiles(ctx, pr, env, tmpBasePath) + if cferr != nil { + return runMergeResult{}, cferr + } + return runMergeResult{ + conflictFiles: files, + }, nil + } + giteaErr := &giteaRunStdError{err: err, stderr: errbuf.String()} - return processGiteaErrorf(giteaErr, "git merge [%s -> %s]\n%s\n%s", + return runMergeResult{}, processGiteaErrorf(giteaErr, "git merge [%s -> %s]\n%s\n%s", pr.HeadBranch, pr.BaseBranch, outbuf.String(), errbuf.String()) } - return nil + return runMergeResult{}, nil } func commitAndSignNoAuthor( @@ -290,7 +292,7 @@ func (a Adapter) Merge( mergeMsg string, identity *types.Identity, env ...string, -) error { +) (types.MergeResult, error) { var ( outbuf, errbuf strings.Builder ) @@ -306,18 +308,26 @@ func (a Adapter) Merge( switch mergeMethod { case enum.MergeMethodMerge: cmd := git.NewCommand(ctx, "merge", "--no-ff", "--no-commit", trackingBranch) - if err := runMergeCommand(ctx, pr, mergeMethod, cmd, tmpBasePath, env); err != nil { - return fmt.Errorf("unable to merge tracking into base: %w", err) + result, err := runMergeCommand(ctx, pr, mergeMethod, cmd, tmpBasePath, env) + if err != nil { + return types.MergeResult{}, fmt.Errorf("unable to merge tracking into base: %w", err) + } + if len(result.conflictFiles) > 0 { + return types.MergeResult{ConflictFiles: result.conflictFiles}, nil } if err := commitAndSignNoAuthor(ctx, pr, mergeMsg, signArg, tmpBasePath, env); err != nil { - return fmt.Errorf("unable to make final commit: %w", err) + return types.MergeResult{}, fmt.Errorf("unable to make final commit: %w", err) } case enum.MergeMethodSquash: // Merge with squash cmd := git.NewCommand(ctx, "merge", "--squash", trackingBranch) - if err := runMergeCommand(ctx, pr, mergeMethod, cmd, tmpBasePath, env); err != nil { - return fmt.Errorf("unable to merge --squash tracking into base: %w", err) + result, err := runMergeCommand(ctx, pr, mergeMethod, cmd, tmpBasePath, env) + if err != nil { + return types.MergeResult{}, fmt.Errorf("unable to merge --squash tracking into base: %w", err) + } + if len(result.conflictFiles) > 0 { + return types.MergeResult{ConflictFiles: result.conflictFiles}, nil } if signArg == "" { @@ -328,7 +338,7 @@ func (a Adapter) Merge( Stdout: &outbuf, Stderr: &errbuf, }); err != nil { - return processGiteaErrorf(err, "git commit [%s -> %s]\n%s\n%s", + return types.MergeResult{}, processGiteaErrorf(err, "git commit [%s -> %s]\n%s\n%s", pr.HeadBranch, pr.BaseBranch, outbuf.String(), errbuf.String()) } } else { @@ -339,19 +349,19 @@ func (a Adapter) Merge( Stdout: &outbuf, Stderr: &errbuf, }); err != nil { - return processGiteaErrorf(err, "git commit [%s -> %s]\n%s\n%s", + return types.MergeResult{}, processGiteaErrorf(err, "git commit [%s -> %s]\n%s\n%s", pr.HeadBranch, pr.BaseBranch, outbuf.String(), errbuf.String()) } } case enum.MergeMethodRebase: - // Checkout head branch + // Create staging branch if err := git.NewCommand(ctx, "checkout", "-b", stagingBranch, trackingBranch). Run(&git.RunOpts{ Dir: tmpBasePath, Stdout: &outbuf, Stderr: &errbuf, }); err != nil { - return fmt.Errorf( + return types.MergeResult{}, fmt.Errorf( "git checkout base prior to merge post staging rebase [%s -> %s]: %w\n%s\n%s", pr.HeadBranch, pr.BaseBranch, err, outbuf.String(), errbuf.String(), ) @@ -359,6 +369,8 @@ func (a Adapter) Merge( outbuf.Reset() errbuf.Reset() + var conflicts bool + // Rebase before merging if err := git.NewCommand(ctx, "rebase", baseBranch). Run(&git.RunOpts{ @@ -368,52 +380,67 @@ func (a Adapter) Merge( }); err != nil { // Rebase will leave a REBASE_HEAD file in .git if there is a conflict if _, statErr := os.Stat(filepath.Join(tmpBasePath, ".git", "REBASE_HEAD")); statErr == nil { - var commitSha string - - // TBD git version we will support - // failingCommitPath := filepath.Join(tmpBasePath, ".git", "rebase-apply", "original-commit") // Git < 2.26 - // if _, cpErr := os.Stat(failingCommitPath); statErr != nil { - // return fmt.Errorf("git rebase staging on to base [%s -> %s]: %v\n%s\n%s", - // pr.HeadBranch, pr.BaseBranch, cpErr, outbuf.String(), errbuf.String()) - // } - - failingCommitPath := filepath.Join(tmpBasePath, ".git", "rebase-merge", "stopped-sha") // Git >= 2.26 - if _, cpErr := os.Stat(failingCommitPath); cpErr != nil { - return fmt.Errorf( - "git rebase staging on to base [%s -> %s]: %w\n%s\n%s", - pr.HeadBranch, pr.BaseBranch, cpErr, outbuf.String(), errbuf.String(), - ) - } - - commitShaBytes, readErr := os.ReadFile(failingCommitPath) - if readErr != nil { - // Abandon this attempt to handle the error - return fmt.Errorf( - "git rebase staging on to base [%s -> %s]: %w\n%s\n%s", - pr.HeadBranch, pr.BaseBranch, readErr, outbuf.String(), errbuf.String(), - ) - } - commitSha = strings.TrimSpace(string(commitShaBytes)) - - log.Debug().Msgf("RebaseConflict at %s [%s -> %s]: %v\n%s\n%s", - commitSha, pr.HeadBranch, pr.BaseBranch, err, outbuf.String(), errbuf.String(), + // Rebase works by processing commit by commit. To get the full list of conflict files + // all commits would have to be applied. It's simpler to revert the rebase and + // get the list conflict using git merge. + conflicts = true + } else { + return types.MergeResult{}, fmt.Errorf( + "git rebase staging on to base [%s -> %s]: %w\n%s\n%s", + pr.HeadBranch, pr.BaseBranch, err, outbuf.String(), errbuf.String(), ) - return &types.MergeConflictsError{ - Method: mergeMethod, - CommitSHA: commitSha, - StdOut: outbuf.String(), - StdErr: errbuf.String(), - Err: err, - } } - return fmt.Errorf( - "git rebase staging on to base [%s -> %s]: %w\n%s\n%s", - pr.HeadBranch, pr.BaseBranch, err, outbuf.String(), errbuf.String(), - ) } outbuf.Reset() errbuf.Reset() + if conflicts { + // Rebase failed because there are conflicts. Abort the rebase. + if err := git.NewCommand(ctx, "rebase", "--abort"). + Run(&git.RunOpts{ + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { + return types.MergeResult{}, fmt.Errorf( + "git abort rebase [%s -> %s]: %w\n%s\n%s", + pr.HeadBranch, pr.BaseBranch, err, outbuf.String(), errbuf.String(), + ) + } + outbuf.Reset() + errbuf.Reset() + + // Go back to the base branch. + if err := git.NewCommand(ctx, "checkout", baseBranch). + Run(&git.RunOpts{ + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { + return types.MergeResult{}, fmt.Errorf( + "return to the base branch [%s -> %s]: %w\n%s\n%s", + pr.HeadBranch, pr.BaseBranch, err, outbuf.String(), errbuf.String(), + ) + } + outbuf.Reset() + errbuf.Reset() + + // Run the ordinary merge to get the list of conflict files. + cmd := git.NewCommand(ctx, "merge", "--no-ff", "--no-commit", trackingBranch) + result, err := runMergeCommand(ctx, pr, mergeMethod, cmd, tmpBasePath, env) + if err != nil { + return types.MergeResult{}, fmt.Errorf( + "git abort rebase [%s -> %s]: %w\n%s\n%s", + pr.HeadBranch, pr.BaseBranch, err, outbuf.String(), errbuf.String(), + ) + } + if len(result.conflictFiles) > 0 { + return types.MergeResult{ConflictFiles: result.conflictFiles}, nil + } + + return types.MergeResult{}, errors.New("rebase reported conflicts, but merge gave no conflict files") + } + // Checkout base branch again if err := git.NewCommand(ctx, "checkout", baseBranch). Run(&git.RunOpts{ @@ -421,7 +448,7 @@ func (a Adapter) Merge( Stdout: &outbuf, Stderr: &errbuf, }); err != nil { - return fmt.Errorf( + return types.MergeResult{}, fmt.Errorf( "git checkout base prior to merge post staging rebase [%s -> %s]: %w\n%s\n%s", pr.HeadBranch, pr.BaseBranch, err, outbuf.String(), errbuf.String(), ) @@ -429,17 +456,20 @@ func (a Adapter) Merge( outbuf.Reset() errbuf.Reset() - cmd := git.NewCommand(ctx, "merge", "--ff-only", stagingBranch) - // Prepare merge with commit - if err := runMergeCommand(ctx, pr, mergeMethod, cmd, tmpBasePath, env); err != nil { - return err + cmd := git.NewCommand(ctx, "merge", "--ff-only", stagingBranch) + result, err := runMergeCommand(ctx, pr, mergeMethod, cmd, tmpBasePath, env) + if err != nil { + return types.MergeResult{}, fmt.Errorf("unable to ff-olny merge tracking into base: %w", err) + } + if len(result.conflictFiles) > 0 { + return types.MergeResult{ConflictFiles: result.conflictFiles}, nil } default: - return fmt.Errorf("wrong merge method provided: %s", mergeMethod) + return types.MergeResult{}, fmt.Errorf("wrong merge method provided: %s", mergeMethod) } - return nil + return types.MergeResult{}, nil } func conflictFiles( diff --git a/git/merge.go b/git/merge.go index f73771e77..0b761fa5e 100644 --- a/git/merge.go +++ b/git/merge.go @@ -98,6 +98,10 @@ type MergeOutput struct { MergeBaseSHA string // MergeSHA is the sha of the commit after merging HeadSHA with BaseSHA. MergeSHA string + + CommitCount int + ChangedFileCount int + ConflictFiles []string } // Merge method executes git merge operation. Refs can be sha, branch or tag. @@ -114,7 +118,7 @@ type MergeOutput struct { // params.HeadExpectedSHA which will be compared with the latest sha from head branch // if they are not the same error will be returned. // -//nolint:gocognit +//nolint:gocognit,gocyclo,cyclop func (s *Service) Merge(ctx context.Context, params *MergeParams) (MergeOutput, error) { if err := params.Validate(); err != nil { return MergeOutput{}, fmt.Errorf("Merge: params not valid: %w", err) @@ -179,6 +183,19 @@ func (s *Service) Merge(ctx context.Context, params *MergeParams) (MergeOutput, fmt.Errorf("unable to write .git/info/sparse-checkout file in tmpRepo.Path: %w", err) } + shortStat, err := s.adapter.DiffShortStat(ctx, tmpRepo.Path, tmpRepo.BaseSHA, tmpRepo.HeadSHA, true) + if err != nil { + return MergeOutput{}, fmt.Errorf("execution of DiffShortStat failed: %w", err) + } + changedFileCount := shortStat.Files + + divergences, err := s.adapter.GetCommitDivergences(ctx, tmpRepo.Path, + []types.CommitDivergenceRequest{{From: tmpRepo.HeadSHA, To: tmpRepo.BaseSHA}}, 0) + if err != nil { + return MergeOutput{}, fmt.Errorf("execution of GetCommitDivergences failed: %w", err) + } + commitCount := int(divergences[0].Ahead) + // Switch off LFS process (set required, clean and smudge here also) if err = s.adapter.Config(ctx, tmpRepo.Path, "filter.lfs.process", ""); err != nil { return MergeOutput{}, err @@ -245,7 +262,7 @@ func (s *Service) Merge(ctx context.Context, params *MergeParams) (MergeOutput, params.Method = enum.MergeMethodMerge } - if err = s.adapter.Merge( + result, err := s.adapter.Merge( ctx, pr, params.Method, @@ -257,10 +274,23 @@ func (s *Service) Merge(ctx context.Context, params *MergeParams) (MergeOutput, Name: author.Name, Email: author.Email, }, - env...); err != nil { + env...) + if err != nil { return MergeOutput{}, fmt.Errorf("merge failed: %w", err) } + if len(result.ConflictFiles) > 0 { + return MergeOutput{ + BaseSHA: tmpRepo.BaseSHA, + HeadSHA: tmpRepo.HeadSHA, + MergeBaseSHA: mergeBaseCommitSHA, + MergeSHA: "", + CommitCount: commitCount, + ChangedFileCount: changedFileCount, + ConflictFiles: result.ConflictFiles, + }, nil + } + mergeCommitSHA, err := s.adapter.GetFullCommitID(ctx, tmpRepo.Path, baseBranch) if err != nil { return MergeOutput{}, fmt.Errorf("failed to get full commit id for the new merge: %w", err) @@ -268,10 +298,13 @@ func (s *Service) Merge(ctx context.Context, params *MergeParams) (MergeOutput, if params.RefType == enum.RefTypeUndefined { return MergeOutput{ - BaseSHA: tmpRepo.BaseSHA, - HeadSHA: tmpRepo.HeadSHA, - MergeBaseSHA: mergeBaseCommitSHA, - MergeSHA: mergeCommitSHA, + BaseSHA: tmpRepo.BaseSHA, + HeadSHA: tmpRepo.HeadSHA, + MergeBaseSHA: mergeBaseCommitSHA, + MergeSHA: mergeCommitSHA, + CommitCount: commitCount, + ChangedFileCount: changedFileCount, + ConflictFiles: nil, }, nil } @@ -293,10 +326,13 @@ func (s *Service) Merge(ctx context.Context, params *MergeParams) (MergeOutput, } return MergeOutput{ - BaseSHA: tmpRepo.BaseSHA, - HeadSHA: tmpRepo.HeadSHA, - MergeBaseSHA: mergeBaseCommitSHA, - MergeSHA: mergeCommitSHA, + BaseSHA: tmpRepo.BaseSHA, + HeadSHA: tmpRepo.HeadSHA, + MergeBaseSHA: mergeBaseCommitSHA, + MergeSHA: mergeCommitSHA, + CommitCount: commitCount, + ChangedFileCount: changedFileCount, + ConflictFiles: nil, }, nil } diff --git a/git/types/errors.go b/git/types/errors.go index 48eb2f80b..713309f88 100644 --- a/git/types/errors.go +++ b/git/types/errors.go @@ -57,48 +57,6 @@ func (e *ValidationError) Error() string { return e.Msg } -// MergeConflictsError represents an error if merging fails with a conflict. -type MergeConflictsError struct { - Method enum.MergeMethod - CommitSHA string - StdOut string - StdErr string - Err error - Files []string -} - -func IsMergeConflictsError(err error) bool { - return errors.Is(err, &MergeConflictsError{}) -} - -func (e *MergeConflictsError) Error() string { - return fmt.Sprintf("Merge Conflict Error: %v: %s\n%s", e.Err, e.StdErr, e.StdOut) -} - -func (e *MergeConflictsError) Unwrap() error { - return e.Err -} - -func (e *MergeConflictsError) Status() errors.Status { - return StatusNotMergeable -} - -func AsMergeConflictsError(err error) (e *MergeConflictsError) { - if err == nil { - return nil - } - if errors.As(err, &e) { - return - } - return -} - -//nolint:errorlint // the purpose of this method is to check whether the target itself if of this type. -func (e *MergeConflictsError) Is(target error) bool { - _, ok := target.(*MergeConflictsError) - return ok -} - // MergeUnrelatedHistoriesError represents an error if merging fails due to unrelated histories. type MergeUnrelatedHistoriesError struct { Method enum.MergeMethod diff --git a/git/types/types.go b/git/types/types.go index 2fa3db2ef..dc37ccd15 100644 --- a/git/types/types.go +++ b/git/types/types.go @@ -365,3 +365,7 @@ type FileContent struct { Path string Content []byte } + +type MergeResult struct { + ConflictFiles []string +}