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>
This commit is contained in:
parent
5814079bf5
commit
09f7d84f4c
2 changed files with 37 additions and 5 deletions
|
@ -430,6 +430,20 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
|
||||||
|
|
||||||
sha, err := baseGitRepo.GetRefCommitID(pull.GetGitRefName())
|
sha, err := baseGitRepo.GetRefCommitID(pull.GetGitRefName())
|
||||||
if err != nil {
|
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)
|
ctx.ServerError(fmt.Sprintf("GetRefCommitID(%s)", pull.GetGitRefName()), err)
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
@ -464,14 +478,12 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
|
||||||
ctx.Data["IsPullRequestBroken"] = true
|
ctx.Data["IsPullRequestBroken"] = true
|
||||||
if pull.IsSameRepo() {
|
if pull.IsSameRepo() {
|
||||||
ctx.Data["HeadTarget"] = pull.HeadBranch
|
ctx.Data["HeadTarget"] = pull.HeadBranch
|
||||||
} else {
|
} else if pull.HeadRepo == nil {
|
||||||
if pull.HeadRepo == nil {
|
|
||||||
ctx.Data["HeadTarget"] = "<deleted>:" + pull.HeadBranch
|
ctx.Data["HeadTarget"] = "<deleted>:" + pull.HeadBranch
|
||||||
} else {
|
} else {
|
||||||
ctx.Data["HeadTarget"] = pull.HeadRepo.OwnerName + ":" + pull.HeadBranch
|
ctx.Data["HeadTarget"] = pull.HeadRepo.OwnerName + ":" + pull.HeadBranch
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
|
||||||
|
|
||||||
compareInfo, err := baseGitRepo.GetCompareInfo(pull.BaseRepo.RepoPath(),
|
compareInfo, err := baseGitRepo.GetCompareInfo(pull.BaseRepo.RepoPath(),
|
||||||
pull.BaseBranch, pull.GetGitRefName())
|
pull.BaseBranch, pull.GetGitRefName())
|
||||||
|
@ -952,6 +964,16 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm)
|
||||||
if models.IsErrUserDoesNotHaveAccessToRepo(err) {
|
if models.IsErrUserDoesNotHaveAccessToRepo(err) {
|
||||||
ctx.Error(400, "UserDoesNotHaveAccessToRepo", err.Error())
|
ctx.Error(400, "UserDoesNotHaveAccessToRepo", err.Error())
|
||||||
return
|
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)
|
ctx.ServerError("NewPullRequest", err)
|
||||||
return
|
return
|
||||||
|
|
|
@ -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/...
|
// 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),
|
Env: models.InternalPushingEnvironment(pr.Issue.Poster, pr.BaseRepo),
|
||||||
}); err != nil {
|
}); 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)
|
return fmt.Errorf("Push: %s:%s %s:%s %v", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), headFile, err)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue