From e524f63d58900557d7d57fc3bcd19d9facc8b8ee Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 2 Nov 2024 19:20:22 +0800 Subject: [PATCH] Fix git error handling (#32401) --- modules/git/error.go | 40 ++++++-------------------- modules/git/repo_attribute.go | 4 +-- routers/api/v1/repo/repo.go | 7 ++--- routers/private/default_branch.go | 11 +++---- routers/web/repo/githttp.go | 2 +- services/gitdiff/gitdiff.go | 2 +- services/mirror/mirror_pull.go | 10 ++----- services/repository/branch.go | 7 +---- services/repository/files/temp_repo.go | 37 +++++++++--------------- services/repository/push.go | 4 +-- 10 files changed, 35 insertions(+), 89 deletions(-) diff --git a/modules/git/error.go b/modules/git/error.go index 91d25eca69..10fb37be07 100644 --- 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 84f85d1b1a..90eb783fe8 100644 --- 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 698ba3cc94..69a95dd5a5 100644 --- 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 03c19c8ff4..8f6e9084df 100644 --- 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 ee1ec1fd0c..58a2bdbab1 100644 --- 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 cf7da0707b..bb1722039e 100644 --- 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 654a50d11e..a81fe6e9bd 100644 --- 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 67df4363e4..600ba96e92 100644 --- 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 ce98967e79..30ab22db1e 100644 --- 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 8b81588c07..36c7927ab6 100644 --- 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