Skip to content
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.

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

Allow Loading of Diffs that are too large #17739

Merged
merged 7 commits into from
Nov 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -2043,6 +2043,7 @@ diff.file_suppressed = File diff suppressed because it is too large
diff.file_suppressed_line_too_long = File diff suppressed because one or more lines are too long
diff.too_many_files = Some files were not shown because too many files have changed in this diff
diff.show_more = Show More
diff.load = Load Diff
diff.generated = generated
diff.vendored = vendored
diff.comment.placeholder = Leave a comment
Expand Down
28 changes: 16 additions & 12 deletions routers/web/repo/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,6 @@ func Diff(ctx *context.Context) {
err error
)

fileOnly := ctx.FormBool("file-only")

if ctx.Data["PageIsWiki"] != nil {
gitRepo, err = git.OpenRepository(ctx.Repo.Repository.WikiPath())
if err != nil {
Expand All @@ -288,13 +286,23 @@ func Diff(ctx *context.Context) {
commitID = commit.ID.String()
}

diff, err := gitdiff.GetDiffCommitWithWhitespaceBehavior(gitRepo,
commitID, ctx.FormString("skip-to"), setting.Git.MaxGitDiffLines,
setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles,
gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)),
false)
fileOnly := ctx.FormBool("file-only")
maxLines, maxFiles := setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffFiles
files := ctx.FormStrings("files")
if fileOnly && (len(files) == 2 || len(files) == 1) {
zeripath marked this conversation as resolved.
Show resolved Hide resolved
maxLines, maxFiles = -1, -1
}

diff, err := gitdiff.GetDiff(gitRepo, &gitdiff.DiffOptions{
AfterCommitID: commitID,
SkipTo: ctx.FormString("skip-to"),
MaxLines: maxLines,
MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters,
MaxFiles: maxFiles,
WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)),
}, files...)
if err != nil {
ctx.NotFound("GetDiffCommitWithWhitespaceBehavior", err)
ctx.NotFound("GetDiff", err)
return
}

Expand Down Expand Up @@ -325,10 +333,6 @@ func Diff(ctx *context.Context) {
ctx.Data["Title"] = commit.Summary() + " · " + base.ShortSha(commitID)
ctx.Data["Commit"] = commit
ctx.Data["Diff"] = diff
if fileOnly {
ctx.HTML(http.StatusOK, tplDiffBox)
return
}

statuses, err := models.GetLatestCommitStatus(ctx.Repo.Repository.ID, commitID, db.ListOptions{})
if err != nil {
Expand Down
20 changes: 17 additions & 3 deletions routers/web/repo/compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -569,9 +569,23 @@ func PrepareCompareDiff(
beforeCommitID = ci.CompareInfo.BaseCommitID
}

diff, err := gitdiff.GetDiffRangeWithWhitespaceBehavior(ci.HeadGitRepo,
beforeCommitID, headCommitID, ctx.FormString("skip-to"), setting.Git.MaxGitDiffLines,
setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, whitespaceBehavior, ci.DirectComparison)
maxLines, maxFiles := setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffFiles
files := ctx.FormStrings("files")
if len(files) == 2 || len(files) == 1 {
zeripath marked this conversation as resolved.
Show resolved Hide resolved
maxLines, maxFiles = -1, -1
}

diff, err := gitdiff.GetDiff(ci.HeadGitRepo,
&gitdiff.DiffOptions{
BeforeCommitID: beforeCommitID,
AfterCommitID: headCommitID,
SkipTo: ctx.FormString("skip-to"),
MaxLines: maxLines,
MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters,
MaxFiles: maxFiles,
WhitespaceBehavior: whitespaceBehavior,
DirectComparison: ci.DirectComparison,
}, ctx.FormStrings("files")...)
if err != nil {
ctx.ServerError("GetDiffRangeWithWhitespaceBehavior", err)
return false
Expand Down
22 changes: 18 additions & 4 deletions routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -633,10 +633,24 @@ func ViewPullFiles(ctx *context.Context) {
ctx.Data["Reponame"] = ctx.Repo.Repository.Name
ctx.Data["AfterCommitID"] = endCommitID

diff, err := gitdiff.GetDiffRangeWithWhitespaceBehavior(gitRepo,
startCommitID, endCommitID, ctx.FormString("skip-to"), setting.Git.MaxGitDiffLines,
setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles,
gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)), false)
fileOnly := ctx.FormBool("file-only")

maxLines, maxFiles := setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffFiles
files := ctx.FormStrings("files")
if fileOnly && (len(files) == 2 || len(files) == 1) {
zeripath marked this conversation as resolved.
Show resolved Hide resolved
maxLines, maxFiles = -1, -1
}

diff, err := gitdiff.GetDiff(gitRepo,
&gitdiff.DiffOptions{
BeforeCommitID: startCommitID,
AfterCommitID: endCommitID,
SkipTo: ctx.FormString("skip-to"),
MaxLines: maxLines,
MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters,
MaxFiles: maxFiles,
WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)),
}, ctx.FormStrings("files")...)
if err != nil {
ctx.ServerError("GetDiffRangeWithWhitespaceBehavior", err)
return
Expand Down
96 changes: 56 additions & 40 deletions services/gitdiff/gitdiff.go
Original file line number Diff line number Diff line change
Expand Up @@ -695,6 +695,7 @@ const cmdDiffHead = "diff --git "

// ParsePatch builds a Diff object from a io.Reader and some parameters.
func ParsePatch(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

skipping := skipToFile != ""
Expand Down Expand Up @@ -726,7 +727,7 @@ parsingLoop:
return diff, fmt.Errorf("invalid first file line: %s", line)
}

if len(diff.Files) >= maxFiles {
if maxFiles > -1 && len(diff.Files) >= maxFiles {
lastFile := createDiffFile(diff, line)
diff.End = lastFile.Name
diff.IsIncomplete = true
Expand Down Expand Up @@ -1038,7 +1039,7 @@ func parseHunks(curFile *DiffFile, maxLines, maxLineCharacters int, input *bufio

switch lineBytes[0] {
case '@':
if curFileLinesCount >= maxLines {
if maxLines > -1 && curFileLinesCount >= maxLines {
curFile.IsIncomplete = true
continue
}
Expand Down Expand Up @@ -1075,7 +1076,7 @@ func parseHunks(curFile *DiffFile, maxLines, maxLineCharacters int, input *bufio
rightLine = lineSectionInfo.RightIdx
continue
case '\\':
if curFileLinesCount >= maxLines {
if maxLines > -1 && curFileLinesCount >= maxLines {
curFile.IsIncomplete = true
continue
}
Expand All @@ -1090,7 +1091,7 @@ func parseHunks(curFile *DiffFile, maxLines, maxLineCharacters int, input *bufio
case '+':
curFileLinesCount++
curFile.Addition++
if curFileLinesCount >= maxLines {
if maxLines > -1 && curFileLinesCount >= maxLines {
curFile.IsIncomplete = true
continue
}
Expand All @@ -1114,7 +1115,7 @@ func parseHunks(curFile *DiffFile, maxLines, maxLineCharacters int, input *bufio
case '-':
curFileLinesCount++
curFile.Deletion++
if curFileLinesCount >= maxLines {
if maxLines > -1 && curFileLinesCount >= maxLines {
curFile.IsIncomplete = true
continue
}
Expand All @@ -1134,7 +1135,7 @@ func parseHunks(curFile *DiffFile, maxLines, maxLineCharacters int, input *bufio
curSection.Lines = append(curSection.Lines, diffLine)
case ' ':
curFileLinesCount++
if curFileLinesCount >= maxLines {
if maxLines > -1 && curFileLinesCount >= maxLines {
curFile.IsIncomplete = true
continue
}
Expand Down Expand Up @@ -1278,13 +1279,25 @@ func readFileName(rd *strings.Reader) (string, bool) {
return name[2:], ambiguity
}

// GetDiffRangeWithWhitespaceBehavior builds a Diff between two commits of a repository.
// DiffOptions represents the options for a DiffRange
type DiffOptions struct {
BeforeCommitID string
AfterCommitID string
SkipTo string
MaxLines int
MaxLineCharacters int
MaxFiles int
WhitespaceBehavior string
DirectComparison bool
}

// GetDiff builds a Diff between two commits of a repository.
// Passing the empty string as beforeCommitID returns a diff from the parent commit.
// The whitespaceBehavior is either an empty string or a git flag
func GetDiffRangeWithWhitespaceBehavior(gitRepo *git.Repository, beforeCommitID, afterCommitID, skipTo string, maxLines, maxLineCharacters, maxFiles int, whitespaceBehavior string, directComparison bool) (*Diff, error) {
func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) {
repoPath := gitRepo.Path

commit, err := gitRepo.GetCommit(afterCommitID)
commit, err := gitRepo.GetCommit(opts.AfterCommitID)
if err != nil {
return nil, err
}
Expand All @@ -1293,45 +1306,54 @@ func GetDiffRangeWithWhitespaceBehavior(gitRepo *git.Repository, beforeCommitID,
defer cancel()

argsLength := 6
if len(whitespaceBehavior) > 0 {
if len(opts.WhitespaceBehavior) > 0 {
argsLength++
}
if len(skipTo) > 0 {
if len(opts.SkipTo) > 0 {
argsLength++
}
if len(files) > 0 {
argsLength += len(files) + 1
}

diffArgs := make([]string, 0, argsLength)
if (len(beforeCommitID) == 0 || beforeCommitID == git.EmptySHA) && commit.ParentCount() == 0 {
if (len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == git.EmptySHA) && commit.ParentCount() == 0 {
diffArgs = append(diffArgs, "diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M")
if len(whitespaceBehavior) != 0 {
diffArgs = append(diffArgs, whitespaceBehavior)
if len(opts.WhitespaceBehavior) != 0 {
diffArgs = append(diffArgs, opts.WhitespaceBehavior)
}
// append empty tree ref
diffArgs = append(diffArgs, "4b825dc642cb6eb9a060e54bf8d69288fbee4904")
diffArgs = append(diffArgs, afterCommitID)
diffArgs = append(diffArgs, opts.AfterCommitID)
} else {
actualBeforeCommitID := beforeCommitID
actualBeforeCommitID := opts.BeforeCommitID
if len(actualBeforeCommitID) == 0 {
parentCommit, _ := commit.Parent(0)
actualBeforeCommitID = parentCommit.ID.String()
}
diffArgs = append(diffArgs, "diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M")
if len(whitespaceBehavior) != 0 {
diffArgs = append(diffArgs, whitespaceBehavior)
if len(opts.WhitespaceBehavior) != 0 {
diffArgs = append(diffArgs, opts.WhitespaceBehavior)
}
diffArgs = append(diffArgs, actualBeforeCommitID)
diffArgs = append(diffArgs, afterCommitID)
beforeCommitID = actualBeforeCommitID
diffArgs = append(diffArgs, opts.AfterCommitID)
opts.BeforeCommitID = actualBeforeCommitID
}

// 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
// the skipping for us
parsePatchSkipToFile := skipTo
if skipTo != "" && git.CheckGitVersionAtLeast("2.31") == nil {
diffArgs = append(diffArgs, "--skip-to="+skipTo)
parsePatchSkipToFile := opts.SkipTo
if opts.SkipTo != "" && git.CheckGitVersionAtLeast("2.31") == nil {
diffArgs = append(diffArgs, "--skip-to="+opts.SkipTo)
parsePatchSkipToFile = ""
}

if len(files) > 0 {
diffArgs = append(diffArgs, "--")
diffArgs = append(diffArgs, files...)
}

cmd := exec.CommandContext(ctx, git.GitExecutable, diffArgs...)

cmd.Dir = repoPath
Expand All @@ -1349,16 +1371,16 @@ func GetDiffRangeWithWhitespaceBehavior(gitRepo *git.Repository, beforeCommitID,
pid := process.GetManager().Add(fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath), cancel)
defer process.GetManager().Remove(pid)

diff, err := ParsePatch(maxLines, maxLineCharacters, maxFiles, stdout, parsePatchSkipToFile)
diff, err := ParsePatch(opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, stdout, parsePatchSkipToFile)
if err != nil {
return nil, fmt.Errorf("unable to ParsePatch: %w", err)
}
diff.Start = skipTo
diff.Start = opts.SkipTo

var checker *git.CheckAttributeReader

if git.CheckGitVersionAtLeast("1.7.8") == nil {
indexFilename, worktree, deleteTemporaryFile, err := gitRepo.ReadTreeToTemporaryIndex(afterCommitID)
indexFilename, worktree, deleteTemporaryFile, err := gitRepo.ReadTreeToTemporaryIndex(opts.AfterCommitID)
if err == nil {
defer deleteTemporaryFile()

Expand All @@ -1370,12 +1392,12 @@ func GetDiffRangeWithWhitespaceBehavior(gitRepo *git.Repository, beforeCommitID,
}
ctx, cancel := context.WithCancel(git.DefaultContext)
if err := checker.Init(ctx); err != nil {
log.Error("Unable to open checker for %s. Error: %v", afterCommitID, err)
log.Error("Unable to open checker for %s. Error: %v", opts.AfterCommitID, err)
} else {
go func() {
err := checker.Run()
if err != nil && err != ctx.Err() {
log.Error("Unable to open checker for %s. Error: %v", afterCommitID, err)
log.Error("Unable to open checker for %s. Error: %v", opts.AfterCommitID, err)
}
cancel()
}()
Expand Down Expand Up @@ -1426,7 +1448,7 @@ func GetDiffRangeWithWhitespaceBehavior(gitRepo *git.Repository, beforeCommitID,
diffFile.IsGenerated = analyze.IsGenerated(diffFile.Name)
}

tailSection := diffFile.GetTailSection(gitRepo, beforeCommitID, afterCommitID)
tailSection := diffFile.GetTailSection(gitRepo, opts.BeforeCommitID, opts.AfterCommitID)
if tailSection != nil {
diffFile.Sections = append(diffFile.Sections, tailSection)
}
Expand All @@ -1437,19 +1459,19 @@ func GetDiffRangeWithWhitespaceBehavior(gitRepo *git.Repository, beforeCommitID,
}

separator := "..."
if directComparison {
if opts.DirectComparison {
separator = ".."
}

shortstatArgs := []string{beforeCommitID + separator + afterCommitID}
if len(beforeCommitID) == 0 || beforeCommitID == git.EmptySHA {
shortstatArgs = []string{git.EmptyTreeSHA, afterCommitID}
shortstatArgs := []string{opts.BeforeCommitID + separator + opts.AfterCommitID}
if len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == git.EmptySHA {
shortstatArgs = []string{git.EmptyTreeSHA, opts.AfterCommitID}
}
diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(repoPath, shortstatArgs...)
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...
shortstatArgs = []string{beforeCommitID, afterCommitID}
shortstatArgs = []string{opts.BeforeCommitID, opts.AfterCommitID}
diff.NumFiles, diff.TotalAddition, diff.TotalDeletion, err = git.GetDiffShortStat(repoPath, shortstatArgs...)
}
if err != nil {
Expand All @@ -1459,12 +1481,6 @@ func GetDiffRangeWithWhitespaceBehavior(gitRepo *git.Repository, beforeCommitID,
return diff, nil
}

// GetDiffCommitWithWhitespaceBehavior builds a Diff representing the given commitID.
// The whitespaceBehavior is either an empty string or a git flag
func GetDiffCommitWithWhitespaceBehavior(gitRepo *git.Repository, commitID, skipTo string, maxLines, maxLineCharacters, maxFiles int, whitespaceBehavior string, directComparison bool) (*Diff, error) {
return GetDiffRangeWithWhitespaceBehavior(gitRepo, "", commitID, skipTo, maxLines, maxLineCharacters, maxFiles, whitespaceBehavior, directComparison)
}

// CommentAsDiff returns c.Patch as *Diff
func CommentAsDiff(c *models.Comment) (*Diff, error) {
diff, err := ParsePatch(setting.Git.MaxGitDiffLines,
Expand Down
11 changes: 9 additions & 2 deletions services/gitdiff/gitdiff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -693,8 +693,15 @@ func TestGetDiffRangeWithWhitespaceBehavior(t *testing.T) {
}
defer gitRepo.Close()
for _, behavior := range []string{"-w", "--ignore-space-at-eol", "-b", ""} {
diffs, err := GetDiffRangeWithWhitespaceBehavior(gitRepo, "559c156f8e0178b71cb44355428f24001b08fc68", "bd7063cc7c04689c4d082183d32a604ed27a24f9", "",
setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffFiles, behavior, false)
diffs, err := GetDiff(gitRepo,
&DiffOptions{
AfterCommitID: "bd7063cc7c04689c4d082183d32a604ed27a24f9",
BeforeCommitID: "559c156f8e0178b71cb44355428f24001b08fc68",
MaxLines: setting.Git.MaxGitDiffLines,
MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters,
MaxFiles: setting.Git.MaxGitDiffFiles,
WhitespaceBehavior: behavior,
})
assert.NoError(t, err, fmt.Sprintf("Error when diff with %s", behavior))
for _, f := range diffs.Files {
assert.True(t, len(f.Sections) > 0, fmt.Sprintf("%s should have sections", f.Name))
Expand Down
1 change: 1 addition & 0 deletions templates/repo/diff/box.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@
{{$.i18n.Tr "repo.diff.file_suppressed_line_too_long"}}
{{else}}
{{$.i18n.Tr "repo.diff.file_suppressed"}}
<a class="ui basic tiny button diff-show-more-button" data-href="{{$.Link}}?file-only=true&files={{$file.Name}}&files={{$file.OldName}}">{{$.i18n.Tr "repo.diff.load"}}</a>
{{end}}
{{else}}
{{$.i18n.Tr "repo.diff.bin_not_shown"}}
Expand Down
Loading