Merge pull request '[PORT] Fix git error handling (gitea#32401)' (#5794) from gusted/forgejo-port-32401 into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/5794
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Reviewed-by: Otto <otto@codeberg.org>

.deadcode-out

@@ -137,9 +137,6 @@ code.gitea.io/gitea/modules/git
 	AddChangesWithArgs
 	CommitChanges
 	CommitChangesWithArgs
-	IsErrExecTimeout
-	ErrExecTimeout.Error
-	ErrUnsupportedVersion.Error
 	SetUpdateHook
 	openRepositoryWithDefaultContext
 	IsTagExist

modules/git/error.go

@@ -4,28 +4,14 @@
 package git
 
 import (
+	"context"
+	"errors"
 	"fmt"
 	"strings"
-	"time"
 
 	"code.gitea.io/gitea/modules/util"
 )
 
-// ErrExecTimeout error when exec timed out
-type ErrExecTimeout struct {
-	Duration time.Duration
-}
-
-// IsErrExecTimeout if some error is ErrExecTimeout
-func IsErrExecTimeout(err error) bool {
-	_, ok := err.(ErrExecTimeout)
-	return ok
-}
-
-func (err ErrExecTimeout) Error() string {
-	return fmt.Sprintf("execution is timeout [duration: %v]", err.Duration)
-}
-
 // ErrNotExist commit not exist error
 type ErrNotExist struct {
 	ID      string
@@ -62,21 +48,6 @@ func IsErrBadLink(err error) bool {
 	return ok
 }
 
-// ErrUnsupportedVersion error when required git version not matched
-type ErrUnsupportedVersion struct {
-	Required string
-}
-
-// IsErrUnsupportedVersion if some error is ErrUnsupportedVersion
-func IsErrUnsupportedVersion(err error) bool {
-	_, ok := err.(ErrUnsupportedVersion)
-	return ok
-}
-
-func (err ErrUnsupportedVersion) Error() string {
-	return fmt.Sprintf("Operation requires higher version [required: %s]", err.Required)
-}
-
 // ErrBranchNotExist represents a "BranchNotExist" kind of error.
 type ErrBranchNotExist struct {
 	Name string
@@ -185,3 +156,10 @@ func IsErrMoreThanOne(err error) bool {
 func (err *ErrMoreThanOne) Error() string {
 	return fmt.Sprintf("ErrMoreThanOne Error: %v: %s\n%s", err.Err, err.StdErr, err.StdOut)
 }
+
+func IsErrCanceledOrKilled(err error) bool {
+	// When "cancel()" a git command's context, the returned error of "Run()" could be one of them:
+	// - context.Canceled
+	// - *exec.ExitError: "signal: killed"
+	return err != nil && (errors.Is(err, context.Canceled) || err.Error() == "signal: killed")
+}

modules/git/repo_attribute.go

@@ -250,7 +250,7 @@ func (repo *Repository) GitAttributeChecker(treeish string, attributes ...string
 			err = e
 		}
 
-		if err != nil { // decorate the returned error
+		if err != nil && !IsErrCanceledOrKilled(err) { // decorate the returned error
 			err = fmt.Errorf("git check-attr (stderr: %q): %w", strings.TrimSpace(stdErr.String()), err)
 			ac.err.Store(err)
 		}

routers/api/v1/repo/repo.go

@@ -21,7 +21,6 @@ import (
 	repo_model "code.gitea.io/gitea/models/repo"
 	unit_model "code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
-	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/label"
 	"code.gitea.io/gitea/modules/log"
@@ -748,10 +747,8 @@ func updateBasicProperties(ctx *context.APIContext, opts api.EditRepoOption) err
 	if opts.DefaultBranch != nil && repo.DefaultBranch != *opts.DefaultBranch && (repo.IsEmpty || ctx.Repo.GitRepo.IsBranchExist(*opts.DefaultBranch)) {
 		if !repo.IsEmpty {
 			if err := gitrepo.SetDefaultBranch(ctx, ctx.Repo.Repository, *opts.DefaultBranch); err != nil {
-				if !git.IsErrUnsupportedVersion(err) {
+				ctx.Error(http.StatusInternalServerError, "SetDefaultBranch", err)
-					ctx.Error(http.StatusInternalServerError, "SetDefaultBranch", err)
+				return err
-					return err
-				}
 			}
 		}
 		repo.DefaultBranch = *opts.DefaultBranch

routers/private/default_branch.go

@@ -8,7 +8,6 @@ import (
 	"net/http"
 
 	repo_model "code.gitea.io/gitea/models/repo"
-	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/private"
 	gitea_context "code.gitea.io/gitea/services/context"
@@ -22,12 +21,10 @@ func SetDefaultBranch(ctx *gitea_context.PrivateContext) {
 
 	ctx.Repo.Repository.DefaultBranch = branch
 	if err := gitrepo.SetDefaultBranch(ctx, ctx.Repo.Repository, ctx.Repo.Repository.DefaultBranch); err != nil {
-		if !git.IsErrUnsupportedVersion(err) {
+		ctx.JSON(http.StatusInternalServerError, private.Response{
-			ctx.JSON(http.StatusInternalServerError, private.Response{
+			Err: fmt.Sprintf("Unable to set default branch on repository: %s/%s Error: %v", ownerName, repoName, err),
-				Err: fmt.Sprintf("Unable to set default branch on repository: %s/%s Error: %v", ownerName, repoName, err),
+		})
-			})
+		return
-			return
-		}
 	}
 
 	if err := repo_model.UpdateDefaultBranch(ctx, ctx.Repo.Repository); err != nil {

routers/web/repo/githttp.go

@@ -467,7 +467,7 @@ func serviceRPC(ctx *context.Context, h *serviceHandler, service string) {
 		Stderr:            &stderr,
 		UseContextTimeout: true,
 	}); err != nil {
-		if err.Error() != "signal: killed" {
+		if !git.IsErrCanceledOrKilled(err) {
 			log.Error("Fail to serve RPC(%s) in %s: %v - %s", service, h.getRepoDir(), err, stderr.String())
 		}
 		return

services/gitdiff/gitdiff.go

@@ -1159,7 +1159,7 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi
 			Dir:     repoPath,
 			Stdout:  writer,
 			Stderr:  stderr,
-		}); err != nil {
+		}); err != nil && !git.IsErrCanceledOrKilled(err) {
 			log.Error("error during GetDiff(git diff dir: %s): %v, stderr: %s", repoPath, err, stderr.String())
 		}
 

services/mirror/mirror_pull.go

@@ -604,14 +604,8 @@ func checkAndUpdateEmptyRepository(ctx context.Context, m *repo_model.Mirror, re
 		}
 		// Update the git repository default branch
 		if err := gitrepo.SetDefaultBranch(ctx, m.Repo, m.Repo.DefaultBranch); err != nil {
-			if !git.IsErrUnsupportedVersion(err) {
+			log.Error("Failed to update default branch of underlying git repository %-v. Error: %v", m.Repo, err)
-				log.Error("Failed to update default branch of underlying git repository %-v. Error: %v", m.Repo, err)
+			return false
-				desc := fmt.Sprintf("Failed to update default branch of underlying git repository '%s': %v", m.Repo.RepoPath(), err)
-				if err = system_model.CreateRepositoryNotice(desc); err != nil {
-					log.Error("CreateRepositoryNotice: %v", err)
-				}
-				return false
-			}
 		}
 		m.Repo.IsEmpty = false
 		// Update the is empty and default_branch columns

services/repository/branch.go

@@ -588,12 +588,7 @@ func SetRepoDefaultBranch(ctx context.Context, repo *repo_model.Repository, gitR
 			log.Error("CancelPreviousJobs: %v", err)
 		}
 
-		if err := gitrepo.SetDefaultBranch(ctx, repo, newBranchName); err != nil {
+		return gitrepo.SetDefaultBranch(ctx, repo, newBranchName)
-			if !git.IsErrUnsupportedVersion(err) {
-				return err
-			}
-		}
-		return nil
 	}); err != nil {
 		return err
 	}

services/repository/files/temp_repo.go

@@ -343,8 +343,7 @@ func (t *TemporaryUploadRepository) Push(doer *user_model.User, commitHash, bran
 func (t *TemporaryUploadRepository) DiffIndex() (*gitdiff.Diff, error) {
 	stdoutReader, stdoutWriter, err := os.Pipe()
 	if err != nil {
-		log.Error("Unable to open stdout pipe: %v", err)
+		return nil, fmt.Errorf("unable to open stdout pipe: %w", err)
-		return nil, fmt.Errorf("Unable to open stdout pipe: %w", err)
 	}
 	defer func() {
 		_ = stdoutReader.Close()
@@ -352,9 +351,7 @@ func (t *TemporaryUploadRepository) DiffIndex() (*gitdiff.Diff, error) {
 	}()
 	stderr := new(bytes.Buffer)
 	var diff *gitdiff.Diff
-	var finalErr error
+	err = git.NewCommand(t.ctx, "diff-index", "--src-prefix=\\a/", "--dst-prefix=\\b/", "--cached", "-p", "HEAD").
-
-	if err := git.NewCommand(t.ctx, "diff-index", "--src-prefix=\\a/", "--dst-prefix=\\b/", "--cached", "-p", "HEAD").
 		Run(&git.RunOpts{
 			Timeout: 30 * time.Second,
 			Dir:     t.basePath,
@@ -362,23 +359,20 @@ func (t *TemporaryUploadRepository) DiffIndex() (*gitdiff.Diff, error) {
 			Stderr:  stderr,
 			PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error {
 				_ = stdoutWriter.Close()
-				diff, finalErr = gitdiff.ParsePatch(t.ctx, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdoutReader, "")
+				defer cancel()
-				if finalErr != nil {
+				var diffErr error
-					log.Error("ParsePatch: %v", finalErr)
+				diff, diffErr = gitdiff.ParsePatch(t.ctx, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdoutReader, "")
-					cancel()
-				}
 				_ = stdoutReader.Close()
-				return finalErr
+				if diffErr != nil {
+					// if the diffErr is not nil, it will be returned as the error of "Run()"
+					return fmt.Errorf("ParsePatch: %w", diffErr)
+				}
+				return nil
 			},
-		}); err != nil {
+		})
-		if finalErr != nil {
+	if err != nil && !git.IsErrCanceledOrKilled(err) {
-			log.Error("Unable to ParsePatch in temporary repo %s (%s). Error: %v", t.repo.FullName(), t.basePath, finalErr)
+		log.Error("Unable to diff-index in temporary repo %s (%s). Error: %v\nStderr: %s", t.repo.FullName(), t.basePath, err, stderr)
-			return nil, finalErr
+		return nil, fmt.Errorf("unable to run diff-index pipeline in temporary repo: %w", err)
-		}
-		log.Error("Unable to run diff-index pipeline in temporary repo %s (%s). Error: %v\nStderr: %s",
-			t.repo.FullName(), t.basePath, err, stderr)
-		return nil, fmt.Errorf("Unable to run diff-index pipeline in temporary repo %s. Error: %w\nStderr: %s",
-			t.repo.FullName(), err, stderr)
 	}
 
 	diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(t.ctx, t.basePath, git.TrustedCmdArgs{"--cached"}, "HEAD")

services/repository/push.go

@@ -183,9 +183,7 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
 						repo.IsEmpty = false
 						if repo.DefaultBranch != setting.Repository.DefaultBranch {
 							if err := gitrepo.SetDefaultBranch(ctx, repo, repo.DefaultBranch); err != nil {
-								if !git.IsErrUnsupportedVersion(err) {
+								return err
-									return err
-								}
 							}
 						}
 						// Update the is empty and default_branch columns