From 089ccb0c80050efc8c3afef2402ea4f4a5ed250e Mon Sep 17 00:00:00 2001 From: zeripath Date: Sat, 22 Feb 2020 13:08:48 +0000 Subject: [PATCH] Handle push rejection message in Merge & Web Editor (#10373) * Handle push rejection message in Merge * placate golangci-lint * Fix sanitize, adjust message handling * oops * Oops * Handle push-rejection in webeditor CRUD too * Apply suggestions from code review Co-authored-by: Lauris BH --- models/error.go | 48 +++++++++++++++++++++++++++++++++ modules/repofiles/temp_repo.go | 24 +++++++++++++++-- options/locale/locale_en-US.ini | 4 +++ routers/api/v1/repo/pull.go | 8 ++++++ routers/repo/editor.go | 21 ++++++++++++--- routers/repo/pull.go | 25 +++++++++-------- routers/utils/utils.go | 12 +++++++++ services/pull/merge.go | 9 +++++++ 8 files changed, 135 insertions(+), 16 deletions(-) diff --git a/models/error.go b/models/error.go index fe9af70f3..a67937273 100644 --- a/models/error.go +++ b/models/error.go @@ -7,6 +7,7 @@ package models import ( "fmt" + "strings" "code.gitea.io/gitea/modules/git" ) @@ -1371,6 +1372,53 @@ func (err ErrMergePushOutOfDate) Error() string { return fmt.Sprintf("Merge PushOutOfDate Error: %v: %s\n%s", err.Err, err.StdErr, err.StdOut) } +// ErrPushRejected represents an error if merging fails due to rejection from a hook +type ErrPushRejected struct { + Style MergeStyle + Message string + StdOut string + StdErr string + Err error +} + +// IsErrPushRejected checks if an error is a ErrPushRejected. +func IsErrPushRejected(err error) bool { + _, ok := err.(ErrPushRejected) + return ok +} + +func (err ErrPushRejected) Error() string { + return fmt.Sprintf("Merge PushRejected Error: %v: %s\n%s", err.Err, err.StdErr, err.StdOut) +} + +// GenerateMessage generates the remote message from the stderr +func (err *ErrPushRejected) GenerateMessage() { + messageBuilder := &strings.Builder{} + i := strings.Index(err.StdErr, "remote: ") + if i < 0 { + err.Message = "" + return + } + for { + if len(err.StdErr) <= i+8 { + break + } + if err.StdErr[i:i+8] != "remote: " { + break + } + i += 8 + nl := strings.IndexByte(err.StdErr[i:], '\n') + if nl > 0 { + messageBuilder.WriteString(err.StdErr[i : i+nl+1]) + i = i + nl + 1 + } else { + messageBuilder.WriteString(err.StdErr[i:]) + i = len(err.StdErr) + } + } + err.Message = strings.TrimSpace(messageBuilder.String()) +} + // ErrRebaseConflicts represents an error if rebase fails with a conflict type ErrRebaseConflicts struct { Style MergeStyle diff --git a/modules/repofiles/temp_repo.go b/modules/repofiles/temp_repo.go index f7e3935c2..d8b816dfa 100644 --- a/modules/repofiles/temp_repo.go +++ b/modules/repofiles/temp_repo.go @@ -242,8 +242,28 @@ func (t *TemporaryUploadRepository) CommitTreeWithDate(author, committer *models func (t *TemporaryUploadRepository) Push(doer *models.User, commitHash string, branch string) error { // Because calls hooks we need to pass in the environment env := models.PushingEnvironment(doer, t.repo) - - if stdout, err := git.NewCommand("push", t.repo.RepoPath(), strings.TrimSpace(commitHash)+":refs/heads/"+strings.TrimSpace(branch)).RunInDirWithEnv(t.basePath, env); err != nil { + stdout := &strings.Builder{} + stderr := &strings.Builder{} + + if err := git.NewCommand("push", t.repo.RepoPath(), strings.TrimSpace(commitHash)+":refs/heads/"+strings.TrimSpace(branch)).RunInDirTimeoutEnvPipeline(env, -1, t.basePath, stdout, stderr); err != nil { + errString := stderr.String() + if strings.Contains(errString, "non-fast-forward") { + return models.ErrMergePushOutOfDate{ + StdOut: stdout.String(), + StdErr: errString, + Err: err, + } + } else if strings.Contains(errString, "! [remote rejected]") { + log.Error("Unable to push back to repo from temporary repo due to rejection: %s (%s)\nStdout: %s\nStderr: %s\nError: %v", + t.repo.FullName(), t.basePath, stdout, errString, err) + err := models.ErrPushRejected{ + StdOut: stdout.String(), + StdErr: errString, + Err: err, + } + err.GenerateMessage() + return err + } log.Error("Unable to push back to repo from temporary repo: %s (%s)\nStdout: %s\nError: %v", t.repo.FullName(), t.basePath, stdout, err) return fmt.Errorf("Unable to push back to repo from temporary repo: %s (%s) Error: %v", diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 2ebedc57d..7fe7bf697 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -784,6 +784,8 @@ editor.commit_empty_file_header = Commit an empty file editor.commit_empty_file_text = The file you're about commit is empty. Proceed? editor.no_changes_to_show = There are no changes to show. editor.fail_to_update_file = Failed to update/create file '%s' with error: %v +editor.push_rejected_no_message = The change was rejected by the server without a message. Please check githooks. +editor.push_rejected = The change was rejected by the server with the following message:
%s
Please check githooks. editor.add_subdir = Add a directory… editor.unable_to_upload_files = Failed to upload files to '%s' with error: %v editor.upload_file_is_locked = File '%s' is locked by %s. @@ -1088,6 +1090,8 @@ pulls.merge_conflict = Merge Failed: There was a conflict whilst merging: %[1]s< pulls.rebase_conflict = Merge Failed: There was a conflict whilst rebasing commit: %[1]s
%[2]s
%[3]s
Hint:Try a different strategy pulls.unrelated_histories = Merge Failed: The merge head and base do not share a common history. Hint: Try a different strategy pulls.merge_out_of_date = Merge Failed: Whilst generating the merge, the base was updated. Hint: Try again. +pulls.push_rejected = Merge Failed: The push was rejected with the following message:
%s
Review the githooks for this repository +pulls.push_rejected_no_message = Merge Failed: The push was rejected but there was no remote message.
Review the githooks 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.status_checking = Some checks are pending pulls.status_checks_success = All checks were successful diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 1abc2806f..41e17f8c2 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -684,6 +684,14 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) { } else if models.IsErrMergePushOutOfDate(err) { ctx.Error(http.StatusConflict, "Merge", "merge push out of date") return + } else if models.IsErrPushRejected(err) { + errPushRej := err.(models.ErrPushRejected) + if len(errPushRej.Message) == 0 { + ctx.Error(http.StatusConflict, "Merge", "PushRejected without remote error message") + return + } + ctx.Error(http.StatusConflict, "Merge", "PushRejected with remote message: "+errPushRej.Message) + return } ctx.Error(http.StatusInternalServerError, "Merge", err) return diff --git a/routers/repo/editor.go b/routers/repo/editor.go index 8d4f1f882..97a3fea24 100644 --- a/routers/repo/editor.go +++ b/routers/repo/editor.go @@ -23,6 +23,7 @@ import ( "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/upload" "code.gitea.io/gitea/modules/util" + "code.gitea.io/gitea/routers/utils" ) const ( @@ -264,10 +265,17 @@ func editFilePost(ctx *context.Context, form auth.EditRepoFileForm, isNewFile bo } else { ctx.Error(500, err.Error()) } - } else if models.IsErrCommitIDDoesNotMatch(err) { + } else if models.IsErrCommitIDDoesNotMatch(err) || models.IsErrMergePushOutOfDate(err) { ctx.RenderWithErr(ctx.Tr("repo.editor.file_changed_while_editing", ctx.Repo.RepoLink+"/compare/"+form.LastCommit+"..."+ctx.Repo.CommitID), tplEditFile, &form) + } else if models.IsErrPushRejected(err) { + errPushRej := err.(models.ErrPushRejected) + if len(errPushRej.Message) == 0 { + ctx.RenderWithErr(ctx.Tr("repo.editor.push_rejected_no_message"), tplEditFile, &form) + } else { + ctx.RenderWithErr(ctx.Tr("repo.editor.push_rejected", utils.SanitizeFlashErrorString(errPushRej.Message)), tplEditFile, &form) + } } else { - ctx.RenderWithErr(ctx.Tr("repo.editor.fail_to_update_file", form.TreePath, err), tplEditFile, &form) + ctx.RenderWithErr(ctx.Tr("repo.editor.fail_to_update_file", form.TreePath, utils.SanitizeFlashErrorString(err.Error())), tplEditFile, &form) } } @@ -428,8 +436,15 @@ func DeleteFilePost(ctx *context.Context, form auth.DeleteRepoFileForm) { } else { ctx.Error(500, err.Error()) } - } else if models.IsErrCommitIDDoesNotMatch(err) { + } else if models.IsErrCommitIDDoesNotMatch(err) || models.IsErrMergePushOutOfDate(err) { ctx.RenderWithErr(ctx.Tr("repo.editor.file_changed_while_deleting", ctx.Repo.RepoLink+"/compare/"+form.LastCommit+"..."+ctx.Repo.CommitID), tplDeleteFile, &form) + } else if models.IsErrPushRejected(err) { + errPushRej := err.(models.ErrPushRejected) + if len(errPushRej.Message) == 0 { + ctx.RenderWithErr(ctx.Tr("repo.editor.push_rejected_no_message"), tplDeleteFile, &form) + } else { + ctx.RenderWithErr(ctx.Tr("repo.editor.push_rejected", utils.SanitizeFlashErrorString(errPushRej.Message)), tplDeleteFile, &form) + } } else { ctx.ServerError("DeleteRepoFile", err) } diff --git a/routers/repo/pull.go b/routers/repo/pull.go index b4760fd52..11c376be7 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -26,6 +26,7 @@ import ( "code.gitea.io/gitea/modules/repofiles" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/util" + "code.gitea.io/gitea/routers/utils" "code.gitea.io/gitea/services/gitdiff" pull_service "code.gitea.io/gitea/services/pull" repo_service "code.gitea.io/gitea/services/repository" @@ -774,27 +775,18 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) { } if err = pull_service.Merge(pr, ctx.User, ctx.Repo.GitRepo, models.MergeStyle(form.Do), message); err != nil { - sanitize := func(x string) string { - runes := []rune(x) - - if len(runes) > 512 { - x = "..." + string(runes[len(runes)-512:]) - } - - return strings.Replace(html.EscapeString(x), "\n", "
", -1) - } if models.IsErrInvalidMergeStyle(err) { ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option")) ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index)) return } else if models.IsErrMergeConflicts(err) { conflictError := err.(models.ErrMergeConflicts) - ctx.Flash.Error(ctx.Tr("repo.pulls.merge_conflict", sanitize(conflictError.StdErr), sanitize(conflictError.StdOut))) + ctx.Flash.Error(ctx.Tr("repo.pulls.merge_conflict", utils.SanitizeFlashErrorString(conflictError.StdErr), utils.SanitizeFlashErrorString(conflictError.StdOut))) ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index)) return } else if models.IsErrRebaseConflicts(err) { conflictError := err.(models.ErrRebaseConflicts) - ctx.Flash.Error(ctx.Tr("repo.pulls.rebase_conflict", sanitize(conflictError.CommitSHA), sanitize(conflictError.StdErr), sanitize(conflictError.StdOut))) + ctx.Flash.Error(ctx.Tr("repo.pulls.rebase_conflict", utils.SanitizeFlashErrorString(conflictError.CommitSHA), utils.SanitizeFlashErrorString(conflictError.StdErr), utils.SanitizeFlashErrorString(conflictError.StdOut))) ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index)) return } else if models.IsErrMergeUnrelatedHistories(err) { @@ -807,6 +799,17 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) { ctx.Flash.Error(ctx.Tr("repo.pulls.merge_out_of_date")) ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index)) return + } else if models.IsErrPushRejected(err) { + log.Debug("MergePushRejected error: %v", err) + pushrejErr := err.(models.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(pr.Index)) + return } ctx.ServerError("Merge", err) return diff --git a/routers/utils/utils.go b/routers/utils/utils.go index 7c90fd704..64b132ff3 100644 --- a/routers/utils/utils.go +++ b/routers/utils/utils.go @@ -5,6 +5,7 @@ package utils import ( + "html" "strings" ) @@ -34,3 +35,14 @@ func IsValidSlackChannel(channelName string) bool { return true } + +// SanitizeFlashErrorString will sanitize a flash error string +func SanitizeFlashErrorString(x string) string { + runes := []rune(x) + + if len(runes) > 512 { + x = "..." + string(runes[len(runes)-512:]) + } + + return strings.Replace(html.EscapeString(x), "\n", "
", -1) +} diff --git a/services/pull/merge.go b/services/pull/merge.go index 8f3e18355..4d02d7193 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -392,6 +392,15 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge StdErr: errbuf.String(), Err: err, } + } else if strings.Contains(errbuf.String(), "! [remote rejected]") { + err := models.ErrPushRejected{ + Style: mergeStyle, + StdOut: outbuf.String(), + StdErr: errbuf.String(), + Err: err, + } + err.GenerateMessage() + return "", err } return "", fmt.Errorf("git push: %s", errbuf.String()) }