refactor: GetDiffShortStat and fix panic caused by inconsistent "changed file number" (#39248)

services/gitdiff/gitdiff.go

@@ -15,6 +15,7 @@ import (
 	"net/url"
 	"path"
 	"sort"
+	"strconv"
 	"strings"
 	"time"
 
@@ -1293,36 +1294,45 @@ func readFileName(rd *strings.Reader) (string, bool) {
 	return name[2:], ambiguity
 }
 
-// DiffOptions represents the options for a DiffRange
-type DiffOptions struct {
+type DiffCommonOptions struct {
 	BeforeCommitID     string
 	AfterCommitID      string
-	SkipTo             string
-	MaxLines           int
-	MaxLineCharacters  int
-	MaxFiles           int
 	WhitespaceBehavior gitcmd.TrustedCmdArgs
-	DirectComparison   bool
 }
 
-func guessBeforeCommitForDiff(ctx context.Context, 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()
+type DiffOptions struct {
+	DiffCommonOptions
+	SkipTo            string
+	MaxLines          int
+	MaxLineCharacters int
+	MaxFiles          int
+}
 
+// prepareDiffCommits prepares the before and after commits for a diff operation based on the provided options.
+// The "before commit" can be nil (the empty tree ID is used) if there is no "before commit" can be determined.
+func prepareDiffCommits(ctx context.Context, gitRepo *git.Repository, opts *DiffCommonOptions) (actualBeforeCommit *git.Commit, actualBeforeCommitID git.ObjectID, afterCommit *git.Commit, err error) {
+	afterCommit, err = gitRepo.GetCommit(ctx, opts.AfterCommitID)
+	if err != nil {
+		return nil, nil, nil, err
+	}
+
+	commitObjectFormat := afterCommit.ID.Type()
+	isBeforeCommitIDEmpty := opts.BeforeCommitID == "" || opts.BeforeCommitID == commitObjectFormat.EmptyObjectID().String()
 	if isBeforeCommitIDEmpty && afterCommit.ParentCount() == 0 {
+		// "git diff 4b825dc642cb6eb9a060e54bf8d69288fbee4904 after-commit" can work with tree ID as before commit ID
 		actualBeforeCommitID = commitObjectFormat.EmptyTree()
 	} else {
 		if isBeforeCommitIDEmpty {
 			actualBeforeCommit, err = afterCommit.Parent(ctx, gitRepo, 0)
 		} else {
-			actualBeforeCommit, err = gitRepo.GetCommit(ctx, beforeCommitID)
+			actualBeforeCommit, err = gitRepo.GetCommit(ctx, opts.BeforeCommitID)
 		}
 		if err != nil {
-			return nil, nil, err
+			return nil, nil, nil, err
 		}
 		actualBeforeCommitID = actualBeforeCommit.ID
 	}
-	return actualBeforeCommit, actualBeforeCommitID, nil
+	return actualBeforeCommit, actualBeforeCommitID, afterCommit, nil
 }
 
 // getDiffBasic builds a Diff between two commits of a repository.
@@ -1330,12 +1340,7 @@ func guessBeforeCommitForDiff(ctx context.Context, gitRepo *git.Repository, befo
 // The whitespaceBehavior is either an empty string or a git flag
 // Returned beforeCommit could be nil if the afterCommit doesn't have parent commit
 func getDiffBasic(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, files ...string) (_ *Diff, beforeCommit, afterCommit *git.Commit, err error) {
-	afterCommit, err = gitRepo.GetCommit(ctx, opts.AfterCommitID)
-	if err != nil {
-		return nil, nil, nil, err
-	}
-
-	beforeCommit, beforeCommitID, err := guessBeforeCommitForDiff(ctx, gitRepo, opts.BeforeCommitID, afterCommit)
+	beforeCommit, beforeCommitID, afterCommit, err := prepareDiffCommits(ctx, gitRepo, &opts.DiffCommonOptions)
 	if err != nil {
 		return nil, nil, nil, err
 	}
@@ -1494,26 +1499,41 @@ func highlightCodeLines(name, lang string, sections []*DiffSection, isLeft bool,
 }
 
 type DiffShortStat struct {
-	NumFiles, TotalAddition, TotalDeletion int
+	NumFiles, TotalAddition, TotalDeletion int // these fields are used in templates directly
 }
 
-func GetDiffShortStat(ctx context.Context, gitRepo *git.Repository, beforeCommitID, afterCommitID string) (*DiffShortStat, error) {
-	afterCommit, err := gitRepo.GetCommit(ctx, afterCommitID)
+func GetDiffShortStat(ctx context.Context, gitRepo *git.Repository, opts *DiffCommonOptions) (*DiffShortStat, error) {
+	_, actualBeforeCommitID, afterCommit, err := prepareDiffCommits(ctx, gitRepo, opts)
 	if err != nil {
 		return nil, err
 	}
 
-	_, actualBeforeCommitID, err := guessBeforeCommitForDiff(ctx, gitRepo, beforeCommitID, afterCommit)
+	stat := &DiffShortStat{}
+	cmd := gitcmd.NewCommand("diff", "--shortstat").
+		AddArguments(opts.WhitespaceBehavior...).
+		AddOptionFormat("--find-renames=%s", setting.Git.DiffRenameSimilarityThreshold).
+		AddDynamicArguments(actualBeforeCommitID.String(), afterCommit.ID.String())
+	// output: "  9902 files changed, 2034198 insertions(+), 298800 deletions(-)\n"
+	stdout, _, err := cmd.WithRepo(gitRepo).RunStdString(ctx)
 	if err != nil {
 		return nil, err
 	}
-
-	diff := &DiffShortStat{}
-	diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStatByCmdArgs(ctx, gitRepo, nil, actualBeforeCommitID.String(), afterCommitID)
-	if err != nil {
-		return nil, err
+	stdout = strings.TrimSpace(stdout)
+	for field := range strings.SplitSeq(stdout, ",") {
+		field = strings.TrimSpace(field)
+		num, suffix, ok := strings.Cut(field, " ")
+		switch {
+		case strings.Contains(suffix, "file") && strings.Contains(suffix, "change"):
+			stat.NumFiles, _ = strconv.Atoi(num)
+		case strings.Contains(suffix, "insertion"):
+			stat.TotalAddition, _ = strconv.Atoi(num)
+		case strings.Contains(suffix, "deletion"):
+			stat.TotalDeletion, _ = strconv.Atoi(num)
+		case ok:
+			setting.PanicInDevOrTesting("unexpected diff shortstat output: %s", stdout)
+		}
 	}
-	return diff, nil
+	return stat, nil
 }
 
 // SyncUserSpecificDiff inserts user-specific data such as which files the user has already viewed on the given diff

services/gitdiff/gitdiff_test.go

@@ -1308,3 +1308,37 @@ D test10.txt`
 	assert.Equal(t, thirdReviewUpdatedFiles, thirdReview.UpdatedFiles)
 	assert.Equal(t, 1, thirdReview.GetViewedFileCount())
 }
+
+func TestGetDiffShortStatWithOptions(t *testing.T) {
+	repo, err := git.ForceFastImportWithInit(t.Context(), t.TempDir(), []git.FastImportCommit{
+		{Ref: "refs/heads/base", Files: []git.FastImportFile{
+			{Path: "real.txt", Content: "a1\na2\n"},
+			{Path: "whitespace.txt", Content: "b\n"},
+		}},
+		{Ref: "refs/heads/head", Files: []git.FastImportFile{
+			{Path: "real.txt", Content: "A1\nA2\nA3\n"},
+			{Path: "whitespace.txt", Content: "b   \n"},
+		}},
+	})
+	require.NoError(t, err)
+	gitRepo, err := git.OpenRepository(t.Context(), repo)
+	require.NoError(t, err)
+	defer gitRepo.Close()
+	t.Run("NoParent", func(t *testing.T) {
+		stat, err := GetDiffShortStat(t.Context(), gitRepo, &DiffCommonOptions{AfterCommitID: "refs/heads/head"})
+		require.NoError(t, err)
+		assert.Equal(t, &DiffShortStat{NumFiles: 2, TotalAddition: 4, TotalDeletion: 0}, stat)
+	})
+	diffOptions := DiffCommonOptions{BeforeCommitID: "refs/heads/base", AfterCommitID: "refs/heads/head"}
+	t.Run("NormalDiff", func(t *testing.T) {
+		stat, err := GetDiffShortStat(t.Context(), gitRepo, &diffOptions)
+		require.NoError(t, err)
+		assert.Equal(t, &DiffShortStat{NumFiles: 2, TotalAddition: 4, TotalDeletion: 3}, stat)
+	})
+	t.Run("IgnoreSpace", func(t *testing.T) {
+		diffOptions.WhitespaceBehavior = GetWhitespaceFlag("ignore-all")
+		stat, err := GetDiffShortStat(t.Context(), gitRepo, &diffOptions)
+		require.NoError(t, err)
+		assert.Equal(t, &DiffShortStat{NumFiles: 1, TotalAddition: 3, TotalDeletion: 2}, stat)
+	})
+}