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 are also included: * Reuse existing repo in `PrepareViewPullInfo` if head and base are the same. The performance impact is going to depend heavily on the individual diff and the hardware it runs on, but when testing locally on a diff changing 100k+ lines over hundreds of files, I'm seeing a roughly 75% reduction in time to load the result of "Show More" ---------
Showing the changes to services/gitdiff/gitdiff.go (1 of 4 files). Show all
Unified · Split
| @@ -379,18 +379,11 @@ func (diffFile *DiffFile) GetType() int { · 378 unmodified lines | |||
| 379 | } | 379 | } |
| 380 | 380 | ||
| 381 | // GetTailSection creates a fake DiffLineSection if the last section is not the end of the file | 381 | // GetTailSection creates a fake DiffLineSection if the last section is not the end of the file |
| 382 | func (diffFile *DiffFile) GetTailSection(gitRepo *git.Repository, leftCommitID, rightCommitID string) *DiffSection { | 382 | func (diffFile *DiffFile) GetTailSection(gitRepo *git.Repository, leftCommit, rightCommit *git.Commit) *DiffSection { |
| 383 | if len(diffFile.Sections) == 0 || diffFile.Type != DiffFileChange || diffFile.IsBin || diffFile.IsLFSFile { | 383 | if len(diffFile.Sections) == 0 || diffFile.Type != DiffFileChange || diffFile.IsBin || diffFile.IsLFSFile { |
| 384 | return nil | 384 | return nil |
| 385 | } | 385 | } |
| 386 | leftCommit, err := gitRepo.GetCommit(leftCommitID) | 386 | |
| 387 | if err != nil { | ||
| 388 | return nil | ||
| 389 | } | ||
| 390 | rightCommit, err := gitRepo.GetCommit(rightCommitID) | ||
| 391 | if err != nil { | ||
| 392 | return nil | ||
| 393 | } | ||
| 394 | lastSection := diffFile.Sections[len(diffFile.Sections)-1] | 387 | lastSection := diffFile.Sections[len(diffFile.Sections)-1] |
| 395 | lastLine := lastSection.Lines[len(lastSection.Lines)-1] | 388 | lastLine := lastSection.Lines[len(lastSection.Lines)-1] |
| 396 | leftLineCount := getCommitFileLineCount(leftCommit, diffFile.Name) | 389 | leftLineCount := getCommitFileLineCount(leftCommit, diffFile.Name) |
| @@ -532,11 +525,6 @@ parsingLoop: · 135 unmodified lines | |||
| 532 | lastFile := createDiffFile(diff, line) | 525 | lastFile := createDiffFile(diff, line) |
| 533 | diff.End = lastFile.Name | 526 | diff.End = lastFile.Name |
| 534 | diff.IsIncomplete = true | 527 | diff.IsIncomplete = true |
| 535 | _, err := io.Copy(io.Discard, reader) | ||
| 536 | if err != nil { | ||
| 537 | // By the definition of io.Copy this never returns io.EOF | ||
| 538 | return diff, fmt.Errorf("error during io.Copy: %w", err) | ||
| 539 | } | ||
| 540 | break parsingLoop | 528 | break parsingLoop |
| 541 | } | 529 | } |
| 542 | 530 | ||
| @@ -1097,6 +1085,7 @@ type DiffOptions struct { · 554 unmodified lines | |||
| 1097 | MaxFiles int | 1085 | MaxFiles int |
| 1098 | WhitespaceBehavior git.TrustedCmdArgs | 1086 | WhitespaceBehavior git.TrustedCmdArgs |
| 1099 | DirectComparison bool | 1087 | DirectComparison bool |
| 1088 | FileOnly bool | ||
| 1100 | } | 1089 | } |
| 1101 | 1090 | ||
| 1102 | // GetDiff builds a Diff between two commits of a repository. | 1091 | // GetDiff builds a Diff between two commits of a repository. |
| @@ -1105,12 +1094,16 @@ type DiffOptions struct { · 2 unmodified lines | |||
| 1105 | func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) { | 1094 | func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) { |
| 1106 | repoPath := gitRepo.Path | 1095 | repoPath := gitRepo.Path |
| 1107 | 1096 | ||
| 1097 | var beforeCommit *git.Commit | ||
| 1108 | commit, err := gitRepo.GetCommit(opts.AfterCommitID) | 1098 | commit, err := gitRepo.GetCommit(opts.AfterCommitID) |
| 1109 | if err != nil { | 1099 | if err != nil { |
| 1110 | return nil, err | 1100 | return nil, err |
| 1111 | } | 1101 | } |
| 1112 | 1102 | ||
| 1113 | cmdDiff := git.NewCommand(gitRepo.Ctx) | 1103 | cmdCtx, cmdCancel := context.WithCancel(ctx) |
| 1104 | defer cmdCancel() | ||
| 1105 | |||
| 1106 | cmdDiff := git.NewCommand(cmdCtx) | ||
| 1114 | objectFormat, err := gitRepo.GetObjectFormat() | 1107 | objectFormat, err := gitRepo.GetObjectFormat() |
| 1115 | if err != nil { | 1108 | if err != nil { |
| 1116 | return nil, err | 1109 | return nil, err |
| @@ -1132,6 +1125,12 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi · 15 unmodified lines | |||
| 1132 | AddArguments(opts.WhitespaceBehavior...). | 1125 | AddArguments(opts.WhitespaceBehavior...). |
| 1133 | AddDynamicArguments(actualBeforeCommitID, opts.AfterCommitID) | 1126 | AddDynamicArguments(actualBeforeCommitID, opts.AfterCommitID) |
| 1134 | opts.BeforeCommitID = actualBeforeCommitID | 1127 | opts.BeforeCommitID = actualBeforeCommitID |
| 1128 | |||
| 1129 | var err error | ||
| 1130 | beforeCommit, err = gitRepo.GetCommit(opts.BeforeCommitID) | ||
| 1131 | if err != nil { | ||
| 1132 | return nil, err | ||
| 1133 | } | ||
| 1135 | } | 1134 | } |
| 1136 | 1135 | ||
| 1137 | // In git 2.31, git diff learned --skip-to which we can use to shortcut skip to file | 1136 | // In git 2.31, git diff learned --skip-to which we can use to shortcut skip to file |
| @@ -1166,7 +1165,9 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi · 28 unmodified lines | |||
| 1166 | _ = writer.Close() | 1165 | _ = writer.Close() |
| 1167 | }() | 1166 | }() |
| 1168 | 1167 | ||
| 1169 | diff, err := ParsePatch(ctx, opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, reader, parsePatchSkipToFile) | 1168 | diff, err := ParsePatch(cmdCtx, opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, reader, parsePatchSkipToFile) |
| 1169 | // Ensure the git process is killed if it didn't exit already | ||
| 1170 | cmdCancel() | ||
| 1170 | if err != nil { | 1171 | if err != nil { |
| 1171 | return nil, fmt.Errorf("unable to ParsePatch: %w", err) | 1172 | return nil, fmt.Errorf("unable to ParsePatch: %w", err) |
| 1172 | } | 1173 | } |
| @@ -1207,37 +1208,28 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi · 34 unmodified lines | |||
| 1207 | diffFile.IsGenerated = analyze.IsGenerated(diffFile.Name) | 1208 | diffFile.IsGenerated = analyze.IsGenerated(diffFile.Name) |
| 1208 | } | 1209 | } |
| 1209 | 1210 | ||
| 1210 | tailSection := diffFile.GetTailSection(gitRepo, opts.BeforeCommitID, opts.AfterCommitID) | 1211 | tailSection := diffFile.GetTailSection(gitRepo, beforeCommit, commit) |
| 1211 | if tailSection != nil { | 1212 | if tailSection != nil { |
| 1212 | diffFile.Sections = append(diffFile.Sections, tailSection) | 1213 | diffFile.Sections = append(diffFile.Sections, tailSection) |
| 1213 | } | 1214 | } |
| 1214 | } | 1215 | } |
| 1215 | 1216 | ||
| 1216 | separator := "..." | 1217 | if opts.FileOnly { |
| 1217 | if opts.DirectComparison { | 1218 | return diff, nil |
| 1218 | separator = ".." | ||
| 1219 | } | 1219 | } |
| 1220 | 1220 | ||
| 1221 | diffPaths := []string{opts.BeforeCommitID + separator + opts.AfterCommitID} | 1221 | stats, err := GetPullDiffStats(gitRepo, opts) |
| 1222 | if len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == objectFormat.EmptyObjectID().String() { | ||
| 1223 | diffPaths = []string{objectFormat.EmptyTree().String(), opts.AfterCommitID} | ||
| 1224 | } | ||
| 1225 | diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) | ||
| 1226 | if err != nil && strings.Contains(err.Error(), "no merge base") { | ||
| 1227 | // git >= 2.28 now returns an error if base and head have become unrelated. | ||
| 1228 | // previously it would return the results of git diff --shortstat base head so let's try that... | ||
| 1229 | diffPaths = []string{opts.BeforeCommitID, opts.AfterCommitID} | ||
| 1230 | diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) | ||
| 1231 | } | ||
| 1232 | if err != nil { | 1222 | if err != nil { |
| 1233 | return nil, err | 1223 | return nil, err |
| 1234 | } | 1224 | } |
| 1235 | 1225 | ||
| 1226 | diff.NumFiles, diff.TotalAddition, diff.TotalDeletion = stats.NumFiles, stats.TotalAddition, stats.TotalDeletion | ||
| 1227 | |||
| 1236 | return diff, nil | 1228 | return diff, nil |
| 1237 | } | 1229 | } |
| 1238 | 1230 | ||
| 1239 | type PullDiffStats struct { | 1231 | type PullDiffStats struct { |
| 1240 | TotalAddition, TotalDeletion int | 1232 | NumFiles, TotalAddition, TotalDeletion int |
| 1241 | } | 1233 | } |
| 1242 | 1234 | ||
| 1243 | // GetPullDiffStats | 1235 | // GetPullDiffStats |
| @@ -1261,12 +1253,12 @@ func GetPullDiffStats(gitRepo *git.Repository, opts *DiffOptions) (*PullDiffStat · 17 unmodified lines | |||
| 1261 | diffPaths = []string{objectFormat.EmptyTree().String(), opts.AfterCommitID} | 1253 | diffPaths = []string{objectFormat.EmptyTree().String(), opts.AfterCommitID} |
| 1262 | } | 1254 | } |
| 1263 | 1255 | ||
| 1264 | _, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) | 1256 | diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) |
| 1265 | if err != nil && strings.Contains(err.Error(), "no merge base") { | 1257 | if err != nil && strings.Contains(err.Error(), "no merge base") { |
| 1266 | // git >= 2.28 now returns an error if base and head have become unrelated. | 1258 | // git >= 2.28 now returns an error if base and head have become unrelated. |
| 1267 | // previously it would return the results of git diff --shortstat base head so let's try that... | 1259 | // previously it would return the results of git diff --shortstat base head so let's try that... |
| 1268 | diffPaths = []string{opts.BeforeCommitID, opts.AfterCommitID} | 1260 | diffPaths = []string{opts.BeforeCommitID, opts.AfterCommitID} |
| 1269 | _, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) | 1261 | diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(gitRepo.Ctx, repoPath, nil, diffPaths...) |
| 1270 | } | 1262 | } |
| 1271 | if err != nil { | 1263 | if err != nil { |
| 1272 | return nil, err | 1264 | return nil, err |