Fix git error handling (#32401)

Author: wxiaoguang

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

@@ -166,9 +166,7 @@ func (c *CheckAttributeReader) Run() error {
 		Stdout: c.stdOut,
 		Stderr: stdErr,
 	})
-	if err != nil && //                      If there is an error we need to return but:
-		c.ctx.Err() != err && //             1. Ignore the context error if the context is cancelled or exceeds the deadline (RunWithContext could return c.ctx.Err() which is Canceled or DeadlineExceeded)
-		err.Error() != "signal: killed" { // 2. We should not pass up errors due to the program being killed
+	if err != nil && !IsErrCanceledOrKilled(err) {
 		return fmt.Errorf("failed to run attr-check. Error: %w\nStderr: %s", err, stdErr.String())
 	}
 	return nil

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"
@@ -739,10 +738,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)
-					return err
-				}
+				ctx.Error(http.StatusInternalServerError, "SetDefaultBranch", err)
+				return err
 			}
 			updateRepoLicense = true
 		}

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"
@@ -23,12 +22,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{
-				Err: fmt.Sprintf("Unable to set default branch on repository: %s/%s Error: %v", ownerName, repoName, err),
-			})
-			return
-		}
+		ctx.JSON(http.StatusInternalServerError, private.Response{
+			Err: fmt.Sprintf("Unable to set default branch on repository: %s/%s Error: %v", ownerName, repoName, err),
+		})
+		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

@@ -1162,7 +1162,7 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi
 			Dir:     repoPath,
 			Stdout:  writer,
 			Stderr:  stderr,
-		}); err != nil && err.Error() != "signal: killed" {
+		}); 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

@@ -613,14 +613,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)
-				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
-			}
+			log.Error("Failed to update default branch of underlying git repository %-v. Error: %v", m.Repo, err)
+			return false
 		}
 		m.Repo.IsEmpty = false
 		// Update the is empty and default_branch columns

services/repository/branch.go

@@ -602,12 +602,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 {
-			if !git.IsErrUnsupportedVersion(err) {
-				return err
-			}
-		}
-		return nil
+		return gitrepo.SetDefaultBranch(ctx, repo, newBranchName)
 	}); err != nil {
 		return err
 	}

services/repository/files/temp_repo.go

@@ -339,18 +339,15 @@ 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()
 		_ = stdoutWriter.Close()
 	}()
 	stderr := new(bytes.Buffer)
 	var diff *gitdiff.Diff
-	var finalErr error
-
-	if err := git.NewCommand(t.ctx, "diff-index", "--src-prefix=\\a/", "--dst-prefix=\\b/", "--cached", "-p", "HEAD").
+	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,
@@ -359,27 +356,19 @@ func (t *TemporaryUploadRepository) DiffIndex() (*gitdiff.Diff, error) {
 			PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error {
 				_ = stdoutWriter.Close()
 				defer cancel()
-				diff, finalErr = gitdiff.ParsePatch(t.ctx, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdoutReader, "")
-				if finalErr != nil {
-					log.Error("ParsePatch: %v", finalErr)
-					cancel()
-				}
+				var diffErr error
+				diff, diffErr = gitdiff.ParsePatch(t.ctx, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdoutReader, "")
 				_ = 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 {
-			log.Error("Unable to ParsePatch in temporary repo %s (%s). Error: %v", t.repo.FullName(), t.basePath, finalErr)
-			return nil, finalErr
-		}
-
-		// If the process exited early, don't error
-		if err != context.Canceled {
-			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)
-		}
+		})
+	if err != nil && !git.IsErrCanceledOrKilled(err) {
+		log.Error("Unable to diff-index 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: %w", err)
 	}
 
 	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