Skip to content

improve performance of diffs #32393

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Nov 2, 2024
Merged

improve performance of diffs #32393

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cc40ab7
improve performance of diffs
bohde Oct 31, 2024
dd9a956
refactor to have a simpler ParsePatch API
bohde Nov 1, 2024
27cb5c4
refactor to use RepositoryFromContextOrOpen
bohde Nov 1, 2024
1e82e2c
fix DiffIndex throwing an error when canceling the process early
bohde Nov 1, 2024
935e2d5
remove AfterCommit & BeforeCommit from DiffOptions
bohde Nov 1, 2024
35eb254
don't reassign ctx
bohde Nov 1, 2024
295de7e
Update services/gitdiff/gitdiff.go
wxiaoguang Nov 1, 2024
69acf6a
revert unrelated changes
wxiaoguang Nov 1, 2024
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactor to have a simpler ParsePatch API
  • Loading branch information
@bohde
bohde committed Nov 1, 2024
commit dd9a9565dfcfa504a9b03f6a000c7e6e168e01ab
2 changes: 1 addition & 1 deletion services/gitdiff/csv_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
18 changes: 5 additions & 13 deletions services/gitdiff/gitdiff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Expand Down
24 changes: 12 additions & 12 deletions services/gitdiff/gitdiff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand Down Expand Up @@ -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), "")
}
}
3 changes: 2 additions & 1 deletion services/repository/files/temp_repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down