From c54639b8eec16be285cd48ce18183d8265d928cc Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 31 Aug 2021 05:02:27 +0200 Subject: [PATCH] Repare and Improve GetDiffRangeWithWhitespaceBehavior (#16894) (#16895) fix pipe leak --- routers/web/repo/commit.go | 10 ++++------ routers/web/repo/compare.go | 8 ++++++-- routers/web/repo/pull.go | 8 ++------ services/gitdiff/gitdiff.go | 29 +++++++---------------------- services/gitdiff/gitdiff_test.go | 8 +++++++- 5 files changed, 26 insertions(+), 37 deletions(-) diff --git a/routers/web/repo/commit.go b/routers/web/repo/commit.go index 45ef22f498..98f5336867 100644 --- a/routers/web/repo/commit.go +++ b/routers/web/repo/commit.go @@ -268,9 +268,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 { @@ -279,10 +278,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) @@ -306,7 +304,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))) diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index e66aa614cb..1e4df802b9 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -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 { @@ -618,11 +618,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))) diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 703bbd837a..251aa1e7e9 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -587,10 +587,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 @@ -607,9 +606,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) @@ -623,7 +619,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))) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 59da680d48..c1d1c40d3d 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -20,6 +20,7 @@ import ( "regexp" "sort" "strings" + "time" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/charset" @@ -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"} @@ -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 diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index 2386552efe..6f8540ab5b 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -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/setting" jsoniter "github.com/json-iterator/go" @@ -513,8 +514,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 {