Fix git error handling (#32401)

diff --git a/modules/git/error.go b/modules/git/error.go
index 91d25eca69a4d209c62e3741f1871ee17d85e143..10fb37be07c13c40cda666d65c10c5c072020ce8 100644 (file)
--- a/modules/git/error.go
+++ b/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")
+}

diff --git a/modules/git/repo_attribute.go b/modules/git/repo_attribute.go
index 84f85d1b1a7cbeb520bb118184770bf72d280955..90eb783fe87619451361c75de9fe5a12c8dba931 100644 (file)
--- a/modules/git/repo_attribute.go
+++ b/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

diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go
index 698ba3cc949d5d8cb3dcbb1cdc199ab9b395ccb6..69a95dd5a58d93133e81816d4fca53a2c5eb134f 100644 (file)
--- a/routers/api/v1/repo/repo.go
+++ b/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
                }

diff --git a/routers/private/default_branch.go b/routers/private/default_branch.go
index 03c19c8ff4020c8544934229d16941a389ebdf47..8f6e9084df79e97549f15e225f6a95c6912fbf70 100644 (file)
--- a/routers/private/default_branch.go
+++ b/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 {

diff --git a/routers/web/repo/githttp.go b/routers/web/repo/githttp.go
index ee1ec1fd0c76397dd13a1809ea43349583a0d0a7..58a2bdbab1c34db3d0d0e888d131f01e3aacd7af 100644 (file)
--- a/routers/web/repo/githttp.go
+++ b/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

diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go
index cf7da0707b8869d51c406ec94cda675f4afdfb40..bb1722039e19d020a3cf733fa4fa079db4d6c4dd 100644 (file)
--- a/services/gitdiff/gitdiff.go
+++ b/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())
                }
 

diff --git a/services/mirror/mirror_pull.go b/services/mirror/mirror_pull.go
index 654a50d11efa6e59dfa4a91dded1a2e35f71b9a3..a81fe6e9bd321d41b5ca2cfd9d69428b60ab979a 100644 (file)
--- a/services/mirror/mirror_pull.go
+++ b/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

diff --git a/services/repository/branch.go b/services/repository/branch.go
index 67df4363e441a5c1f2795e54facd4f3084c5f857..600ba96e92e17a49dd169976b821b1413c62e9c5 100644 (file)
--- a/services/repository/branch.go
+++ b/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
        }

diff --git a/services/repository/files/temp_repo.go b/services/repository/files/temp_repo.go
index ce98967e7976a57a08de7f6e547031cc1e558ea1..30ab22db1e47687b6f85e074d8dbc98b1d777cdf 100644 (file)
--- a/services/repository/files/temp_repo.go
+++ b/services/repository/files/temp_repo.go
@@ -339,8 +339,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()
@@ -348,9 +347,7 @@ func (t *TemporaryUploadRepository) DiffIndex() (*gitdiff.Diff, error) {
        }()
        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")

diff --git a/services/repository/push.go b/services/repository/push.go
index 8b81588c072854f2350caf7716cf8ab57c4bbfe4..36c7927ab61f38c5d9472ebfbc3e5911efb2e52c 100644 (file)
--- a/services/repository/push.go
+++ b/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