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

Repair and Improve GetDiffRangeWithWhitespaceBehavior #16894

Merged
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
10 changes: 4 additions & 6 deletions routers/web/repo/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -259,9 +259,8 @@ func Diff(ctx *context.Context) {
repoName := ctx.Repo.Repository.Name
commitID := ctx.Params(":sha")
var (
gitRepo *git.Repository
err error
repoPath string
gitRepo *git.Repository
err error
)

if ctx.Data["PageIsWiki"] != nil {
Expand All @@ -270,10 +269,9 @@ func Diff(ctx *context.Context) {
ctx.ServerError("Repo.GitRepo.GetCommit", err)
return
}
repoPath = ctx.Repo.Repository.WikiPath()
defer gitRepo.Close()
} else {
gitRepo = ctx.Repo.GitRepo
repoPath = models.RepoPath(userName, repoName)
}

commit, err := gitRepo.GetCommit(commitID)
Expand All @@ -297,7 +295,7 @@ func Diff(ctx *context.Context) {
ctx.Data["CommitStatus"] = models.CalcCommitStatus(statuses)
ctx.Data["CommitStatuses"] = statuses

diff, err := gitdiff.GetDiffCommitWithWhitespaceBehavior(repoPath,
diff, err := gitdiff.GetDiffCommitWithWhitespaceBehavior(gitRepo,
commitID, setting.Git.MaxGitDiffLines,
setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles,
gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)))
Expand Down
8 changes: 6 additions & 2 deletions routers/web/repo/compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ func PrepareCompareDiff(
return true
}

diff, err := gitdiff.GetDiffRangeWithWhitespaceBehavior(models.RepoPath(headUser.Name, headRepo.Name),
diff, err := gitdiff.GetDiffRangeWithWhitespaceBehavior(headGitRepo,
compareInfo.MergeBase, headCommitID, setting.Git.MaxGitDiffLines,
setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, whitespaceBehavior)
if err != nil {
Expand Down Expand Up @@ -616,11 +616,15 @@ func getBranchesAndTagsForRepo(user *models.User, repo *models.Repository) (bool
// CompareDiff show different from one commit to another commit
func CompareDiff(ctx *context.Context) {
headUser, headRepo, headGitRepo, compareInfo, baseBranch, headBranch := ParseCompareInfo(ctx)
defer func() {
if headGitRepo != nil {
headGitRepo.Close()
}
}()

if ctx.Written() {
return
}
defer headGitRepo.Close()

nothingToCompare := PrepareCompareDiff(ctx, headUser, headRepo, headGitRepo, compareInfo, baseBranch, headBranch,
gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)))
Expand Down
8 changes: 2 additions & 6 deletions routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -599,10 +599,9 @@ func ViewPullFiles(ctx *context.Context) {
pull := issue.PullRequest

var (
diffRepoPath string
startCommitID string
endCommitID string
gitRepo *git.Repository
gitRepo = ctx.Repo.GitRepo
)

var prInfo *git.CompareInfo
Expand All @@ -619,9 +618,6 @@ func ViewPullFiles(ctx *context.Context) {
return
}

diffRepoPath = ctx.Repo.GitRepo.Path
gitRepo = ctx.Repo.GitRepo

headCommitID, err := gitRepo.GetRefCommitID(pull.GetGitRefName())
if err != nil {
ctx.ServerError("GetRefCommitID", err)
Expand All @@ -635,7 +631,7 @@ func ViewPullFiles(ctx *context.Context) {
ctx.Data["Reponame"] = ctx.Repo.Repository.Name
ctx.Data["AfterCommitID"] = endCommitID

diff, err := gitdiff.GetDiffRangeWithWhitespaceBehavior(diffRepoPath,
diff, err := gitdiff.GetDiffRangeWithWhitespaceBehavior(gitRepo,
startCommitID, endCommitID, setting.Git.MaxGitDiffLines,
setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles,
gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)))
Expand Down
29 changes: 7 additions & 22 deletions services/gitdiff/gitdiff.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"regexp"
"sort"
"strings"
"time"

"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/charset"
Expand Down Expand Up @@ -1205,31 +1206,20 @@ func readFileName(rd *strings.Reader) (string, bool) {
return name[2:], ambiguity
}

// GetDiffRange builds a Diff between two commits of a repository.
// passing the empty string as beforeCommitID returns a diff from the
// parent commit.
func GetDiffRange(repoPath, beforeCommitID, afterCommitID string, maxLines, maxLineCharacters, maxFiles int) (*Diff, error) {
return GetDiffRangeWithWhitespaceBehavior(repoPath, beforeCommitID, afterCommitID, maxLines, maxLineCharacters, maxFiles, "")
}

// GetDiffRangeWithWhitespaceBehavior 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(repoPath, beforeCommitID, afterCommitID string, maxLines, maxLineCharacters, maxFiles int, whitespaceBehavior string) (*Diff, error) {
gitRepo, err := git.OpenRepository(repoPath)
if err != nil {
return nil, err
}
defer gitRepo.Close()
func GetDiffRangeWithWhitespaceBehavior(gitRepo *git.Repository, beforeCommitID, afterCommitID string, maxLines, maxLineCharacters, maxFiles int, whitespaceBehavior string) (*Diff, error) {
repoPath := gitRepo.Path

commit, err := gitRepo.GetCommit(afterCommitID)
if err != nil {
return nil, err
}

// FIXME: graceful: These commands should likely have a timeout
ctx, cancel := context.WithCancel(git.DefaultContext)
ctx, cancel := context.WithTimeout(git.DefaultContext, time.Duration(setting.Git.Timeout.Default)*time.Second)
defer cancel()

var cmd *exec.Cmd
if (len(beforeCommitID) == 0 || beforeCommitID == git.EmptySHA) && commit.ParentCount() == 0 {
diffArgs := []string{"diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M"}
Expand Down Expand Up @@ -1303,15 +1293,10 @@ func GetDiffRangeWithWhitespaceBehavior(repoPath, beforeCommitID, afterCommitID
return diff, nil
}

// GetDiffCommit builds a Diff representing the given commitID.
func GetDiffCommit(repoPath, commitID string, maxLines, maxLineCharacters, maxFiles int) (*Diff, error) {
return GetDiffRangeWithWhitespaceBehavior(repoPath, "", commitID, maxLines, maxLineCharacters, maxFiles, "")
}

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

// CommentAsDiff returns c.Patch as *Diff
Expand Down
8 changes: 7 additions & 1 deletion services/gitdiff/gitdiff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"testing"

"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/highlight"
"code.gitea.io/gitea/modules/json"
"code.gitea.io/gitea/modules/setting"
Expand Down Expand Up @@ -514,8 +515,13 @@ func TestDiffLine_GetCommentSide(t *testing.T) {
}

func TestGetDiffRangeWithWhitespaceBehavior(t *testing.T) {
gitRepo, err := git.OpenRepository("./testdata/academic-module")
if !assert.NoError(t, err) {
return
}
defer gitRepo.Close()
for _, behavior := range []string{"-w", "--ignore-space-at-eol", "-b", ""} {
diffs, err := GetDiffRangeWithWhitespaceBehavior("./testdata/academic-module", "559c156f8e0178b71cb44355428f24001b08fc68", "bd7063cc7c04689c4d082183d32a604ed27a24f9",
diffs, err := GetDiffRangeWithWhitespaceBehavior(gitRepo, "559c156f8e0178b71cb44355428f24001b08fc68", "bd7063cc7c04689c4d082183d32a604ed27a24f9",
setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffFiles, behavior)
assert.NoError(t, err, fmt.Sprintf("Error when diff with %s", behavior))
for _, f := range diffs.Files {
Expand Down