Handle push rejection in branch and upload (#10854)

* Handle push rejections and push out-of-date in branch creation and
file upload.
* Remove the duplicated sanitize from services/pull/merge
* Move the errors Err(Merge)PushOutOfDate and ErrPushRejected to
modules/git
* Handle errors better in the upload file dialogs

Fix #10460

Signed-off-by: Andrew Thornton <[email protected]>
Co-authored-by: guillep2k <[email protected]>

Author: zeripath

models/error.go

@@ -7,7 +7,6 @@ package models
 
 import (
 	"fmt"
-	"strings"
 
 	"code.gitea.io/gitea/modules/git"
 )
@@ -1388,71 +1387,6 @@ func (err ErrMergeUnrelatedHistories) Error() string {
 	return fmt.Sprintf("Merge UnrelatedHistories Error: %v: %s\n%s", err.Err, err.StdErr, err.StdOut)
 }
 
-// ErrMergePushOutOfDate represents an error if merging fails due to unrelated histories
-type ErrMergePushOutOfDate struct {
-	Style  MergeStyle
-	StdOut string
-	StdErr string
-	Err    error
-}
-
-// IsErrMergePushOutOfDate checks if an error is a ErrMergePushOutOfDate.
-func IsErrMergePushOutOfDate(err error) bool {
-	_, ok := err.(ErrMergePushOutOfDate)
-	return ok
-}
-
-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

modules/git/error.go

@@ -6,6 +6,7 @@ package git
 
 import (
 	"fmt"
+	"strings"
 	"time"
 )
 
@@ -85,3 +86,76 @@ func IsErrBranchNotExist(err error) bool {
 func (err ErrBranchNotExist) Error() string {
 	return fmt.Sprintf("branch does not exist [name: %s]", err.Name)
 }
+
+// ErrPushOutOfDate represents an error if merging fails due to unrelated histories
+type ErrPushOutOfDate struct {
+	StdOut string
+	StdErr string
+	Err    error
+}
+
+// IsErrPushOutOfDate checks if an error is a ErrPushOutOfDate.
+func IsErrPushOutOfDate(err error) bool {
+	_, ok := err.(*ErrPushOutOfDate)
+	return ok
+}
+
+func (err *ErrPushOutOfDate) Error() string {
+	return fmt.Sprintf("PushOutOfDate Error: %v: %s\n%s", err.Err, err.StdErr, err.StdOut)
+}
+
+// Unwrap unwraps the underlying error
+func (err *ErrPushOutOfDate) Unwrap() error {
+	return fmt.Errorf("%v - %s", err.Err, err.StdErr)
+}
+
+// ErrPushRejected represents an error if merging fails due to rejection from a hook
+type ErrPushRejected struct {
+	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("PushRejected Error: %v: %s\n%s", err.Err, err.StdErr, err.StdOut)
+}
+
+// Unwrap unwraps the underlying error
+func (err *ErrPushRejected) Unwrap() error {
+	return fmt.Errorf("%v - %s", err.Err, err.StdErr)
+}
+
+// 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())
+}

modules/git/repo.go

@@ -255,7 +255,31 @@ func Push(repoPath string, opts PushOptions) error {
 		cmd.AddArguments("-f")
 	}
 	cmd.AddArguments("--", opts.Remote, opts.Branch)
-	_, err := cmd.RunInDirWithEnv(repoPath, opts.Env)
+	var outbuf, errbuf strings.Builder
+
+	err := cmd.RunInDirTimeoutEnvPipeline(opts.Env, -1, repoPath, &outbuf, &errbuf)
+	if err != nil {
+		if strings.Contains(errbuf.String(), "non-fast-forward") {
+			return &ErrPushOutOfDate{
+				StdOut: outbuf.String(),
+				StdErr: errbuf.String(),
+				Err:    err,
+			}
+		} else if strings.Contains(errbuf.String(), "! [remote rejected]") {
+			err := &ErrPushRejected{
+				StdOut: outbuf.String(),
+				StdErr: errbuf.String(),
+				Err:    err,
+			}
+			err.GenerateMessage()
+			return err
+		}
+	}
+
+	if errbuf.Len() > 0 && err != nil {
+		return fmt.Errorf("%v - %s", err, errbuf.String())
+	}
+
 	return err
 }
 

modules/repofiles/temp_repo.go

@@ -242,30 +242,21 @@ 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)
-	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()
+	if err := git.Push(t.basePath, git.PushOptions{
+		Remote: t.repo.RepoPath(),
+		Branch: strings.TrimSpace(commitHash) + ":refs/heads/" + strings.TrimSpace(branch),
+		Env:    env,
+	}); err != nil {
+		if git.IsErrPushOutOfDate(err) {
+			return err
+		} else if git.IsErrPushRejected(err) {
+			rejectErr := err.(*git.ErrPushRejected)
+			log.Info("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, rejectErr.StdOut, rejectErr.StdErr, rejectErr.Err)
 			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)
+		log.Error("Unable to push back to repo from temporary repo: %s (%s)\nError: %v",
+			t.repo.FullName(), t.basePath, err)
 		return fmt.Errorf("Unable to push back to repo from temporary repo: %s (%s) Error: %v",
 			t.repo.FullName(), t.basePath, err)
 	}

modules/repofiles/update.go

@@ -446,6 +446,7 @@ func CreateOrUpdateRepoFile(repo *models.Repository, doer *models.User, opts *Up
 
 	// Then push this tree to NewBranch
 	if err := t.Push(doer, commitHash, opts.NewBranch); err != nil {
+		log.Error("%T %v", err, err)
 		return nil, err
 	}
 

modules/repository/branch.go

@@ -109,6 +109,9 @@ func CreateNewBranch(doer *models.User, repo *models.Repository, oldBranchName,
 		Branch: branchName,
 		Env:    models.PushingEnvironment(doer, repo),
 	}); err != nil {
+		if git.IsErrPushOutOfDate(err) || git.IsErrPushRejected(err) {
+			return err
+		}
 		return fmt.Errorf("Push: %v", err)
 	}
 
@@ -156,6 +159,9 @@ func CreateNewBranchFromCommit(doer *models.User, repo *models.Repository, commi
 		Branch: branchName,
 		Env:    models.PushingEnvironment(doer, repo),
 	}); err != nil {
+		if git.IsErrPushOutOfDate(err) || git.IsErrPushRejected(err) {
+			return err
+		}
 		return fmt.Errorf("Push: %v", err)
 	}
 

routers/api/v1/repo/pull.go

@@ -678,11 +678,11 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) {
 		} else if models.IsErrMergeUnrelatedHistories(err) {
 			conflictError := err.(models.ErrMergeUnrelatedHistories)
 			ctx.JSON(http.StatusConflict, conflictError)
-		} else if models.IsErrMergePushOutOfDate(err) {
+		} else if git.IsErrPushOutOfDate(err) {
 			ctx.Error(http.StatusConflict, "Merge", "merge push out of date")
 			return
-		} else if models.IsErrPushRejected(err) {
-			errPushRej := err.(models.ErrPushRejected)
+		} else if git.IsErrPushRejected(err) {
+			errPushRej := err.(*git.ErrPushRejected)
 			if len(errPushRej.Message) == 0 {
 				ctx.Error(http.StatusConflict, "Merge", "PushRejected without remote error message")
 				return

routers/repo/branch.go

@@ -17,6 +17,7 @@ import (
 	"code.gitea.io/gitea/modules/repofiles"
 	repo_module "code.gitea.io/gitea/modules/repository"
 	"code.gitea.io/gitea/modules/util"
+	"code.gitea.io/gitea/routers/utils"
 )
 
 const (
@@ -335,9 +336,8 @@ func CreateBranch(ctx *context.Context, form auth.NewBranchForm) {
 			ctx.Redirect(ctx.Repo.RepoLink + "/src/" + ctx.Repo.BranchNameSubURL())
 			return
 		}
-		if models.IsErrBranchAlreadyExists(err) {
-			e := err.(models.ErrBranchAlreadyExists)
-			ctx.Flash.Error(ctx.Tr("repo.branch.branch_already_exists", e.BranchName))
+		if models.IsErrBranchAlreadyExists(err) || git.IsErrPushOutOfDate(err) {
+			ctx.Flash.Error(ctx.Tr("repo.branch.branch_already_exists", form.NewBranchName))
 			ctx.Redirect(ctx.Repo.RepoLink + "/src/" + ctx.Repo.BranchNameSubURL())
 			return
 		}
@@ -347,6 +347,16 @@ func CreateBranch(ctx *context.Context, form auth.NewBranchForm) {
 			ctx.Redirect(ctx.Repo.RepoLink + "/src/" + ctx.Repo.BranchNameSubURL())
 			return
 		}
+		if git.IsErrPushRejected(err) {
+			e := err.(*git.ErrPushRejected)
+			if len(e.Message) == 0 {
+				ctx.Flash.Error(ctx.Tr("repo.editor.push_rejected_no_message"))
+			} else {
+				ctx.Flash.Error(ctx.Tr("repo.editor.push_rejected", utils.SanitizeFlashErrorString(e.Message)))
+			}
+			ctx.Redirect(ctx.Repo.RepoLink + "/src/" + ctx.Repo.BranchNameSubURL())
+			return
+		}
 
 		ctx.ServerError("CreateNewBranch", err)
 		return