diff --git a/gitrpc/errors.go b/gitrpc/errors.go index 079d889b6..442b88108 100644 --- a/gitrpc/errors.go +++ b/gitrpc/errors.go @@ -13,3 +13,4 @@ var ErrAlreadyExists = errors.New("already exists") var ErrInvalidArgument = errors.New("invalid argument") var ErrNotFound = errors.New("not found") var ErrPreconditionFailed = errors.New("precondition failed") +var ErrNotMergeable = errors.New("not mergeable") diff --git a/gitrpc/internal/gitea/merge.go b/gitrpc/internal/gitea/merge.go index 50d1a91a4..97dc4d9f7 100644 --- a/gitrpc/internal/gitea/merge.go +++ b/gitrpc/internal/gitea/merge.go @@ -295,7 +295,11 @@ func (g Adapter) GetMergeBase(ctx context.Context, repoPath, remote, base, head } stdout, _, err := git.NewCommand(ctx, "merge-base", "--", base, head).RunStdString(&git.RunOpts{Dir: repoPath}) - return strings.TrimSpace(stdout), base, err + if err != nil { + return "", "", processGiteaErrorf(err, "failed to get merge-base") + } + + return strings.TrimSpace(stdout), base, nil } // giteaRunStdError is an implementation of the RunStdError interface in the gitea codebase. diff --git a/gitrpc/internal/service/mapping.go b/gitrpc/internal/service/mapping.go index 3e33eb88f..8dad0b96e 100644 --- a/gitrpc/internal/service/mapping.go +++ b/gitrpc/internal/service/mapping.go @@ -33,9 +33,9 @@ func processGitErrorf(err error, format string, args ...interface{}) error { case errors.Is(err, types.ErrInvalidArgument): return status.Errorf(codes.InvalidArgument, message) case types.IsMergeConflictsError(err): - return status.Errorf(codes.FailedPrecondition, message) + return status.Errorf(codes.Aborted, message) case types.IsMergeUnrelatedHistoriesError(err): - return status.Errorf(codes.FailedPrecondition, message) + return status.Errorf(codes.Aborted, message) default: return status.Errorf(codes.Unknown, message) } diff --git a/gitrpc/internal/service/merge.go b/gitrpc/internal/service/merge.go index 84848f0c5..091a750bf 100644 --- a/gitrpc/internal/service/merge.go +++ b/gitrpc/internal/service/merge.go @@ -73,9 +73,20 @@ func (s MergeService) Merge( // no error check needed, all branches were created when creating the temporary repo baseBranch := "base" trackingBranch := "tracking" - baseCommitSHA, _, _ := s.adapter.GetMergeBase(ctx, tmpBasePath, "origin", baseBranch, trackingBranch) - headCommit, _ := s.adapter.GetCommit(ctx, tmpBasePath, trackingBranch) + headCommit, err := s.adapter.GetCommit(ctx, tmpBasePath, trackingBranch) + if err != nil { + return nil, fmt.Errorf("failed to get commit of tracking branch (head): %w", err) + } headCommitSHA := headCommit.SHA + baseCommit, err := s.adapter.GetCommit(ctx, tmpBasePath, baseBranch) + if err != nil { + return nil, fmt.Errorf("failed to get commit of base branch: %w", err) + } + baseCommitSHA := baseCommit.SHA + mergeBaseCommitSHA, _, err := s.adapter.GetMergeBase(ctx, tmpBasePath, "origin", baseBranch, trackingBranch) + if err != nil { + return nil, fmt.Errorf("failed to get merge base: %w", err) + } if request.HeadExpectedSha != "" && request.HeadExpectedSha != headCommitSHA { return nil, status.Errorf( @@ -170,9 +181,10 @@ func (s MergeService) Merge( refType := enum.RefFromRPC(request.RefType) if refType == enum.RefTypeUndefined { return &rpc.MergeResponse{ - MergeSha: mergeCommitSHA, - BaseSha: baseCommitSHA, - HeadSha: headCommitSHA, + BaseSha: baseCommitSHA, + HeadSha: headCommitSHA, + MergeBaseSha: mergeBaseCommitSHA, + MergeSha: mergeCommitSHA, }, nil } @@ -193,9 +205,10 @@ func (s MergeService) Merge( } return &rpc.MergeResponse{ - MergeSha: mergeCommitSHA, - BaseSha: baseCommitSHA, - HeadSha: headCommitSHA, + BaseSha: baseCommitSHA, + HeadSha: headCommitSHA, + MergeBaseSha: mergeBaseCommitSHA, + MergeSha: mergeCommitSHA, }, nil } diff --git a/gitrpc/mapping.go b/gitrpc/mapping.go index 6ad01fddb..2b471deaf 100644 --- a/gitrpc/mapping.go +++ b/gitrpc/mapping.go @@ -39,6 +39,9 @@ func processRPCErrorf(err error, format string, args ...interface{}) error { return ErrInvalidArgument case rpcErr.Code() == codes.FailedPrecondition: return ErrPreconditionFailed + case rpcErr.Code() == codes.Aborted: + // TODO: this should not be so generic ... + return ErrNotMergeable default: return fallbackErr } diff --git a/gitrpc/merge.go b/gitrpc/merge.go index f86a18520..68eebf21f 100644 --- a/gitrpc/merge.go +++ b/gitrpc/merge.go @@ -41,9 +41,14 @@ type MergeParams struct { // MergeOutput is result object from merging and returns // base, head and commit sha. type MergeOutput struct { - MergedSHA string - BaseSHA string - HeadSHA string + // BaseSHA is the sha of the latest commit on the base branch that was used for merging. + BaseSHA string + // HeadSHA is the sha of the latest commit on the head branch that was used for merging. + HeadSHA string + // MergeBaseSHA is the sha of the merge base of the HeadSHA and BaseSHA + MergeBaseSHA string + // MergeSHA is the sha of the commit after merging HeadSHA with BaseSHA. + MergeSHA string } // Merge method executes git merge operation. Refs can be sha, branch or tag. @@ -83,8 +88,9 @@ func (c *Client) Merge(ctx context.Context, params *MergeParams) (MergeOutput, e } return MergeOutput{ - MergedSHA: resp.GetMergeSha(), - BaseSHA: resp.GetBaseSha(), - HeadSHA: resp.GetHeadSha(), + BaseSHA: resp.GetBaseSha(), + HeadSHA: resp.GetHeadSha(), + MergeBaseSHA: resp.GetMergeBaseSha(), + MergeSHA: resp.GetMergeSha(), }, nil } diff --git a/gitrpc/proto/merge.proto b/gitrpc/proto/merge.proto index fb9cfc489..792198757 100644 --- a/gitrpc/proto/merge.proto +++ b/gitrpc/proto/merge.proto @@ -46,8 +46,12 @@ message MergeRequest { } message MergeResponse { - // The merge_sha is merge commit between head_sha and base_sha - string merge_sha = 1; - string base_sha = 2; - string head_sha = 3; + // base_sha is the sha of the latest commit on the base branch that was used for merging. + string base_sha = 1; + // head_sha is the sha of the latest commit on the head branch that was used for merging. + string head_sha = 2; + // merge_base_sha is the sha of the merge base of the head_sha and base_sha + string merge_base_sha = 3; + // merge_sha is the sha of the commit after merging head_sha with base_sha. + string merge_sha = 4; } \ No newline at end of file diff --git a/gitrpc/rpc/merge.pb.go b/gitrpc/rpc/merge.pb.go index 57ac28eea..cda62b44e 100644 --- a/gitrpc/rpc/merge.pb.go +++ b/gitrpc/rpc/merge.pb.go @@ -175,10 +175,14 @@ type MergeResponse struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - // The merge_sha is merge commit between head_sha and base_sha - MergeSha string `protobuf:"bytes,1,opt,name=merge_sha,json=mergeSha,proto3" json:"merge_sha,omitempty"` - BaseSha string `protobuf:"bytes,2,opt,name=base_sha,json=baseSha,proto3" json:"base_sha,omitempty"` - HeadSha string `protobuf:"bytes,3,opt,name=head_sha,json=headSha,proto3" json:"head_sha,omitempty"` + // base_sha is the sha of the latest commit on the base branch that was used for merging. + BaseSha string `protobuf:"bytes,1,opt,name=base_sha,json=baseSha,proto3" json:"base_sha,omitempty"` + // head_sha is the sha of the latest commit on the head branch that was used for merging. + HeadSha string `protobuf:"bytes,2,opt,name=head_sha,json=headSha,proto3" json:"head_sha,omitempty"` + // merge_base_sha is the sha of the merge base of the head_sha and base_sha + MergeBaseSha string `protobuf:"bytes,3,opt,name=merge_base_sha,json=mergeBaseSha,proto3" json:"merge_base_sha,omitempty"` + // merge_sha is the sha of the commit after merging head_sha with base_sha. + MergeSha string `protobuf:"bytes,4,opt,name=merge_sha,json=mergeSha,proto3" json:"merge_sha,omitempty"` } func (x *MergeResponse) Reset() { @@ -213,13 +217,6 @@ func (*MergeResponse) Descriptor() ([]byte, []int) { return file_merge_proto_rawDescGZIP(), []int{1} } -func (x *MergeResponse) GetMergeSha() string { - if x != nil { - return x.MergeSha - } - return "" -} - func (x *MergeResponse) GetBaseSha() string { if x != nil { return x.BaseSha @@ -234,6 +231,20 @@ func (x *MergeResponse) GetHeadSha() string { return "" } +func (x *MergeResponse) GetMergeBaseSha() string { + if x != nil { + return x.MergeBaseSha + } + return "" +} + +func (x *MergeResponse) GetMergeSha() string { + if x != nil { + return x.MergeSha + } + return "" +} + var File_merge_proto protoreflect.FileDescriptor var file_merge_proto_rawDesc = []byte{ @@ -266,20 +277,22 @@ var file_merge_proto_rawDesc = []byte{ 0x6f, 0x72, 0x63, 0x65, 0x12, 0x2c, 0x0a, 0x12, 0x64, 0x65, 0x6c, 0x65, 0x74, 0x65, 0x5f, 0x68, 0x65, 0x61, 0x64, 0x5f, 0x62, 0x72, 0x61, 0x6e, 0x63, 0x68, 0x18, 0x0c, 0x20, 0x01, 0x28, 0x08, 0x52, 0x10, 0x64, 0x65, 0x6c, 0x65, 0x74, 0x65, 0x48, 0x65, 0x61, 0x64, 0x42, 0x72, 0x61, 0x6e, - 0x63, 0x68, 0x22, 0x62, 0x0a, 0x0d, 0x4d, 0x65, 0x72, 0x67, 0x65, 0x52, 0x65, 0x73, 0x70, 0x6f, - 0x6e, 0x73, 0x65, 0x12, 0x1b, 0x0a, 0x09, 0x6d, 0x65, 0x72, 0x67, 0x65, 0x5f, 0x73, 0x68, 0x61, - 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x08, 0x6d, 0x65, 0x72, 0x67, 0x65, 0x53, 0x68, 0x61, - 0x12, 0x19, 0x0a, 0x08, 0x62, 0x61, 0x73, 0x65, 0x5f, 0x73, 0x68, 0x61, 0x18, 0x02, 0x20, 0x01, - 0x28, 0x09, 0x52, 0x07, 0x62, 0x61, 0x73, 0x65, 0x53, 0x68, 0x61, 0x12, 0x19, 0x0a, 0x08, 0x68, - 0x65, 0x61, 0x64, 0x5f, 0x73, 0x68, 0x61, 0x18, 0x03, 0x20, 0x01, 0x28, 0x09, 0x52, 0x07, 0x68, - 0x65, 0x61, 0x64, 0x53, 0x68, 0x61, 0x32, 0x40, 0x0a, 0x0c, 0x4d, 0x65, 0x72, 0x67, 0x65, 0x53, - 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x12, 0x30, 0x0a, 0x05, 0x4d, 0x65, 0x72, 0x67, 0x65, 0x12, - 0x11, 0x2e, 0x72, 0x70, 0x63, 0x2e, 0x4d, 0x65, 0x72, 0x67, 0x65, 0x52, 0x65, 0x71, 0x75, 0x65, - 0x73, 0x74, 0x1a, 0x12, 0x2e, 0x72, 0x70, 0x63, 0x2e, 0x4d, 0x65, 0x72, 0x67, 0x65, 0x52, 0x65, - 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x22, 0x00, 0x42, 0x27, 0x5a, 0x25, 0x67, 0x69, 0x74, 0x68, - 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x68, 0x61, 0x72, 0x6e, 0x65, 0x73, 0x73, 0x2f, 0x67, - 0x69, 0x74, 0x6e, 0x65, 0x73, 0x73, 0x2f, 0x67, 0x69, 0x74, 0x72, 0x70, 0x63, 0x2f, 0x72, 0x70, - 0x63, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, + 0x63, 0x68, 0x22, 0x88, 0x01, 0x0a, 0x0d, 0x4d, 0x65, 0x72, 0x67, 0x65, 0x52, 0x65, 0x73, 0x70, + 0x6f, 0x6e, 0x73, 0x65, 0x12, 0x19, 0x0a, 0x08, 0x62, 0x61, 0x73, 0x65, 0x5f, 0x73, 0x68, 0x61, + 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x07, 0x62, 0x61, 0x73, 0x65, 0x53, 0x68, 0x61, 0x12, + 0x19, 0x0a, 0x08, 0x68, 0x65, 0x61, 0x64, 0x5f, 0x73, 0x68, 0x61, 0x18, 0x02, 0x20, 0x01, 0x28, + 0x09, 0x52, 0x07, 0x68, 0x65, 0x61, 0x64, 0x53, 0x68, 0x61, 0x12, 0x24, 0x0a, 0x0e, 0x6d, 0x65, + 0x72, 0x67, 0x65, 0x5f, 0x62, 0x61, 0x73, 0x65, 0x5f, 0x73, 0x68, 0x61, 0x18, 0x03, 0x20, 0x01, + 0x28, 0x09, 0x52, 0x0c, 0x6d, 0x65, 0x72, 0x67, 0x65, 0x42, 0x61, 0x73, 0x65, 0x53, 0x68, 0x61, + 0x12, 0x1b, 0x0a, 0x09, 0x6d, 0x65, 0x72, 0x67, 0x65, 0x5f, 0x73, 0x68, 0x61, 0x18, 0x04, 0x20, + 0x01, 0x28, 0x09, 0x52, 0x08, 0x6d, 0x65, 0x72, 0x67, 0x65, 0x53, 0x68, 0x61, 0x32, 0x40, 0x0a, + 0x0c, 0x4d, 0x65, 0x72, 0x67, 0x65, 0x53, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x12, 0x30, 0x0a, + 0x05, 0x4d, 0x65, 0x72, 0x67, 0x65, 0x12, 0x11, 0x2e, 0x72, 0x70, 0x63, 0x2e, 0x4d, 0x65, 0x72, + 0x67, 0x65, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x12, 0x2e, 0x72, 0x70, 0x63, 0x2e, + 0x4d, 0x65, 0x72, 0x67, 0x65, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x22, 0x00, 0x42, + 0x27, 0x5a, 0x25, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x68, 0x61, + 0x72, 0x6e, 0x65, 0x73, 0x73, 0x2f, 0x67, 0x69, 0x74, 0x6e, 0x65, 0x73, 0x73, 0x2f, 0x67, 0x69, + 0x74, 0x72, 0x70, 0x63, 0x2f, 0x72, 0x70, 0x63, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, } var ( diff --git a/internal/api/controller/pullreq/merge.go b/internal/api/controller/pullreq/merge.go index e1ae76cfb..52a24a839 100644 --- a/internal/api/controller/pullreq/merge.go +++ b/internal/api/controller/pullreq/merge.go @@ -23,8 +23,8 @@ import ( ) type MergeInput struct { - Method enum.MergeMethod `json:"method"` - HeadSHA string `json:"head_sha"` + Method enum.MergeMethod `json:"method"` + SourceSHA string `json:"source_sha"` } // Merge merges the pull request. @@ -116,23 +116,26 @@ func (c *Controller) Merge( Author: rpcIdentityFromPrincipal(session.Principal), RefType: gitrpcenum.RefTypeBranch, RefName: pr.TargetBranch, - HeadExpectedSHA: in.HeadSHA, + HeadExpectedSHA: in.SourceSHA, }) if err != nil { return types.MergeResponse{}, err } pr, err = c.pullreqStore.UpdateOptLock(ctx, pr, func(pr *types.PullReq) error { - now := time.Now().UnixMilli() - pr.MergeStrategy = &in.Method - pr.Merged = &now - pr.MergedBy = &session.Principal.ID pr.State = enum.PullReqStateMerged - // update all SHAs (might be empty if previous merge check failed) - pr.MergeRefSHA = &mergeOutput.MergedSHA - pr.MergeBaseSHA = &mergeOutput.BaseSHA - pr.MergeHeadSHA = &mergeOutput.HeadSHA + now := time.Now().UnixMilli() + pr.Merged = &now + pr.MergedBy = &session.Principal.ID + pr.MergeMethod = &in.Method + + // update all Merge specific information (might be empty if previous merge check failed) + pr.MergeCheckStatus = enum.MergeCheckStatusMergeable + pr.MergeTargetSHA = &mergeOutput.BaseSHA + pr.MergeBaseSHA = &mergeOutput.MergeBaseSHA + pr.MergeSHA = &mergeOutput.MergeSHA + pr.MergeConflicts = nil pr.ActivitySeq++ // because we need to write the activity entry return nil @@ -143,9 +146,9 @@ func (c *Controller) Merge( activityPayload := &types.PullRequestActivityPayloadMerge{ MergeMethod: in.Method, - MergedSHA: mergeOutput.MergedSHA, - BaseSHA: mergeOutput.BaseSHA, - HeadSHA: mergeOutput.HeadSHA, + MergeSHA: mergeOutput.MergeSHA, + TargetSHA: mergeOutput.BaseSHA, + SourceSHA: mergeOutput.HeadSHA, } if _, errAct := c.activityStore.CreateWithPayload(ctx, pr, session.Principal.ID, activityPayload); errAct != nil { // non-critical error @@ -155,9 +158,9 @@ func (c *Controller) Merge( c.eventReporter.Merged(ctx, &pullreqevents.MergedPayload{ Base: eventBase(pr, &session.Principal), MergeMethod: in.Method, - MergedSHA: mergeOutput.MergedSHA, - BaseSHA: mergeOutput.BaseSHA, - HeadSHA: mergeOutput.HeadSHA, + MergeSHA: mergeOutput.MergeSHA, + TargetSHA: mergeOutput.BaseSHA, + SourceSHA: mergeOutput.HeadSHA, }) return types.MergeResponse{ diff --git a/internal/api/controller/pullreq/pr_commits.go b/internal/api/controller/pullreq/pr_commits.go index 786ef181c..579f675c3 100644 --- a/internal/api/controller/pullreq/pr_commits.go +++ b/internal/api/controller/pullreq/pr_commits.go @@ -34,9 +34,11 @@ func (c *Controller) Commits( } gitRef := pr.SourceBranch + if pr.SourceSHA != "" { + gitRef = pr.SourceSHA + } afterRef := pr.TargetBranch if pr.State == enum.PullReqStateMerged { - gitRef = *pr.MergeHeadSHA afterRef = *pr.MergeBaseSHA } diff --git a/internal/api/controller/pullreq/pr_create.go b/internal/api/controller/pullreq/pr_create.go index 0d110bc23..1fd949f62 100644 --- a/internal/api/controller/pullreq/pr_create.go +++ b/internal/api/controller/pullreq/pr_create.go @@ -107,27 +107,28 @@ func newPullReq( ) *types.PullReq { now := time.Now().UnixMilli() return &types.PullReq{ - ID: 0, // the ID will be populated in the data layer - Version: 0, - Number: number, - CreatedBy: session.Principal.ID, - Created: now, - Updated: now, - Edited: now, - State: enum.PullReqStateOpen, - IsDraft: in.IsDraft, - Title: in.Title, - Description: in.Description, - SourceRepoID: sourceRepo.ID, - SourceBranch: in.SourceBranch, - SourceSHA: sourceSHA, - TargetRepoID: targetRepo.ID, - TargetBranch: in.TargetBranch, - ActivitySeq: 0, - MergedBy: nil, - Merged: nil, - MergeStrategy: nil, - Author: *session.Principal.ToPrincipalInfo(), - Merger: nil, + ID: 0, // the ID will be populated in the data layer + Version: 0, + Number: number, + CreatedBy: session.Principal.ID, + Created: now, + Updated: now, + Edited: now, + State: enum.PullReqStateOpen, + IsDraft: in.IsDraft, + Title: in.Title, + Description: in.Description, + SourceRepoID: sourceRepo.ID, + SourceBranch: in.SourceBranch, + SourceSHA: sourceSHA, + TargetRepoID: targetRepo.ID, + TargetBranch: in.TargetBranch, + ActivitySeq: 0, + MergedBy: nil, + Merged: nil, + MergeCheckStatus: enum.MergeCheckStatusUnchecked, + MergeMethod: nil, + Author: *session.Principal.ToPrincipalInfo(), + Merger: nil, } } diff --git a/internal/api/controller/pullreq/pr_diff.go b/internal/api/controller/pullreq/pr_diff.go index 12a14f326..627cd854a 100644 --- a/internal/api/controller/pullreq/pr_diff.go +++ b/internal/api/controller/pullreq/pr_diff.go @@ -33,14 +33,12 @@ func (c *Controller) RawDiff( } headRef := pr.SourceBranch + if pr.SourceSHA != "" { + headRef = pr.SourceSHA + } baseRef := pr.TargetBranch if pr.State == enum.PullReqStateMerged { - if pr.MergeBaseSHA != nil { - baseRef = *pr.MergeBaseSHA - } - if pr.MergeHeadSHA != nil { - headRef = *pr.MergeHeadSHA - } + baseRef = *pr.MergeBaseSHA } return c.gitRPCClient.RawDiff(ctx, &gitrpc.DiffParams{ diff --git a/internal/api/controller/pullreq/pr_find.go b/internal/api/controller/pullreq/pr_find.go index b7bdff7ec..26558f1f1 100644 --- a/internal/api/controller/pullreq/pr_find.go +++ b/internal/api/controller/pullreq/pr_find.go @@ -7,7 +7,6 @@ package pullreq import ( "context" "fmt" - "strings" "github.com/harness/gitness/gitrpc" "github.com/harness/gitness/internal/api/usererror" @@ -37,14 +36,14 @@ func (c *Controller) Find( return nil, err } + headRef := pr.SourceBranch + if pr.SourceSHA != "" { + headRef = pr.SourceSHA + } baseRef := pr.TargetBranch - headRef := pr.SourceSHA if pr.MergeBaseSHA != nil { baseRef = *pr.MergeBaseSHA } - if pr.MergeHeadSHA != nil { - headRef = *pr.MergeHeadSHA - } output, err := c.gitRPCClient.DiffStats(ctx, &gitrpc.DiffParams{ ReadParams: gitrpc.CreateRPCReadParams(repo), @@ -58,27 +57,5 @@ func (c *Controller) Find( pr.Stats.DiffStats.Commits = output.Commits pr.Stats.DiffStats.FilesChanged = output.FilesChanged - updateMergeStatus(pr) - return pr, nil } - -func updateMergeStatus(pr *types.PullReq) { - mc := "" - if pr.MergeConflicts != nil { - mc = strings.TrimSpace(*pr.MergeConflicts) - } - - switch { - case pr.State == enum.PullReqStateClosed: - pr.MergeStatus = enum.MergeStatusClosed - case pr.IsDraft: - pr.MergeStatus = enum.MergeStatusDraft - case mc != "": - pr.MergeStatus = enum.MergeStatusConflict - case pr.MergeRefSHA != nil: - pr.MergeStatus = enum.MergeStatusMergeable - default: - pr.MergeStatus = enum.MergeStatusUnchecked - } -} diff --git a/internal/api/controller/pullreq/pr_list.go b/internal/api/controller/pullreq/pr_list.go index 3dbd8a404..f52b38c03 100644 --- a/internal/api/controller/pullreq/pr_list.go +++ b/internal/api/controller/pullreq/pr_list.go @@ -64,9 +64,5 @@ func (c *Controller) List( return nil, 0, err } - for i := range list { - updateMergeStatus(list[i]) - } - return list, count, nil } diff --git a/internal/api/controller/pullreq/pr_state.go b/internal/api/controller/pullreq/pr_state.go index 2006906a4..1c1ca40af 100644 --- a/internal/api/controller/pullreq/pr_state.go +++ b/internal/api/controller/pullreq/pr_state.go @@ -122,9 +122,12 @@ func (c *Controller) State(ctx context.Context, pr.IsDraft = in.IsDraft pr.Edited = time.Now().UnixMilli() if in.State == enum.PullReqStateClosed { - pr.MergeRefSHA = nil + // clear all merge (check) related fields + pr.MergeCheckStatus = enum.MergeCheckStatusUnchecked + pr.MergeTargetSHA = nil pr.MergeBaseSHA = nil - pr.MergeHeadSHA = nil + pr.MergeSHA = nil + pr.MergeConflicts = nil } pr.ActivitySeq++ // because we need to add the activity entry return nil diff --git a/internal/api/controller/repo/merge_check.go b/internal/api/controller/repo/merge_check.go index 5afd8a985..caebbcea7 100644 --- a/internal/api/controller/repo/merge_check.go +++ b/internal/api/controller/repo/merge_check.go @@ -50,7 +50,7 @@ func (c *Controller) MergeCheck( HeadRepoUID: writeParams.RepoUID, // forks are not supported for now HeadBranch: info.HeadRef, }) - if errors.Is(err, gitrpc.ErrPreconditionFailed) { + if errors.Is(err, gitrpc.ErrNotMergeable) { return MergeCheck{ Mergeable: false, }, nil diff --git a/internal/api/usererror/translate.go b/internal/api/usererror/translate.go index 03c37b924..d99df99cb 100644 --- a/internal/api/usererror/translate.go +++ b/internal/api/usererror/translate.go @@ -60,6 +60,8 @@ func Translate(err error) *Error { return ErrNotFound case errors.Is(err, gitrpc.ErrPreconditionFailed): return ErrPreconditionFailed + case errors.Is(err, gitrpc.ErrNotMergeable): + return ErrNotMergeable // webhook errors case errors.Is(err, webhook.ErrWebhookNotRetriggerable): diff --git a/internal/api/usererror/usererror.go b/internal/api/usererror/usererror.go index 904e4bb79..5b51cd83f 100644 --- a/internal/api/usererror/usererror.go +++ b/internal/api/usererror/usererror.go @@ -31,6 +31,9 @@ var ( // ErrPreconditionFailed is returned when a precondition failed. ErrPreconditionFailed = New(http.StatusPreconditionFailed, "Precondition failed") + // ErrNotMergeable is returned when a branch can't be merged. + ErrNotMergeable = New(http.StatusPreconditionFailed, "Branch can't be merged") + // ErrNoChange is returned when no change was found based on the request. ErrNoChange = New(http.StatusBadRequest, "No Change") diff --git a/internal/events/pullreq/events_state.go b/internal/events/pullreq/events_state.go index b0759453f..81a2c3b86 100644 --- a/internal/events/pullreq/events_state.go +++ b/internal/events/pullreq/events_state.go @@ -97,9 +97,9 @@ const MergedEvent events.EventType = "merged" type MergedPayload struct { Base MergeMethod enum.MergeMethod `json:"merge_method"` - MergedSHA string `json:"merged_sha"` - BaseSHA string `json:"base_sha"` - HeadSHA string `json:"head_sha"` + MergeSHA string `json:"merge_sha"` + TargetSHA string `json:"target_sha"` + SourceSHA string `json:"source_sha"` } func (r *Reporter) Merged(ctx context.Context, payload *MergedPayload) { diff --git a/internal/services/pullreq/handlers_branch.go b/internal/services/pullreq/handlers_branch.go index 5da0ecb5c..ce440ad0d 100644 --- a/internal/services/pullreq/handlers_branch.go +++ b/internal/services/pullreq/handlers_branch.go @@ -31,10 +31,15 @@ func (s *Service) triggerPREventOnBranchUpdate(ctx context.Context, "failed to set SourceSHA for PR %d to value '%s', expected SHA '%s' but current pr has '%s'", pr.Number, event.Payload.NewSHA, event.Payload.OldSHA, pr.SourceSHA) } + pr.SourceSHA = event.Payload.NewSHA - pr.MergeRefSHA = nil + + // reset merge-check fields for new run + pr.MergeCheckStatus = enum.MergeCheckStatusUnchecked + pr.MergeTargetSHA = nil pr.MergeBaseSHA = nil - pr.MergeHeadSHA = nil + pr.MergeSHA = nil + pr.MergeConflicts = nil return nil }) if err != nil { @@ -76,11 +81,15 @@ func (s *Service) closePullReqOnBranchDelete(ctx context.Context, ) error { s.forEveryOpenPR(ctx, event.Payload.RepoID, event.Payload.Ref, func(pr *types.PullReq) error { pr, err := s.pullreqStore.UpdateOptLock(ctx, pr, func(pr *types.PullReq) error { - pr.State = enum.PullReqStateClosed - pr.MergeRefSHA = nil - pr.MergeBaseSHA = nil - pr.MergeHeadSHA = nil pr.ActivitySeq++ // because we need to write the activity + + pr.State = enum.PullReqStateClosed + pr.MergeCheckStatus = enum.MergeCheckStatusUnchecked + pr.MergeTargetSHA = nil + pr.MergeBaseSHA = nil + pr.MergeSHA = nil + pr.MergeConflicts = nil + return nil }) if err != nil { diff --git a/internal/services/pullreq/handlers_mergeable.go b/internal/services/pullreq/handlers_mergeable.go index 1edb415ef..742c20446 100644 --- a/internal/services/pullreq/handlers_mergeable.go +++ b/internal/services/pullreq/handlers_mergeable.go @@ -207,23 +207,36 @@ func (s *Service) updateMergeData( Force: true, }) if errors.Is(err, gitrpc.ErrPreconditionFailed) { - // TODO: in case of merge conflict, update conflicts in pr entry - for that we need a - // nice way to distinguish between the two errors. - return events.NewDiscardEventErrorf("Source branch '%s' is not on SHA '%s' anymore or is not mergeable.", + return events.NewDiscardEventErrorf("Source branch '%s' is not on SHA '%s' anymore.", pr.SourceBranch, newSHA) } - if err != nil { + + isNotMergeableError := errors.Is(err, gitrpc.ErrNotMergeable) + if err != nil && !isNotMergeableError { return fmt.Errorf("merge check failed for %s and %s with err: %w", targetRepo.UID+":"+pr.TargetBranch, sourceRepo.UID+":"+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) } - pr.MergeRefSHA = &output.MergedSHA - pr.MergeBaseSHA = &output.BaseSHA - pr.MergeHeadSHA = &output.HeadSHA + + if isNotMergeableError { + // TODO: gitrpc should return sha's either way, and also conflicting files! + pr.MergeCheckStatus = enum.MergeCheckStatusConflict + pr.MergeTargetSHA = nil + pr.MergeBaseSHA = nil + pr.MergeSHA = nil + pr.MergeConflicts = nil + } else { + pr.MergeCheckStatus = enum.MergeCheckStatusMergeable + pr.MergeTargetSHA = &output.BaseSHA + pr.MergeBaseSHA = &output.MergeBaseSHA + pr.MergeSHA = &output.MergeSHA + pr.MergeConflicts = nil + } return nil }) if err != nil { diff --git a/internal/services/webhook/types.go b/internal/services/webhook/types.go index 93d16ed9e..935e0874d 100644 --- a/internal/services/webhook/types.go +++ b/internal/services/webhook/types.go @@ -99,7 +99,7 @@ func pullReqInfoFrom(pr *types.PullReq) PullReqInfo { SourceBranch: pr.SourceBranch, TargetRepoID: pr.TargetRepoID, TargetBranch: pr.TargetBranch, - MergeStrategy: pr.MergeStrategy, + MergeStrategy: pr.MergeMethod, } } diff --git a/internal/store/database/migrate/postgres/0003_create_table_pullreqs.up.sql b/internal/store/database/migrate/postgres/0003_create_table_pullreqs.up.sql index ddda8c631..ee3f1e223 100644 --- a/internal/store/database/migrate/postgres/0003_create_table_pullreqs.up.sql +++ b/internal/store/database/migrate/postgres/0003_create_table_pullreqs.up.sql @@ -19,10 +19,11 @@ pullreq_id SERIAL PRIMARY KEY ,pullreq_activity_seq INTEGER DEFAULT 0 ,pullreq_merged_by INTEGER ,pullreq_merged BIGINT -,pullreq_merge_strategy TEXT -,pullreq_merge_head_sha TEXT +,pullreq_merge_method TEXT +,pullreq_merge_check_status TEXT +,pullreq_merge_target_sha TEXT ,pullreq_merge_base_sha TEXT -,pullreq_merge_ref_sha TEXT +,pullreq_merge_sha TEXT ,pullreq_merge_conflicts TEXT ,CONSTRAINT fk_pullreq_created_by FOREIGN KEY (pullreq_created_by) REFERENCES principals (principal_id) MATCH SIMPLE diff --git a/internal/store/database/migrate/sqlite/0003_create_table_pullreqs.up.sql b/internal/store/database/migrate/sqlite/0003_create_table_pullreqs.up.sql index 86799abec..94a8b5286 100644 --- a/internal/store/database/migrate/sqlite/0003_create_table_pullreqs.up.sql +++ b/internal/store/database/migrate/sqlite/0003_create_table_pullreqs.up.sql @@ -19,10 +19,11 @@ pullreq_id INTEGER PRIMARY KEY AUTOINCREMENT ,pullreq_activity_seq INTEGER DEFAULT 0 ,pullreq_merged_by INTEGER ,pullreq_merged BIGINT -,pullreq_merge_strategy TEXT -,pullreq_merge_head_sha TEXT +,pullreq_merge_method TEXT +,pullreq_merge_check_status TEXT NOT NULL +,pullreq_merge_target_sha TEXT ,pullreq_merge_base_sha TEXT -,pullreq_merge_ref_sha TEXT +,pullreq_merge_sha TEXT ,pullreq_merge_conflicts TEXT ,CONSTRAINT fk_pullreq_created_by FOREIGN KEY (pullreq_created_by) REFERENCES principals (principal_id) MATCH SIMPLE diff --git a/internal/store/database/pullreq.go b/internal/store/database/pullreq.go index e80c4b49c..6443a01ce 100644 --- a/internal/store/database/pullreq.go +++ b/internal/store/database/pullreq.go @@ -67,13 +67,15 @@ type pullReq struct { ActivitySeq int64 `db:"pullreq_activity_seq"` - MergedBy null.Int `db:"pullreq_merged_by"` - Merged null.Int `db:"pullreq_merged"` - MergeStrategy null.String `db:"pullreq_merge_strategy"` - MergeHeadSHA null.String `db:"pullreq_merge_head_sha"` - MergeBaseSHA null.String `db:"pullreq_merge_base_sha"` - MergeRefSHA null.String `db:"pullreq_merge_ref_sha"` - MergeConflicts null.String `db:"pullreq_merge_conflicts"` + MergedBy null.Int `db:"pullreq_merged_by"` + Merged null.Int `db:"pullreq_merged"` + MergeMethod null.String `db:"pullreq_merge_method"` + + MergeCheckStatus enum.MergeCheckStatus `db:"pullreq_merge_check_status"` + MergeTargetSHA null.String `db:"pullreq_merge_target_sha"` + MergeBaseSHA null.String `db:"pullreq_merge_base_sha"` + MergeSHA null.String `db:"pullreq_merge_sha"` + MergeConflicts null.String `db:"pullreq_merge_conflicts"` } const ( @@ -98,10 +100,11 @@ const ( ,pullreq_activity_seq ,pullreq_merged_by ,pullreq_merged - ,pullreq_merge_strategy - ,pullreq_merge_head_sha + ,pullreq_merge_method + ,pullreq_merge_check_status + ,pullreq_merge_target_sha ,pullreq_merge_base_sha - ,pullreq_merge_ref_sha + ,pullreq_merge_sha ,pullreq_merge_conflicts` pullReqSelectBase = ` @@ -185,8 +188,11 @@ func (s *PullReqStore) Create(ctx context.Context, pr *types.PullReq) error { ,pullreq_activity_seq ,pullreq_merged_by ,pullreq_merged - ,pullreq_merge_strategy - ,pullreq_merge_ref_sha + ,pullreq_merge_method + ,pullreq_merge_check_status + ,pullreq_merge_target_sha + ,pullreq_merge_base_sha + ,pullreq_merge_sha ,pullreq_merge_conflicts ) values ( :pullreq_version @@ -208,8 +214,11 @@ func (s *PullReqStore) Create(ctx context.Context, pr *types.PullReq) error { ,:pullreq_activity_seq ,:pullreq_merged_by ,:pullreq_merged - ,:pullreq_merge_strategy - ,:pullreq_merge_ref_sha + ,:pullreq_merge_method + ,:pullreq_merge_check_status + ,:pullreq_merge_target_sha + ,:pullreq_merge_base_sha + ,:pullreq_merge_sha ,:pullreq_merge_conflicts ) RETURNING pullreq_id` @@ -244,10 +253,11 @@ func (s *PullReqStore) Update(ctx context.Context, pr *types.PullReq) error { ,pullreq_source_sha = :pullreq_source_sha ,pullreq_merged_by = :pullreq_merged_by ,pullreq_merged = :pullreq_merged - ,pullreq_merge_strategy = :pullreq_merge_strategy - ,pullreq_merge_head_sha = :pullreq_merge_head_sha + ,pullreq_merge_method = :pullreq_merge_method + ,pullreq_merge_check_status = :pullreq_merge_check_status + ,pullreq_merge_target_sha = :pullreq_merge_target_sha ,pullreq_merge_base_sha = :pullreq_merge_base_sha - ,pullreq_merge_ref_sha = :pullreq_merge_ref_sha + ,pullreq_merge_sha = :pullreq_merge_sha ,pullreq_merge_conflicts = :pullreq_merge_conflicts WHERE pullreq_id = :pullreq_id AND pullreq_version = :pullreq_version - 1` @@ -448,33 +458,34 @@ func (s *PullReqStore) List(ctx context.Context, opts *types.PullReqFilter) ([]* func mapPullReq(pr *pullReq) *types.PullReq { return &types.PullReq{ - ID: pr.ID, - Version: pr.Version, - Number: pr.Number, - CreatedBy: pr.CreatedBy, - Created: pr.Created, - Updated: pr.Updated, - Edited: pr.Edited, - State: pr.State, - IsDraft: pr.IsDraft, - CommentCount: pr.CommentCount, - Title: pr.Title, - Description: pr.Description, - SourceRepoID: pr.SourceRepoID, - SourceBranch: pr.SourceBranch, - SourceSHA: pr.SourceSHA, - TargetRepoID: pr.TargetRepoID, - TargetBranch: pr.TargetBranch, - ActivitySeq: pr.ActivitySeq, - MergedBy: pr.MergedBy.Ptr(), - Merged: pr.Merged.Ptr(), - MergeStrategy: (*enum.MergeMethod)(pr.MergeStrategy.Ptr()), - MergeHeadSHA: pr.MergeHeadSHA.Ptr(), - MergeBaseSHA: pr.MergeBaseSHA.Ptr(), - MergeRefSHA: pr.MergeRefSHA.Ptr(), - MergeConflicts: pr.MergeConflicts.Ptr(), - Author: types.PrincipalInfo{}, - Merger: nil, + ID: pr.ID, + Version: pr.Version, + Number: pr.Number, + CreatedBy: pr.CreatedBy, + Created: pr.Created, + Updated: pr.Updated, + Edited: pr.Edited, + State: pr.State, + IsDraft: pr.IsDraft, + CommentCount: pr.CommentCount, + Title: pr.Title, + Description: pr.Description, + SourceRepoID: pr.SourceRepoID, + SourceBranch: pr.SourceBranch, + SourceSHA: pr.SourceSHA, + TargetRepoID: pr.TargetRepoID, + TargetBranch: pr.TargetBranch, + ActivitySeq: pr.ActivitySeq, + MergedBy: pr.MergedBy.Ptr(), + Merged: pr.Merged.Ptr(), + MergeMethod: (*enum.MergeMethod)(pr.MergeMethod.Ptr()), + MergeCheckStatus: pr.MergeCheckStatus, + MergeTargetSHA: pr.MergeTargetSHA.Ptr(), + MergeBaseSHA: pr.MergeBaseSHA.Ptr(), + MergeSHA: pr.MergeSHA.Ptr(), + MergeConflicts: pr.MergeConflicts.Ptr(), + Author: types.PrincipalInfo{}, + Merger: nil, Stats: types.PullReqStats{ Conversations: pr.CommentCount, DiffStats: types.DiffStats{ @@ -487,31 +498,32 @@ func mapPullReq(pr *pullReq) *types.PullReq { func mapInternalPullReq(pr *types.PullReq) *pullReq { m := &pullReq{ - ID: pr.ID, - Version: pr.Version, - Number: pr.Number, - CreatedBy: pr.CreatedBy, - Created: pr.Created, - Updated: pr.Updated, - Edited: pr.Edited, - State: pr.State, - IsDraft: pr.IsDraft, - CommentCount: pr.CommentCount, - Title: pr.Title, - Description: pr.Description, - SourceRepoID: pr.SourceRepoID, - SourceBranch: pr.SourceBranch, - SourceSHA: pr.SourceSHA, - TargetRepoID: pr.TargetRepoID, - TargetBranch: pr.TargetBranch, - ActivitySeq: pr.ActivitySeq, - MergedBy: null.IntFromPtr(pr.MergedBy), - Merged: null.IntFromPtr(pr.Merged), - MergeStrategy: null.StringFromPtr((*string)(pr.MergeStrategy)), - MergeHeadSHA: null.StringFromPtr(pr.MergeHeadSHA), - MergeBaseSHA: null.StringFromPtr(pr.MergeBaseSHA), - MergeRefSHA: null.StringFromPtr(pr.MergeRefSHA), - MergeConflicts: null.StringFromPtr(pr.MergeConflicts), + ID: pr.ID, + Version: pr.Version, + Number: pr.Number, + CreatedBy: pr.CreatedBy, + Created: pr.Created, + Updated: pr.Updated, + Edited: pr.Edited, + State: pr.State, + IsDraft: pr.IsDraft, + CommentCount: pr.CommentCount, + Title: pr.Title, + Description: pr.Description, + SourceRepoID: pr.SourceRepoID, + SourceBranch: pr.SourceBranch, + SourceSHA: pr.SourceSHA, + TargetRepoID: pr.TargetRepoID, + TargetBranch: pr.TargetBranch, + ActivitySeq: pr.ActivitySeq, + MergedBy: null.IntFromPtr(pr.MergedBy), + Merged: null.IntFromPtr(pr.Merged), + MergeMethod: null.StringFromPtr((*string)(pr.MergeMethod)), + MergeCheckStatus: pr.MergeCheckStatus, + MergeTargetSHA: null.StringFromPtr(pr.MergeTargetSHA), + MergeBaseSHA: null.StringFromPtr(pr.MergeBaseSHA), + MergeSHA: null.StringFromPtr(pr.MergeSHA), + MergeConflicts: null.StringFromPtr(pr.MergeConflicts), } return m diff --git a/types/enum/pullreq.go b/types/enum/pullreq.go index 942fe868c..030c5eea9 100644 --- a/types/enum/pullreq.go +++ b/types/enum/pullreq.go @@ -189,12 +189,13 @@ var mergeMethods = sortEnum([]MergeMethod{ MergeMethodRebase, }) -type MergeStatus string +type MergeCheckStatus string const ( - MergeStatusUnchecked MergeStatus = "unchecked" // The merge status has not been checked. - MergeStatusConflict MergeStatus = "conflict" // Can’t merge into the target branch due to a potential conflict. - MergeStatusDraft MergeStatus = "draft_pr" // Can’t merge because the pull request is a draft. - MergeStatusClosed MergeStatus = "closed" // The merge request must be open before merge. - MergeStatusMergeable MergeStatus = "mergeable" // The branch can merge cleanly into the target branch. + // MergeCheckStatusUnchecked merge status has not been checked. + MergeCheckStatusUnchecked MergeCheckStatus = "unchecked" + // MergeCheckStatusConflict can’t merge into the target branch due to a potential conflict. + MergeCheckStatusConflict MergeCheckStatus = "conflict" + // MergeCheckStatusMergeable branch can merged cleanly into the target branch. + MergeCheckStatusMergeable MergeCheckStatus = "mergeable" ) diff --git a/types/pullreq.go b/types/pullreq.go index 846fedd17..e73409c18 100644 --- a/types/pullreq.go +++ b/types/pullreq.go @@ -35,15 +35,15 @@ type PullReq struct { ActivitySeq int64 `json:"-"` // not returned, because it's a server's internal field - MergedBy *int64 `json:"-"` // not returned, because the merger info is in the Merger field - Merged *int64 `json:"merged"` - MergeStrategy *enum.MergeMethod `json:"merge_strategy"` + MergedBy *int64 `json:"-"` // not returned, because the merger info is in the Merger field + Merged *int64 `json:"merged"` + MergeMethod *enum.MergeMethod `json:"merge_method"` - MergeHeadSHA *string `json:"merge_head_sha"` - MergeBaseSHA *string `json:"merge_base_sha"` - MergeRefSHA *string `json:"merge_ref_sha"` - MergeStatus enum.MergeStatus `json:"merge_status"` - MergeConflicts *string `json:"merge_conflicts,omitempty"` + MergeCheckStatus enum.MergeCheckStatus `json:"merge_check_status"` + MergeTargetSHA *string `json:"merge_target_sha"` + MergeBaseSHA *string `json:"merge_base_sha"` + MergeSHA *string `json:"merge_sha"` + MergeConflicts *string `json:"merge_conflicts,omitempty"` Author PrincipalInfo `json:"author"` Merger *PrincipalInfo `json:"merger"` diff --git a/types/pullreq_activity.go b/types/pullreq_activity.go index 5b48a7fab..b8e3b553f 100644 --- a/types/pullreq_activity.go +++ b/types/pullreq_activity.go @@ -166,9 +166,9 @@ func (a *PullRequestActivityPayloadComment) ActivityType() enum.PullReqActivityT type PullRequestActivityPayloadMerge struct { MergeMethod enum.MergeMethod `json:"merge_method"` - MergedSHA string `json:"merged_sha"` - BaseSHA string `json:"base_sha"` - HeadSHA string `json:"head_sha"` + MergeSHA string `json:"merge_sha"` + TargetSHA string `json:"target_sha"` + SourceSHA string `json:"source_sha"` } func (a *PullRequestActivityPayloadMerge) ActivityType() enum.PullReqActivityType {