Decouple diff stats query from actual diffing (#33810)

The diff stats are no longer part of the diff generation.
Use `GetDiffShortStat` instead to get the total number of changed files,
added lines, and deleted lines.
As such, `gitdiff.GetDiff` can be simplified:
It should not do more than expected.

And do not run "git diff --shortstat" for pull list. Fix #31492

modules/git/repo_compare.go

@@ -174,17 +174,9 @@ func (repo *Repository) GetDiffNumChangedFiles(base, head string, directComparis
 	return w.numLines, nil
 }
 
-// GetDiffShortStat counts number of changed files, number of additions and deletions
+// GetDiffShortStatByCmdArgs counts number of changed files, number of additions and deletions
-func (repo *Repository) GetDiffShortStat(base, head string) (numFiles, totalAdditions, totalDeletions int, err error) {
+// TODO: it can be merged with another "GetDiffShortStat" in the future
-	numFiles, totalAdditions, totalDeletions, err = GetDiffShortStat(repo.Ctx, repo.Path, nil, base+"..."+head)
+func GetDiffShortStatByCmdArgs(ctx context.Context, repoPath string, trustedArgs TrustedCmdArgs, dynamicArgs ...string) (numFiles, totalAdditions, totalDeletions int, err error) {
-	if err != nil && strings.Contains(err.Error(), "no merge base") {
-		return GetDiffShortStat(repo.Ctx, repo.Path, nil, base, head)
-	}
-	return numFiles, totalAdditions, totalDeletions, err
-}
-
-// GetDiffShortStat counts number of changed files, number of additions and deletions
-func GetDiffShortStat(ctx context.Context, repoPath string, trustedArgs TrustedCmdArgs, dynamicArgs ...string) (numFiles, totalAdditions, totalDeletions int, err error) {
 	// Now if we call:
 	// $ git diff --shortstat 1ebb35b98889ff77299f24d82da426b434b0cca0...788b8b1440462d477f45b0088875
 	// we get:

modules/structs/pull.go

@@ -25,11 +25,13 @@ type PullRequest struct {
 	Draft                   bool       `json:"draft"`
 	IsLocked                bool       `json:"is_locked"`
 	Comments                int        `json:"comments"`
+
 	// number of review comments made on the diff of a PR review (not including comments on commits or issues in a PR)
-	ReviewComments int `json:"review_comments"`
+	ReviewComments int `json:"review_comments,omitempty"`
-	Additions      int `json:"additions"`
+
-	Deletions      int `json:"deletions"`
+	Additions    *int `json:"additions,omitempty"`
-	ChangedFiles   int `json:"changed_files"`
+	Deletions    *int `json:"deletions,omitempty"`
+	ChangedFiles *int `json:"changed_files,omitempty"`
 
 	HTMLURL  string `json:"html_url"`
 	DiffURL  string `json:"diff_url"`

routers/api/v1/repo/pull.go

@@ -1591,6 +1591,7 @@ func GetPullRequestFiles(ctx *context.APIContext) {
 	maxLines := setting.Git.MaxGitDiffLines
 
 	// FIXME: If there are too many files in the repo, may cause some unpredictable issues.
+	// FIXME: it doesn't need to call "GetDiff" to do various parsing and highlighting
 	diff, err := gitdiff.GetDiff(ctx, baseGitRepo,
 		&gitdiff.DiffOptions{
 			BeforeCommitID:     startCommitID,
@@ -1606,9 +1607,14 @@ func GetPullRequestFiles(ctx *context.APIContext) {
 		return
 	}
 
+	diffShortStat, err := gitdiff.GetDiffShortStat(baseGitRepo, startCommitID, endCommitID)
+	if err != nil {
+		ctx.APIErrorInternal(err)
+		return
+	}
 	listOptions := utils.GetListOptions(ctx)
 
-	totalNumberOfFiles := diff.NumFiles
+	totalNumberOfFiles := diffShortStat.NumFiles
 	totalNumberOfPages := int(math.Ceil(float64(totalNumberOfFiles) / float64(listOptions.PageSize)))
 
 	start, limit := listOptions.GetSkipTake()

routers/web/repo/commit.go

@@ -321,12 +321,17 @@ func Diff(ctx *context.Context) {
 		MaxLineCharacters:  setting.Git.MaxGitDiffLineCharacters,
 		MaxFiles:           maxFiles,
 		WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)),
-		FileOnly:           fileOnly,
 	}, files...)
 	if err != nil {
 		ctx.NotFound(err)
 		return
 	}
+	diffShortStat, err := gitdiff.GetDiffShortStat(gitRepo, "", commitID)
+	if err != nil {
+		ctx.ServerError("GetDiffShortStat", err)
+		return
+	}
+	ctx.Data["DiffShortStat"] = diffShortStat
 
 	parents := make([]string, commit.ParentCount())
 	for i := 0; i < commit.ParentCount(); i++ {
@@ -383,7 +388,7 @@ func Diff(ctx *context.Context) {
 	ctx.Data["Verification"] = verification
 	ctx.Data["Author"] = user_model.ValidateCommitWithEmail(ctx, commit)
 	ctx.Data["Parents"] = parents
-	ctx.Data["DiffNotAvailable"] = diff.NumFiles == 0
+	ctx.Data["DiffNotAvailable"] = diffShortStat.NumFiles == 0
 
 	if err := asymkey_model.CalculateTrustStatus(verification, ctx.Repo.Repository.GetTrustModel(), func(user *user_model.User) (bool, error) {
 		return repo_model.IsOwnerMemberCollaborator(ctx, ctx.Repo.Repository, user.ID)

routers/web/repo/compare.go

@@ -624,14 +624,19 @@ func PrepareCompareDiff(
 			MaxFiles:           maxFiles,
 			WhitespaceBehavior: whitespaceBehavior,
 			DirectComparison:   ci.DirectComparison,
-			FileOnly:           fileOnly,
 		}, ctx.FormStrings("files")...)
 	if err != nil {
-		ctx.ServerError("GetDiffRangeWithWhitespaceBehavior", err)
+		ctx.ServerError("GetDiff", err)
 		return false
 	}
+	diffShortStat, err := gitdiff.GetDiffShortStat(ci.HeadGitRepo, beforeCommitID, headCommitID)
+	if err != nil {
+		ctx.ServerError("GetDiffShortStat", err)
+		return false
+	}
+	ctx.Data["DiffShortStat"] = diffShortStat
 	ctx.Data["Diff"] = diff
-	ctx.Data["DiffNotAvailable"] = diff.NumFiles == 0
+	ctx.Data["DiffNotAvailable"] = diffShortStat.NumFiles == 0
 
 	if !fileOnly {
 		diffTree, err := gitdiff.GetDiffTree(ctx, ci.HeadGitRepo, false, beforeCommitID, headCommitID)

routers/web/repo/editor.go

@@ -423,7 +423,7 @@ func DiffPreviewPost(ctx *context.Context) {
 		return
 	}
 
-	if diff.NumFiles != 0 {
+	if len(diff.Files) != 0 {
 		ctx.Data["File"] = diff.Files[0]
 	}
 

routers/web/repo/pull.go

@@ -200,22 +200,13 @@ func GetPullDiffStats(ctx *context.Context) {
 		return
 	}
 
-	diffOptions := &gitdiff.DiffOptions{
+	diffShortStat, err := gitdiff.GetDiffShortStat(ctx.Repo.GitRepo, mergeBaseCommitID, headCommitID)
-		BeforeCommitID:     mergeBaseCommitID,
-		AfterCommitID:      headCommitID,
-		MaxLines:           setting.Git.MaxGitDiffLines,
-		MaxLineCharacters:  setting.Git.MaxGitDiffLineCharacters,
-		MaxFiles:           setting.Git.MaxGitDiffFiles,
-		WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)),
-	}
-
-	diff, err := gitdiff.GetPullDiffStats(ctx.Repo.GitRepo, diffOptions)
 	if err != nil {
-		ctx.ServerError("GetPullDiffStats", err)
+		ctx.ServerError("GetDiffShortStat", err)
 		return
 	}
 
-	ctx.Data["Diff"] = diff
+	ctx.Data["DiffShortStat"] = diffShortStat
 }
 
 func GetMergedBaseCommitID(ctx *context.Context, issue *issues_model.Issue) string {
@@ -752,36 +743,43 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
 		MaxLineCharacters:  setting.Git.MaxGitDiffLineCharacters,
 		MaxFiles:           maxFiles,
 		WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)),
-		FileOnly:           fileOnly,
 	}
 
 	if !willShowSpecifiedCommit {
 		diffOptions.BeforeCommitID = startCommitID
 	}
 
-	var methodWithError string
+	diff, err := gitdiff.GetDiff(ctx, gitRepo, diffOptions, files...)
-	var diff *gitdiff.Diff
+	if err != nil {
-	shouldGetUserSpecificDiff := false
+		ctx.ServerError("GetDiff", err)
+		return
+	}
 
 	// if we're not logged in or only a single commit (or commit range) is shown we
 	// have to load only the diff and not get the viewed information
 	// as the viewed information is designed to be loaded only on latest PR
 	// diff and if you're signed in.
+	shouldGetUserSpecificDiff := false
 	if !ctx.IsSigned || willShowSpecifiedCommit || willShowSpecifiedCommitRange {
-		diff, err = gitdiff.GetDiff(ctx, gitRepo, diffOptions, files...)
+		// do nothing
-		methodWithError = "GetDiff"
 	} else {
-		diff, err = gitdiff.SyncAndGetUserSpecificDiff(ctx, ctx.Doer.ID, pull, gitRepo, diffOptions, files...)
-		methodWithError = "SyncAndGetUserSpecificDiff"
 		shouldGetUserSpecificDiff = true
-	}
+		err = gitdiff.SyncUserSpecificDiff(ctx, ctx.Doer.ID, pull, gitRepo, diff, diffOptions, files...)
 		if err != nil {
-		ctx.ServerError(methodWithError, err)
+			ctx.ServerError("SyncUserSpecificDiff", err)
 			return
 		}
+	}
+
+	diffShortStat, err := gitdiff.GetDiffShortStat(ctx.Repo.GitRepo, startCommitID, endCommitID)
+	if err != nil {
+		ctx.ServerError("GetDiffShortStat", err)
+		return
+	}
+	ctx.Data["DiffShortStat"] = diffShortStat
 
 	ctx.PageData["prReview"] = map[string]any{
-		"numberOfFiles":       diff.NumFiles,
+		"numberOfFiles":       diffShortStat.NumFiles,
 		"numberOfViewedFiles": diff.NumViewedFiles,
 	}
 
@@ -840,7 +838,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
 	}
 
 	ctx.Data["Diff"] = diff
-	ctx.Data["DiffNotAvailable"] = diff.NumFiles == 0
+	ctx.Data["DiffNotAvailable"] = diffShortStat.NumFiles == 0
 
 	baseCommit, err := ctx.Repo.GitRepo.GetCommit(startCommitID)
 	if err != nil {

services/convert/git_commit.go

@@ -210,17 +210,15 @@ func ToCommit(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Rep
 
 	// Get diff stats for commit
 	if opts.Stat {
-		diff, err := gitdiff.GetDiff(ctx, gitRepo, &gitdiff.DiffOptions{
+		diffShortStat, err := gitdiff.GetDiffShortStat(gitRepo, "", commit.ID.String())
-			AfterCommitID: commit.ID.String(),
-		})
 		if err != nil {
 			return nil, err
 		}
 
 		res.Stats = &api.CommitStats{
-			Total:     diff.TotalAddition + diff.TotalDeletion,
+			Total:     diffShortStat.TotalAddition + diffShortStat.TotalDeletion,
-			Additions: diff.TotalAddition,
+			Additions: diffShortStat.TotalAddition,
-			Deletions: diff.TotalDeletion,
+			Deletions: diffShortStat.TotalDeletion,
 		}
 	}
 

services/convert/pull.go

@@ -17,8 +17,10 @@ import (
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/gitrepo"
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/util"
+	"code.gitea.io/gitea/services/gitdiff"
 )
 
 // ToAPIPullRequest assumes following fields have been assigned with valid values:
@@ -239,9 +241,13 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u
 		// Calculate diff
 		startCommitID = pr.MergeBase
 
-		apiPullRequest.ChangedFiles, apiPullRequest.Additions, apiPullRequest.Deletions, err = gitRepo.GetDiffShortStat(startCommitID, endCommitID)
+		diffShortStats, err := gitdiff.GetDiffShortStat(gitRepo, startCommitID, endCommitID)
 		if err != nil {
 			log.Error("GetDiffShortStat: %v", err)
+		} else {
+			apiPullRequest.ChangedFiles = &diffShortStats.NumFiles
+			apiPullRequest.Additions = &diffShortStats.TotalAddition
+			apiPullRequest.Deletions = &diffShortStats.TotalDeletion
 		}
 	}
 
@@ -462,12 +468,6 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs
 				return nil, err
 			}
 
-			// Outer scope variables to be used in diff calculation
-			var (
-				startCommitID string
-				endCommitID   string
-			)
-
 			if git.IsErrBranchNotExist(err) {
 				headCommitID, err := headGitRepo.GetRefCommitID(apiPullRequest.Head.Ref)
 				if err != nil && !git.IsErrNotExist(err) {
@@ -476,7 +476,6 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs
 				}
 				if err == nil {
 					apiPullRequest.Head.Sha = headCommitID
-					endCommitID = headCommitID
 				}
 			} else {
 				commit, err := headBranch.GetCommit()
@@ -487,17 +486,8 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs
 				if err == nil {
 					apiPullRequest.Head.Ref = pr.HeadBranch
 					apiPullRequest.Head.Sha = commit.ID.String()
-					endCommitID = commit.ID.String()
 				}
 			}
-
-			// Calculate diff
-			startCommitID = pr.MergeBase
-
-			apiPullRequest.ChangedFiles, apiPullRequest.Additions, apiPullRequest.Deletions, err = gitRepo.GetDiffShortStat(startCommitID, endCommitID)
-			if err != nil {
-				log.Error("GetDiffShortStat: %v", err)
-			}
 		}
 
 		if len(apiPullRequest.Head.Sha) == 0 && len(apiPullRequest.Head.Ref) != 0 {
@@ -518,6 +508,12 @@ func ToAPIPullRequests(ctx context.Context, baseRepo *repo_model.Repository, prs
 			apiPullRequest.MergedBy = ToUser(ctx, pr.Merger, nil)
 		}
 
+		// Do not provide "ChangeFiles/Additions/Deletions" for the PR list, because the "diff" is quite slow
+		// If callers are interested in these values, they should do a separate request to get the PR details
+		if apiPullRequest.ChangedFiles != nil || apiPullRequest.Additions != nil || apiPullRequest.Deletions != nil {
+			setting.PanicInDevOrTesting("ChangedFiles/Additions/Deletions should not be set in PR list")
+		}
+
 		apiPullRequests = append(apiPullRequests, apiPullRequest)
 	}
 

services/gitdiff/gitdiff.go

@@ -382,8 +382,8 @@ func (diffFile *DiffFile) GetType() int {
 }
 
 // GetTailSection creates a fake DiffLineSection if the last section is not the end of the file
-func (diffFile *DiffFile) GetTailSection(gitRepo *git.Repository, leftCommit, rightCommit *git.Commit) *DiffSection {
+func (diffFile *DiffFile) GetTailSection(leftCommit, rightCommit *git.Commit) *DiffSection {
-	if len(diffFile.Sections) == 0 || diffFile.Type != DiffFileChange || diffFile.IsBin || diffFile.IsLFSFile {
+	if len(diffFile.Sections) == 0 || leftCommit == nil || diffFile.Type != DiffFileChange || diffFile.IsBin || diffFile.IsLFSFile {
 		return nil
 	}
 
@@ -453,8 +453,6 @@ func getCommitFileLineCount(commit *git.Commit, filePath string) int {
 // Diff represents a difference between two git trees.
 type Diff struct {
 	Start, End     string
-	NumFiles                     int
-	TotalAddition, TotalDeletion int
 	Files          []*DiffFile
 	IsIncomplete   bool
 	NumViewedFiles int // user-specific
@@ -699,8 +697,6 @@ parsingLoop:
 				}
 				// Otherwise do nothing with this line, but now switch to parsing hunks
 				lineBytes, isFragment, err := parseHunks(ctx, curFile, maxLines, maxLineCharacters, input)
-				diff.TotalAddition += curFile.Addition
-				diff.TotalDeletion += curFile.Deletion
 				if err != nil {
 					if err != io.EOF {
 						return diff, err
@@ -773,7 +769,6 @@ parsingLoop:
 		}
 	}
 
-	diff.NumFiles = len(diff.Files)
 	return diff, nil
 }
 
@@ -1104,7 +1099,26 @@ type DiffOptions struct {
 	MaxFiles           int
 	WhitespaceBehavior git.TrustedCmdArgs
 	DirectComparison   bool
-	FileOnly           bool
+}
+
+func guessBeforeCommitForDiff(gitRepo *git.Repository, beforeCommitID string, afterCommit *git.Commit) (actualBeforeCommit *git.Commit, actualBeforeCommitID git.ObjectID, err error) {
+	commitObjectFormat := afterCommit.ID.Type()
+	isBeforeCommitIDEmpty := beforeCommitID == "" || beforeCommitID == commitObjectFormat.EmptyObjectID().String()
+
+	if isBeforeCommitIDEmpty && afterCommit.ParentCount() == 0 {
+		actualBeforeCommitID = commitObjectFormat.EmptyTree()
+	} else {
+		if isBeforeCommitIDEmpty {
+			actualBeforeCommit, err = afterCommit.Parent(0)
+		} else {
+			actualBeforeCommit, err = gitRepo.GetCommit(beforeCommitID)
+		}
+		if err != nil {
+			return nil, nil, err
+		}
+		actualBeforeCommitID = actualBeforeCommit.ID
+	}
+	return actualBeforeCommit, actualBeforeCommitID, nil
 }
 
 // GetDiff builds a Diff between two commits of a repository.
@@ -1113,46 +1127,19 @@ type DiffOptions struct {
 func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) {
 	repoPath := gitRepo.Path
 
-	var beforeCommit *git.Commit
+	afterCommit, err := gitRepo.GetCommit(opts.AfterCommitID)
-	commit, err := gitRepo.GetCommit(opts.AfterCommitID)
 	if err != nil {
 		return nil, err
 	}
 
-	cmdCtx, cmdCancel := context.WithCancel(ctx)
+	actualBeforeCommit, actualBeforeCommitID, err := guessBeforeCommitForDiff(gitRepo, opts.BeforeCommitID, afterCommit)
-	defer cmdCancel()
-
-	cmdDiff := git.NewCommand()
-	objectFormat, err := gitRepo.GetObjectFormat()
 	if err != nil {
 		return nil, err
 	}
 
-	if (len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == objectFormat.EmptyObjectID().String()) && commit.ParentCount() == 0 {
+	cmdDiff := git.NewCommand().
-		cmdDiff.AddArguments("diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M").
+		AddArguments("diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M").
-			AddArguments(opts.WhitespaceBehavior...).
+		AddArguments(opts.WhitespaceBehavior...)
-			AddDynamicArguments(objectFormat.EmptyTree().String()).
-			AddDynamicArguments(opts.AfterCommitID)
-	} else {
-		actualBeforeCommitID := opts.BeforeCommitID
-		if len(actualBeforeCommitID) == 0 {
-			parentCommit, err := commit.Parent(0)
-			if err != nil {
-				return nil, err
-			}
-			actualBeforeCommitID = parentCommit.ID.String()
-		}
-
-		cmdDiff.AddArguments("diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M").
-			AddArguments(opts.WhitespaceBehavior...).
-			AddDynamicArguments(actualBeforeCommitID, opts.AfterCommitID)
-		opts.BeforeCommitID = actualBeforeCommitID
-
-		beforeCommit, err = gitRepo.GetCommit(opts.BeforeCommitID)
-		if err != nil {
-			return nil, err
-		}
-	}
 
 	// In git 2.31, git diff learned --skip-to which we can use to shortcut skip to file
 	// so if we are using at least this version of git we don't have to tell ParsePatch to do
@@ -1163,8 +1150,12 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi
 		parsePatchSkipToFile = ""
 	}
 
+	cmdDiff.AddDynamicArguments(actualBeforeCommitID.String(), opts.AfterCommitID)
 	cmdDiff.AddDashesAndList(files...)
 
+	cmdCtx, cmdCancel := context.WithCancel(ctx)
+	defer cmdCancel()
+
 	reader, writer := io.Pipe()
 	defer func() {
 		_ = reader.Close()
@@ -1214,7 +1205,7 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi
 
 		// Populate Submodule URLs
 		if diffFile.SubmoduleDiffInfo != nil {
-			diffFile.SubmoduleDiffInfo.PopulateURL(diffFile, beforeCommit, commit)
+			diffFile.SubmoduleDiffInfo.PopulateURL(diffFile, actualBeforeCommit, afterCommit)
 		}
 
 		if !isVendored.Has() {
@@ -1227,75 +1218,45 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi
 		}
 		diffFile.IsGenerated = isGenerated.Value()
 
-		tailSection := diffFile.GetTailSection(gitRepo, beforeCommit, commit)
+		tailSection := diffFile.GetTailSection(actualBeforeCommit, afterCommit)
 		if tailSection != nil {
 			diffFile.Sections = append(diffFile.Sections, tailSection)
 		}
 	}
-
-	if opts.FileOnly {
 	return diff, nil
 }
 
-	stats, err := GetPullDiffStats(gitRepo, opts)
+type DiffShortStat struct {
-	if err != nil {
-		return nil, err
-	}
-
-	diff.NumFiles, diff.TotalAddition, diff.TotalDeletion = stats.NumFiles, stats.TotalAddition, stats.TotalDeletion
-
-	return diff, nil
-}
-
-type PullDiffStats struct {
 	NumFiles, TotalAddition, TotalDeletion int
 }
 
-// GetPullDiffStats
+func GetDiffShortStat(gitRepo *git.Repository, beforeCommitID, afterCommitID string) (*DiffShortStat, error) {
-func GetPullDiffStats(gitRepo *git.Repository, opts *DiffOptions) (*PullDiffStats, error) {
 	repoPath := gitRepo.Path
 
-	diff := &PullDiffStats{}
+	afterCommit, err := gitRepo.GetCommit(afterCommitID)
-
-	separator := "..."
-	if opts.DirectComparison {
-		separator = ".."
-	}
-
-	objectFormat, err := gitRepo.GetObjectFormat()
 	if err != nil {
 		return nil, err
 	}
 
-	diffPaths := []string{opts.BeforeCommitID + separator + opts.AfterCommitID}
+	_, actualBeforeCommitID, err := guessBeforeCommitForDiff(gitRepo, beforeCommitID, afterCommit)
-	if len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == objectFormat.EmptyObjectID().String() {
-		diffPaths = []string{objectFormat.EmptyTree().String(), opts.AfterCommitID}
-	}
-
-	diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...)
-	if err != nil && strings.Contains(err.Error(), "no merge base") {
-		// git >= 2.28 now returns an error if base and head have become unrelated.
-		// previously it would return the results of git diff --shortstat base head so let's try that...
-		diffPaths = []string{opts.BeforeCommitID, opts.AfterCommitID}
-		diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...)
-	}
 	if err != nil {
 		return nil, err
 	}
 
+	diff := &DiffShortStat{}
+	diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStatByCmdArgs(gitRepo.Ctx, repoPath, nil, actualBeforeCommitID.String(), afterCommitID)
+	if err != nil {
+		return nil, err
+	}
 	return diff, nil
 }
 
-// SyncAndGetUserSpecificDiff is like GetDiff, except that user specific data such as which files the given user has already viewed on the given PR will also be set
+// SyncUserSpecificDiff inserts user-specific data such as which files the user has already viewed on the given diff
-// Additionally, the database asynchronously is updated if files have changed since the last review
+// Additionally, the database is updated asynchronously if files have changed since the last review
-func SyncAndGetUserSpecificDiff(ctx context.Context, userID int64, pull *issues_model.PullRequest, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) {
+func SyncUserSpecificDiff(ctx context.Context, userID int64, pull *issues_model.PullRequest, gitRepo *git.Repository, diff *Diff, opts *DiffOptions, files ...string) error {
-	diff, err := GetDiff(ctx, gitRepo, opts, files...)
-	if err != nil {
-		return nil, err
-	}
 	review, err := pull_model.GetNewestReviewState(ctx, userID, pull.ID)
 	if err != nil || review == nil || review.UpdatedFiles == nil {
-		return diff, err
+		return err
 	}
 
 	latestCommit := opts.AfterCommitID
@@ -1348,11 +1309,11 @@ outer:
 		err := pull_model.UpdateReviewState(ctx, review.UserID, review.PullID, review.CommitSHA, filesChangedSinceLastDiff)
 		if err != nil {
 			log.Warn("Could not update review for user %d, pull %d, commit %s and the changed files %v: %v", review.UserID, review.PullID, review.CommitSHA, filesChangedSinceLastDiff, err)
-			return nil, err
+			return err
 		}
 	}
 
-	return diff, nil
+	return nil
 }
 
 // CommentAsDiff returns c.Patch as *Diff

services/gitdiff/gitdiff_test.go

@@ -182,16 +182,10 @@ diff --git "\\a/README.md" "\\b/README.md"
 			}
 
 			gotMarshaled, _ := json.MarshalIndent(got, "", "  ")
-			if got.NumFiles != 1 {
+			if len(got.Files) != 1 {
 				t.Errorf("ParsePath(%q) did not receive 1 file:\n%s", testcase.name, string(gotMarshaled))
 				return
 			}
-			if got.TotalAddition != testcase.addition {
-				t.Errorf("ParsePath(%q) does not have correct totalAddition %d, wanted %d", testcase.name, got.TotalAddition, testcase.addition)
-			}
-			if got.TotalDeletion != testcase.deletion {
-				t.Errorf("ParsePath(%q) did not have correct totalDeletion %d, wanted %d", testcase.name, got.TotalDeletion, testcase.deletion)
-			}
 			file := got.Files[0]
 			if file.Addition != testcase.addition {
 				t.Errorf("ParsePath(%q) does not have correct file addition %d, wanted %d", testcase.name, file.Addition, testcase.addition)
@@ -407,16 +401,10 @@ index 6961180..9ba1a00 100644
 			}
 
 			gotMarshaled, _ := json.MarshalIndent(got, "", "  ")
-			if got.NumFiles != 1 {
+			if len(got.Files) != 1 {
 				t.Errorf("ParsePath(%q) did not receive 1 file:\n%s", testcase.name, string(gotMarshaled))
 				return
 			}
-			if got.TotalAddition != testcase.addition {
-				t.Errorf("ParsePath(%q) does not have correct totalAddition %d, wanted %d", testcase.name, got.TotalAddition, testcase.addition)
-			}
-			if got.TotalDeletion != testcase.deletion {
-				t.Errorf("ParsePath(%q) did not have correct totalDeletion %d, wanted %d", testcase.name, got.TotalDeletion, testcase.deletion)
-			}
 			file := got.Files[0]
 			if file.Addition != testcase.addition {
 				t.Errorf("ParsePath(%q) does not have correct file addition %d, wanted %d", testcase.name, file.Addition, testcase.addition)

services/repository/files/diff_test.go

@@ -30,8 +30,6 @@ func TestGetDiffPreview(t *testing.T) {
 	content := "# repo1\n\nDescription for repo1\nthis is a new line"
 
 	expectedDiff := &gitdiff.Diff{
-		TotalAddition: 2,
-		TotalDeletion: 1,
 		Files: []*gitdiff.DiffFile{
 			{
 				Name:        "README.md",
@@ -114,7 +112,6 @@ func TestGetDiffPreview(t *testing.T) {
 		},
 		IsIncomplete: false,
 	}
-	expectedDiff.NumFiles = len(expectedDiff.Files)
 
 	t.Run("with given branch", func(t *testing.T) {
 		diff, err := GetDiffPreview(ctx, ctx.Repo.Repository, branch, treePath, content)

services/repository/files/temp_repo.go

@@ -409,11 +409,6 @@ func (t *TemporaryUploadRepository) DiffIndex() (*gitdiff.Diff, error) {
 		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")
-	if err != nil {
-		return nil, err
-	}
-
 	return diff, nil
 }
 

templates/repo/diff/box.tmpl

@@ -1,4 +1,4 @@
-{{$showFileTree := (and (not .DiffNotAvailable) (gt .Diff.NumFiles 1))}}
+{{$showFileTree := (and (not .DiffNotAvailable) (gt .DiffShortStat.NumFiles 1))}}
 <div>
 	<div class="diff-detail-box">
 		<div class="tw-flex tw-items-center tw-flex-wrap tw-gap-2 tw-ml-0.5">
@@ -19,7 +19,7 @@
 			{{end}}
 			{{if not .DiffNotAvailable}}
 				<div class="diff-detail-stats tw-flex tw-items-center tw-flex-wrap">
-					{{svg "octicon-diff" 16 "tw-mr-1"}}{{ctx.Locale.Tr "repo.diff.stats_desc" .Diff.NumFiles .Diff.TotalAddition .Diff.TotalDeletion}}
+					{{svg "octicon-diff" 16 "tw-mr-1"}}{{ctx.Locale.Tr "repo.diff.stats_desc" .DiffShortStat.NumFiles .DiffShortStat.TotalAddition .DiffShortStat.TotalDeletion}}
 				</div>
 			{{end}}
 		</div>
@@ -27,9 +27,9 @@
 			{{if and .PageIsPullFiles $.SignedUserID (not .DiffNotAvailable)}}
 				<div class="not-mobile tw-flex tw-items-center tw-flex-col tw-whitespace-nowrap tw-mr-1">
 					<label for="viewed-files-summary" id="viewed-files-summary-label" data-text-changed-template="{{ctx.Locale.Tr "repo.pulls.viewed_files_label"}}">
-						{{ctx.Locale.Tr "repo.pulls.viewed_files_label" .Diff.NumViewedFiles .Diff.NumFiles}}
+						{{ctx.Locale.Tr "repo.pulls.viewed_files_label" .Diff.NumViewedFiles .DiffShortStat.NumFiles}}
 					</label>
-					<progress id="viewed-files-summary" value="{{.Diff.NumViewedFiles}}" max="{{.Diff.NumFiles}}"></progress>
+					<progress id="viewed-files-summary" value="{{.Diff.NumViewedFiles}}" max="{{.DiffShortStat.NumFiles}}"></progress>
 				</div>
 			{{end}}
 			{{template "repo/diff/whitespace_dropdown" .}}

templates/repo/pulls/tab_menu.tmpl

@@ -15,11 +15,11 @@
 			{{template "shared/misc/tabtitle" (ctx.Locale.Tr "repo.pulls.tab_files")}}
 			<span class="ui small label">{{if .NumFiles}}{{.NumFiles}}{{else}}-{{end}}</span>
 		</a>
-		{{if or .Diff.TotalAddition .Diff.TotalDeletion}}
+		{{if or .DiffShortStat.TotalAddition .DiffShortStat.TotalDeletion}}
 		<span class="tw-ml-auto tw-pl-3 tw-whitespace-nowrap tw-pr-0 tw-font-bold tw-flex tw-items-center tw-gap-2">
-			<span><span class="text green">{{if .Diff.TotalAddition}}+{{.Diff.TotalAddition}}{{end}}</span> <span class="text red">{{if .Diff.TotalDeletion}}-{{.Diff.TotalDeletion}}{{end}}</span></span>
+			<span><span class="text green">{{if .DiffShortStat.TotalAddition}}+{{.DiffShortStat.TotalAddition}}{{end}}</span> <span class="text red">{{if .DiffShortStat.TotalDeletion}}-{{.DiffShortStat.TotalDeletion}}{{end}}</span></span>
 			<span class="diff-stats-bar">
-				<div class="diff-stats-add-bar" style="width: {{Eval 100 "*" .Diff.TotalAddition "/" "(" .Diff.TotalAddition "+" .Diff.TotalDeletion "+" 0.0 ")"}}%"></div>
+				<div class="diff-stats-add-bar" style="width: {{Eval 100 "*" .DiffShortStat.TotalAddition "/" "(" .DiffShortStat.TotalAddition "+" .DiffShortStat.TotalDeletion "+" 0.0 ")"}}%"></div>
 			</span>
 		</span>
 		{{end}}

tests/integration/api_pull_test.go

@@ -50,7 +50,6 @@ func TestAPIViewPulls(t *testing.T) {
 	assert.Empty(t, pull.RequestedReviewersTeams)
 	assert.EqualValues(t, 5, pull.RequestedReviewers[0].ID)
 	assert.EqualValues(t, 6, pull.RequestedReviewers[1].ID)
-	assert.EqualValues(t, 1, pull.ChangedFiles)
 
 	if assert.EqualValues(t, 5, pull.ID) {
 		resp = ctx.Session.MakeRequest(t, NewRequest(t, "GET", pull.DiffURL), http.StatusOK)
@@ -58,22 +57,23 @@ func TestAPIViewPulls(t *testing.T) {
 		assert.NoError(t, err)
 		patch, err := gitdiff.ParsePatch(t.Context(), 1000, 5000, 10, bytes.NewReader(bs), "")
 		assert.NoError(t, err)
-		if assert.Len(t, patch.Files, pull.ChangedFiles) {
+		if assert.Len(t, patch.Files, 1) {
 			assert.Equal(t, "File-WoW", patch.Files[0].Name)
 			// FIXME: The old name should be empty if it's a file add type
 			assert.Equal(t, "File-WoW", patch.Files[0].OldName)
-			assert.EqualValues(t, pull.Additions, patch.Files[0].Addition)
+			assert.EqualValues(t, 1, patch.Files[0].Addition)
-			assert.EqualValues(t, pull.Deletions, patch.Files[0].Deletion)
+			assert.EqualValues(t, 0, patch.Files[0].Deletion)
 			assert.Equal(t, gitdiff.DiffFileAdd, patch.Files[0].Type)
 		}
 
 		t.Run(fmt.Sprintf("APIGetPullFiles_%d", pull.ID),
 			doAPIGetPullFiles(ctx, pull, func(t *testing.T, files []*api.ChangedFile) {
-				if assert.Len(t, files, pull.ChangedFiles) {
+				if assert.Len(t, files, 1) {
 					assert.Equal(t, "File-WoW", files[0].Filename)
 					assert.Empty(t, files[0].PreviousFilename)
-					assert.EqualValues(t, pull.Additions, files[0].Additions)
+					assert.EqualValues(t, 1, files[0].Additions)
-					assert.EqualValues(t, pull.Deletions, files[0].Deletions)
+					assert.EqualValues(t, 1, files[0].Changes)
+					assert.EqualValues(t, 0, files[0].Deletions)
 					assert.Equal(t, "added", files[0].Status)
 				}
 			}))
@@ -87,7 +87,6 @@ func TestAPIViewPulls(t *testing.T) {
 	assert.EqualValues(t, 4, pull.RequestedReviewers[1].ID)
 	assert.EqualValues(t, 2, pull.RequestedReviewers[2].ID)
 	assert.EqualValues(t, 5, pull.RequestedReviewers[3].ID)
-	assert.EqualValues(t, 1, pull.ChangedFiles)
 
 	if assert.EqualValues(t, 2, pull.ID) {
 		resp = ctx.Session.MakeRequest(t, NewRequest(t, "GET", pull.DiffURL), http.StatusOK)
@@ -95,45 +94,44 @@ func TestAPIViewPulls(t *testing.T) {
 		assert.NoError(t, err)
 		patch, err := gitdiff.ParsePatch(t.Context(), 1000, 5000, 10, bytes.NewReader(bs), "")
 		assert.NoError(t, err)
-		if assert.Len(t, patch.Files, pull.ChangedFiles) {
+		if assert.Len(t, patch.Files, 1) {
 			assert.Equal(t, "README.md", patch.Files[0].Name)
 			assert.Equal(t, "README.md", patch.Files[0].OldName)
-			assert.EqualValues(t, pull.Additions, patch.Files[0].Addition)
+			assert.EqualValues(t, 4, patch.Files[0].Addition)
-			assert.EqualValues(t, pull.Deletions, patch.Files[0].Deletion)
+			assert.EqualValues(t, 1, patch.Files[0].Deletion)
 			assert.Equal(t, gitdiff.DiffFileChange, patch.Files[0].Type)
 		}
 
 		t.Run(fmt.Sprintf("APIGetPullFiles_%d", pull.ID),
 			doAPIGetPullFiles(ctx, pull, func(t *testing.T, files []*api.ChangedFile) {
-				if assert.Len(t, files, pull.ChangedFiles) {
+				if assert.Len(t, files, 1) {
 					assert.Equal(t, "README.md", files[0].Filename)
 					// FIXME: The PreviousFilename name should be the same as Filename if it's a file change
 					assert.Equal(t, "", files[0].PreviousFilename)
-					assert.EqualValues(t, pull.Additions, files[0].Additions)
+					assert.EqualValues(t, 4, files[0].Additions)
-					assert.EqualValues(t, pull.Deletions, files[0].Deletions)
+					assert.EqualValues(t, 1, files[0].Deletions)
 					assert.Equal(t, "changed", files[0].Status)
 				}
 			}))
 	}
 
-	pull = pulls[2]
+	pull = pulls[0]
 	assert.EqualValues(t, 1, pull.Poster.ID)
-	assert.Len(t, pull.RequestedReviewers, 1)
+	assert.Len(t, pull.RequestedReviewers, 2)
 	assert.Empty(t, pull.RequestedReviewersTeams)
-	assert.EqualValues(t, 1, pull.RequestedReviewers[0].ID)
+	assert.EqualValues(t, 5, pull.RequestedReviewers[0].ID)
-	assert.EqualValues(t, 0, pull.ChangedFiles)
 
-	if assert.EqualValues(t, 1, pull.ID) {
+	if assert.EqualValues(t, 5, pull.ID) {
 		resp = ctx.Session.MakeRequest(t, NewRequest(t, "GET", pull.DiffURL), http.StatusOK)
 		bs, err := io.ReadAll(resp.Body)
 		assert.NoError(t, err)
 		patch, err := gitdiff.ParsePatch(t.Context(), 1000, 5000, 10, bytes.NewReader(bs), "")
 		assert.NoError(t, err)
-		assert.EqualValues(t, pull.ChangedFiles, patch.NumFiles)
+		assert.Len(t, patch.Files, 1)
 
 		t.Run(fmt.Sprintf("APIGetPullFiles_%d", pull.ID),
 			doAPIGetPullFiles(ctx, pull, func(t *testing.T, files []*api.ChangedFile) {
-				assert.Len(t, files, pull.ChangedFiles)
+				assert.Len(t, files, 1)
 			}))
 	}
 }

tests/integration/repo_webhook_test.go

@@ -24,6 +24,7 @@ import (
 
 	"github.com/PuerkitoBio/goquery"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestNewWebHookLink(t *testing.T) {
@@ -377,12 +378,14 @@ func Test_WebhookPullRequest(t *testing.T) {
 
 		// 3. validate the webhook is triggered
 		assert.EqualValues(t, "pull_request", triggeredEvent)
-		assert.Len(t, payloads, 1)
+		require.Len(t, payloads, 1)
 		assert.EqualValues(t, "repo1", payloads[0].PullRequest.Base.Repository.Name)
 		assert.EqualValues(t, "user2/repo1", payloads[0].PullRequest.Base.Repository.FullName)
 		assert.EqualValues(t, "repo1", payloads[0].PullRequest.Head.Repository.Name)
 		assert.EqualValues(t, "user2/repo1", payloads[0].PullRequest.Head.Repository.FullName)
-		assert.EqualValues(t, 0, payloads[0].PullRequest.Additions)
+		assert.EqualValues(t, 0, *payloads[0].PullRequest.Additions)
+		assert.EqualValues(t, 0, *payloads[0].PullRequest.ChangedFiles)
+		assert.EqualValues(t, 0, *payloads[0].PullRequest.Deletions)
 	})
 }