From aaa5712db25c64436f80fbaea8c978ce0c3a990f Mon Sep 17 00:00:00 2001 From: sillyguodong Date: Thu, 30 Mar 2023 10:54:53 +0800 Subject: [PATCH 1/6] fix_pr_sync_bugs --- options/locale/locale_en-US.ini | 1 + routers/web/repo/issue.go | 31 ++++++++++++++ services/pull/pull.go | 75 +++++++++++++++++++++++++++------ 3 files changed, 93 insertions(+), 14 deletions(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 3f47af826e54b..58a90f3706a59 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1648,6 +1648,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.
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 this pull request have been merged 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 diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 612222598f2fa..1d341ea3f55bb 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -2768,6 +2768,37 @@ 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() + err = pull.LoadBaseRepo(ctx) + if 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) + 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) + 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) { diff --git a/services/pull/pull.go b/services/pull/pull.go index e1d5a6f86dd42..75a5f32e459c4 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -265,22 +265,49 @@ 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() + err = pr.LoadBaseRepo(ctx) + if 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: %w", 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) } } @@ -294,6 +321,10 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, } if err == nil { for _, pr := range prs { + if pr.Issue.IsClosed { + // The closed PR never trigger action or webhook + continue + } if newCommitID != "" && newCommitID != git.EmptySHA { changed, err := checkIfPRContentChanged(ctx, pr, oldCommitID, newCommitID) if err != nil { @@ -349,6 +380,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) { From 54afd9afd43bfefccede89f7fc4048cf043cc583 Mon Sep 17 00:00:00 2001 From: sillyguodong Date: Thu, 30 Mar 2023 11:08:07 +0800 Subject: [PATCH 2/6] update prompt --- options/locale/locale_en-US.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 58a90f3706a59..c7dd253f91943 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1648,7 +1648,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.
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 this pull request have been merged by another pull request` +pulls.open_unmerged_pull_merged = `You cannot perform a reopen operation because all the changes 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 From 70f48d56bc451fdbf3da3c91f7899c0f86a01787 Mon Sep 17 00:00:00 2001 From: sillyguodong <33891828+sillyguodong@users.noreply.github.com> Date: Fri, 31 Mar 2023 22:29:10 +0800 Subject: [PATCH 3/6] Update services/pull/pull.go Co-authored-by: Jason Song --- services/pull/pull.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index 75a5f32e459c4..a7f4704ec1d45 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -278,8 +278,7 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, } else { // Get head commit ID of PR prHeadRef := pr.GetGitRefName() - err = pr.LoadBaseRepo(ctx) - if err != nil { + if err := pr.LoadBaseRepo(ctx); err != nil { log.Error("LoadBaseRepo(%d), error: %v", pr.ID, err) return } From f18a87282562d4476f1929db903ae61439fec8ac Mon Sep 17 00:00:00 2001 From: sillyguodong <33891828+sillyguodong@users.noreply.github.com> Date: Fri, 31 Mar 2023 22:30:24 +0800 Subject: [PATCH 4/6] Update routers/web/repo/issue.go Co-authored-by: Jason Song --- routers/web/repo/issue.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 1d341ea3f55bb..41297e0c19a2d 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -2771,8 +2771,7 @@ func NewComment(ctx *context.Context) { // Check whether the head commit of PR exists in base branch // Get head commit ID of PR prHeadRef := pull.GetGitRefName() - err = pull.LoadBaseRepo(ctx) - if err != nil { + if err := pull.LoadBaseRepo(ctx); err != nil { ctx.ServerError("Unable to load base repo", err) return } From ff6deed6404a9698a18da3781cb98d8c6325d48b Mon Sep 17 00:00:00 2001 From: sillyguodong <33891828+sillyguodong@users.noreply.github.com> Date: Fri, 31 Mar 2023 22:30:38 +0800 Subject: [PATCH 5/6] Update services/pull/pull.go Co-authored-by: Jason Song --- services/pull/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/pull/pull.go b/services/pull/pull.go index a7f4704ec1d45..da7fca39fe82e 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -284,7 +284,7 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, } prHeadCommitID, err := git.GetFullCommitID(ctx, pr.BaseRepo.RepoPath(), prHeadRef) if err != nil { - log.Error("GetFullCommitID(%s) in %s: %w", prHeadRef, pr.BaseRepo.FullName(), err) + log.Error("GetFullCommitID(%s) in %s: %v", prHeadRef, pr.BaseRepo.FullName(), err) return } // Open the base repo of PR From db62be8d8c369c141c8ff1a6337c6f7ba3875581 Mon Sep 17 00:00:00 2001 From: sillyguodong <33891828+sillyguodong@users.noreply.github.com> Date: Fri, 31 Mar 2023 22:31:02 +0800 Subject: [PATCH 6/6] Update options/locale/locale_en-US.ini Co-authored-by: silverwind --- options/locale/locale_en-US.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index c7dd253f91943..53f2ddf2d94cb 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1648,7 +1648,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.
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 all the changes of this pull request have been merged into the base branch by another pull request.` +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