diff options
Diffstat (limited to 'services/pull')
| -rw-r--r-- | services/pull/check.go | 153 | ||||
| -rw-r--r-- | services/pull/check_test.go | 7 | ||||
| -rw-r--r-- | services/pull/commit_status.go | 111 | ||||
| -rw-r--r-- | services/pull/commit_status_test.go | 101 | ||||
| -rw-r--r-- | services/pull/merge.go | 126 | ||||
| -rw-r--r-- | services/pull/merge_ff_only.go | 2 | ||||
| -rw-r--r-- | services/pull/merge_merge.go | 2 | ||||
| -rw-r--r-- | services/pull/merge_prepare.go | 34 | ||||
| -rw-r--r-- | services/pull/merge_rebase.go | 12 | ||||
| -rw-r--r-- | services/pull/merge_squash.go | 20 | ||||
| -rw-r--r-- | services/pull/merge_test.go | 25 | ||||
| -rw-r--r-- | services/pull/patch.go | 47 | ||||
| -rw-r--r-- | services/pull/patch_unmerged.go | 4 | ||||
| -rw-r--r-- | services/pull/protected_branch.go | 49 | ||||
| -rw-r--r-- | services/pull/pull.go | 101 | ||||
| -rw-r--r-- | services/pull/review.go | 4 | ||||
| -rw-r--r-- | services/pull/reviewer.go | 2 | ||||
| -rw-r--r-- | services/pull/temp_repo.go | 40 | ||||
| -rw-r--r-- | services/pull/update.go | 32 | ||||
| -rw-r--r-- | services/pull/update_rebase.go | 6 |
20 files changed, 514 insertions, 364 deletions
diff --git a/services/pull/check.go b/services/pull/check.go index e1adc3ca3b..5d8990aa00 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -10,6 +10,7 @@ import ( "fmt" "strconv" "strings" + "time" "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" @@ -25,6 +26,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/queue" + "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" asymkey_service "code.gitea.io/gitea/services/asymkey" notify_service "code.gitea.io/gitea/services/notify" @@ -34,27 +36,88 @@ import ( var prPatchCheckerQueue *queue.WorkerPoolQueue[string] var ( - ErrIsClosed = errors.New("pull is closed") - ErrUserNotAllowedToMerge = ErrDisallowedToMerge{} - ErrHasMerged = errors.New("has already been merged") - ErrIsWorkInProgress = errors.New("work in progress PRs cannot be merged") - ErrIsChecking = errors.New("cannot merge while conflict checking is in progress") - ErrNotMergeableState = errors.New("not in mergeable state") - ErrDependenciesLeft = errors.New("is blocked by an open dependency") + ErrIsClosed = errors.New("pull is closed") + ErrNoPermissionToMerge = errors.New("no permission to merge") + ErrNotReadyToMerge = errors.New("not ready to merge") + ErrHasMerged = errors.New("has already been merged") + ErrIsWorkInProgress = errors.New("work in progress PRs cannot be merged") + ErrIsChecking = errors.New("cannot merge while conflict checking is in progress") + ErrNotMergeableState = errors.New("not in mergeable state") + ErrDependenciesLeft = errors.New("is blocked by an open dependency") ) -// AddToTaskQueue adds itself to pull request test task queue. -func AddToTaskQueue(ctx context.Context, pr *issues_model.PullRequest) { +func markPullRequestStatusAsChecking(ctx context.Context, pr *issues_model.PullRequest) bool { pr.Status = issues_model.PullRequestStatusChecking err := pr.UpdateColsIfNotMerged(ctx, "status") if err != nil { - log.Error("AddToTaskQueue(%-v).UpdateCols.(add to queue): %v", pr, err) + log.Error("UpdateColsIfNotMerged failed, pr: %-v, err: %v", pr, err) + return false + } + pr, err = issues_model.GetPullRequestByID(ctx, pr.ID) + if err != nil { + log.Error("GetPullRequestByID failed, pr: %-v, err: %v", pr, err) + return false + } + return pr.Status == issues_model.PullRequestStatusChecking +} + +var AddPullRequestToCheckQueue = realAddPullRequestToCheckQueue + +func realAddPullRequestToCheckQueue(prID int64) { + err := prPatchCheckerQueue.Push(strconv.FormatInt(prID, 10)) + if err != nil && !errors.Is(err, queue.ErrAlreadyInQueue) { + log.Error("Error adding %v to the pull requests check queue: %v", prID, err) + } +} + +func StartPullRequestCheckImmediately(ctx context.Context, pr *issues_model.PullRequest) { + if !markPullRequestStatusAsChecking(ctx, pr) { return } - log.Trace("Adding %-v to the test pull requests queue", pr) - err = prPatchCheckerQueue.Push(strconv.FormatInt(pr.ID, 10)) - if err != nil && err != queue.ErrAlreadyInQueue { - log.Error("Error adding %-v to the test pull requests queue: %v", pr, err) + AddPullRequestToCheckQueue(pr.ID) +} + +// StartPullRequestCheckDelayable will delay the check if the pull request was not updated recently. +// When the "base" branch gets updated, all PRs targeting that "base" branch need to re-check whether +// they are mergeable. +// When there are too many stale PRs, each "base" branch update will consume a lot of system resources. +// So we can delay the checks for PRs that were not updated recently, only mark their status as +// "checking", and then next time when these PRs are updated or viewed, the real checks will run. +func StartPullRequestCheckDelayable(ctx context.Context, pr *issues_model.PullRequest) { + if !markPullRequestStatusAsChecking(ctx, pr) { + return + } + + if setting.Repository.PullRequest.DelayCheckForInactiveDays >= 0 { + if err := pr.LoadIssue(ctx); err != nil { + return + } + duration := 24 * time.Hour * time.Duration(setting.Repository.PullRequest.DelayCheckForInactiveDays) + if pr.Issue.UpdatedUnix.AddDuration(duration) <= timeutil.TimeStampNow() { + return + } + } + + AddPullRequestToCheckQueue(pr.ID) +} + +func StartPullRequestCheckOnView(ctx context.Context, pr *issues_model.PullRequest) { + // TODO: its correctness totally depends on the "unique queue" feature and the global lock. + // So duplicate "start" requests will be ignored if there is already a task in the queue or one is running. + // Ideally in the future we should decouple the "unique queue" feature from the "start" request. + if pr.Status == issues_model.PullRequestStatusChecking { + if setting.IsInTesting { + // In testing mode, there might be an "immediate" queue, which is not a real queue, everything is executed in the same goroutine + // So we can't use the global lock here, otherwise it will cause a deadlock. + AddPullRequestToCheckQueue(pr.ID) + } else { + // When a PR check starts, the task is popped from the queue and the task handler acquires the global lock + // So we need to acquire the global lock here to prevent from duplicate tasks + _, _ = globallock.TryLockAndDo(ctx, getPullWorkingLockKey(pr.ID), func(ctx context.Context) error { + AddPullRequestToCheckQueue(pr.ID) // the queue is a unique queue and won't add the same task again + return nil + }) + } } } @@ -84,7 +147,7 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc log.Error("Error whilst checking if %-v is allowed to merge %-v: %v", doer, pr, err) return err } else if !allowedMerge { - return ErrUserNotAllowedToMerge + return ErrNoPermissionToMerge } if mergeCheckType == MergeCheckTypeManually { @@ -105,7 +168,7 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc } if err := CheckPullBranchProtections(ctx, pr, false); err != nil { - if !IsErrDisallowedToMerge(err) { + if !errors.Is(err, ErrNotReadyToMerge) { log.Error("Error whilst checking pull branch protection for %-v: %v", pr, err) return err } @@ -172,10 +235,10 @@ func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer return sign, err } -// checkAndUpdateStatus checks if pull request is possible to leaving checking status, +// markPullRequestAsMergeable checks if pull request is possible to leaving checking status, // and set to be either conflict or mergeable. -func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) { - // If status has not been changed to conflict by testPatch then we are 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 pr.Status == issues_model.PullRequestStatusChecking { pr.Status = issues_model.PullRequestStatusMergeable } @@ -206,9 +269,9 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com prHeadRef := pr.GetGitRefName() // Check if the pull request is merged into BaseBranch - if _, _, err := git.NewCommand(ctx, "merge-base", "--is-ancestor"). + if _, _, err := git.NewCommand("merge-base", "--is-ancestor"). AddDynamicArguments(prHeadRef, pr.BaseBranch). - RunStdString(&git.RunOpts{Dir: pr.BaseRepo.RepoPath()}); err != nil { + RunStdString(ctx, &git.RunOpts{Dir: pr.BaseRepo.RepoPath()}); err != nil { if strings.Contains(err.Error(), "exit status 1") { // prHeadRef is not an ancestor of the base branch return nil, nil @@ -234,9 +297,9 @@ func getMergeCommit(ctx context.Context, pr *issues_model.PullRequest) (*git.Com objectFormat := git.ObjectFormatFromName(pr.BaseRepo.ObjectFormatName) // Get the commit from BaseBranch where the pull request got merged - mergeCommit, _, err := git.NewCommand(ctx, "rev-list", "--ancestry-path", "--merges", "--reverse"). - AddDynamicArguments(prHeadCommitID + ".." + pr.BaseBranch). - RunStdString(&git.RunOpts{Dir: pr.BaseRepo.RepoPath()}) + mergeCommit, _, err := git.NewCommand("rev-list", "--ancestry-path", "--merges", "--reverse"). + AddDynamicArguments(prHeadCommitID+".."+pr.BaseBranch). + RunStdString(ctx, &git.RunOpts{Dir: pr.BaseRepo.RepoPath()}) if err != nil { return nil, fmt.Errorf("git rev-list --ancestry-path --merges --reverse: %w", err) } else if len(mergeCommit) < objectFormat.FullLength() { @@ -310,6 +373,10 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { // InitializePullRequests checks and tests untested patches of pull requests. func InitializePullRequests(ctx context.Context) { + // If we prefer to delay the checks, then no need to do any check during startup, there should be not much difference + if setting.Repository.PullRequest.DelayCheckForInactiveDays >= 0 { + return + } prs, err := issues_model.GetPullRequestIDsByCheckStatus(ctx, issues_model.PullRequestStatusChecking) if err != nil { log.Error("Find Checking PRs: %v", err) @@ -320,24 +387,12 @@ func InitializePullRequests(ctx context.Context) { case <-ctx.Done(): return default: - log.Trace("Adding PR[%d] to the pull requests patch checking queue", prID) - if err := prPatchCheckerQueue.Push(strconv.FormatInt(prID, 10)); err != nil { - log.Error("Error adding PR[%d] to the pull requests patch checking queue %v", prID, err) - } + AddPullRequestToCheckQueue(prID) } } } -// handle passed PR IDs and test the PRs -func handler(items ...string) []string { - for _, s := range items { - id, _ := strconv.ParseInt(s, 10, 64) - testPR(id) - } - return nil -} - -func testPR(id int64) { +func checkPullRequestMergeable(id int64) { ctx := graceful.GetManager().HammerContext() releaser, err := globallock.Lock(ctx, getPullWorkingLockKey(id)) if err != nil { @@ -351,7 +406,7 @@ func testPR(id int64) { pr, err := issues_model.GetPullRequestByID(ctx, id) if err != nil { - log.Error("Unable to GetPullRequestByID[%d] for testPR: %v", id, err) + log.Error("Unable to GetPullRequestByID[%d] for checkPullRequestMergeable: %v", id, err) return } @@ -370,15 +425,15 @@ func testPR(id int64) { return } - if err := TestPatch(pr); err != nil { - log.Error("testPatch[%-v]: %v", pr, err) + if err := testPullRequestBranchMergeable(pr); err != nil { + log.Error("testPullRequestTmpRepoBranchMergeable[%-v]: %v", pr, err) pr.Status = issues_model.PullRequestStatusError if err := pr.UpdateCols(ctx, "status"); err != nil { log.Error("update pr [%-v] status to PullRequestStatusError failed: %v", pr, err) } return } - checkAndUpdateStatus(ctx, pr) + markPullRequestAsMergeable(ctx, pr) } // CheckPRsForBaseBranch check all pulls with baseBrannch @@ -387,20 +442,24 @@ func CheckPRsForBaseBranch(ctx context.Context, baseRepo *repo_model.Repository, if err != nil { return err } - for _, pr := range prs { - AddToTaskQueue(ctx, pr) + StartPullRequestCheckImmediately(ctx, pr) } - return nil } // Init runs the task queue to test all the checking status pull requests func Init() error { - prPatchCheckerQueue = queue.CreateUniqueQueue(graceful.GetManager().ShutdownContext(), "pr_patch_checker", handler) + prPatchCheckerQueue = queue.CreateUniqueQueue(graceful.GetManager().ShutdownContext(), "pr_patch_checker", func(items ...string) []string { + for _, s := range items { + id, _ := strconv.ParseInt(s, 10, 64) + checkPullRequestMergeable(id) + } + return nil + }) if prPatchCheckerQueue == nil { - return fmt.Errorf("unable to create pr_patch_checker queue") + return errors.New("unable to create pr_patch_checker queue") } go graceful.GetManager().RunWithCancel(prPatchCheckerQueue) diff --git a/services/pull/check_test.go b/services/pull/check_test.go index dcf5f7b93a..fa3a676ef1 100644 --- a/services/pull/check_test.go +++ b/services/pull/check_test.go @@ -5,7 +5,6 @@ package pull import ( - "context" "strconv" "testing" "time" @@ -33,11 +32,11 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) { cfg, err := setting.GetQueueSettings(setting.CfgProvider, "pr_patch_checker") assert.NoError(t, err) - prPatchCheckerQueue, err = queue.NewWorkerPoolQueueWithContext(context.Background(), "pr_patch_checker", cfg, testHandler, true) + prPatchCheckerQueue, err = queue.NewWorkerPoolQueueWithContext(t.Context(), "pr_patch_checker", cfg, testHandler, true) assert.NoError(t, err) pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) - AddToTaskQueue(db.DefaultContext, pr) + StartPullRequestCheckImmediately(db.DefaultContext, pr) assert.Eventually(t, func() bool { pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) @@ -52,7 +51,7 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) { select { case id := <-idChan: - assert.EqualValues(t, pr.ID, id) + assert.Equal(t, pr.ID, id) case <-time.After(time.Second): assert.FailNow(t, "Timeout: nothing was added to pullRequestQueue") } diff --git a/services/pull/commit_status.go b/services/pull/commit_status.go index aa1ad7cd66..7952ca6fe3 100644 --- a/services/pull/commit_status.go +++ b/services/pull/commit_status.go @@ -10,94 +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/git" + "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 @@ -118,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") @@ -131,10 +96,10 @@ func GetPullRequestCommitStatusState(ctx context.Context, pr *issues_model.PullR } defer closer.Close() - if pr.Flow == issues_model.PullRequestFlowGithub && !headGitRepo.IsBranchExist(pr.HeadBranch) { + 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 && !git.IsReferenceExist(ctx, headGitRepo.Path, pr.GetGitRefName()) { + if pr.Flow == issues_model.PullRequestFlowAGit && !gitrepo.IsReferenceExist(ctx, pr.HeadRepo, pr.GetGitRefName()) { return "", errors.New("Head branch does not exist, can not merge") } @@ -152,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 9c909ef795..cd9aeb2ad1 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -6,12 +6,15 @@ package pull 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" @@ -59,7 +62,7 @@ func getMergeMessage(ctx context.Context, baseGitRepo *git.Repository, pr *issue issueReference = "!" } - reviewedOn := fmt.Sprintf("Reviewed-on: %s", httplib.MakeAbsoluteURL(ctx, pr.Issue.Link())) + reviewedOn := "Reviewed-on: " + httplib.MakeAbsoluteURL(ctx, pr.Issue.Link()) reviewedBy := pr.GetApprovers(ctx) if mergeStyle != "" { @@ -93,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)) @@ -160,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 @@ -211,7 +247,15 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U } defer releaser() defer func() { - go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "") + go AddTestPullRequestTask(TestPullRequestOptions{ + RepoID: pr.BaseRepo.ID, + Doer: doer, + Branch: pr.BaseBranch, + IsSync: false, + IsForcePush: false, + OldCommitID: "", + NewCommitID: "", + }) }() _, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, repo_module.PushTriggerPRMergeToBase) @@ -281,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 { @@ -356,12 +400,12 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use ) mergeCtx.env = append(mergeCtx.env, repo_module.EnvPushTrigger+"="+string(pushTrigger)) - pushCmd := git.NewCommand(ctx, "push", "origin").AddDynamicArguments(baseBranch + ":" + git.BranchPrefix + pr.BaseBranch) + pushCmd := git.NewCommand("push", "origin").AddDynamicArguments(baseBranch + ":" + git.BranchPrefix + pr.BaseBranch) // Push back to upstream. // This cause an api call to "/api/internal/hook/post-receive/...", // If it's merge, all db transaction and operations should be there but not here to prevent deadlock. - if err := pushCmd.Run(mergeCtx.RunOpts()); err != nil { + if err := pushCmd.Run(ctx, mergeCtx.RunOpts()); err != nil { if strings.Contains(mergeCtx.errbuf.String(), "non-fast-forward") { return "", &git.ErrPushOutOfDate{ StdOut: mergeCtx.outbuf.String(), @@ -386,13 +430,16 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use } func commitAndSignNoAuthor(ctx *mergeContext, message string) error { - cmdCommit := git.NewCommand(ctx, "commit").AddOptionFormat("--message=%s", message) - if ctx.signKeyID == "" { + cmdCommit := git.NewCommand("commit").AddOptionFormat("--message=%s", message) + 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.RunOpts()); err != nil { + 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()) return fmt.Errorf("git commit %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) } @@ -453,7 +500,7 @@ func (err ErrMergeDivergingFastForwardOnly) Error() string { } func runMergeCommand(ctx *mergeContext, mergeStyle repo_model.MergeStyle, cmd *git.Command) error { - if err := cmd.Run(ctx.RunOpts()); err != nil { + if err := cmd.Run(ctx, ctx.RunOpts()); err != nil { // Merge will leave a MERGE_HEAD file in the .git folder if there is a conflict if _, statErr := os.Stat(filepath.Join(ctx.tmpBasePath, ".git", "MERGE_HEAD")); statErr == nil { // We have a merge conflict error @@ -509,25 +556,6 @@ func IsUserAllowedToMerge(ctx context.Context, pr *issues_model.PullRequest, p a return false, nil } -// ErrDisallowedToMerge represents an error that a branch is protected and the current user is not allowed to modify it. -type ErrDisallowedToMerge struct { - Reason string -} - -// IsErrDisallowedToMerge checks if an error is an ErrDisallowedToMerge. -func IsErrDisallowedToMerge(err error) bool { - _, ok := err.(ErrDisallowedToMerge) - return ok -} - -func (err ErrDisallowedToMerge) Error() string { - return fmt.Sprintf("not allowed to merge [reason: %s]", err.Reason) -} - -func (err ErrDisallowedToMerge) Unwrap() error { - return util.ErrPermissionDenied -} - // CheckPullBranchProtections checks whether the PR is ready to be merged (reviews and status checks) func CheckPullBranchProtections(ctx context.Context, pr *issues_model.PullRequest, skipProtectedFilesCheck bool) (err error) { if err = pr.LoadBaseRepo(ctx); err != nil { @@ -547,31 +575,21 @@ func CheckPullBranchProtections(ctx context.Context, pr *issues_model.PullReques return err } if !isPass { - return ErrDisallowedToMerge{ - Reason: "Not all required status checks successful", - } + return util.ErrorWrap(ErrNotReadyToMerge, "Not all required status checks successful") } if !issues_model.HasEnoughApprovals(ctx, pb, pr) { - return ErrDisallowedToMerge{ - Reason: "Does not have enough approvals", - } + return util.ErrorWrap(ErrNotReadyToMerge, "Does not have enough approvals") } if issues_model.MergeBlockedByRejectedReview(ctx, pb, pr) { - return ErrDisallowedToMerge{ - Reason: "There are requested changes", - } + return util.ErrorWrap(ErrNotReadyToMerge, "There are requested changes") } if issues_model.MergeBlockedByOfficialReviewRequests(ctx, pb, pr) { - return ErrDisallowedToMerge{ - Reason: "There are official review requests", - } + return util.ErrorWrap(ErrNotReadyToMerge, "There are official review requests") } if issues_model.MergeBlockedByOutdatedBranch(pb, pr) { - return ErrDisallowedToMerge{ - Reason: "The head branch is behind the base branch", - } + return util.ErrorWrap(ErrNotReadyToMerge, "The head branch is behind the base branch") } if skipProtectedFilesCheck { @@ -579,9 +597,7 @@ func CheckPullBranchProtections(ctx context.Context, pr *issues_model.PullReques } if pb.MergeBlockedByProtectedFiles(pr.ChangedProtectedFiles) { - return ErrDisallowedToMerge{ - Reason: "Changed protected files", - } + return util.ErrorWrap(ErrNotReadyToMerge, "Changed protected files") } return nil @@ -613,13 +629,13 @@ func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *use objectFormat := git.ObjectFormatFromName(pr.BaseRepo.ObjectFormatName) if len(commitID) != objectFormat.FullLength() { - return fmt.Errorf("Wrong commit ID") + return errors.New("Wrong commit ID") } commit, err := baseGitRepo.GetCommit(commitID) if err != nil { if git.IsErrNotExist(err) { - return fmt.Errorf("Wrong commit ID") + return errors.New("Wrong commit ID") } return err } @@ -630,14 +646,14 @@ func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *use return err } if !ok { - return fmt.Errorf("Wrong commit ID") + return errors.New("Wrong commit ID") } var merged bool if merged, err = SetMerged(ctx, pr, commitID, timeutil.TimeStamp(commit.Author.When.Unix()), doer, issues_model.PullRequestStatusManuallyMerged); err != nil { return err } else if !merged { - return fmt.Errorf("SetMerged failed") + return errors.New("SetMerged failed") } return nil }) @@ -700,7 +716,7 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID return false, fmt.Errorf("ChangeIssueStatus: %w", err) } - // We need to save all of the data used to compute this merge as it may have already been changed by TestPatch. FIXME: need to set some state to prevent TestPatch from running whilst we are merging. + // 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"). diff --git a/services/pull/merge_ff_only.go b/services/pull/merge_ff_only.go index f57c732104..6c3a68b95b 100644 --- a/services/pull/merge_ff_only.go +++ b/services/pull/merge_ff_only.go @@ -11,7 +11,7 @@ import ( // doMergeStyleFastForwardOnly merges the tracking into the current HEAD - which is assumed to be staging branch (equal to the pr.BaseBranch) func doMergeStyleFastForwardOnly(ctx *mergeContext) error { - cmd := git.NewCommand(ctx, "merge", "--ff-only").AddDynamicArguments(trackingBranch) + cmd := git.NewCommand("merge", "--ff-only").AddDynamicArguments(trackingBranch) if err := runMergeCommand(ctx, repo_model.MergeStyleFastForwardOnly, cmd); err != nil { log.Error("%-v Unable to merge tracking into base: %v", ctx.pr, err) return err diff --git a/services/pull/merge_merge.go b/services/pull/merge_merge.go index bf56c071db..118d21c7cd 100644 --- a/services/pull/merge_merge.go +++ b/services/pull/merge_merge.go @@ -11,7 +11,7 @@ import ( // doMergeStyleMerge merges the tracking branch into the current HEAD - which is assumed to be the staging branch (equal to the pr.BaseBranch) func doMergeStyleMerge(ctx *mergeContext, message string) error { - cmd := git.NewCommand(ctx, "merge", "--no-ff", "--no-commit").AddDynamicArguments(trackingBranch) + cmd := git.NewCommand("merge", "--no-ff", "--no-commit").AddDynamicArguments(trackingBranch) if err := runMergeCommand(ctx, repo_model.MergeStyleMerge, cmd); err != nil { log.Error("%-v Unable to merge tracking into base: %v", ctx.pr, err) return err diff --git a/services/pull/merge_prepare.go b/services/pull/merge_prepare.go index 2e1cc8cf85..31a1e13734 100644 --- a/services/pull/merge_prepare.go +++ b/services/pull/merge_prepare.go @@ -23,11 +23,11 @@ import ( ) type mergeContext struct { - *prContext + *prTmpRepoContext 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 } @@ -68,12 +68,12 @@ func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullReque } mergeCtx = &mergeContext{ - prContext: prCtx, - doer: doer, + prTmpRepoContext: prCtx, + doer: doer, } if expectedHeadCommitID != "" { - trackingCommitID, _, err := git.NewCommand(ctx, "show-ref", "--hash").AddDynamicArguments(git.BranchPrefix + trackingBranch).RunStdString(&git.RunOpts{Dir: mergeCtx.tmpBasePath}) + trackingCommitID, _, err := git.NewCommand("show-ref", "--hash").AddDynamicArguments(git.BranchPrefix+trackingBranch).RunStdString(ctx, &git.RunOpts{Dir: mergeCtx.tmpBasePath}) if err != nil { defer cancel() log.Error("failed to get sha of head branch in %-v: show-ref[%s] --hash refs/heads/tracking: %v", mergeCtx.pr, mergeCtx.tmpBasePath, err) @@ -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 } @@ -151,8 +151,8 @@ func prepareTemporaryRepoForMerge(ctx *mergeContext) error { } setConfig := func(key, value string) error { - if err := git.NewCommand(ctx, "config", "--local").AddDynamicArguments(key, value). - Run(ctx.RunOpts()); err != nil { + if err := git.NewCommand("config", "--local").AddDynamicArguments(key, value). + Run(ctx, ctx.RunOpts()); err != nil { log.Error("git config [%s -> %q]: %v\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String()) return fmt.Errorf("git config [%s -> %q]: %w\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String()) } @@ -184,8 +184,8 @@ func prepareTemporaryRepoForMerge(ctx *mergeContext) error { } // Read base branch index - if err := git.NewCommand(ctx, "read-tree", "HEAD"). - Run(ctx.RunOpts()); err != nil { + if err := git.NewCommand("read-tree", "HEAD"). + Run(ctx, ctx.RunOpts()); err != nil { log.Error("git read-tree HEAD: %v\n%s\n%s", err, ctx.outbuf.String(), ctx.errbuf.String()) return fmt.Errorf("Unable to read base branch in to the index: %w\n%s\n%s", err, ctx.outbuf.String(), ctx.errbuf.String()) } @@ -221,8 +221,8 @@ func getDiffTree(ctx context.Context, repoPath, baseBranch, headBranch string, o return 0, nil, nil } - err = git.NewCommand(ctx, "diff-tree", "--no-commit-id", "--name-only", "-r", "-r", "-z", "--root").AddDynamicArguments(baseBranch, headBranch). - Run(&git.RunOpts{ + err = git.NewCommand("diff-tree", "--no-commit-id", "--name-only", "-r", "-r", "-z", "--root").AddDynamicArguments(baseBranch, headBranch). + Run(ctx, &git.RunOpts{ Dir: repoPath, Stdout: diffOutWriter, PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error { @@ -272,16 +272,16 @@ func (err ErrRebaseConflicts) Error() string { // if there is a conflict it will return an ErrRebaseConflicts func rebaseTrackingOnToBase(ctx *mergeContext, mergeStyle repo_model.MergeStyle) error { // Checkout head branch - if err := git.NewCommand(ctx, "checkout", "-b").AddDynamicArguments(stagingBranch, trackingBranch). - Run(ctx.RunOpts()); err != nil { + if err := git.NewCommand("checkout", "-b").AddDynamicArguments(stagingBranch, trackingBranch). + Run(ctx, ctx.RunOpts()); err != nil { return fmt.Errorf("unable to git checkout tracking as staging in temp repo for %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) } ctx.outbuf.Reset() ctx.errbuf.Reset() // Rebase before merging - if err := git.NewCommand(ctx, "rebase").AddDynamicArguments(baseBranch). - Run(ctx.RunOpts()); err != nil { + if err := git.NewCommand("rebase").AddDynamicArguments(baseBranch). + Run(ctx, ctx.RunOpts()); err != nil { // Rebase will leave a REBASE_HEAD file in .git if there is a conflict if _, statErr := os.Stat(filepath.Join(ctx.tmpBasePath, ".git", "REBASE_HEAD")); statErr == nil { var commitSha string diff --git a/services/pull/merge_rebase.go b/services/pull/merge_rebase.go index ecf376220e..dd7c8761f0 100644 --- a/services/pull/merge_rebase.go +++ b/services/pull/merge_rebase.go @@ -16,7 +16,7 @@ import ( // getRebaseAmendMessage composes the message to amend commits in rebase merge of a pull request. func getRebaseAmendMessage(ctx *mergeContext, baseGitRepo *git.Repository) (message string, err error) { // Get existing commit message. - commitMessage, _, err := git.NewCommand(ctx, "show", "--format=%B", "-s").RunStdString(&git.RunOpts{Dir: ctx.tmpBasePath}) + commitMessage, _, err := git.NewCommand("show", "--format=%B", "-s").RunStdString(ctx, &git.RunOpts{Dir: ctx.tmpBasePath}) if err != nil { return "", err } @@ -42,7 +42,7 @@ func doMergeRebaseFastForward(ctx *mergeContext) error { return fmt.Errorf("Failed to get full commit id for HEAD: %w", err) } - cmd := git.NewCommand(ctx, "merge", "--ff-only").AddDynamicArguments(stagingBranch) + cmd := git.NewCommand("merge", "--ff-only").AddDynamicArguments(stagingBranch) if err := runMergeCommand(ctx, repo_model.MergeStyleRebase, cmd); err != nil { log.Error("Unable to merge staging into base: %v", err) return err @@ -73,7 +73,7 @@ func doMergeRebaseFastForward(ctx *mergeContext) error { } if newMessage != "" { - if err := git.NewCommand(ctx, "commit", "--amend").AddOptionFormat("--message=%s", newMessage).Run(&git.RunOpts{Dir: ctx.tmpBasePath}); err != nil { + if err := git.NewCommand("commit", "--amend").AddOptionFormat("--message=%s", newMessage).Run(ctx, &git.RunOpts{Dir: ctx.tmpBasePath}); err != nil { log.Error("Unable to amend commit message: %v", err) return err } @@ -84,7 +84,7 @@ func doMergeRebaseFastForward(ctx *mergeContext) error { // Perform rebase merge with merge commit. func doMergeRebaseMergeCommit(ctx *mergeContext, message string) error { - cmd := git.NewCommand(ctx, "merge").AddArguments("--no-ff", "--no-commit").AddDynamicArguments(stagingBranch) + cmd := git.NewCommand("merge").AddArguments("--no-ff", "--no-commit").AddDynamicArguments(stagingBranch) if err := runMergeCommand(ctx, repo_model.MergeStyleRebaseMerge, cmd); err != nil { log.Error("Unable to merge staging into base: %v", err) @@ -105,8 +105,8 @@ func doMergeStyleRebase(ctx *mergeContext, mergeStyle repo_model.MergeStyle, mes } // Checkout base branch again - if err := git.NewCommand(ctx, "checkout").AddDynamicArguments(baseBranch). - Run(ctx.RunOpts()); err != nil { + if err := git.NewCommand("checkout").AddDynamicArguments(baseBranch). + Run(ctx, ctx.RunOpts()); err != nil { log.Error("git checkout base prior to merge post staging rebase %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) return fmt.Errorf("git checkout base prior to merge post staging rebase %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String()) } diff --git a/services/pull/merge_squash.go b/services/pull/merge_squash.go index 7258671888..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" @@ -58,7 +57,7 @@ func doMergeStyleSquash(ctx *mergeContext, message string) error { return fmt.Errorf("getAuthorSignatureSquash: %w", err) } - cmdMerge := git.NewCommand(ctx, "merge", "--squash").AddDynamicArguments(trackingBranch) + cmdMerge := git.NewCommand("merge", "--squash").AddDynamicArguments(trackingBranch) if err := runMergeCommand(ctx, repo_model.MergeStyleSquash, cmdMerge); err != nil { log.Error("%-v Unable to merge --squash tracking into base: %v", ctx.pr, err) return err @@ -66,20 +65,21 @@ func doMergeStyleSquash(ctx *mergeContext, message string) error { if setting.Repository.PullRequest.AddCoCommitterTrailers && ctx.committer.String() != sig.String() { // add trailer - if !strings.Contains(message, fmt.Sprintf("Co-authored-by: %s", sig.String())) { - message += fmt.Sprintf("\nCo-authored-by: %s", 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(ctx, "commit"). + 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.RunOpts()); err != nil { + 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()) return fmt.Errorf("git commit [%s:%s -> %s:%s]: %w\n%s\n%s", ctx.pr.HeadRepo.FullName(), ctx.pr.HeadBranch, ctx.pr.BaseRepo.FullName(), ctx.pr.BaseBranch, 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 13623d73c6..153e0baf87 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -67,9 +67,8 @@ var patchErrorSuffices = []string{ ": does not exist in index", } -// TestPatch will test whether a simple patch will apply -func TestPatch(pr *issues_model.PullRequest) error { - ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("TestPatch: %s", pr)) +func testPullRequestBranchMergeable(pr *issues_model.PullRequest) error { + ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("testPullRequestBranchMergeable: %s", pr)) defer finished() prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr) @@ -81,10 +80,10 @@ func TestPatch(pr *issues_model.PullRequest) error { } defer cancel() - return testPatch(ctx, prCtx, pr) + return testPullRequestTmpRepoBranchMergeable(ctx, prCtx, pr) } -func testPatch(ctx context.Context, prCtx *prContext, pr *issues_model.PullRequest) error { +func testPullRequestTmpRepoBranchMergeable(ctx context.Context, prCtx *prTmpRepoContext, pr *issues_model.PullRequest) error { gitRepo, err := git.OpenRepository(ctx, prCtx.tmpBasePath) if err != nil { return fmt.Errorf("OpenRepository: %w", err) @@ -92,7 +91,7 @@ func testPatch(ctx context.Context, prCtx *prContext, pr *issues_model.PullReque defer gitRepo.Close() // 1. update merge base - pr.MergeBase, _, err = git.NewCommand(ctx, "merge-base", "--", "base", "tracking").RunStdString(&git.RunOpts{Dir: prCtx.tmpBasePath}) + pr.MergeBase, _, err = git.NewCommand("merge-base", "--", "base", "tracking").RunStdString(ctx, &git.RunOpts{Dir: prCtx.tmpBasePath}) if err != nil { var err2 error pr.MergeBase, err2 = gitRepo.GetRefCommitID(git.BranchPrefix + "base") @@ -134,7 +133,7 @@ type errMergeConflict struct { } func (e *errMergeConflict) Error() string { - return fmt.Sprintf("conflict detected at: %s", e.filename) + return "conflict detected at: " + e.filename } func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, filesToRemove *[]string, filesToAdd *[]git.IndexObjectInfo) error { @@ -192,7 +191,7 @@ func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, f } // Need to get the objects from the object db to attempt to merge - root, _, err := git.NewCommand(ctx, "unpack-file").AddDynamicArguments(file.stage1.sha).RunStdString(&git.RunOpts{Dir: tmpBasePath}) + root, _, err := git.NewCommand("unpack-file").AddDynamicArguments(file.stage1.sha).RunStdString(ctx, &git.RunOpts{Dir: tmpBasePath}) if err != nil { return fmt.Errorf("unable to get root object: %s at path: %s for merging. Error: %w", file.stage1.sha, file.stage1.path, err) } @@ -201,7 +200,7 @@ func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, f _ = util.Remove(filepath.Join(tmpBasePath, root)) }() - base, _, err := git.NewCommand(ctx, "unpack-file").AddDynamicArguments(file.stage2.sha).RunStdString(&git.RunOpts{Dir: tmpBasePath}) + base, _, err := git.NewCommand("unpack-file").AddDynamicArguments(file.stage2.sha).RunStdString(ctx, &git.RunOpts{Dir: tmpBasePath}) if err != nil { return fmt.Errorf("unable to get base object: %s at path: %s for merging. Error: %w", file.stage2.sha, file.stage2.path, err) } @@ -209,7 +208,7 @@ func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, f defer func() { _ = util.Remove(base) }() - head, _, err := git.NewCommand(ctx, "unpack-file").AddDynamicArguments(file.stage3.sha).RunStdString(&git.RunOpts{Dir: tmpBasePath}) + head, _, err := git.NewCommand("unpack-file").AddDynamicArguments(file.stage3.sha).RunStdString(ctx, &git.RunOpts{Dir: tmpBasePath}) if err != nil { return fmt.Errorf("unable to get head object:%s at path: %s for merging. Error: %w", file.stage3.sha, file.stage3.path, err) } @@ -219,13 +218,13 @@ func attemptMerge(ctx context.Context, file *unmergedFile, tmpBasePath string, f }() // now git merge-file annoyingly takes a different order to the merge-tree ... - _, _, conflictErr := git.NewCommand(ctx, "merge-file").AddDynamicArguments(base, root, head).RunStdString(&git.RunOpts{Dir: tmpBasePath}) + _, _, conflictErr := git.NewCommand("merge-file").AddDynamicArguments(base, root, head).RunStdString(ctx, &git.RunOpts{Dir: tmpBasePath}) if conflictErr != nil { return &errMergeConflict{file.stage2.path} } // base now contains the merged data - hash, _, err := git.NewCommand(ctx, "hash-object", "-w", "--path").AddDynamicArguments(file.stage2.path, base).RunStdString(&git.RunOpts{Dir: tmpBasePath}) + hash, _, err := git.NewCommand("hash-object", "-w", "--path").AddDynamicArguments(file.stage2.path, base).RunStdString(ctx, &git.RunOpts{Dir: tmpBasePath}) if err != nil { return err } @@ -250,7 +249,7 @@ func AttemptThreeWayMerge(ctx context.Context, gitPath string, gitRepo *git.Repo defer cancel() // First we use read-tree to do a simple three-way merge - if _, _, err := git.NewCommand(ctx, "read-tree", "-m").AddDynamicArguments(base, ours, theirs).RunStdString(&git.RunOpts{Dir: gitPath}); err != nil { + if _, _, err := git.NewCommand("read-tree", "-m").AddDynamicArguments(base, ours, theirs).RunStdString(ctx, &git.RunOpts{Dir: gitPath}); err != nil { log.Error("Unable to run read-tree -m! Error: %v", err) return false, nil, fmt.Errorf("unable to run read-tree -m! Error: %w", err) } @@ -324,9 +323,9 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * // No conflicts detected so we need to check if the patch is empty... // a. Write the newly merged tree and check the new tree-hash var treeHash string - treeHash, _, err = git.NewCommand(ctx, "write-tree").RunStdString(&git.RunOpts{Dir: tmpBasePath}) + treeHash, _, err = git.NewCommand("write-tree").RunStdString(ctx, &git.RunOpts{Dir: tmpBasePath}) if err != nil { - lsfiles, _, _ := git.NewCommand(ctx, "ls-files", "-u").RunStdString(&git.RunOpts{Dir: tmpBasePath}) + lsfiles, _, _ := git.NewCommand("ls-files", "-u").RunStdString(ctx, &git.RunOpts{Dir: tmpBasePath}) return false, fmt.Errorf("unable to write unconflicted tree: %w\n`git ls-files -u`:\n%s", err, lsfiles) } treeHash = strings.TrimSpace(treeHash) @@ -355,23 +354,19 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * } // 3b. Create a plain patch from head to base - tmpPatchFile, err := os.CreateTemp("", "patch") + tmpPatchFile, cleanup, err := setting.AppDataTempDir("git-repo-content").CreateTempFileRandom("patch") if err != nil { log.Error("Unable to create temporary patch file! Error: %v", err) return false, fmt.Errorf("unable to create temporary patch file! Error: %w", err) } - defer func() { - _ = util.Remove(tmpPatchFile.Name()) - }() + defer cleanup() if err := gitRepo.GetDiffBinary(pr.MergeBase+"...tracking", tmpPatchFile); err != nil { - tmpPatchFile.Close() log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) return false, fmt.Errorf("unable to get patch file from %s to %s in %s Error: %w", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err) } stat, err := tmpPatchFile.Stat() if err != nil { - tmpPatchFile.Close() return false, fmt.Errorf("unable to stat patch file: %w", err) } patchPath := tmpPatchFile.Name() @@ -384,10 +379,10 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * return false, nil } - log.Trace("PullRequest[%d].testPatch (patchPath): %s", pr.ID, patchPath) + log.Trace("PullRequest[%d].testPullRequestTmpRepoBranchMergeable (patchPath): %s", pr.ID, patchPath) // 4. Read the base branch in to the index of the temporary repository - _, _, err = git.NewCommand(gitRepo.Ctx, "read-tree", "base").RunStdString(&git.RunOpts{Dir: tmpBasePath}) + _, _, err = git.NewCommand("read-tree", "base").RunStdString(gitRepo.Ctx, &git.RunOpts{Dir: tmpBasePath}) if err != nil { return false, fmt.Errorf("git read-tree %s: %w", pr.BaseBranch, err) } @@ -400,7 +395,7 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * prConfig := prUnit.PullRequestsConfig() // 6. Prepare the arguments to apply the patch against the index - cmdApply := git.NewCommand(gitRepo.Ctx, "apply", "--check", "--cached") + cmdApply := git.NewCommand("apply", "--check", "--cached") if prConfig.IgnoreWhitespaceConflicts { cmdApply.AddArguments("--ignore-whitespace") } @@ -431,7 +426,7 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * // 8. Run the check command conflict = false - err = cmdApply.Run(&git.RunOpts{ + err = cmdApply.Run(gitRepo.Ctx, &git.RunOpts{ Dir: tmpBasePath, Stderr: stderrWriter, PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error { @@ -454,7 +449,7 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * scanner := bufio.NewScanner(stderrReader) for scanner.Scan() { line := scanner.Text() - log.Trace("PullRequest[%d].testPatch: stderr: %s", pr.ID, line) + log.Trace("PullRequest[%d].testPullRequestTmpRepoBranchMergeable: stderr: %s", pr.ID, line) if strings.HasPrefix(line, prefix) { conflict = true filepath := strings.TrimSpace(strings.Split(line[len(prefix):], ":")[0]) diff --git a/services/pull/patch_unmerged.go b/services/pull/patch_unmerged.go index c60c48d923..200d2233e9 100644 --- a/services/pull/patch_unmerged.go +++ b/services/pull/patch_unmerged.go @@ -72,8 +72,8 @@ func readUnmergedLsFileLines(ctx context.Context, tmpBasePath string, outputChan }() stderr := &strings.Builder{} - err = git.NewCommand(ctx, "ls-files", "-u", "-z"). - Run(&git.RunOpts{ + err = git.NewCommand("ls-files", "-u", "-z"). + Run(ctx, &git.RunOpts{ Dir: tmpBasePath, Stdout: lsFilesWriter, Stderr: stderr, diff --git a/services/pull/protected_branch.go b/services/pull/protected_branch.go new file mode 100644 index 0000000000..181bd32f44 --- /dev/null +++ b/services/pull/protected_branch.go @@ -0,0 +1,49 @@ +// Copyright 2025 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package pull + +import ( + "context" + + git_model "code.gitea.io/gitea/models/git" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/modules/gitrepo" +) + +func CreateOrUpdateProtectedBranch(ctx context.Context, repo *repo_model.Repository, + protectBranch *git_model.ProtectedBranch, whitelistOptions git_model.WhitelistOptions, +) error { + err := git_model.UpdateProtectBranch(ctx, repo, protectBranch, whitelistOptions) + if err != nil { + return err + } + + isPlainRule := !git_model.IsRuleNameSpecial(protectBranch.RuleName) + var isBranchExist bool + if isPlainRule { + // TODO: read the database directly to check if the branch exists + isBranchExist = gitrepo.IsBranchExist(ctx, repo, protectBranch.RuleName) + } + + if isBranchExist { + if err := CheckPRsForBaseBranch(ctx, repo, protectBranch.RuleName); err != nil { + return err + } + } else { + if !isPlainRule { + // FIXME: since we only need to recheck files protected rules, we could improve this + matchedBranches, err := git_model.FindAllMatchedBranches(ctx, repo.ID, protectBranch.RuleName) + if err != nil { + return err + } + for _, branchName := range matchedBranches { + if err = CheckPRsForBaseBranch(ctx, repo, branchName); err != nil { + return err + } + } + } + } + + return nil +} diff --git a/services/pull/pull.go b/services/pull/pull.go index 5d3758eca6..701c4f4d32 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -96,7 +96,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } defer cancel() - if err := testPatch(ctx, prCtx, pr); err != nil { + if err := testPullRequestTmpRepoBranchMergeable(ctx, prCtx, pr); err != nil { return err } @@ -176,7 +176,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } if !pr.IsWorkInProgress(ctx) { - reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(ctx, issue, pr) + reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(ctx, pr) if err != nil { return err } @@ -314,12 +314,12 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer pr.BaseBranch = targetBranch // Refresh patch - if err := TestPatch(pr); err != nil { + if err := testPullRequestBranchMergeable(pr); err != nil { return err } // Update target branch, PR diff and status - // This is the same as checkAndUpdateStatus in check service, but also updates base_branch + // This is the same as markPullRequestAsMergeable in check service, but also updates base_branch if pr.Status == issues_model.PullRequestStatusChecking { pr.Status = issues_model.PullRequestStatusMergeable } @@ -372,19 +372,29 @@ func checkForInvalidation(ctx context.Context, requests issues_model.PullRequest return nil } +type TestPullRequestOptions struct { + RepoID int64 + Doer *user_model.User + Branch string + IsSync bool // True means it's a pull request synchronization, false means it's triggered for pull request merging or updating + IsForcePush bool + OldCommitID string + NewCommitID string +} + // AddTestPullRequestTask adds new test tasks by given head/base repository and head/base branch, // and generate new patch for testing as needed. -func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, isSync bool, oldCommitID, newCommitID string) { - log.Trace("AddTestPullRequestTask [head_repo_id: %d, head_branch: %s]: finding pull requests", repoID, branch) +func AddTestPullRequestTask(opts TestPullRequestOptions) { + log.Trace("AddTestPullRequestTask [head_repo_id: %d, head_branch: %s]: finding pull requests", opts.RepoID, opts.Branch) graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) { // There is no sensible way to shut this down ":-(" // If you don't let it run all the way then you will lose data // TODO: graceful: AddTestPullRequestTask needs to become a queue! // GetUnmergedPullRequestsByHeadInfo() only return open and unmerged PR. - prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, repoID, branch) + prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, opts.RepoID, opts.Branch) if err != nil { - log.Error("Find pull requests [head_repo_id: %d, head_branch: %s]: %v", repoID, branch, err) + log.Error("Find pull requests [head_repo_id: %d, head_branch: %s]: %v", opts.RepoID, opts.Branch, err) return } @@ -399,26 +409,25 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, continue } - AddToTaskQueue(ctx, pr) - comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID) + StartPullRequestCheckImmediately(ctx, pr) + comment, err := CreatePushPullComment(ctx, opts.Doer, pr, opts.OldCommitID, opts.NewCommitID) if err == nil && comment != nil { - notify_service.PullRequestPushCommits(ctx, doer, pr, comment) + notify_service.PullRequestPushCommits(ctx, opts.Doer, pr, comment) } } - if isSync { - requests := issues_model.PullRequestList(prs) - if err = requests.LoadAttributes(ctx); err != nil { + if opts.IsSync { + if err = prs.LoadAttributes(ctx); err != nil { log.Error("PullRequestList.LoadAttributes: %v", err) } - if invalidationErr := checkForInvalidation(ctx, requests, repoID, doer, branch); invalidationErr != nil { + if invalidationErr := checkForInvalidation(ctx, prs, opts.RepoID, opts.Doer, opts.Branch); invalidationErr != nil { log.Error("checkForInvalidation: %v", invalidationErr) } if err == nil { for _, pr := range prs { objectFormat := git.ObjectFormatFromName(pr.BaseRepo.ObjectFormatName) - if newCommitID != "" && newCommitID != objectFormat.EmptyObjectID().String() { - changed, err := checkIfPRContentChanged(ctx, pr, oldCommitID, newCommitID) + if opts.NewCommitID != "" && opts.NewCommitID != objectFormat.EmptyObjectID().String() { + changed, err := checkIfPRContentChanged(ctx, pr, opts.OldCommitID, opts.NewCommitID) if err != nil { log.Error("checkIfPRContentChanged: %v", err) } @@ -434,12 +443,12 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, log.Error("GetFirstMatchProtectedBranchRule: %v", err) } if pb != nil && pb.DismissStaleApprovals { - if err := DismissApprovalReviews(ctx, doer, pr); err != nil { + if err := DismissApprovalReviews(ctx, opts.Doer, pr); err != nil { log.Error("DismissApprovalReviews: %v", err) } } } - if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, newCommitID); err != nil { + if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, opts.NewCommitID); err != nil { log.Error("MarkReviewsAsNotStale: %v", err) } divergence, err := GetDiverging(ctx, pr) @@ -453,21 +462,31 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, } } - notify_service.PullRequestSynchronized(ctx, doer, pr) + if !pr.IsWorkInProgress(ctx) { + reviewNotifiers, err := issue_service.PullRequestCodeOwnersReview(ctx, pr) + if err != nil { + log.Error("PullRequestCodeOwnersReview: %v", err) + } + if len(reviewNotifiers) > 0 { + issue_service.ReviewRequestNotify(ctx, pr.Issue, opts.Doer, reviewNotifiers) + } + } + + notify_service.PullRequestSynchronized(ctx, opts.Doer, pr) } } } - log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", repoID, branch) - prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repoID, branch) + log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", opts.RepoID, opts.Branch) + prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, opts.RepoID, opts.Branch) if err != nil { - log.Error("Find pull requests [base_repo_id: %d, base_branch: %s]: %v", repoID, branch, err) + log.Error("Find pull requests [base_repo_id: %d, base_branch: %s]: %v", opts.RepoID, opts.Branch, err) return } for _, pr := range prs { divergence, err := GetDiverging(ctx, pr) if err != nil { - if git_model.IsErrBranchNotExist(err) && !git.IsBranchExist(ctx, pr.HeadRepo.RepoPath(), pr.HeadBranch) { + if git_model.IsErrBranchNotExist(err) && !gitrepo.IsBranchExist(ctx, pr.HeadRepo, pr.HeadBranch) { log.Warn("Cannot test PR %s/%d: head_branch %s no longer exists", pr.BaseRepo.Name, pr.IssueID, pr.HeadBranch) } else { log.Error("GetDiverging: %v", err) @@ -478,7 +497,7 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, log.Error("UpdateCommitDivergence: %v", err) } } - AddToTaskQueue(ctx, pr) + StartPullRequestCheckDelayable(ctx, pr) } }) } @@ -505,14 +524,14 @@ func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest, return false, fmt.Errorf("GetMergeBase: %w", err) } - cmd := git.NewCommand(ctx, "diff", "--name-only", "-z").AddDynamicArguments(newCommitID, oldCommitID, base) + cmd := git.NewCommand("diff", "--name-only", "-z").AddDynamicArguments(newCommitID, oldCommitID, base) stdoutReader, stdoutWriter, err := os.Pipe() if err != nil { return false, fmt.Errorf("unable to open pipe for to run diff: %w", err) } stderr := new(bytes.Buffer) - if err := cmd.Run(&git.RunOpts{ + if err := cmd.Run(ctx, &git.RunOpts{ Dir: prCtx.tmpBasePath, Stdout: stdoutWriter, Stderr: stderr, @@ -629,7 +648,7 @@ func UpdateRef(ctx context.Context, pr *issues_model.PullRequest) (err error) { return err } - _, _, err = git.NewCommand(ctx, "update-ref").AddDynamicArguments(pr.GetGitRefName(), pr.HeadCommitID).RunStdString(&git.RunOpts{Dir: pr.BaseRepo.RepoPath()}) + _, _, err = git.NewCommand("update-ref").AddDynamicArguments(pr.GetGitRefName(), 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) } @@ -645,7 +664,7 @@ func retargetBranchPulls(ctx context.Context, doer *user_model.User, repoID int6 return err } - if err := issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil { + if err := prs.LoadAttributes(ctx); err != nil { return err } @@ -672,11 +691,11 @@ func AdjustPullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User return err } - if err := issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil { + if err := prs.LoadAttributes(ctx); err != nil { return err } - issues_model.PullRequestList(prs).SetHeadRepo(repo) - if err := issues_model.PullRequestList(prs).LoadRepositories(ctx); err != nil { + prs.SetHeadRepo(repo) + if err := prs.LoadRepositories(ctx); err != nil { return err } @@ -707,11 +726,11 @@ func AdjustPullsCausedByBranchDeleted(ctx context.Context, doer *user_model.User return err } - if err := issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil { + if err := prs.LoadAttributes(ctx); err != nil { return err } - issues_model.PullRequestList(prs).SetBaseRepo(repo) - if err := issues_model.PullRequestList(prs).LoadRepositories(ctx); err != nil { + prs.SetBaseRepo(repo) + if err := prs.LoadRepositories(ctx); err != nil { return err } @@ -739,12 +758,12 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re var errs []error for _, branch := range branches { - prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, repo.ID, branch.Name) + prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, repo.ID, branch) if err != nil { return err } - if err = issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil { + if err = prs.LoadAttributes(ctx); err != nil { return err } @@ -926,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 { @@ -985,7 +998,7 @@ func getAllCommitStatus(ctx context.Context, gitRepo *git.Repository, pr *issues 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 } diff --git a/services/pull/review.go b/services/pull/review.go index 78723a58ae..5c80e7b338 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -395,7 +395,7 @@ func DismissReview(ctx context.Context, reviewID, repoID int64, message string, } if review.Type != issues_model.ReviewTypeApprove && review.Type != issues_model.ReviewTypeReject { - return nil, fmt.Errorf("not need to dismiss this review because it's type is not Approve or change request") + return nil, errors.New("not need to dismiss this review because it's type is not Approve or change request") } // load data for notify @@ -405,7 +405,7 @@ func DismissReview(ctx context.Context, reviewID, repoID int64, message string, // Check if the review's repoID is the one we're currently expecting. if review.Issue.RepoID != repoID { - return nil, fmt.Errorf("reviews's repository is not the same as the one we expect") + return nil, errors.New("reviews's repository is not the same as the one we expect") } issue := review.Issue 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 e5753178b8..72406482e0 100644 --- a/services/pull/temp_repo.go +++ b/services/pull/temp_repo.go @@ -15,6 +15,7 @@ import ( issues_model "code.gitea.io/gitea/models/issues" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/log" repo_module "code.gitea.io/gitea/modules/repository" ) @@ -27,7 +28,7 @@ const ( stagingBranch = "staging" // this is used for a working branch ) -type prContext struct { +type prTmpRepoContext struct { context.Context tmpBasePath string pr *issues_model.PullRequest @@ -35,7 +36,7 @@ type prContext struct { errbuf *strings.Builder // any use should be preceded by a Reset and preferably after use } -func (ctx *prContext) RunOpts() *git.RunOpts { +func (ctx *prTmpRepoContext) RunOpts() *git.RunOpts { ctx.outbuf.Reset() ctx.errbuf.Reset() return &git.RunOpts{ @@ -47,7 +48,7 @@ func (ctx *prContext) RunOpts() *git.RunOpts { // createTemporaryRepoForPR creates a temporary repo with "base" for pr.BaseBranch and "tracking" for pr.HeadBranch // it also create a second base branch called "original_base" -func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) (prCtx *prContext, cancel context.CancelFunc, err error) { +func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) (prCtx *prTmpRepoContext, cancel context.CancelFunc, err error) { if err := pr.LoadHeadRepo(ctx); err != nil { log.Error("%-v LoadHeadRepo: %v", pr, err) return nil, nil, fmt.Errorf("%v LoadHeadRepo: %w", pr, err) @@ -73,23 +74,20 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) } // Clone base repo. - tmpBasePath, err := repo_module.CreateTemporaryPath("pull") + tmpBasePath, cleanup, err := repo_module.CreateTemporaryPath("pull") if err != nil { log.Error("CreateTemporaryPath[%-v]: %v", pr, err) return nil, nil, err } - prCtx = &prContext{ + cancel = cleanup + + prCtx = &prTmpRepoContext{ Context: ctx, tmpBasePath: tmpBasePath, pr: pr, outbuf: &strings.Builder{}, errbuf: &strings.Builder{}, } - cancel = func() { - if err := repo_module.RemoveTemporaryPath(tmpBasePath); err != nil { - log.Error("Error whilst removing removing temporary repo for %-v: %v", pr, err) - } - } baseRepoPath := pr.BaseRepo.RepoPath() headRepoPath := pr.HeadRepo.RepoPath() @@ -133,22 +131,22 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) return nil, nil, fmt.Errorf("Unable to add base repository to temporary repo [%s -> tmpBasePath]: %w", pr.BaseRepo.FullName(), err) } - if err := git.NewCommand(ctx, "remote", "add", "-t").AddDynamicArguments(pr.BaseBranch).AddArguments("-m").AddDynamicArguments(pr.BaseBranch).AddDynamicArguments("origin", baseRepoPath). - Run(prCtx.RunOpts()); err != nil { + if err := git.NewCommand("remote", "add", "-t").AddDynamicArguments(pr.BaseBranch).AddArguments("-m").AddDynamicArguments(pr.BaseBranch).AddDynamicArguments("origin", baseRepoPath). + Run(ctx, prCtx.RunOpts()); err != nil { log.Error("%-v Unable to add base repository as origin [%s -> %s]: %v\n%s\n%s", pr, pr.BaseRepo.FullName(), tmpBasePath, err, prCtx.outbuf.String(), prCtx.errbuf.String()) cancel() return nil, nil, fmt.Errorf("Unable to add base repository as origin [%s -> tmpBasePath]: %w\n%s\n%s", pr.BaseRepo.FullName(), err, prCtx.outbuf.String(), prCtx.errbuf.String()) } - if err := git.NewCommand(ctx, "fetch", "origin").AddArguments(fetchArgs...).AddDashesAndList(pr.BaseBranch+":"+baseBranch, pr.BaseBranch+":original_"+baseBranch). - Run(prCtx.RunOpts()); err != nil { + if err := git.NewCommand("fetch", "origin").AddArguments(fetchArgs...).AddDashesAndList(pr.BaseBranch+":"+baseBranch, pr.BaseBranch+":original_"+baseBranch). + Run(ctx, prCtx.RunOpts()); err != nil { log.Error("%-v Unable to fetch origin base branch [%s:%s -> base, original_base in %s]: %v:\n%s\n%s", pr, pr.BaseRepo.FullName(), pr.BaseBranch, tmpBasePath, err, prCtx.outbuf.String(), prCtx.errbuf.String()) cancel() return nil, nil, fmt.Errorf("Unable to fetch origin base branch [%s:%s -> base, original_base in tmpBasePath]: %w\n%s\n%s", pr.BaseRepo.FullName(), pr.BaseBranch, err, prCtx.outbuf.String(), prCtx.errbuf.String()) } - if err := git.NewCommand(ctx, "symbolic-ref").AddDynamicArguments("HEAD", git.BranchPrefix+baseBranch). - Run(prCtx.RunOpts()); err != nil { + if err := git.NewCommand("symbolic-ref").AddDynamicArguments("HEAD", git.BranchPrefix+baseBranch). + Run(ctx, prCtx.RunOpts()); err != nil { log.Error("%-v Unable to set HEAD as base branch in [%s]: %v\n%s\n%s", pr, tmpBasePath, err, prCtx.outbuf.String(), prCtx.errbuf.String()) cancel() return nil, nil, fmt.Errorf("Unable to set HEAD as base branch in tmpBasePath: %w\n%s\n%s", err, prCtx.outbuf.String(), prCtx.errbuf.String()) @@ -160,8 +158,8 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) return nil, nil, fmt.Errorf("Unable to add head base repository to temporary repo [%s -> tmpBasePath]: %w", pr.HeadRepo.FullName(), err) } - if err := git.NewCommand(ctx, "remote", "add").AddDynamicArguments(remoteRepoName, headRepoPath). - Run(prCtx.RunOpts()); err != nil { + if err := git.NewCommand("remote", "add").AddDynamicArguments(remoteRepoName, headRepoPath). + Run(ctx, prCtx.RunOpts()); err != nil { log.Error("%-v Unable to add head repository as head_repo [%s -> %s]: %v\n%s\n%s", pr, pr.HeadRepo.FullName(), tmpBasePath, err, prCtx.outbuf.String(), prCtx.errbuf.String()) cancel() return nil, nil, fmt.Errorf("Unable to add head repository as head_repo [%s -> tmpBasePath]: %w\n%s\n%s", pr.HeadRepo.FullName(), err, prCtx.outbuf.String(), prCtx.errbuf.String()) @@ -178,10 +176,10 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) } else { headBranch = pr.GetGitRefName() } - if err := git.NewCommand(ctx, "fetch").AddArguments(fetchArgs...).AddDynamicArguments(remoteRepoName, headBranch+":"+trackingBranch). - Run(prCtx.RunOpts()); err != nil { + if err := git.NewCommand("fetch").AddArguments(fetchArgs...).AddDynamicArguments(remoteRepoName, headBranch+":"+trackingBranch). + Run(ctx, prCtx.RunOpts()); err != nil { cancel() - if !git.IsBranchExist(ctx, pr.HeadRepo.RepoPath(), pr.HeadBranch) { + if !gitrepo.IsBranchExist(ctx, pr.HeadRepo, pr.HeadBranch) { return nil, nil, git_model.ErrBranchNotExist{ BranchName: pr.HeadBranch, } diff --git a/services/pull/update.go b/services/pull/update.go index abf7ad4509..b8f84e3d65 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -5,6 +5,7 @@ package pull import ( "context" + "errors" "fmt" git_model "code.gitea.io/gitea/models/git" @@ -23,7 +24,7 @@ import ( func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, message string, rebase bool) error { if pr.Flow == issues_model.PullRequestFlowAGit { // TODO: update of agit flow pull request's head branch is unsupported - return fmt.Errorf("update of agit flow pull request's head branch is unsupported") + return errors.New("update of agit flow pull request's head branch is unsupported") } releaser, err := globallock.Lock(ctx, getPullWorkingLockKey(pr.ID)) @@ -40,14 +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(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "") - }() - - 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) @@ -65,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 @@ -81,11 +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(doer, reversePR.HeadRepo.ID, reversePR.HeadBranch, false, "", "") - }() - return err } diff --git a/services/pull/update_rebase.go b/services/pull/update_rebase.go index 3e2a7be132..9ff062f99c 100644 --- a/services/pull/update_rebase.go +++ b/services/pull/update_rebase.go @@ -27,7 +27,7 @@ func updateHeadByRebaseOnToBase(ctx context.Context, pr *issues_model.PullReques defer cancel() // Determine the old merge-base before the rebase - we use this for LFS push later on - oldMergeBase, _, _ := git.NewCommand(ctx, "merge-base").AddDashesAndList(baseBranch, trackingBranch).RunStdString(&git.RunOpts{Dir: mergeCtx.tmpBasePath}) + oldMergeBase, _, _ := git.NewCommand("merge-base").AddDashesAndList(baseBranch, trackingBranch).RunStdString(ctx, &git.RunOpts{Dir: mergeCtx.tmpBasePath}) oldMergeBase = strings.TrimSpace(oldMergeBase) // Rebase the tracking branch on to the base as the staging branch @@ -62,7 +62,7 @@ func updateHeadByRebaseOnToBase(ctx context.Context, pr *issues_model.PullReques headUser = pr.HeadRepo.Owner } - pushCmd := git.NewCommand(ctx, "push", "-f", "head_repo"). + pushCmd := git.NewCommand("push", "-f", "head_repo"). AddDynamicArguments(stagingBranch + ":" + git.BranchPrefix + pr.HeadBranch) // Push back to the head repository. @@ -71,7 +71,7 @@ func updateHeadByRebaseOnToBase(ctx context.Context, pr *issues_model.PullReques mergeCtx.outbuf.Reset() mergeCtx.errbuf.Reset() - if err := pushCmd.Run(&git.RunOpts{ + if err := pushCmd.Run(ctx, &git.RunOpts{ Env: repo_module.FullPushingEnvironment( headUser, doer, |