From e42a1dbb6b503b2ab5c7dfe97b44f6eda915daa5 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 23 Jan 2026 10:10:11 +0800 Subject: [PATCH] Refactor GetRepoRawDiffForFile to avoid unnecessary pipe or goroutine (#36434) --- modules/git/diff.go | 70 ++++++++++++++++++++------ routers/web/repo/editor_cherry_pick.go | 2 +- services/migrations/gitea_uploader.go | 19 ++----- services/pull/review.go | 19 ++----- 4 files changed, 63 insertions(+), 47 deletions(-) diff --git a/modules/git/diff.go b/modules/git/diff.go index 7f805478d5..a198695fc0 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -5,6 +5,7 @@ package git import ( "bufio" + "context" "fmt" "io" "regexp" @@ -14,34 +15,64 @@ import ( "code.gitea.io/gitea/modules/git/gitcmd" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/util" ) -// RawDiffType type of a raw diff. +// RawDiffType output format: diff or patch type RawDiffType string -// RawDiffType possible values. const ( RawDiffNormal RawDiffType = "diff" RawDiffPatch RawDiffType = "patch" ) // GetRawDiff dumps diff results of repository in given commit ID to io.Writer. -func GetRawDiff(repo *Repository, commitID string, diffType RawDiffType, writer io.Writer) error { - return GetRepoRawDiffForFile(repo, "", commitID, diffType, "", writer) -} - -// GetRepoRawDiffForFile dumps diff results of file in given commit ID to io.Writer according given repository -func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diffType RawDiffType, file string, writer io.Writer) error { - commit, err := repo.GetCommit(endCommit) +func GetRawDiff(repo *Repository, commitID string, diffType RawDiffType, writer io.Writer) (retErr error) { + diffOutput, diffFinish, err := getRepoRawDiffForFile(repo.Ctx, repo, "", commitID, diffType, "") if err != nil { return err } + defer func() { + err := diffFinish() + if retErr == nil { + retErr = err // only return command's error if no previous error + } + }() + _, err = io.Copy(writer, diffOutput) + return err +} + +// GetFileDiffCutAroundLine cuts the old or new part of the diff of a file around a specific line number +func GetFileDiffCutAroundLine( + repo *Repository, startCommit, endCommit, treePath string, + line int64, old bool, numbersOfLine int, +) (_ string, retErr error) { + diffOutput, diffFinish, err := getRepoRawDiffForFile(repo.Ctx, repo, startCommit, endCommit, RawDiffNormal, treePath) + if err != nil { + return "", err + } + defer func() { + err := diffFinish() + if retErr == nil { + retErr = err // only return command's error if no previous error + } + }() + return CutDiffAroundLine(diffOutput, line, old, numbersOfLine) +} + +// getRepoRawDiffForFile returns an io.Reader for the diff results of file in given commit ID +// and a "finish" function to wait for the git command and clean up resources after reading is done. +func getRepoRawDiffForFile(ctx context.Context, repo *Repository, startCommit, endCommit string, diffType RawDiffType, file string) (io.Reader, func() gitcmd.RunStdError, error) { + commit, err := repo.GetCommit(endCommit) + if err != nil { + return nil, nil, err + } var files []string if len(file) > 0 { files = append(files, file) } - cmd := gitcmd.NewCommand() + cmd := gitcmd.NewCommand().WithDir(repo.Path) switch diffType { case RawDiffNormal: if len(startCommit) != 0 { @@ -53,7 +84,7 @@ func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diff } else { c, err := commit.Parent(0) if err != nil { - return err + return nil, nil, err } cmd.AddArguments("diff"). AddOptionFormat("--find-renames=%s", setting.Git.DiffRenameSimilarityThreshold). @@ -68,18 +99,25 @@ func GetRepoRawDiffForFile(repo *Repository, startCommit, endCommit string, diff } else { c, err := commit.Parent(0) if err != nil { - return err + return nil, nil, err } query := fmt.Sprintf("%s...%s", endCommit, c.ID.String()) cmd.AddArguments("format-patch", "--no-signature", "--stdout").AddDynamicArguments(query).AddDashesAndList(files...) } default: - return fmt.Errorf("invalid diffType: %s", diffType) + return nil, nil, util.NewInvalidArgumentErrorf("invalid diff type: %s", diffType) } - return cmd.WithDir(repo.Path). - WithStdoutCopy(writer). - RunWithStderr(repo.Ctx) + stdoutReader, stdoutReaderClose := cmd.MakeStdoutPipe() + err = cmd.StartWithStderr(ctx) + if err != nil { + stdoutReaderClose() + return nil, nil, err + } + return stdoutReader, func() gitcmd.RunStdError { + stdoutReaderClose() + return cmd.WaitWithStderr() + }, nil } // ParseDiffHunkString parse the diff hunk content and return diff --git a/routers/web/repo/editor_cherry_pick.go b/routers/web/repo/editor_cherry_pick.go index ca0e19517a..605a35b100 100644 --- a/routers/web/repo/editor_cherry_pick.go +++ b/routers/web/repo/editor_cherry_pick.go @@ -67,7 +67,7 @@ func CherryPickPost(ctx *context.Context) { if parsed.form.Revert { err = gitrepo.GetReverseRawDiff(ctx, ctx.Repo.Repository, fromCommitID, buf) } else { - err = git.GetRawDiff(ctx.Repo.GitRepo, fromCommitID, "patch", buf) + err = git.GetRawDiff(ctx.Repo.GitRepo, fromCommitID, git.RawDiffPatch, buf) } if err == nil { opts.Content = buf.String() diff --git a/services/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go index eeada3100d..6f5b9bb33a 100644 --- a/services/migrations/gitea_uploader.go +++ b/services/migrations/gitea_uploader.go @@ -895,21 +895,10 @@ func (g *GiteaLocalUploader) CreateReviews(ctx context.Context, reviews ...*base // SECURITY: The TreePath must be cleaned! use relative path comment.TreePath = util.PathJoinRel(comment.TreePath) - var patch string - reader, writer := io.Pipe() // FIXME: use os.Pipe to avoid deadlock - defer func() { - _ = reader.Close() - _ = writer.Close() - }() - go func(comment *base.ReviewComment) { - if err := git.GetRepoRawDiffForFile(g.gitRepo, pr.MergeBase, headCommitID, git.RawDiffNormal, comment.TreePath, writer); err != nil { - // We should ignore the error since the commit maybe removed when force push to the pull request - log.Warn("GetRepoRawDiffForFile failed when migrating [%s, %s, %s, %s]: %v", g.gitRepo.Path, pr.MergeBase, headCommitID, comment.TreePath, err) - } - _ = writer.Close() - }(comment) - - patch, _ = git.CutDiffAroundLine(reader, int64((&issues_model.Comment{Line: int64(line + comment.Position - 1)}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines) + patch, _ := git.GetFileDiffCutAroundLine( + g.gitRepo, pr.MergeBase, headCommitID, comment.TreePath, + int64((&issues_model.Comment{Line: int64(line + comment.Position - 1)}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines, + ) if comment.CreatedAt.IsZero() { comment.CreatedAt = review.CreatedAt diff --git a/services/pull/review.go b/services/pull/review.go index bcd7392063..acbb620e92 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -8,7 +8,6 @@ import ( "context" "errors" "fmt" - "io" "strings" "code.gitea.io/gitea/models/db" @@ -274,22 +273,12 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo if len(commitID) == 0 { commitID = headCommitID } - reader, writer := io.Pipe() // FIXME: use os.Pipe to avoid deadlock - defer func() { - _ = reader.Close() - _ = writer.Close() - }() - go func() { - if err := git.GetRepoRawDiffForFile(gitRepo, pr.MergeBase, headCommitID, git.RawDiffNormal, treePath, writer); err != nil { - _ = writer.CloseWithError(fmt.Errorf("GetRawDiffForLine[%s, %s, %s, %s]: %w", gitRepo.Path, pr.MergeBase, headCommitID, treePath, err)) - return - } - _ = writer.Close() - }() - patch, err = git.CutDiffAroundLine(reader, int64((&issues_model.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines) + patch, err = git.GetFileDiffCutAroundLine( + gitRepo, pr.MergeBase, headCommitID, treePath, + int64((&issues_model.Comment{Line: line}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines, + ) if err != nil { - log.Error("Error whilst generating patch: %v", err) return nil, err }