From 09f7d84f4cbbfdf81caec922638d8fec4cb7dfca Mon Sep 17 00:00:00 2001
From: zeripath <art27@cantab.net>
Date: Mon, 8 Jun 2020 19:07:41 +0100
Subject: [PATCH] Ensure rejected push to refs/pull/index/head fails nicely
(#11724)
A pre-receive hook that rejects pushes to refs/pull/index/head
will cause a broken PR which causes an internal server error
whenever it is viewed. This PR handles prevents the internal server
error by handling non-existent pr heads and sends a flash error
informing the creator there was a problem.
Signed-off-by: Andrew Thornton <art27@cantab.net>
---
routers/repo/pull.go | 32 +++++++++++++++++++++++++++-----
services/pull/pull.go | 10 ++++++++++
2 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/routers/repo/pull.go b/routers/repo/pull.go
index 30913e4766..42846fb93d 100644
--- a/routers/repo/pull.go
+++ b/routers/repo/pull.go
@@ -430,6 +430,20 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
sha, err := baseGitRepo.GetRefCommitID(pull.GetGitRefName())
if err != nil {
+ if git.IsErrNotExist(err) {
+ ctx.Data["IsPullRequestBroken"] = true
+ if pull.IsSameRepo() {
+ ctx.Data["HeadTarget"] = pull.HeadBranch
+ } else if pull.HeadRepo == nil {
+ ctx.Data["HeadTarget"] = "<deleted>:" + pull.HeadBranch
+ } else {
+ ctx.Data["HeadTarget"] = pull.HeadRepo.OwnerName + ":" + pull.HeadBranch
+ }
+ ctx.Data["BaseTarget"] = pull.BaseBranch
+ ctx.Data["NumCommits"] = 0
+ ctx.Data["NumFiles"] = 0
+ return nil
+ }
ctx.ServerError(fmt.Sprintf("GetRefCommitID(%s)", pull.GetGitRefName()), err)
return nil
}
@@ -464,12 +478,10 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
ctx.Data["IsPullRequestBroken"] = true
if pull.IsSameRepo() {
ctx.Data["HeadTarget"] = pull.HeadBranch
+ } else if pull.HeadRepo == nil {
+ ctx.Data["HeadTarget"] = "<deleted>:" + pull.HeadBranch
} else {
- if pull.HeadRepo == nil {
- ctx.Data["HeadTarget"] = "<deleted>:" + pull.HeadBranch
- } else {
- ctx.Data["HeadTarget"] = pull.HeadRepo.OwnerName + ":" + pull.HeadBranch
- }
+ ctx.Data["HeadTarget"] = pull.HeadRepo.OwnerName + ":" + pull.HeadBranch
}
}
@@ -952,6 +964,16 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm)
if models.IsErrUserDoesNotHaveAccessToRepo(err) {
ctx.Error(400, "UserDoesNotHaveAccessToRepo", err.Error())
return
+ } else if git.IsErrPushRejected(err) {
+ pushrejErr := err.(*git.ErrPushRejected)
+ message := pushrejErr.Message
+ if len(message) == 0 {
+ ctx.Flash.Error(ctx.Tr("repo.pulls.push_rejected_no_message"))
+ } else {
+ ctx.Flash.Error(ctx.Tr("repo.pulls.push_rejected", utils.SanitizeFlashErrorString(pushrejErr.Message)))
+ }
+ ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pullIssue.Index))
+ return
}
ctx.ServerError("NewPullRequest", err)
return
diff --git a/services/pull/pull.go b/services/pull/pull.go
index c051641a5b..e8912e47c6 100644
--- a/services/pull/pull.go
+++ b/services/pull/pull.go
@@ -443,6 +443,16 @@ func PushToBaseRepo(pr *models.PullRequest) (err error) {
// Use InternalPushingEnvironment here because we know that pre-receive and post-receive do not run on a refs/pulls/...
Env: models.InternalPushingEnvironment(pr.Issue.Poster, pr.BaseRepo),
}); err != nil {
+ if git.IsErrPushOutOfDate(err) {
+ // This should not happen as we're using force!
+ log.Error("Unable to push PR head for %s#%d (%-v:%s) due to ErrPushOfDate: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, headFile, err)
+ return err
+ } else if git.IsErrPushRejected(err) {
+ rejectErr := err.(*git.ErrPushRejected)
+ log.Info("Unable to push PR head for %s#%d (%-v:%s) due to rejection:\nStdout: %s\nStderr: %s\nError: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, headFile, rejectErr.StdOut, rejectErr.StdErr, rejectErr.Err)
+ return err
+ }
+ log.Error("Unable to push PR head for %s#%d (%-v:%s) due to Error: %v", pr.BaseRepo.FullName(), pr.Index, pr.BaseRepo, headFile, err)
return fmt.Errorf("Push: %s:%s %s:%s %v", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), headFile, err)
}