From cc40ab78478042d958aac69f33921501def9c129 Mon Sep 17 00:00:00 2001 From: Rowan Bohde Date: Thu, 31 Oct 2024 11:38:36 -0500 Subject: [PATCH 1/8] improve performance of diffs This has two major changes that significantly reduce the amount of work done for large diffs: * Kill a running git process when reaching the maximum number of files in a diff, preventing it from processing the entire diff. * When loading a diff with the URL param `file-only=true`, skip loading stats. This speeds up loading both hidden files of a diff and sections of a diff when clicking the "Show More" button. A couple of minor things from profiling: * Reuse open git commits if possible to avoid querying the repo. * Reuse existing repo in `PrepareViewPullInfo` if head and base are the same. --- routers/web/repo/commit.go | 1 + routers/web/repo/compare.go | 3 + routers/web/repo/pull.go | 40 +++++++------ services/gitdiff/csv_test.go | 2 +- services/gitdiff/gitdiff.go | 77 ++++++++++++++------------ services/gitdiff/gitdiff_test.go | 24 ++++---- services/repository/files/temp_repo.go | 2 +- 7 files changed, 84 insertions(+), 65 deletions(-) diff --git a/routers/web/repo/commit.go b/routers/web/repo/commit.go index 0e4e10bf50944..d7865e18d63f9 100644 --- a/routers/web/repo/commit.go +++ b/routers/web/repo/commit.go @@ -328,6 +328,7 @@ 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("GetDiff", err) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index e6a04782e5bbf..39279728375f0 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -611,6 +611,8 @@ func PrepareCompareDiff( maxLines, maxFiles = -1, -1 } + fileOnly := ctx.FormBool("file-only") + diff, err := gitdiff.GetDiff(ctx, ci.HeadGitRepo, &gitdiff.DiffOptions{ BeforeCommitID: beforeCommitID, @@ -621,6 +623,7 @@ func PrepareCompareDiff( MaxFiles: maxFiles, WhitespaceBehavior: whitespaceBehavior, DirectComparison: ci.DirectComparison, + FileOnly: fileOnly, }, ctx.FormStrings("files")...) if err != nil { ctx.ServerError("GetDiffRangeWithWhitespaceBehavior", err) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 0efe1be76a310..1c498868313df 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -395,12 +395,17 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C var headBranchSha string // HeadRepo may be missing if pull.HeadRepo != nil { - headGitRepo, err := gitrepo.OpenRepository(ctx, pull.HeadRepo) - if err != nil { - ctx.ServerError("OpenRepository", err) - return nil + var headGitRepo *git.Repository + if ctx.Repo != nil && ctx.Repo.Repository != nil && pull.HeadRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil { + headGitRepo = ctx.Repo.GitRepo + } else { + headGitRepo, err = gitrepo.OpenRepository(ctx, pull.HeadRepo) + if err != nil { + ctx.ServerError("OpenRepository", err) + return nil + } + defer headGitRepo.Close() } - defer headGitRepo.Close() if pull.Flow == issues_model.PullRequestFlowGithub { headBranchExist = headGitRepo.IsBranchExist(pull.HeadBranch) @@ -740,17 +745,31 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi maxLines, maxFiles = -1, -1 } + baseCommit, err := ctx.Repo.GitRepo.GetCommit(startCommitID) + if err != nil { + ctx.ServerError("GetCommit", err) + return + } + commit, err := gitRepo.GetCommit(endCommitID) + if err != nil { + ctx.ServerError("GetCommit", err) + return + } + diffOptions := &gitdiff.DiffOptions{ AfterCommitID: endCommitID, + AfterCommit: commit, SkipTo: ctx.FormString("skip-to"), MaxLines: maxLines, MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters, MaxFiles: maxFiles, WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)), + FileOnly: fileOnly, } if !willShowSpecifiedCommit { diffOptions.BeforeCommitID = startCommitID + diffOptions.BeforeCommit = baseCommit } var methodWithError string @@ -813,17 +832,6 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi ctx.Data["Diff"] = diff ctx.Data["DiffNotAvailable"] = diff.NumFiles == 0 - baseCommit, err := ctx.Repo.GitRepo.GetCommit(startCommitID) - if err != nil { - ctx.ServerError("GetCommit", err) - return - } - commit, err := gitRepo.GetCommit(endCommitID) - if err != nil { - ctx.ServerError("GetCommit", err) - return - } - if ctx.IsSigned && ctx.Doer != nil { if ctx.Data["CanMarkConversation"], err = issues_model.CanMarkConversation(ctx, issue, ctx.Doer); err != nil { ctx.ServerError("CanMarkConversation", err) diff --git a/services/gitdiff/csv_test.go b/services/gitdiff/csv_test.go index c006a7c2bd8d0..82033593c612e 100644 --- a/services/gitdiff/csv_test.go +++ b/services/gitdiff/csv_test.go @@ -191,7 +191,7 @@ c,d,e`, } for n, c := range cases { - diff, err := ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.diff), "") + diff, err := ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.diff), "", nil) if err != nil { t.Errorf("ParsePatch failed: %s", err) } diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 0ddd5a48e2148..f59ad76624486 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -380,18 +380,11 @@ 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, leftCommitID, rightCommitID string) *DiffSection { +func (diffFile *DiffFile) GetTailSection(gitRepo *git.Repository, leftCommit, rightCommit *git.Commit) *DiffSection { if len(diffFile.Sections) == 0 || diffFile.Type != DiffFileChange || diffFile.IsBin || diffFile.IsLFSFile { return nil } - leftCommit, err := gitRepo.GetCommit(leftCommitID) - if err != nil { - return nil - } - rightCommit, err := gitRepo.GetCommit(rightCommitID) - if err != nil { - return nil - } + lastSection := diffFile.Sections[len(diffFile.Sections)-1] lastLine := lastSection.Lines[len(lastSection.Lines)-1] leftLineCount := getCommitFileLineCount(leftCommit, diffFile.Name) @@ -494,7 +487,7 @@ func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, c const cmdDiffHead = "diff --git " // ParsePatch builds a Diff object from a io.Reader and some parameters. -func ParsePatch(ctx context.Context, maxLines, maxLineCharacters, maxFiles int, reader io.Reader, skipToFile string) (*Diff, error) { +func ParsePatch(ctx context.Context, maxLines, maxLineCharacters, maxFiles int, reader io.Reader, skipToFile string, cancel context.CancelFunc) (*Diff, error) { log.Debug("ParsePatch(%d, %d, %d, ..., %s)", maxLines, maxLineCharacters, maxFiles, skipToFile) var curFile *DiffFile @@ -536,6 +529,11 @@ parsingLoop: lastFile := createDiffFile(diff, line) diff.End = lastFile.Name diff.IsIncomplete = true + + // signal that we are exiting this diff early + if cancel != nil { + cancel() + } _, err := io.Copy(io.Discard, reader) if err != nil { // By the definition of io.Copy this never returns io.EOF @@ -1094,13 +1092,16 @@ func readFileName(rd *strings.Reader) (string, bool) { // DiffOptions represents the options for a DiffRange type DiffOptions struct { BeforeCommitID string + BeforeCommit *git.Commit AfterCommitID string + AfterCommit *git.Commit SkipTo string MaxLines int MaxLineCharacters int MaxFiles int WhitespaceBehavior git.TrustedCmdArgs DirectComparison bool + FileOnly bool } // GetDiff builds a Diff between two commits of a repository. @@ -1109,12 +1110,19 @@ type DiffOptions struct { func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) { repoPath := gitRepo.Path - commit, err := gitRepo.GetCommit(opts.AfterCommitID) - if err != nil { - return nil, err + commit := opts.AfterCommit + if commit == nil { + var err error + commit, err = gitRepo.GetCommit(opts.AfterCommitID) + if err != nil { + return nil, err + } } - cmdDiff := git.NewCommand(gitRepo.Ctx) + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + cmdDiff := git.NewCommand(ctx) objectFormat, err := gitRepo.GetObjectFormat() if err != nil { return nil, err @@ -1136,6 +1144,14 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi AddArguments(opts.WhitespaceBehavior...). AddDynamicArguments(actualBeforeCommitID, opts.AfterCommitID) opts.BeforeCommitID = actualBeforeCommitID + + if opts.BeforeCommit == nil { + var err error + opts.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 @@ -1163,14 +1179,14 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi Dir: repoPath, Stdout: writer, Stderr: stderr, - }); err != nil { + }); err != nil && err.Error() != "signal: killed" { log.Error("error during GetDiff(git diff dir: %s): %v, stderr: %s", repoPath, err, stderr.String()) } _ = writer.Close() }() - diff, err := ParsePatch(ctx, opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, reader, parsePatchSkipToFile) + diff, err := ParsePatch(ctx, opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, reader, parsePatchSkipToFile, cancel) if err != nil { return nil, fmt.Errorf("unable to ParsePatch: %w", err) } @@ -1205,37 +1221,28 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi } diffFile.IsGenerated = isGenerated.Value() - tailSection := diffFile.GetTailSection(gitRepo, opts.BeforeCommitID, opts.AfterCommitID) + tailSection := diffFile.GetTailSection(gitRepo, opts.BeforeCommit, commit) if tailSection != nil { diffFile.Sections = append(diffFile.Sections, tailSection) } } - separator := "..." - if opts.DirectComparison { - separator = ".." + if opts.FileOnly { + return diff, nil } - diffPaths := []string{opts.BeforeCommitID + separator + opts.AfterCommitID} - 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...) - } + 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 { - TotalAddition, TotalDeletion int + NumFiles, TotalAddition, TotalDeletion int } // GetPullDiffStats @@ -1259,12 +1266,12 @@ func GetPullDiffStats(gitRepo *git.Repository, opts *DiffOptions) (*PullDiffStat diffPaths = []string{objectFormat.EmptyTree().String(), opts.AfterCommitID} } - _, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) + 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.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) + diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) } if err != nil { return nil, err @@ -1345,7 +1352,7 @@ outer: // CommentAsDiff returns c.Patch as *Diff func CommentAsDiff(ctx context.Context, c *issues_model.Comment) (*Diff, error) { diff, err := ParsePatch(ctx, setting.Git.MaxGitDiffLines, - setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.Patch), "") + setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.Patch), "", nil) if err != nil { log.Error("Unable to parse patch: %v", err) return nil, err diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index adcac355a7bb4..b2d2c19e7a93e 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -175,7 +175,7 @@ diff --git "\\a/README.md" "\\b/README.md" } for _, testcase := range tests { t.Run(testcase.name, func(t *testing.T) { - got, err := ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff), testcase.skipTo) + got, err := ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff), testcase.skipTo, nil) if (err != nil) != testcase.wantErr { t.Errorf("ParsePatch(%q) error = %v, wantErr %v", testcase.name, err, testcase.wantErr) return @@ -400,7 +400,7 @@ index 6961180..9ba1a00 100644 for _, testcase := range tests { t.Run(testcase.name, func(t *testing.T) { - got, err := ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff), "") + got, err := ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff), "", nil) if (err != nil) != testcase.wantErr { t.Errorf("ParsePatch(%q) error = %v, wantErr %v", testcase.name, err, testcase.wantErr) return @@ -449,21 +449,21 @@ index 0000000..6bb8f39 diffBuilder.WriteString("+line" + strconv.Itoa(i) + "\n") } diff = diffBuilder.String() - result, err := ParsePatch(db.DefaultContext, 20, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "") + result, err := ParsePatch(db.DefaultContext, 20, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "", nil) if err != nil { t.Errorf("There should not be an error: %v", err) } if !result.Files[0].IsIncomplete { t.Errorf("Files should be incomplete! %v", result.Files[0]) } - result, err = ParsePatch(db.DefaultContext, 40, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "") + result, err = ParsePatch(db.DefaultContext, 40, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "", nil) if err != nil { t.Errorf("There should not be an error: %v", err) } if result.Files[0].IsIncomplete { t.Errorf("Files should not be incomplete! %v", result.Files[0]) } - result, err = ParsePatch(db.DefaultContext, 40, 5, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "") + result, err = ParsePatch(db.DefaultContext, 40, 5, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "", nil) if err != nil { t.Errorf("There should not be an error: %v", err) } @@ -494,14 +494,14 @@ index 0000000..6bb8f39 diffBuilder.WriteString("+line" + strconv.Itoa(35) + "\n") diff = diffBuilder.String() - result, err = ParsePatch(db.DefaultContext, 20, 4096, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "") + result, err = ParsePatch(db.DefaultContext, 20, 4096, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "", nil) if err != nil { t.Errorf("There should not be an error: %v", err) } if !result.Files[0].IsIncomplete { t.Errorf("Files should be incomplete! %v", result.Files[0]) } - result, err = ParsePatch(db.DefaultContext, 40, 4096, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "") + result, err = ParsePatch(db.DefaultContext, 40, 4096, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "", nil) if err != nil { t.Errorf("There should not be an error: %v", err) } @@ -520,7 +520,7 @@ index 0000000..6bb8f39 Docker Pulls + cut off + cut off` - _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "") + _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "", nil) if err != nil { t.Errorf("ParsePatch failed: %s", err) } @@ -536,7 +536,7 @@ index 0000000..6bb8f39 Docker Pulls + cut off + cut off` - _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff2), "") + _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff2), "", nil) if err != nil { t.Errorf("ParsePatch failed: %s", err) } @@ -552,7 +552,7 @@ index 0000000..6bb8f39 Docker Pulls + cut off + cut off` - _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff2a), "") + _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff2a), "", nil) if err != nil { t.Errorf("ParsePatch failed: %s", err) } @@ -568,7 +568,7 @@ index 0000000..6bb8f39 Docker Pulls + cut off + cut off` - _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff3), "") + _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff3), "", nil) if err != nil { t.Errorf("ParsePatch failed: %s", err) } @@ -665,6 +665,6 @@ func TestNoCrashes(t *testing.T) { } for _, testcase := range tests { // It shouldn't crash, so don't care about the output. - ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff), "") + ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff), "", nil) } } diff --git a/services/repository/files/temp_repo.go b/services/repository/files/temp_repo.go index d70b1e8d54e77..3688965bb658c 100644 --- a/services/repository/files/temp_repo.go +++ b/services/repository/files/temp_repo.go @@ -358,7 +358,7 @@ func (t *TemporaryUploadRepository) DiffIndex() (*gitdiff.Diff, error) { Stderr: stderr, PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error { _ = stdoutWriter.Close() - diff, finalErr = gitdiff.ParsePatch(t.ctx, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdoutReader, "") + diff, finalErr = gitdiff.ParsePatch(t.ctx, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdoutReader, "", cancel) if finalErr != nil { log.Error("ParsePatch: %v", finalErr) cancel() From dd9a9565dfcfa504a9b03f6a000c7e6e168e01ab Mon Sep 17 00:00:00 2001 From: Rowan Bohde Date: Fri, 1 Nov 2024 10:28:38 -0500 Subject: [PATCH 2/8] refactor to have a simpler ParsePatch API --- services/gitdiff/csv_test.go | 2 +- services/gitdiff/gitdiff.go | 18 +++++------------- services/gitdiff/gitdiff_test.go | 24 ++++++++++++------------ services/repository/files/temp_repo.go | 3 ++- 4 files changed, 20 insertions(+), 27 deletions(-) diff --git a/services/gitdiff/csv_test.go b/services/gitdiff/csv_test.go index 82033593c612e..c006a7c2bd8d0 100644 --- a/services/gitdiff/csv_test.go +++ b/services/gitdiff/csv_test.go @@ -191,7 +191,7 @@ c,d,e`, } for n, c := range cases { - diff, err := ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.diff), "", nil) + diff, err := ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.diff), "") if err != nil { t.Errorf("ParsePatch failed: %s", err) } diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index f59ad76624486..a764cca9ee7cc 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -487,7 +487,7 @@ func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, c const cmdDiffHead = "diff --git " // ParsePatch builds a Diff object from a io.Reader and some parameters. -func ParsePatch(ctx context.Context, maxLines, maxLineCharacters, maxFiles int, reader io.Reader, skipToFile string, cancel context.CancelFunc) (*Diff, error) { +func ParsePatch(ctx context.Context, maxLines, maxLineCharacters, maxFiles int, reader io.Reader, skipToFile string) (*Diff, error) { log.Debug("ParsePatch(%d, %d, %d, ..., %s)", maxLines, maxLineCharacters, maxFiles, skipToFile) var curFile *DiffFile @@ -529,16 +529,6 @@ parsingLoop: lastFile := createDiffFile(diff, line) diff.End = lastFile.Name diff.IsIncomplete = true - - // signal that we are exiting this diff early - if cancel != nil { - cancel() - } - _, err := io.Copy(io.Discard, reader) - if err != nil { - // By the definition of io.Copy this never returns io.EOF - return diff, fmt.Errorf("error during io.Copy: %w", err) - } break parsingLoop } @@ -1186,7 +1176,9 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi _ = writer.Close() }() - diff, err := ParsePatch(ctx, opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, reader, parsePatchSkipToFile, cancel) + diff, err := ParsePatch(ctx, opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, reader, parsePatchSkipToFile) + // Ensure the git process is killed if it didn't exit already + cancel() if err != nil { return nil, fmt.Errorf("unable to ParsePatch: %w", err) } @@ -1352,7 +1344,7 @@ outer: // CommentAsDiff returns c.Patch as *Diff func CommentAsDiff(ctx context.Context, c *issues_model.Comment) (*Diff, error) { diff, err := ParsePatch(ctx, setting.Git.MaxGitDiffLines, - setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.Patch), "", nil) + setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.Patch), "") if err != nil { log.Error("Unable to parse patch: %v", err) return nil, err diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index b2d2c19e7a93e..adcac355a7bb4 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -175,7 +175,7 @@ diff --git "\\a/README.md" "\\b/README.md" } for _, testcase := range tests { t.Run(testcase.name, func(t *testing.T) { - got, err := ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff), testcase.skipTo, nil) + got, err := ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff), testcase.skipTo) if (err != nil) != testcase.wantErr { t.Errorf("ParsePatch(%q) error = %v, wantErr %v", testcase.name, err, testcase.wantErr) return @@ -400,7 +400,7 @@ index 6961180..9ba1a00 100644 for _, testcase := range tests { t.Run(testcase.name, func(t *testing.T) { - got, err := ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff), "", nil) + got, err := ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff), "") if (err != nil) != testcase.wantErr { t.Errorf("ParsePatch(%q) error = %v, wantErr %v", testcase.name, err, testcase.wantErr) return @@ -449,21 +449,21 @@ index 0000000..6bb8f39 diffBuilder.WriteString("+line" + strconv.Itoa(i) + "\n") } diff = diffBuilder.String() - result, err := ParsePatch(db.DefaultContext, 20, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "", nil) + result, err := ParsePatch(db.DefaultContext, 20, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "") if err != nil { t.Errorf("There should not be an error: %v", err) } if !result.Files[0].IsIncomplete { t.Errorf("Files should be incomplete! %v", result.Files[0]) } - result, err = ParsePatch(db.DefaultContext, 40, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "", nil) + result, err = ParsePatch(db.DefaultContext, 40, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "") if err != nil { t.Errorf("There should not be an error: %v", err) } if result.Files[0].IsIncomplete { t.Errorf("Files should not be incomplete! %v", result.Files[0]) } - result, err = ParsePatch(db.DefaultContext, 40, 5, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "", nil) + result, err = ParsePatch(db.DefaultContext, 40, 5, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "") if err != nil { t.Errorf("There should not be an error: %v", err) } @@ -494,14 +494,14 @@ index 0000000..6bb8f39 diffBuilder.WriteString("+line" + strconv.Itoa(35) + "\n") diff = diffBuilder.String() - result, err = ParsePatch(db.DefaultContext, 20, 4096, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "", nil) + result, err = ParsePatch(db.DefaultContext, 20, 4096, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "") if err != nil { t.Errorf("There should not be an error: %v", err) } if !result.Files[0].IsIncomplete { t.Errorf("Files should be incomplete! %v", result.Files[0]) } - result, err = ParsePatch(db.DefaultContext, 40, 4096, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "", nil) + result, err = ParsePatch(db.DefaultContext, 40, 4096, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "") if err != nil { t.Errorf("There should not be an error: %v", err) } @@ -520,7 +520,7 @@ index 0000000..6bb8f39 Docker Pulls + cut off + cut off` - _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "", nil) + _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff), "") if err != nil { t.Errorf("ParsePatch failed: %s", err) } @@ -536,7 +536,7 @@ index 0000000..6bb8f39 Docker Pulls + cut off + cut off` - _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff2), "", nil) + _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff2), "") if err != nil { t.Errorf("ParsePatch failed: %s", err) } @@ -552,7 +552,7 @@ index 0000000..6bb8f39 Docker Pulls + cut off + cut off` - _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff2a), "", nil) + _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff2a), "") if err != nil { t.Errorf("ParsePatch failed: %s", err) } @@ -568,7 +568,7 @@ index 0000000..6bb8f39 Docker Pulls + cut off + cut off` - _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff3), "", nil) + _, err = ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(diff3), "") if err != nil { t.Errorf("ParsePatch failed: %s", err) } @@ -665,6 +665,6 @@ func TestNoCrashes(t *testing.T) { } for _, testcase := range tests { // It shouldn't crash, so don't care about the output. - ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff), "", nil) + ParsePatch(db.DefaultContext, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(testcase.gitdiff), "") } } diff --git a/services/repository/files/temp_repo.go b/services/repository/files/temp_repo.go index 3688965bb658c..97044342688ed 100644 --- a/services/repository/files/temp_repo.go +++ b/services/repository/files/temp_repo.go @@ -358,7 +358,8 @@ func (t *TemporaryUploadRepository) DiffIndex() (*gitdiff.Diff, error) { Stderr: stderr, PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error { _ = stdoutWriter.Close() - diff, finalErr = gitdiff.ParsePatch(t.ctx, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdoutReader, "", cancel) + defer cancel() + diff, finalErr = gitdiff.ParsePatch(t.ctx, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdoutReader, "") if finalErr != nil { log.Error("ParsePatch: %v", finalErr) cancel() From 27cb5c4a6709ad04dca5c87001fd90b0d8e4f0a6 Mon Sep 17 00:00:00 2001 From: Rowan Bohde Date: Fri, 1 Nov 2024 10:46:38 -0500 Subject: [PATCH 3/8] refactor to use RepositoryFromContextOrOpen --- routers/web/repo/pull.go | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 1c498868313df..02878746ca126 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -395,17 +395,12 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C var headBranchSha string // HeadRepo may be missing if pull.HeadRepo != nil { - var headGitRepo *git.Repository - if ctx.Repo != nil && ctx.Repo.Repository != nil && pull.HeadRepoID == ctx.Repo.Repository.ID && ctx.Repo.GitRepo != nil { - headGitRepo = ctx.Repo.GitRepo - } else { - headGitRepo, err = gitrepo.OpenRepository(ctx, pull.HeadRepo) - if err != nil { - ctx.ServerError("OpenRepository", err) - return nil - } - defer headGitRepo.Close() + headGitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, pull.HeadRepo) + if err != nil { + ctx.ServerError("RepositoryFromContextOrOpen", err) + return nil } + defer closer.Close() if pull.Flow == issues_model.PullRequestFlowGithub { headBranchExist = headGitRepo.IsBranchExist(pull.HeadBranch) From 1e82e2c650a3e957d5d01ae5bb21577d7fc523f8 Mon Sep 17 00:00:00 2001 From: Rowan Bohde Date: Fri, 1 Nov 2024 10:54:02 -0500 Subject: [PATCH 4/8] fix DiffIndex throwing an error when canceling the process early --- services/repository/files/temp_repo.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/services/repository/files/temp_repo.go b/services/repository/files/temp_repo.go index 97044342688ed..ce98967e7976a 100644 --- a/services/repository/files/temp_repo.go +++ b/services/repository/files/temp_repo.go @@ -372,10 +372,14 @@ func (t *TemporaryUploadRepository) DiffIndex() (*gitdiff.Diff, error) { log.Error("Unable to ParsePatch in temporary repo %s (%s). Error: %v", t.repo.FullName(), t.basePath, finalErr) return nil, finalErr } - log.Error("Unable to run diff-index pipeline in temporary repo %s (%s). Error: %v\nStderr: %s", - t.repo.FullName(), t.basePath, err, stderr) - return nil, fmt.Errorf("Unable to run diff-index pipeline in temporary repo %s. Error: %w\nStderr: %s", - t.repo.FullName(), err, stderr) + + // If the process exited early, don't error + if err != context.Canceled { + log.Error("Unable to run diff-index pipeline in temporary repo %s (%s). Error: %v\nStderr: %s", + t.repo.FullName(), t.basePath, err, stderr) + return nil, fmt.Errorf("Unable to run diff-index pipeline in temporary repo %s. Error: %w\nStderr: %s", + t.repo.FullName(), err, stderr) + } } diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(t.ctx, t.basePath, git.TrustedCmdArgs{"--cached"}, "HEAD") From 935e2d56e5ad8bddf572948979ea92e2fdabd385 Mon Sep 17 00:00:00 2001 From: Rowan Bohde Date: Fri, 1 Nov 2024 12:11:37 -0500 Subject: [PATCH 5/8] remove AfterCommit & BeforeCommit from DiffOptions --- routers/web/repo/pull.go | 2 -- services/gitdiff/gitdiff.go | 25 +++++++++---------------- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 02878746ca126..61c9f2651a929 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -753,7 +753,6 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi diffOptions := &gitdiff.DiffOptions{ AfterCommitID: endCommitID, - AfterCommit: commit, SkipTo: ctx.FormString("skip-to"), MaxLines: maxLines, MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters, @@ -764,7 +763,6 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi if !willShowSpecifiedCommit { diffOptions.BeforeCommitID = startCommitID - diffOptions.BeforeCommit = baseCommit } var methodWithError string diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index a764cca9ee7cc..138e85a3491ff 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1082,9 +1082,7 @@ func readFileName(rd *strings.Reader) (string, bool) { // DiffOptions represents the options for a DiffRange type DiffOptions struct { BeforeCommitID string - BeforeCommit *git.Commit AfterCommitID string - AfterCommit *git.Commit SkipTo string MaxLines int MaxLineCharacters int @@ -1100,13 +1098,10 @@ type DiffOptions struct { func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) { repoPath := gitRepo.Path - commit := opts.AfterCommit - if commit == nil { - var err error - commit, err = gitRepo.GetCommit(opts.AfterCommitID) - if err != nil { - return nil, err - } + var beforeCommit *git.Commit + commit, err := gitRepo.GetCommit(opts.AfterCommitID) + if err != nil { + return nil, err } ctx, cancel := context.WithCancel(ctx) @@ -1135,12 +1130,10 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi AddDynamicArguments(actualBeforeCommitID, opts.AfterCommitID) opts.BeforeCommitID = actualBeforeCommitID - if opts.BeforeCommit == nil { - var err error - opts.BeforeCommit, err = gitRepo.GetCommit(opts.BeforeCommitID) - if err != nil { - return nil, err - } + var err error + beforeCommit, err = gitRepo.GetCommit(opts.BeforeCommitID) + if err != nil { + return nil, err } } @@ -1213,7 +1206,7 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi } diffFile.IsGenerated = isGenerated.Value() - tailSection := diffFile.GetTailSection(gitRepo, opts.BeforeCommit, commit) + tailSection := diffFile.GetTailSection(gitRepo, beforeCommit, commit) if tailSection != nil { diffFile.Sections = append(diffFile.Sections, tailSection) } From 35eb254d8cb48130880ada5744963a49a86e92ff Mon Sep 17 00:00:00 2001 From: Rowan Bohde Date: Fri, 1 Nov 2024 12:13:06 -0500 Subject: [PATCH 6/8] don't reassign ctx --- services/gitdiff/gitdiff.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 138e85a3491ff..e3ac9fe845185 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1104,8 +1104,8 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi return nil, err } - ctx, cancel := context.WithCancel(ctx) - defer cancel() + cmdCtx, cmdCancel := context.WithCancel(ctx) + defer cmdCancel() cmdDiff := git.NewCommand(ctx) objectFormat, err := gitRepo.GetObjectFormat() @@ -1169,9 +1169,9 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi _ = writer.Close() }() - diff, err := ParsePatch(ctx, opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, reader, parsePatchSkipToFile) + diff, err := ParsePatch(cmdCtx, opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, reader, parsePatchSkipToFile) // Ensure the git process is killed if it didn't exit already - cancel() + cmdCancel() if err != nil { return nil, fmt.Errorf("unable to ParsePatch: %w", err) } From 295de7e3076e7da3cc66b072bb15e0ca9bec11e0 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 2 Nov 2024 01:17:54 +0800 Subject: [PATCH 7/8] Update services/gitdiff/gitdiff.go --- services/gitdiff/gitdiff.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index e3ac9fe845185..cf7da0707b886 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1107,7 +1107,7 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi cmdCtx, cmdCancel := context.WithCancel(ctx) defer cmdCancel() - cmdDiff := git.NewCommand(ctx) + cmdDiff := git.NewCommand(cmdCtx) objectFormat, err := gitRepo.GetObjectFormat() if err != nil { return nil, err From 69acf6a3ddba60089e5d9c15a4249a76a58de53a Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sat, 2 Nov 2024 01:21:22 +0800 Subject: [PATCH 8/8] revert unrelated changes --- routers/web/repo/pull.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 61c9f2651a929..cc554a71ff605 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -740,17 +740,6 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi maxLines, maxFiles = -1, -1 } - baseCommit, err := ctx.Repo.GitRepo.GetCommit(startCommitID) - if err != nil { - ctx.ServerError("GetCommit", err) - return - } - commit, err := gitRepo.GetCommit(endCommitID) - if err != nil { - ctx.ServerError("GetCommit", err) - return - } - diffOptions := &gitdiff.DiffOptions{ AfterCommitID: endCommitID, SkipTo: ctx.FormString("skip-to"), @@ -825,6 +814,17 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi ctx.Data["Diff"] = diff ctx.Data["DiffNotAvailable"] = diff.NumFiles == 0 + baseCommit, err := ctx.Repo.GitRepo.GetCommit(startCommitID) + if err != nil { + ctx.ServerError("GetCommit", err) + return + } + commit, err := gitRepo.GetCommit(endCommitID) + if err != nil { + ctx.ServerError("GetCommit", err) + return + } + if ctx.IsSigned && ctx.Doer != nil { if ctx.Data["CanMarkConversation"], err = issues_model.CanMarkConversation(ctx, issue, ctx.Doer); err != nil { ctx.ServerError("CanMarkConversation", err)