From da2026f7886182482f5c1b1785a7b6093cc14cae Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 1 Jun 2020 20:52:08 +0100 Subject: [PATCH 1/2] Ensure rejected push to refs/pull/index/head fails nicely 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 --- routers/repo/pull.go | 26 ++++++++++++++++++++++++++ services/pull/pull.go | 10 ++++++++++ 2 files changed, 36 insertions(+) diff --git a/routers/repo/pull.go b/routers/repo/pull.go index d4c99e27699b5..7d35851e79064 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -428,6 +428,22 @@ 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"] = ":" + 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 } @@ -950,6 +966,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 c051641a5b00c..e8912e47c685c 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) } From 5252d4b6760ed443c3fa03bef32b091562e29470 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sat, 6 Jun 2020 13:52:26 +0100 Subject: [PATCH 2/2] as per @lafriks Signed-off-by: Andrew Thornton --- routers/repo/pull.go | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 7d35851e79064..4f15622fe7754 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -432,12 +432,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"] = ":" + pull.HeadBranch } else { - if pull.HeadRepo == nil { - ctx.Data["HeadTarget"] = ":" + pull.HeadBranch - } else { - ctx.Data["HeadTarget"] = pull.HeadRepo.OwnerName + ":" + pull.HeadBranch - } + ctx.Data["HeadTarget"] = pull.HeadRepo.OwnerName + ":" + pull.HeadBranch } ctx.Data["BaseTarget"] = pull.BaseBranch ctx.Data["NumCommits"] = 0 @@ -478,12 +476,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"] = ":" + pull.HeadBranch } else { - if pull.HeadRepo == nil { - ctx.Data["HeadTarget"] = ":" + pull.HeadBranch - } else { - ctx.Data["HeadTarget"] = pull.HeadRepo.OwnerName + ":" + pull.HeadBranch - } + ctx.Data["HeadTarget"] = pull.HeadRepo.OwnerName + ":" + pull.HeadBranch } }