diff options
Diffstat (limited to 'services/pull')
-rw-r--r-- | services/pull/check.go | 21 | ||||
-rw-r--r-- | services/pull/check_test.go | 52 | ||||
-rw-r--r-- | services/pull/commit_status.go | 110 | ||||
-rw-r--r-- | services/pull/commit_status_test.go | 101 | ||||
-rw-r--r-- | services/pull/merge.go | 118 | ||||
-rw-r--r-- | services/pull/merge_prepare.go | 6 | ||||
-rw-r--r-- | services/pull/merge_squash.go | 14 | ||||
-rw-r--r-- | services/pull/merge_test.go | 25 | ||||
-rw-r--r-- | services/pull/patch.go | 2 | ||||
-rw-r--r-- | services/pull/pull.go | 30 | ||||
-rw-r--r-- | services/pull/review.go | 10 | ||||
-rw-r--r-- | services/pull/reviewer.go | 2 | ||||
-rw-r--r-- | services/pull/temp_repo.go | 2 | ||||
-rw-r--r-- | services/pull/update.go | 45 |
14 files changed, 312 insertions, 226 deletions
diff --git a/services/pull/check.go b/services/pull/check.go index 5d8990aa00..7fcec22f49 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -1,5 +1,4 @@ -// Copyright 2019 The Gitea Authors. -// All rights reserved. +// Copyright 2019 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT package pull @@ -16,6 +15,7 @@ import ( git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" access_model "code.gitea.io/gitea/models/perm/access" + "code.gitea.io/gitea/models/pull" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" @@ -29,6 +29,7 @@ import ( "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" asymkey_service "code.gitea.io/gitea/services/asymkey" + "code.gitea.io/gitea/services/automergequeue" notify_service "code.gitea.io/gitea/services/notify" ) @@ -230,7 +231,7 @@ func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer return true, nil } - sign, _, _, err := asymkey_service.SignMerge(ctx, pr, doer, pr.BaseRepo.RepoPath(), pr.BaseBranch, pr.GetGitRefName()) + sign, _, _, err := asymkey_service.SignMerge(ctx, pr, doer, pr.BaseRepo.RepoPath(), pr.BaseBranch, pr.GetGitHeadRefName()) return sign, err } @@ -238,7 +239,7 @@ func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer // markPullRequestAsMergeable checks if pull request is possible to leaving checking status, // and set to be either conflict or mergeable. func markPullRequestAsMergeable(ctx context.Context, pr *issues_model.PullRequest) { - // If status has not been changed to conflict by testPullRequestTmpRepoBranchMergeable then we are mergeable + // If the status has not been changed to conflict by testPullRequestTmpRepoBranchMergeable then we are mergeable if pr.Status == issues_model.PullRequestStatusChecking { pr.Status = issues_model.PullRequestStatusMergeable } @@ -257,6 +258,16 @@ func markPullRequestAsMergeable(ctx context.Context, pr *issues_model.PullReques if err := pr.UpdateColsIfNotMerged(ctx, "merge_base", "status", "conflicted_files", "changed_protected_files"); err != nil { log.Error("Update[%-v]: %v", pr, err) } + + // if there is a scheduled merge for this pull request, start the auto merge check (again) + exist, _, err := pull.GetScheduledMergeByPullID(ctx, pr.ID) + if err != nil { + log.Error("GetScheduledMergeByPullID[%-v]: %v", pr, err) + return + } else if !exist { + return + } + automergequeue.StartPRCheckAndAutoMerge(ctx, pr) } // getMergeCommit checks if a pull request has been merged @@ -266,7 +277,7 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com return nil, fmt.Errorf("unable to load base repo for %s: %w", pr, err) } - prHeadRef := pr.GetGitRefName() + prHeadRef := pr.GetGitHeadRefName() // Check if the pull request is merged into BaseBranch if _, _, err := git.NewCommand("merge-base", "--is-ancestor"). diff --git a/services/pull/check_test.go b/services/pull/check_test.go index fa3a676ef1..eb66615dcf 100644 --- a/services/pull/check_test.go +++ b/services/pull/check_test.go @@ -1,5 +1,4 @@ -// Copyright 2019 The Gitea Authors. -// All rights reserved. +// Copyright 2019 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT package pull @@ -11,11 +10,18 @@ import ( "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/models/pull" + repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/graceful" "code.gitea.io/gitea/modules/queue" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" + "code.gitea.io/gitea/services/automergequeue" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestPullRequest_AddToTaskQueue(t *testing.T) { @@ -63,6 +69,46 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) { pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) assert.Equal(t, issues_model.PullRequestStatusChecking, pr.Status) - prPatchCheckerQueue.ShutdownWait(5 * time.Second) + prPatchCheckerQueue.ShutdownWait(time.Second) prPatchCheckerQueue = nil } + +func TestMarkPullRequestAsMergeable(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + prPatchCheckerQueue = queue.CreateUniqueQueue(graceful.GetManager().ShutdownContext(), "pr_patch_checker", func(items ...string) []string { return nil }) + go prPatchCheckerQueue.Run() + defer func() { + prPatchCheckerQueue.ShutdownWait(time.Second) + prPatchCheckerQueue = nil + }() + + addToQueueShaChan := make(chan string, 1) + defer test.MockVariableValue(&automergequeue.AddToQueue, func(pr *issues_model.PullRequest, sha string) { + addToQueueShaChan <- sha + })() + ctx := t.Context() + _, _ = db.GetEngine(ctx).ID(2).Update(&issues_model.PullRequest{Status: issues_model.PullRequestStatusChecking}) + pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) + require.False(t, pr.HasMerged) + require.Equal(t, issues_model.PullRequestStatusChecking, pr.Status) + + err := pull.ScheduleAutoMerge(ctx, &user_model.User{ID: 99999}, pr.ID, repo_model.MergeStyleMerge, "test msg", true) + require.NoError(t, err) + + exist, scheduleMerge, err := pull.GetScheduledMergeByPullID(ctx, pr.ID) + require.NoError(t, err) + assert.True(t, exist) + assert.True(t, scheduleMerge.Doer.IsGhost()) + + markPullRequestAsMergeable(ctx, pr) + pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) + require.Equal(t, issues_model.PullRequestStatusMergeable, pr.Status) + + select { + case sha := <-addToQueueShaChan: + assert.Equal(t, "985f0301dba5e7b34be866819cd15ad3d8f508ee", sha) // ref: refs/pull/3/head + case <-time.After(1 * time.Second): + assert.FailNow(t, "Timeout: nothing was added to automergequeue") + } +} diff --git a/services/pull/commit_status.go b/services/pull/commit_status.go index 0bfff21746..d15d318149 100644 --- a/services/pull/commit_status.go +++ b/services/pull/commit_status.go @@ -10,93 +10,59 @@ import ( "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" + "code.gitea.io/gitea/modules/commitstatus" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/log" - "code.gitea.io/gitea/modules/structs" "github.com/gobwas/glob" "github.com/pkg/errors" ) // MergeRequiredContextsCommitStatus returns a commit status state for given required contexts -func MergeRequiredContextsCommitStatus(commitStatuses []*git_model.CommitStatus, requiredContexts []string) structs.CommitStatusState { - // matchedCount is the number of `CommitStatus.Context` that match any context of `requiredContexts` - matchedCount := 0 - returnedStatus := structs.CommitStatusSuccess - - if len(requiredContexts) > 0 { - requiredContextsGlob := make(map[string]glob.Glob, len(requiredContexts)) - for _, ctx := range requiredContexts { - if gp, err := glob.Compile(ctx); err != nil { - log.Error("glob.Compile %s failed. Error: %v", ctx, err) - } else { - requiredContextsGlob[ctx] = gp - } - } - - for _, gp := range requiredContextsGlob { - var targetStatus structs.CommitStatusState - for _, commitStatus := range commitStatuses { - if gp.Match(commitStatus.Context) { - targetStatus = commitStatus.State - matchedCount++ - break - } - } - - // If required rule not match any action, then it is pending - if targetStatus == "" { - if structs.CommitStatusPending.NoBetterThan(returnedStatus) { - returnedStatus = structs.CommitStatusPending - } - break - } - - if targetStatus.NoBetterThan(returnedStatus) { - returnedStatus = targetStatus - } - } +func MergeRequiredContextsCommitStatus(commitStatuses []*git_model.CommitStatus, requiredContexts []string) commitstatus.CommitStatusState { + if len(commitStatuses) == 0 { + return commitstatus.CommitStatusPending } - if matchedCount == 0 && returnedStatus == structs.CommitStatusSuccess { - status := git_model.CalcCommitStatus(commitStatuses) - if status != nil { - return status.State - } - return structs.CommitStatusSuccess + if len(requiredContexts) == 0 { + return git_model.CalcCommitStatus(commitStatuses).State } - return returnedStatus -} - -// IsCommitStatusContextSuccess returns true if all required status check contexts succeed. -func IsCommitStatusContextSuccess(commitStatuses []*git_model.CommitStatus, requiredContexts []string) bool { - // If no specific context is required, require that last commit status is a success - if len(requiredContexts) == 0 { - status := git_model.CalcCommitStatus(commitStatuses) - if status == nil || status.State != structs.CommitStatusSuccess { - return false + requiredContextsGlob := make(map[string]glob.Glob, len(requiredContexts)) + for _, ctx := range requiredContexts { + if gp, err := glob.Compile(ctx); err != nil { + log.Error("glob.Compile %s failed. Error: %v", ctx, err) + } else { + requiredContextsGlob[ctx] = gp } - return true } - for _, ctx := range requiredContexts { - var found bool + requiredCommitStatuses := make([]*git_model.CommitStatus, 0, len(commitStatuses)) + allRequiredContextsMatched := true + for _, gp := range requiredContextsGlob { + requiredContextMatched := false for _, commitStatus := range commitStatuses { - if commitStatus.Context == ctx { - if commitStatus.State != structs.CommitStatusSuccess { - return false - } - - found = true - break + if gp.Match(commitStatus.Context) { + requiredCommitStatuses = append(requiredCommitStatuses, commitStatus) + requiredContextMatched = true } } - if !found { - return false - } + allRequiredContextsMatched = allRequiredContextsMatched && requiredContextMatched + } + if len(requiredCommitStatuses) == 0 { + return commitstatus.CommitStatusPending + } + + returnedStatus := git_model.CalcCommitStatus(requiredCommitStatuses).State + if allRequiredContextsMatched { + return returnedStatus + } + + if returnedStatus == commitstatus.CommitStatusFailure { + return commitstatus.CommitStatusFailure } - return true + // even if part of success, return pending + return commitstatus.CommitStatusPending } // IsPullCommitStatusPass returns if all required status checks PASS @@ -117,7 +83,7 @@ func IsPullCommitStatusPass(ctx context.Context, pr *issues_model.PullRequest) ( } // GetPullRequestCommitStatusState returns pull request merged commit status state -func GetPullRequestCommitStatusState(ctx context.Context, pr *issues_model.PullRequest) (structs.CommitStatusState, error) { +func GetPullRequestCommitStatusState(ctx context.Context, pr *issues_model.PullRequest) (commitstatus.CommitStatusState, error) { // Ensure HeadRepo is loaded if err := pr.LoadHeadRepo(ctx); err != nil { return "", errors.Wrap(err, "LoadHeadRepo") @@ -133,7 +99,7 @@ func GetPullRequestCommitStatusState(ctx context.Context, pr *issues_model.PullR if pr.Flow == issues_model.PullRequestFlowGithub && !gitrepo.IsBranchExist(ctx, pr.HeadRepo, pr.HeadBranch) { return "", errors.New("Head branch does not exist, can not merge") } - if pr.Flow == issues_model.PullRequestFlowAGit && !gitrepo.IsReferenceExist(ctx, pr.HeadRepo, pr.GetGitRefName()) { + if pr.Flow == issues_model.PullRequestFlowAGit && !gitrepo.IsReferenceExist(ctx, pr.HeadRepo, pr.GetGitHeadRefName()) { return "", errors.New("Head branch does not exist, can not merge") } @@ -141,7 +107,7 @@ func GetPullRequestCommitStatusState(ctx context.Context, pr *issues_model.PullR if pr.Flow == issues_model.PullRequestFlowGithub { sha, err = headGitRepo.GetBranchCommitID(pr.HeadBranch) } else { - sha, err = headGitRepo.GetRefCommitID(pr.GetGitRefName()) + sha, err = headGitRepo.GetRefCommitID(pr.GetGitHeadRefName()) } if err != nil { return "", err @@ -151,7 +117,7 @@ func GetPullRequestCommitStatusState(ctx context.Context, pr *issues_model.PullR return "", errors.Wrap(err, "LoadBaseRepo") } - commitStatuses, _, err := git_model.GetLatestCommitStatus(ctx, pr.BaseRepo.ID, sha, db.ListOptionsAll) + commitStatuses, err := git_model.GetLatestCommitStatus(ctx, pr.BaseRepo.ID, sha, db.ListOptionsAll) if err != nil { return "", errors.Wrap(err, "GetLatestCommitStatus") } diff --git a/services/pull/commit_status_test.go b/services/pull/commit_status_test.go index 592acdd55c..a58e788c04 100644 --- a/services/pull/commit_status_test.go +++ b/services/pull/commit_status_test.go @@ -8,58 +8,85 @@ import ( "testing" git_model "code.gitea.io/gitea/models/git" - "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/commitstatus" "github.com/stretchr/testify/assert" ) func TestMergeRequiredContextsCommitStatus(t *testing.T) { - testCases := [][]*git_model.CommitStatus{ + cases := []struct { + commitStatuses []*git_model.CommitStatus + requiredContexts []string + expected commitstatus.CommitStatusState + }{ { - {Context: "Build 1", State: structs.CommitStatusSuccess}, - {Context: "Build 2", State: structs.CommitStatusSuccess}, - {Context: "Build 3", State: structs.CommitStatusSuccess}, + commitStatuses: []*git_model.CommitStatus{}, + requiredContexts: []string{}, + expected: commitstatus.CommitStatusPending, }, { - {Context: "Build 1", State: structs.CommitStatusSuccess}, - {Context: "Build 2", State: structs.CommitStatusSuccess}, - {Context: "Build 2t", State: structs.CommitStatusPending}, + commitStatuses: []*git_model.CommitStatus{ + {Context: "Build xxx", State: commitstatus.CommitStatusSkipped}, + }, + requiredContexts: []string{"Build*"}, + expected: commitstatus.CommitStatusSuccess, }, { - {Context: "Build 1", State: structs.CommitStatusSuccess}, - {Context: "Build 2", State: structs.CommitStatusSuccess}, - {Context: "Build 2t", State: structs.CommitStatusFailure}, + commitStatuses: []*git_model.CommitStatus{ + {Context: "Build 1", State: commitstatus.CommitStatusSkipped}, + {Context: "Build 2", State: commitstatus.CommitStatusSuccess}, + {Context: "Build 3", State: commitstatus.CommitStatusSuccess}, + }, + requiredContexts: []string{"Build*"}, + expected: commitstatus.CommitStatusSuccess, }, { - {Context: "Build 1", State: structs.CommitStatusSuccess}, - {Context: "Build 2", State: structs.CommitStatusSuccess}, - {Context: "Build 2t", State: structs.CommitStatusSuccess}, + commitStatuses: []*git_model.CommitStatus{ + {Context: "Build 1", State: commitstatus.CommitStatusSuccess}, + {Context: "Build 2", State: commitstatus.CommitStatusSuccess}, + {Context: "Build 2t", State: commitstatus.CommitStatusPending}, + }, + requiredContexts: []string{"Build*", "Build 2t*"}, + expected: commitstatus.CommitStatusPending, }, { - {Context: "Build 1", State: structs.CommitStatusSuccess}, - {Context: "Build 2", State: structs.CommitStatusSuccess}, - {Context: "Build 2t", State: structs.CommitStatusSuccess}, + commitStatuses: []*git_model.CommitStatus{ + {Context: "Build 1", State: commitstatus.CommitStatusSuccess}, + {Context: "Build 2", State: commitstatus.CommitStatusSuccess}, + {Context: "Build 2t", State: commitstatus.CommitStatusFailure}, + }, + requiredContexts: []string{"Build*", "Build 2t*"}, + expected: commitstatus.CommitStatusFailure, + }, + { + commitStatuses: []*git_model.CommitStatus{ + {Context: "Build 1", State: commitstatus.CommitStatusSuccess}, + {Context: "Build 2", State: commitstatus.CommitStatusSuccess}, + {Context: "Build 2t", State: commitstatus.CommitStatusFailure}, + }, + requiredContexts: []string{"Build*"}, + expected: commitstatus.CommitStatusFailure, + }, + { + commitStatuses: []*git_model.CommitStatus{ + {Context: "Build 1", State: commitstatus.CommitStatusSuccess}, + {Context: "Build 2", State: commitstatus.CommitStatusSuccess}, + {Context: "Build 2t", State: commitstatus.CommitStatusSuccess}, + }, + requiredContexts: []string{"Build*", "Build 2t*", "Build 3*"}, + expected: commitstatus.CommitStatusPending, + }, + { + commitStatuses: []*git_model.CommitStatus{ + {Context: "Build 1", State: commitstatus.CommitStatusSuccess}, + {Context: "Build 2", State: commitstatus.CommitStatusSuccess}, + {Context: "Build 2t", State: commitstatus.CommitStatusSuccess}, + }, + requiredContexts: []string{"Build*", "Build *", "Build 2t*", "Build 1*"}, + expected: commitstatus.CommitStatusSuccess, }, } - testCasesRequiredContexts := [][]string{ - {"Build*"}, - {"Build*", "Build 2t*"}, - {"Build*", "Build 2t*"}, - {"Build*", "Build 2t*", "Build 3*"}, - {"Build*", "Build *", "Build 2t*", "Build 1*"}, - } - - testCasesExpected := []structs.CommitStatusState{ - structs.CommitStatusSuccess, - structs.CommitStatusPending, - structs.CommitStatusFailure, - structs.CommitStatusPending, - structs.CommitStatusSuccess, - } - - for i, commitStatuses := range testCases { - if MergeRequiredContextsCommitStatus(commitStatuses, testCasesRequiredContexts[i]) != testCasesExpected[i] { - assert.Fail(t, "Test case failed", "Test case %d failed", i+1) - } + for i, c := range cases { + assert.Equal(t, c.expected, MergeRequiredContextsCommitStatus(c.commitStatuses, c.requiredContexts), "case %d", i) } } diff --git a/services/pull/merge.go b/services/pull/merge.go index 256db847ef..a941c20435 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -8,11 +8,13 @@ import ( "context" "errors" "fmt" + "maps" "os" "path/filepath" "regexp" "strconv" "strings" + "unicode" "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" @@ -94,9 +96,7 @@ func getMergeMessage(ctx context.Context, baseGitRepo *git.Repository, pr *issue vars["HeadRepoOwnerName"] = pr.HeadRepo.OwnerName vars["HeadRepoName"] = pr.HeadRepo.Name } - for extraKey, extraValue := range extraVars { - vars[extraKey] = extraValue - } + maps.Copy(vars, extraVars) refs, err := pr.ResolveCrossReferences(ctx) if err == nil { closeIssueIndexes := make([]string, 0, len(refs)) @@ -161,6 +161,41 @@ func GetDefaultMergeMessage(ctx context.Context, baseGitRepo *git.Repository, pr return getMergeMessage(ctx, baseGitRepo, pr, mergeStyle, nil) } +func AddCommitMessageTailer(message, tailerKey, tailerValue string) string { + tailerLine := tailerKey + ": " + tailerValue + message = strings.ReplaceAll(message, "\r\n", "\n") + message = strings.ReplaceAll(message, "\r", "\n") + if strings.Contains(message, "\n"+tailerLine+"\n") || strings.HasSuffix(message, "\n"+tailerLine) { + return message + } + + if !strings.HasSuffix(message, "\n") { + message += "\n" + } + pos1 := strings.LastIndexByte(message[:len(message)-1], '\n') + pos2 := -1 + if pos1 != -1 { + pos2 = strings.IndexByte(message[pos1:], ':') + if pos2 != -1 { + pos2 += pos1 + } + } + var lastLineKey string + if pos1 != -1 && pos2 != -1 { + lastLineKey = message[pos1+1 : pos2] + } + + isLikelyTailerLine := lastLineKey != "" && unicode.IsUpper(rune(lastLineKey[0])) && strings.Contains(message, "-") + for i := 0; isLikelyTailerLine && i < len(lastLineKey); i++ { + r := rune(lastLineKey[i]) + isLikelyTailerLine = unicode.IsLetter(r) || unicode.IsDigit(r) || r == '-' + } + if !strings.HasSuffix(message, "\n\n") && !isLikelyTailerLine { + message += "\n" + } + return message + tailerLine +} + // ErrInvalidMergeStyle represents an error if merging with disabled merge strategy type ErrInvalidMergeStyle struct { ID int64 @@ -290,7 +325,7 @@ func handleCloseCrossReferences(ctx context.Context, pr *issues_model.PullReques } // doMergeAndPush performs the merge operation without changing any pull information in database and pushes it up to the base repository -func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string, pushTrigger repo_module.PushTrigger) (string, error) { //nolint:unparam +func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string, pushTrigger repo_module.PushTrigger) (string, error) { //nolint:unparam // non-error result is never used // Clone base repo. mergeCtx, cancel, err := createTemporaryRepoForMerge(ctx, pr, doer, expectedHeadCommitID) if err != nil { @@ -396,10 +431,13 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use func commitAndSignNoAuthor(ctx *mergeContext, message string) error { cmdCommit := git.NewCommand("commit").AddOptionFormat("--message=%s", message) - if ctx.signKeyID == "" { + if ctx.signKey == nil { cmdCommit.AddArguments("--no-gpg-sign") } else { - cmdCommit.AddOptionFormat("-S%s", ctx.signKeyID) + if ctx.signKey.Format != "" { + cmdCommit.AddConfig("gpg.format", ctx.signKey.Format) + } + cmdCommit.AddOptionFormat("-S%s", ctx.signKey.KeyID) } if err := cmdCommit.Run(ctx, ctx.RunOpts()); err != nil { log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) @@ -649,48 +687,40 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID return false, fmt.Errorf("unable to merge PullRequest[%d], some required fields are empty", pr.Index) } - ctx, committer, err := db.TxContext(ctx) - if err != nil { - return false, err - } - defer committer.Close() - - pr.Issue = nil - if err := pr.LoadIssue(ctx); err != nil { - return false, err - } - - if err := pr.Issue.LoadRepo(ctx); err != nil { - return false, err - } + return db.WithTx2(ctx, func(ctx context.Context) (bool, error) { + pr.Issue = nil + if err := pr.LoadIssue(ctx); err != nil { + return false, err + } - if err := pr.Issue.Repo.LoadOwner(ctx); err != nil { - return false, err - } + if err := pr.Issue.LoadRepo(ctx); err != nil { + return false, err + } - // Removing an auto merge pull and ignore if not exist - if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) { - return false, fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", pr.ID, err) - } + if err := pr.Issue.Repo.LoadOwner(ctx); err != nil { + return false, err + } - // Set issue as closed - if _, err := issues_model.SetIssueAsClosed(ctx, pr.Issue, pr.Merger, true); err != nil { - return false, fmt.Errorf("ChangeIssueStatus: %w", err) - } + // Removing an auto merge pull and ignore if not exist + if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) { + return false, fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", pr.ID, err) + } - // We need to save all of the data used to compute this merge as it may have already been changed by testPullRequestBranchMergeable. FIXME: need to set some state to prevent testPullRequestBranchMergeable from running whilst we are merging. - if cnt, err := db.GetEngine(ctx).Where("id = ?", pr.ID). - And("has_merged = ?", false). - Cols("has_merged, status, merge_base, merged_commit_id, merger_id, merged_unix, conflicted_files"). - Update(pr); err != nil { - return false, fmt.Errorf("failed to update pr[%d]: %w", pr.ID, err) - } else if cnt != 1 { - return false, issues_model.ErrIssueAlreadyChanged - } + // Set issue as closed + if _, err := issues_model.SetIssueAsClosed(ctx, pr.Issue, pr.Merger, true); err != nil { + return false, fmt.Errorf("ChangeIssueStatus: %w", err) + } - if err := committer.Commit(); err != nil { - return false, err - } + // We need to save all of the data used to compute this merge as it may have already been changed by testPullRequestBranchMergeable. FIXME: need to set some state to prevent testPullRequestBranchMergeable from running whilst we are merging. + if cnt, err := db.GetEngine(ctx).Where("id = ?", pr.ID). + And("has_merged = ?", false). + Cols("has_merged, status, merge_base, merged_commit_id, merger_id, merged_unix, conflicted_files"). + Update(pr); err != nil { + return false, fmt.Errorf("failed to update pr[%d]: %w", pr.ID, err) + } else if cnt != 1 { + return false, issues_model.ErrIssueAlreadyChanged + } - return true, nil + return true, nil + }) } diff --git a/services/pull/merge_prepare.go b/services/pull/merge_prepare.go index 719cc6b965..31a1e13734 100644 --- a/services/pull/merge_prepare.go +++ b/services/pull/merge_prepare.go @@ -27,7 +27,7 @@ type mergeContext struct { doer *user_model.User sig *git.Signature committer *git.Signature - signKeyID string // empty for no-sign, non-empty to sign + signKey *git.SigningKey env []string } @@ -99,9 +99,9 @@ func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullReque mergeCtx.committer = mergeCtx.sig // Determine if we should sign - sign, keyID, signer, _ := asymkey_service.SignMerge(ctx, mergeCtx.pr, mergeCtx.doer, mergeCtx.tmpBasePath, "HEAD", trackingBranch) + sign, key, signer, _ := asymkey_service.SignMerge(ctx, mergeCtx.pr, mergeCtx.doer, mergeCtx.tmpBasePath, "HEAD", trackingBranch) if sign { - mergeCtx.signKeyID = keyID + mergeCtx.signKey = key if pr.BaseRepo.GetTrustModel() == repo_model.CommitterTrustModel || pr.BaseRepo.GetTrustModel() == repo_model.CollaboratorCommitterTrustModel { mergeCtx.committer = signer } diff --git a/services/pull/merge_squash.go b/services/pull/merge_squash.go index 72660cd3c5..0049c0b117 100644 --- a/services/pull/merge_squash.go +++ b/services/pull/merge_squash.go @@ -5,7 +5,6 @@ package pull import ( "fmt" - "strings" repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" @@ -66,18 +65,19 @@ func doMergeStyleSquash(ctx *mergeContext, message string) error { if setting.Repository.PullRequest.AddCoCommitterTrailers && ctx.committer.String() != sig.String() { // add trailer - if !strings.Contains(message, "Co-authored-by: "+sig.String()) { - message += "\nCo-authored-by: " + sig.String() - } - message += fmt.Sprintf("\nCo-committed-by: %s\n", sig.String()) + message = AddCommitMessageTailer(message, "Co-authored-by", sig.String()) + message = AddCommitMessageTailer(message, "Co-committed-by", sig.String()) // FIXME: this one should be removed, it is not really used or widely used } cmdCommit := git.NewCommand("commit"). AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email). AddOptionFormat("--message=%s", message) - if ctx.signKeyID == "" { + if ctx.signKey == nil { cmdCommit.AddArguments("--no-gpg-sign") } else { - cmdCommit.AddOptionFormat("-S%s", ctx.signKeyID) + if ctx.signKey.Format != "" { + cmdCommit.AddConfig("gpg.format", ctx.signKey.Format) + } + cmdCommit.AddOptionFormat("-S%s", ctx.signKey.KeyID) } if err := cmdCommit.Run(ctx, ctx.RunOpts()); err != nil { log.Error("git commit %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) diff --git a/services/pull/merge_test.go b/services/pull/merge_test.go index 6df6f55d46..91abeb9d9c 100644 --- a/services/pull/merge_test.go +++ b/services/pull/merge_test.go @@ -65,3 +65,28 @@ func Test_expandDefaultMergeMessage(t *testing.T) { }) } } + +func TestAddCommitMessageTailer(t *testing.T) { + // add tailer for empty message + assert.Equal(t, "\n\nTest-tailer: TestValue", AddCommitMessageTailer("", "Test-tailer", "TestValue")) + + // add tailer for message without newlines + assert.Equal(t, "title\n\nTest-tailer: TestValue", AddCommitMessageTailer("title", "Test-tailer", "TestValue")) + assert.Equal(t, "title\n\nNot tailer: xxx\n\nTest-tailer: TestValue", AddCommitMessageTailer("title\n\nNot tailer: xxx", "Test-tailer", "TestValue")) + assert.Equal(t, "title\n\nNotTailer: xxx\n\nTest-tailer: TestValue", AddCommitMessageTailer("title\n\nNotTailer: xxx", "Test-tailer", "TestValue")) + assert.Equal(t, "title\n\nnot-tailer: xxx\n\nTest-tailer: TestValue", AddCommitMessageTailer("title\n\nnot-tailer: xxx", "Test-tailer", "TestValue")) + + // add tailer for message with one EOL + assert.Equal(t, "title\n\nTest-tailer: TestValue", AddCommitMessageTailer("title\n", "Test-tailer", "TestValue")) + + // add tailer for message with two EOLs + assert.Equal(t, "title\n\nTest-tailer: TestValue", AddCommitMessageTailer("title\n\n", "Test-tailer", "TestValue")) + + // add tailer for message with existing tailer (won't duplicate) + assert.Equal(t, "title\n\nTest-tailer: TestValue", AddCommitMessageTailer("title\n\nTest-tailer: TestValue", "Test-tailer", "TestValue")) + assert.Equal(t, "title\n\nTest-tailer: TestValue\n", AddCommitMessageTailer("title\n\nTest-tailer: TestValue\n", "Test-tailer", "TestValue")) + + // add tailer for message with existing tailer and different value (will append) + assert.Equal(t, "title\n\nTest-tailer: v1\nTest-tailer: v2", AddCommitMessageTailer("title\n\nTest-tailer: v1", "Test-tailer", "v2")) + assert.Equal(t, "title\n\nTest-tailer: v1\nTest-tailer: v2", AddCommitMessageTailer("title\n\nTest-tailer: v1\n", "Test-tailer", "v2")) +} diff --git a/services/pull/patch.go b/services/pull/patch.go index 153e0baf87..9d9b8d0d07 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -41,7 +41,7 @@ func DownloadDiffOrPatch(ctx context.Context, pr *issues_model.PullRequest, w io } defer closer.Close() - compareArg := pr.MergeBase + "..." + pr.GetGitRefName() + compareArg := pr.MergeBase + "..." + pr.GetGitHeadRefName() switch { case patch: err = gitRepo.GetPatch(compareArg, w) diff --git a/services/pull/pull.go b/services/pull/pull.go index 81be797832..2829e15441 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -143,7 +143,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } compareInfo, err := baseGitRepo.GetCompareInfo(pr.BaseRepo.RepoPath(), - git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName(), false, false) + git.BranchPrefix+pr.BaseBranch, pr.GetGitHeadRefName(), false, false) if err != nil { return err } @@ -184,7 +184,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { return nil }); err != nil { // cleanup: this will only remove the reference, the real commit will be clean up when next GC - if err1 := baseGitRepo.RemoveReference(pr.GetGitRefName()); err1 != nil { + if err1 := baseGitRepo.RemoveReference(pr.GetGitHeadRefName()); err1 != nil { log.Error("RemoveReference: %v", err1) } return err @@ -567,7 +567,7 @@ func PushToBaseRepo(ctx context.Context, pr *issues_model.PullRequest) (err erro } func pushToBaseRepoHelper(ctx context.Context, pr *issues_model.PullRequest, prefixHeadBranch string) (err error) { - log.Trace("PushToBaseRepo[%d]: pushing commits to base repo '%s'", pr.BaseRepoID, pr.GetGitRefName()) + log.Trace("PushToBaseRepo[%d]: pushing commits to base repo '%s'", pr.BaseRepoID, pr.GetGitHeadRefName()) if err := pr.LoadHeadRepo(ctx); err != nil { log.Error("Unable to load head repository for PR[%d] Error: %v", pr.ID, err) @@ -588,7 +588,7 @@ func pushToBaseRepoHelper(ctx context.Context, pr *issues_model.PullRequest, pre return fmt.Errorf("unable to load poster %d for pr %d: %w", pr.Issue.PosterID, pr.ID, err) } - gitRefName := pr.GetGitRefName() + gitRefName := pr.GetGitHeadRefName() if err := git.Push(ctx, headRepoPath, git.PushOptions{ Remote: baseRepoPath, @@ -642,13 +642,13 @@ func UpdatePullsRefs(ctx context.Context, repo *repo_model.Repository, update *r // UpdateRef update refs/pull/id/head directly for agit flow pull request func UpdateRef(ctx context.Context, pr *issues_model.PullRequest) (err error) { - log.Trace("UpdateRef[%d]: upgate pull request ref in base repo '%s'", pr.ID, pr.GetGitRefName()) + log.Trace("UpdateRef[%d]: upgate pull request ref in base repo '%s'", pr.ID, pr.GetGitHeadRefName()) if err := pr.LoadBaseRepo(ctx); err != nil { log.Error("Unable to load base repository for PR[%d] Error: %v", pr.ID, err) return err } - _, _, err = git.NewCommand("update-ref").AddDynamicArguments(pr.GetGitRefName(), pr.HeadCommitID).RunStdString(ctx, &git.RunOpts{Dir: pr.BaseRepo.RepoPath()}) + _, _, err = git.NewCommand("update-ref").AddDynamicArguments(pr.GetGitHeadRefName(), pr.HeadCommitID).RunStdString(ctx, &git.RunOpts{Dir: pr.BaseRepo.RepoPath()}) if err != nil { log.Error("Unable to update ref in base repository for PR[%d] Error: %v", pr.ID, err) } @@ -816,9 +816,9 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ if pr.Flow == issues_model.PullRequestFlowGithub { headCommit, err = gitRepo.GetBranchCommit(pr.HeadBranch) } else { - pr.HeadCommitID, err = gitRepo.GetRefCommitID(pr.GetGitRefName()) + pr.HeadCommitID, err = gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) if err != nil { - log.Error("Unable to get head commit: %s Error: %v", pr.GetGitRefName(), err) + log.Error("Unable to get head commit: %s Error: %v", pr.GetGitHeadRefName(), err) return "" } headCommit, err = gitRepo.GetCommit(pr.HeadCommitID) @@ -945,12 +945,6 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ return stringBuilder.String() } -// GetIssuesLastCommitStatus returns a map of issue ID to the most recent commit's latest status -func GetIssuesLastCommitStatus(ctx context.Context, issues issues_model.IssueList) (map[int64]*git_model.CommitStatus, error) { - _, lastStatus, err := GetIssuesAllCommitStatus(ctx, issues) - return lastStatus, err -} - // GetIssuesAllCommitStatus returns a map of issue ID to a list of all statuses for the most recent commit as well as a map of issue ID to only the commit's latest status func GetIssuesAllCommitStatus(ctx context.Context, issues issues_model.IssueList) (map[int64][]*git_model.CommitStatus, map[int64]*git_model.CommitStatus, error) { if err := issues.LoadPullRequests(ctx); err != nil { @@ -999,12 +993,12 @@ func GetIssuesAllCommitStatus(ctx context.Context, issues issues_model.IssueList // getAllCommitStatus get pr's commit statuses. func getAllCommitStatus(ctx context.Context, gitRepo *git.Repository, pr *issues_model.PullRequest) (statuses []*git_model.CommitStatus, lastStatus *git_model.CommitStatus, err error) { - sha, shaErr := gitRepo.GetRefCommitID(pr.GetGitRefName()) + sha, shaErr := gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) if shaErr != nil { return nil, nil, shaErr } - statuses, _, err = git_model.GetLatestCommitStatus(ctx, pr.BaseRepo.ID, sha, db.ListOptionsAll) + statuses, err = git_model.GetLatestCommitStatus(ctx, pr.BaseRepo.ID, sha, db.ListOptionsAll) lastStatus = git_model.CalcCommitStatus(statuses) return statuses, lastStatus, err } @@ -1049,7 +1043,7 @@ func IsHeadEqualWithBranch(ctx context.Context, pr *issues_model.PullRequest, br return false, err } } else { - pr.HeadCommitID, err = baseGitRepo.GetRefCommitID(pr.GetGitRefName()) + pr.HeadCommitID, err = baseGitRepo.GetRefCommitID(pr.GetGitHeadRefName()) if err != nil { return false, err } @@ -1083,7 +1077,7 @@ func GetPullCommits(ctx *gitea_context.Context, issue *issues_model.Issue) ([]Co if pull.HasMerged { baseBranch = pull.MergeBase } - prInfo, err := baseGitRepo.GetCompareInfo(pull.BaseRepo.RepoPath(), baseBranch, pull.GetGitRefName(), true, false) + prInfo, err := baseGitRepo.GetCompareInfo(pull.BaseRepo.RepoPath(), baseBranch, pull.GetGitHeadRefName(), true, false) if err != nil { return nil, "", err } diff --git a/services/pull/review.go b/services/pull/review.go index 5c80e7b338..ee18db3859 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -200,7 +200,7 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo defer closer.Close() invalidated := false - head := pr.GetGitRefName() + head := pr.GetGitHeadRefName() if line > 0 { if reviewID != 0 { first, err := issues_model.FindComments(ctx, &issues_model.FindCommentsOptions{ @@ -237,16 +237,16 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo if err == nil { commitID = commit.ID.String() } else if !(strings.Contains(err.Error(), "exit status 128 - fatal: no such path") || notEnoughLines.MatchString(err.Error())) { - return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %w", pr.GetGitRefName(), gitRepo.Path, treePath, line, err) + return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %w", pr.GetGitHeadRefName(), gitRepo.Path, treePath, line, err) } } } // Only fetch diff if comment is review comment if len(patch) == 0 && reviewID != 0 { - headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName()) + headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) if err != nil { - return nil, fmt.Errorf("GetRefCommitID[%s]: %w", pr.GetGitRefName(), err) + return nil, fmt.Errorf("GetRefCommitID[%s]: %w", pr.GetGitHeadRefName(), err) } if len(commitID) == 0 { commitID = headCommitID @@ -301,7 +301,7 @@ func SubmitReview(ctx context.Context, doer *user_model.User, gitRepo *git.Repos return nil, nil, ErrSubmitReviewOnClosedPR } - headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitRefName()) + headCommitID, err := gitRepo.GetRefCommitID(pr.GetGitHeadRefName()) if err != nil { return nil, nil, err } diff --git a/services/pull/reviewer.go b/services/pull/reviewer.go index bf0d8cb298..52f2f3401c 100644 --- a/services/pull/reviewer.go +++ b/services/pull/reviewer.go @@ -85,5 +85,5 @@ func GetReviewerTeams(ctx context.Context, repo *repo_model.Repository) ([]*orga return nil, nil } - return organization.GetTeamsWithAccessToRepoUnit(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead, unit.TypePullRequests) + return organization.GetTeamsWithAccessToAnyRepoUnit(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead, unit.TypePullRequests) } diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go index 72406482e0..89f150fd92 100644 --- a/services/pull/temp_repo.go +++ b/services/pull/temp_repo.go @@ -174,7 +174,7 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) } else if len(pr.HeadCommitID) == objectFormat.FullLength() { // for not created pull request headBranch = pr.HeadCommitID } else { - headBranch = pr.GetGitRefName() + headBranch = pr.GetGitHeadRefName() } if err := git.NewCommand("fetch").AddArguments(fetchArgs...).AddDynamicArguments(remoteRepoName, headBranch+":"+trackingBranch). Run(ctx, prCtx.RunOpts()); err != nil { diff --git a/services/pull/update.go b/services/pull/update.go index 5cc5e2b134..b8f84e3d65 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -41,22 +41,6 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. return fmt.Errorf("HeadBranch of PR %d is up to date", pr.Index) } - if rebase { - defer func() { - go AddTestPullRequestTask(TestPullRequestOptions{ - RepoID: pr.BaseRepo.ID, - Doer: doer, - Branch: pr.BaseBranch, - IsSync: false, - IsForcePush: false, - OldCommitID: "", - NewCommitID: "", - }) - }() - - return updateHeadByRebaseOnToBase(ctx, pr, doer) - } - if err := pr.LoadBaseRepo(ctx); err != nil { log.Error("unable to load BaseRepo for %-v during update-by-merge: %v", pr, err) return fmt.Errorf("unable to load BaseRepo for PR[%d] during update-by-merge: %w", pr.ID, err) @@ -74,6 +58,22 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. return fmt.Errorf("unable to load HeadRepo for PR[%d] during update-by-merge: %w", pr.ID, err) } + defer func() { + go AddTestPullRequestTask(TestPullRequestOptions{ + RepoID: pr.BaseRepo.ID, + Doer: doer, + Branch: pr.BaseBranch, + IsSync: false, + IsForcePush: false, + OldCommitID: "", + NewCommitID: "", + }) + }() + + if rebase { + return updateHeadByRebaseOnToBase(ctx, pr, doer) + } + // TODO: FakePR: it is somewhat hacky, but it is the only way to "merge" at the moment // ideally in the future the "merge" functions should be refactored to decouple from the PullRequest // now use a fake reverse PR to switch head&base repos/branches @@ -90,19 +90,6 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model. } _, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message, repository.PushTriggerPRUpdateWithBase) - - defer func() { - go AddTestPullRequestTask(TestPullRequestOptions{ - RepoID: reversePR.HeadRepo.ID, - Doer: doer, - Branch: reversePR.HeadBranch, - IsSync: false, - IsForcePush: false, - OldCommitID: "", - NewCommitID: "", - }) - }() - return err } |