From be5652c51ac8df28ca69d879ca017dc477b7c314 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 * Fixed conflicts in: modules/repofiles/temp_repo.go Co-authored-by: Lauris BH --- models/error.go | 48 +++++++++++++++++++++++++++++++++ modules/repofiles/temp_repo.go | 28 ++++++++++++++++--- 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, 137 insertions(+), 18 deletions(-) diff --git a/models/error.go b/models/error.go index b9ebab9c6b93..f1d240444176 100644 --- a/models/error.go +++ b/models/error.go @@ -7,6 +7,7 @@ package models import ( "fmt" + "strings" "code.gitea.io/gitea/modules/git" ) @@ -1370,6 +1371,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 f9ea4ba1554a..d751c835ec9a 100644 --- a/modules/repofiles/temp_repo.go +++ b/modules/repofiles/temp_repo.go @@ -242,10 +242,30 @@ 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 _, err := git.NewCommand("push", t.repo.RepoPath(), strings.TrimSpace(commitHash)+":refs/heads/"+strings.TrimSpace(branch)).RunInDirWithEnv(t.basePath, env); err != nil { - log.Error("Unable to push back to repo from temporary repo: %s (%s) Error: %v", - t.repo.FullName(), t.basePath, err) + 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", t.repo.FullName(), t.basePath, err) } diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 61d3371072b7..79dd188f28d4 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -776,6 +776,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. @@ -1075,6 +1077,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 ef581a70b7aa..1f1c3b716946 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -645,6 +645,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 fec572852567..e33db22ad7a7 100644 --- a/routers/repo/editor.go +++ b/routers/repo/editor.go @@ -22,6 +22,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 ( @@ -262,10 +263,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) } } @@ -426,8 +434,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 df6a92fb4cce..8e3efcc75df4 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -25,6 +25,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" @@ -663,27 +664,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) { @@ -696,6 +688,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 7c90fd7048ab..64b132ff3e05 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 3958216b5cac..7aa80354d89c 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -335,6 +335,15 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor 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()) }