Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the handling of closed PR when push commits #23830

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -1683,6 +1683,7 @@ pulls.push_rejected = Merge Failed: The push was rejected. Review the Git Hooks
pulls.push_rejected_summary = Full Rejection Message
pulls.push_rejected_no_message = Merge Failed: The push was rejected but there was no remote message.<br>Review the Git Hooks for this repository
pulls.open_unmerged_pull_exists = `You cannot perform a reopen operation because there is a pending pull request (#%d) with identical properties.`
pulls.open_unmerged_pull_merged = `You cannot perform a reopen operation because the commits of this pull request have been merged into the base branch by another pull request.`
pulls.status_checking = Some checks are pending
pulls.status_checks_success = All checks were successful
pulls.status_checks_warning = Some checks reported warnings
Expand Down
30 changes: 30 additions & 0 deletions routers/web/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -2762,6 +2762,36 @@ func NewComment(ctx *context.Context) {
if form.Status == "reopen" && issue.IsPull {
pull := issue.PullRequest
var err error
// Check whether the head commit of PR exists in base branch
// Get head commit ID of PR
prHeadRef := pull.GetGitRefName()
if err := pull.LoadBaseRepo(ctx); err != nil {
ctx.ServerError("Unable to load base repo", err)
return
}
prHeadCommitID, err := git.GetFullCommitID(ctx, pull.BaseRepo.RepoPath(), prHeadRef)
if err != nil {
ctx.ServerError("Get head commit Id fail", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we return 500 directly? Maybe just returning a flash with error is better.

return
}
// Open base repo
baseRepo, closer, err := git.RepositoryFromContextOrOpen(ctx, pull.BaseRepo.RepoPath())
if err != nil {
ctx.ServerError("Unable to open base repo", err)
return
}
defer closer.Close()
ok, err := baseRepo.IsCommitInBranch(prHeadCommitID, pull.BaseBranch)
if err != nil {
ctx.ServerError("", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above.

return
}
if ok {
ctx.Flash.Info(ctx.Tr("repo.pulls.open_unmerged_pull_merged"))
ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, issue.Index))
return
}

pr, err = issues_model.GetUnmergedPullRequest(ctx, pull.HeadRepoID, pull.BaseRepoID, pull.HeadBranch, pull.BaseBranch, pull.Flow)
if err != nil {
if !issues_model.IsErrPullRequestNotExist(err) {
Expand Down
70 changes: 56 additions & 14 deletions services/pull/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -265,22 +265,48 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,

for _, pr := range prs {
log.Trace("Updating PR[%d]: composing new test task", pr.ID)
if pr.Flow == issues_model.PullRequestFlowGithub {
if err := PushToBaseRepo(ctx, pr); err != nil {
log.Error("PushToBaseRepo: %v", err)

if err = pr.LoadIssue(ctx); err != nil {
log.Error("Load PR[%d]'s issue fail, error: %v", pr.ID, err)
return
}
if !pr.Issue.IsClosed {
// PR is unmerged but open.
// need do these: trigger action, create comment, notification(ui and email), execute `pushToBaseRepo()`
pushPRToBaseRepo(ctx, pr)
commentAndNotifyPullRequestPushComments(ctx, doer, pr, oldCommitID, newCommitID)
} else {
// Get head commit ID of PR
prHeadRef := pr.GetGitRefName()
if err := pr.LoadBaseRepo(ctx); err != nil {
log.Error("LoadBaseRepo(%d), error: %v", pr.ID, err)
return
}
prHeadCommitID, err := git.GetFullCommitID(ctx, pr.BaseRepo.RepoPath(), prHeadRef)
if err != nil {
log.Error("GetFullCommitID(%s) in %s: %v", prHeadRef, pr.BaseRepo.FullName(), err)
return
}
// Open the base repo of PR
baseRepo, closer, err := git.RepositoryFromContextOrOpen(ctx, pr.BaseRepo.RepoPath())
if err != nil {
log.Error("Unable to open base repo, Error: %v", err)
return
}
defer closer.Close()
// If we can find it that the `prHeadCommitID` already exists in PR's `base_branch`.
// It means that all changes of the PR have been merged into PR's base_branch`.
// So we don't need to create action task, comment, notification, execute `pushToBaseRepo()` in this case.
ok, err := baseRepo.IsCommitInBranch(prHeadCommitID, pr.BaseBranch)
if err != nil {
log.Error("Check It that whether prHeadRef is in the baseBranch fail, Error: %v", err)
return
}
if ok {
continue
}
} else {
continue
}

// If the PR is closed, someone still push some commits to the PR,
// 1. We will insert comments of commits, but hidden until the PR is reopened.
// 2. We won't send any notification.
AddToTaskQueue(pr)
comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID)
if err == nil && comment != nil && !pr.Issue.IsClosed {
notification.NotifyPullRequestPushCommits(ctx, doer, pr, comment)
pushPRToBaseRepo(ctx, pr)
commentAndNotifyPullRequestPushComments(ctx, doer, pr, oldCommitID, newCommitID)
}
}

Expand Down Expand Up @@ -353,6 +379,22 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,
})
}

func commentAndNotifyPullRequestPushComments(ctx context.Context, doer *user_model.User, pr *issues_model.PullRequest, oldCommitID, newCommitID string) {
comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID)
if err == nil && comment != nil {
notification.NotifyPullRequestPushCommits(ctx, doer, pr, comment)
}
AddToTaskQueue(pr)
}

func pushPRToBaseRepo(ctx context.Context, pr *issues_model.PullRequest) {
if pr.Flow == issues_model.PullRequestFlowGithub {
if err := PushToBaseRepo(ctx, pr); err != nil {
log.Error("PushToBaseRepo: %v", err)
}
}
}

// checkIfPRContentChanged checks if diff to target branch has changed by push
// A commit can be considered to leave the PR untouched if the patch/diff with its merge base is unchanged
func checkIfPRContentChanged(ctx context.Context, pr *issues_model.PullRequest, oldCommitID, newCommitID string) (hasChanged bool, err error) {
Expand Down