Refactor GetRepoRawDiffForFile to avoid unnecessary pipe or goroutine (#36434)

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 {
+func GetRawDiff(repo *Repository, commitID string, diffType RawDiffType, writer io.Writer) (retErr error) {
-	return GetRepoRawDiffForFile(repo, "", commitID, diffType, "", writer)
+	diffOutput, diffFinish, err := getRepoRawDiffForFile(repo.Ctx, repo, "", commitID, diffType, "")
-}
-
-// 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)
 	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).
+	stdoutReader, stdoutReaderClose := cmd.MakeStdoutPipe()
-		WithStdoutCopy(writer).
+	err = cmd.StartWithStderr(ctx)
-		RunWithStderr(repo.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

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()

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
+			patch, _ := git.GetFileDiffCutAroundLine(
-			reader, writer := io.Pipe() // FIXME: use os.Pipe to avoid deadlock
+				g.gitRepo, pr.MergeBase, headCommitID, comment.TreePath,
-			defer func() {
+				int64((&issues_model.Comment{Line: int64(line + comment.Position - 1)}).UnsignedLine()), line < 0, setting.UI.CodeCommentLines,
-				_ = 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)
 
 			if comment.CreatedAt.IsZero() {
 				comment.CreatedAt = review.CreatedAt

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
 		}