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>

diff --git a/routers/repo/pull.go b/routers/repo/pull.go
index 30913e476650a546061f6866dc5b83ad1e809fd9..42846fb93dc71caf4cb87a8bcc83176c0c25805e 100644 (file)
--- 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 c051641a5b00c124963c7ff1524c39443178d788..e8912e47c685c2c91d2ea5d1f2bedd44a51243ff 100644 (file)
--- 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)
        }