From 6422f05a4e2610a31b6137267b7bf53ae1b2b093 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 8 Mar 2025 17:36:08 +0800 Subject: [PATCH] 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 | 14 +-- modules/structs/pull.go | 10 +- routers/api/v1/repo/pull.go | 8 +- routers/web/repo/commit.go | 9 +- routers/web/repo/compare.go | 11 +- routers/web/repo/editor.go | 2 +- routers/web/repo/pull.go | 44 ++++---- services/convert/git_commit.go | 10 +- services/convert/pull.go | 30 +++--- services/gitdiff/gitdiff.go | 141 +++++++++---------------- services/gitdiff/gitdiff_test.go | 16 +-- services/repository/files/diff_test.go | 3 - services/repository/files/temp_repo.go | 5 - templates/repo/diff/box.tmpl | 8 +- templates/repo/pulls/tab_menu.tmpl | 6 +- tests/integration/api_pull_test.go | 40 ++++--- tests/integration/repo_webhook_test.go | 7 +- 17 files changed, 154 insertions(+), 210 deletions(-) diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 3995cb9ff5..ff44506e13 100644 --- a/modules/git/repo_compare.go +++ b/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 -func (repo *Repository) GetDiffShortStat(base, head string) (numFiles, totalAdditions, totalDeletions int, err error) { - numFiles, totalAdditions, totalDeletions, err = GetDiffShortStat(repo.Ctx, repo.Path, nil, base+"..."+head) - 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) { +// GetDiffShortStatByCmdArgs counts number of changed files, number of additions and deletions +// TODO: it can be merged with another "GetDiffShortStat" in the future +func GetDiffShortStatByCmdArgs(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: diff --git a/modules/structs/pull.go b/modules/structs/pull.go index 55831e642c..f53d6adafc 100644 --- a/modules/structs/pull.go +++ b/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"` - Additions int `json:"additions"` - Deletions int `json:"deletions"` - ChangedFiles int `json:"changed_files"` + ReviewComments int `json:"review_comments,omitempty"` + + Additions *int `json:"additions,omitempty"` + Deletions *int `json:"deletions,omitempty"` + ChangedFiles *int `json:"changed_files,omitempty"` HTMLURL string `json:"html_url"` DiffURL string `json:"diff_url"` diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 412f2cfb58..3698ff6010 100644 --- a/routers/api/v1/repo/pull.go +++ b/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() diff --git a/routers/web/repo/commit.go b/routers/web/repo/commit.go index 2728eda8b6..997e9f6869 100644 --- a/routers/web/repo/commit.go +++ b/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) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index 5165636a52..d9c6305ce1 100644 --- a/routers/web/repo/compare.go +++ b/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) diff --git a/routers/web/repo/editor.go b/routers/web/repo/editor.go index 3107d7b849..113622f872 100644 --- a/routers/web/repo/editor.go +++ b/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] } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 71057ec653..edbf399c77 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -200,22 +200,13 @@ func GetPullDiffStats(ctx *context.Context) { return } - diffOptions := &gitdiff.DiffOptions{ - 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) + diffShortStat, err := gitdiff.GetDiffShortStat(ctx.Repo.GitRepo, mergeBaseCommitID, headCommitID) 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 - var diff *gitdiff.Diff - shouldGetUserSpecificDiff := false + diff, err := gitdiff.GetDiff(ctx, gitRepo, diffOptions, files...) + if err != nil { + 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...) - methodWithError = "GetDiff" + // do nothing } 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("SyncUserSpecificDiff", err) + return + } } + + diffShortStat, err := gitdiff.GetDiffShortStat(ctx.Repo.GitRepo, startCommitID, endCommitID) if err != nil { - ctx.ServerError(methodWithError, err) + 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 { diff --git a/services/convert/git_commit.go b/services/convert/git_commit.go index e0efcddbcb..3ec81b52ee 100644 --- a/services/convert/git_commit.go +++ b/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{ - AfterCommitID: commit.ID.String(), - }) + diffShortStat, err := gitdiff.GetDiffShortStat(gitRepo, "", commit.ID.String()) if err != nil { return nil, err } res.Stats = &api.CommitStats{ - Total: diff.TotalAddition + diff.TotalDeletion, - Additions: diff.TotalAddition, - Deletions: diff.TotalDeletion, + Total: diffShortStat.TotalAddition + diffShortStat.TotalDeletion, + Additions: diffShortStat.TotalAddition, + Deletions: diffShortStat.TotalDeletion, } } diff --git a/services/convert/pull.go b/services/convert/pull.go index ad4f08fa91..928534ce5e 100644 --- a/services/convert/pull.go +++ b/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) } diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index fbfdf42e19..bf39e70127 100644 --- a/services/gitdiff/gitdiff.go +++ b/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 { - if len(diffFile.Sections) == 0 || diffFile.Type != DiffFileChange || diffFile.IsBin || diffFile.IsLFSFile { +func (diffFile *DiffFile) GetTailSection(leftCommit, rightCommit *git.Commit) *DiffSection { + if len(diffFile.Sections) == 0 || leftCommit == nil || diffFile.Type != DiffFileChange || diffFile.IsBin || diffFile.IsLFSFile { return nil } @@ -452,12 +452,10 @@ 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 + Start, End string + Files []*DiffFile + IsIncomplete bool + NumViewedFiles int // user-specific } // LoadComments loads comments into each line @@ -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 - commit, err := gitRepo.GetCommit(opts.AfterCommitID) + afterCommit, err := gitRepo.GetCommit(opts.AfterCommitID) if err != nil { return nil, err } - cmdCtx, cmdCancel := context.WithCancel(ctx) - defer cmdCancel() - - cmdDiff := git.NewCommand() - objectFormat, err := gitRepo.GetObjectFormat() + actualBeforeCommit, actualBeforeCommitID, err := guessBeforeCommitForDiff(gitRepo, opts.BeforeCommitID, afterCommit) if err != nil { return nil, err } - if (len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == objectFormat.EmptyObjectID().String()) && commit.ParentCount() == 0 { - cmdDiff.AddArguments("diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M"). - 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 - } - } + cmdDiff := git.NewCommand(). + AddArguments("diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M"). + AddArguments(opts.WhitespaceBehavior...) // 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) - if err != nil { - return nil, err - } - - diff.NumFiles, diff.TotalAddition, diff.TotalDeletion = stats.NumFiles, stats.TotalAddition, stats.TotalDeletion - return diff, nil } -type PullDiffStats struct { +type DiffShortStat struct { NumFiles, TotalAddition, TotalDeletion int } -// GetPullDiffStats -func GetPullDiffStats(gitRepo *git.Repository, opts *DiffOptions) (*PullDiffStats, error) { +func GetDiffShortStat(gitRepo *git.Repository, beforeCommitID, afterCommitID string) (*DiffShortStat, error) { repoPath := gitRepo.Path - diff := &PullDiffStats{} - - separator := "..." - if opts.DirectComparison { - separator = ".." - } - - objectFormat, err := gitRepo.GetObjectFormat() + afterCommit, err := gitRepo.GetCommit(afterCommitID) if err != nil { return nil, err } - diffPaths := []string{opts.BeforeCommitID + separator + opts.AfterCommitID} - if len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == objectFormat.EmptyObjectID().String() { - diffPaths = []string{objectFormat.EmptyTree().String(), opts.AfterCommitID} + _, actualBeforeCommitID, err := guessBeforeCommitForDiff(gitRepo, beforeCommitID, afterCommit) + if err != nil { + return nil, err } - 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...) - } + 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 -// Additionally, the database asynchronously is updated 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) { - diff, err := GetDiff(ctx, gitRepo, opts, files...) - if err != nil { - return nil, err - } +// SyncUserSpecificDiff inserts user-specific data such as which files the user has already viewed on the given diff +// Additionally, the database is updated asynchronously if files have changed since the last review +func SyncUserSpecificDiff(ctx context.Context, userID int64, pull *issues_model.PullRequest, gitRepo *git.Repository, diff *Diff, opts *DiffOptions, files ...string) error { 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 diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index 5a0e7405b1..41b4077cf2 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/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) diff --git a/services/repository/files/diff_test.go b/services/repository/files/diff_test.go index b7bdcd8ecf..38cb1d675b 100644 --- a/services/repository/files/diff_test.go +++ b/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) diff --git a/services/repository/files/temp_repo.go b/services/repository/files/temp_repo.go index 4c4a85c5b1..95528a6fd3 100644 --- a/services/repository/files/temp_repo.go +++ b/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 } diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index 1a0a5c04a8..0770e287fb 100644 --- a/templates/repo/diff/box.tmpl +++ b/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))}}
@@ -19,7 +19,7 @@ {{end}} {{if not .DiffNotAvailable}}
- {{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}}
{{end}}
@@ -27,9 +27,9 @@ {{if and .PageIsPullFiles $.SignedUserID (not .DiffNotAvailable)}}
- +
{{end}} {{template "repo/diff/whitespace_dropdown" .}} diff --git a/templates/repo/pulls/tab_menu.tmpl b/templates/repo/pulls/tab_menu.tmpl index 8b192c44db..a0ecdf96cd 100644 --- a/templates/repo/pulls/tab_menu.tmpl +++ b/templates/repo/pulls/tab_menu.tmpl @@ -15,11 +15,11 @@ {{template "shared/misc/tabtitle" (ctx.Locale.Tr "repo.pulls.tab_files")}} {{if .NumFiles}}{{.NumFiles}}{{else}}-{{end}} - {{if or .Diff.TotalAddition .Diff.TotalDeletion}} + {{if or .DiffShortStat.TotalAddition .DiffShortStat.TotalDeletion}} - {{if .Diff.TotalAddition}}+{{.Diff.TotalAddition}}{{end}} {{if .Diff.TotalDeletion}}-{{.Diff.TotalDeletion}}{{end}} + {{if .DiffShortStat.TotalAddition}}+{{.DiffShortStat.TotalAddition}}{{end}} {{if .DiffShortStat.TotalDeletion}}-{{.DiffShortStat.TotalDeletion}}{{end}} -
+
{{end}} diff --git a/tests/integration/api_pull_test.go b/tests/integration/api_pull_test.go index d71c78ea10..39c6c34a30 100644 --- a/tests/integration/api_pull_test.go +++ b/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, pull.Deletions, patch.Files[0].Deletion) + assert.EqualValues(t, 1, patch.Files[0].Addition) + 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, pull.Deletions, files[0].Deletions) + assert.EqualValues(t, 1, files[0].Additions) + 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, pull.Deletions, patch.Files[0].Deletion) + assert.EqualValues(t, 4, patch.Files[0].Addition) + 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, pull.Deletions, files[0].Deletions) + assert.EqualValues(t, 4, files[0].Additions) + 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, 0, pull.ChangedFiles) + assert.EqualValues(t, 5, pull.RequestedReviewers[0].ID) - 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) })) } } diff --git a/tests/integration/repo_webhook_test.go b/tests/integration/repo_webhook_test.go index 883d9b224d..596ccce266 100644 --- a/tests/integration/repo_webhook_test.go +++ b/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) }) }