From ff5629268c5c01d3f460570baa571baef3f896cd Mon Sep 17 00:00:00 2001 From: Matthew Walowski Date: Mon, 8 May 2023 00:10:53 -0700 Subject: [PATCH] Pass 'not' to commit count (#24473) Due to #24409 , we can now specify '--not' when getting all commits from a repo to exclude commits from a different branch. When I wrote that PR, I forgot to also update the code that counts the number of commits in the repo. So now, if the --not option is used, it may return too many commits, which can indicate that another page of data is available when it is not. This PR passes --not to the commands that count the number of commits in a repo --- modules/context/repo.go | 7 ++- modules/git/commit.go | 36 +++++++++----- modules/git/commit_test.go | 21 ++++++++- modules/git/repo_commit.go | 40 ++++++++++++---- routers/api/v1/repo/commits.go | 27 +++++++++-- routers/api/v1/repo/wiki.go | 7 ++- routers/web/feed/file.go | 8 +++- routers/web/repo/commit.go | 7 ++- routers/web/repo/wiki.go | 7 ++- .../integration/api_repo_git_commits_test.go | 47 +++++++++++++++++++ 10 files changed, 177 insertions(+), 30 deletions(-) diff --git a/modules/context/repo.go b/modules/context/repo.go index 6b811054c..a1c8f4364 100644 --- a/modules/context/repo.go +++ b/modules/context/repo.go @@ -208,7 +208,12 @@ func (r *Repository) GetCommitGraphsCount(ctx context.Context, hidePRRefs bool, if len(branches) == 0 { return git.AllCommitsCount(ctx, r.Repository.RepoPath(), hidePRRefs, files...) } - return git.CommitsCountFiles(ctx, r.Repository.RepoPath(), branches, files) + return git.CommitsCount(ctx, + git.CommitsCountOptions{ + RepoPath: r.Repository.RepoPath(), + Revision: branches, + RelPath: files, + }) }) } diff --git a/modules/git/commit.go b/modules/git/commit.go index f28c315cb..ff654f394 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -160,15 +160,29 @@ func AllCommitsCount(ctx context.Context, repoPath string, hidePRRefs bool, file return strconv.ParseInt(strings.TrimSpace(stdout), 10, 64) } -// CommitsCountFiles returns number of total commits of until given revision. -func CommitsCountFiles(ctx context.Context, repoPath string, revision, relpath []string) (int64, error) { +// CommitsCountOptions the options when counting commits +type CommitsCountOptions struct { + RepoPath string + Not string + Revision []string + RelPath []string +} + +// CommitsCount returns number of total commits of until given revision. +func CommitsCount(ctx context.Context, opts CommitsCountOptions) (int64, error) { cmd := NewCommand(ctx, "rev-list", "--count") - cmd.AddDynamicArguments(revision...) - if len(relpath) > 0 { - cmd.AddDashesAndList(relpath...) + + cmd.AddDynamicArguments(opts.Revision...) + + if opts.Not != "" { + cmd.AddOptionValues("--not", opts.Not) } - stdout, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath}) + if len(opts.RelPath) > 0 { + cmd.AddDashesAndList(opts.RelPath...) + } + + stdout, _, err := cmd.RunStdString(&RunOpts{Dir: opts.RepoPath}) if err != nil { return 0, err } @@ -176,14 +190,12 @@ func CommitsCountFiles(ctx context.Context, repoPath string, revision, relpath [ return strconv.ParseInt(strings.TrimSpace(stdout), 10, 64) } -// CommitsCount returns number of total commits of until given revision. -func CommitsCount(ctx context.Context, repoPath string, revision ...string) (int64, error) { - return CommitsCountFiles(ctx, repoPath, revision, []string{}) -} - // CommitsCount returns number of total commits of until current revision. func (c *Commit) CommitsCount() (int64, error) { - return CommitsCount(c.repo.Ctx, c.repo.Path, c.ID.String()) + return CommitsCount(c.repo.Ctx, CommitsCountOptions{ + RepoPath: c.repo.Path, + Revision: []string{c.ID.String()}, + }) } // CommitsByRange returns the specific page commits before current revision, every page's number default by CommitsRangeSize diff --git a/modules/git/commit_test.go b/modules/git/commit_test.go index 1d6fb0018..acf4beb02 100644 --- a/modules/git/commit_test.go +++ b/modules/git/commit_test.go @@ -14,11 +14,30 @@ import ( func TestCommitsCount(t *testing.T) { bareRepo1Path := filepath.Join(testReposDir, "repo1_bare") - commitsCount, err := CommitsCount(DefaultContext, bareRepo1Path, "8006ff9adbf0cb94da7dad9e537e53817f9fa5c0") + commitsCount, err := CommitsCount(DefaultContext, + CommitsCountOptions{ + RepoPath: bareRepo1Path, + Revision: []string{"8006ff9adbf0cb94da7dad9e537e53817f9fa5c0"}, + }) + assert.NoError(t, err) assert.Equal(t, int64(3), commitsCount) } +func TestCommitsCountWithoutBase(t *testing.T) { + bareRepo1Path := filepath.Join(testReposDir, "repo1_bare") + + commitsCount, err := CommitsCount(DefaultContext, + CommitsCountOptions{ + RepoPath: bareRepo1Path, + Not: "master", + Revision: []string{"branch1"}, + }) + + assert.NoError(t, err) + assert.Equal(t, int64(2), commitsCount) +} + func TestGetFullCommitID(t *testing.T) { bareRepo1Path := filepath.Join(testReposDir, "repo1_bare") diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 30a82eb29..6fc306362 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -205,12 +205,24 @@ func (repo *Repository) FileChangedBetweenCommits(filename, id1, id2 string) (bo // FileCommitsCount return the number of files at a revision func (repo *Repository) FileCommitsCount(revision, file string) (int64, error) { - return CommitsCountFiles(repo.Ctx, repo.Path, []string{revision}, []string{file}) + return CommitsCount(repo.Ctx, + CommitsCountOptions{ + RepoPath: repo.Path, + Revision: []string{revision}, + RelPath: []string{file}, + }) +} + +type CommitsByFileAndRangeOptions struct { + Revision string + File string + Not string + Page int } // CommitsByFileAndRange return the commits according revision file and the page -func (repo *Repository) CommitsByFileAndRange(revision, file string, page int) ([]*Commit, error) { - skip := (page - 1) * setting.Git.CommitsRangeSize +func (repo *Repository) CommitsByFileAndRange(opts CommitsByFileAndRangeOptions) ([]*Commit, error) { + skip := (opts.Page - 1) * setting.Git.CommitsRangeSize stdoutReader, stdoutWriter := io.Pipe() defer func() { @@ -220,10 +232,15 @@ func (repo *Repository) CommitsByFileAndRange(revision, file string, page int) ( go func() { stderr := strings.Builder{} gitCmd := NewCommand(repo.Ctx, "rev-list"). - AddOptionFormat("--max-count=%d", setting.Git.CommitsRangeSize*page). + AddOptionFormat("--max-count=%d", setting.Git.CommitsRangeSize*opts.Page). AddOptionFormat("--skip=%d", skip) - gitCmd.AddDynamicArguments(revision) - gitCmd.AddDashesAndList(file) + gitCmd.AddDynamicArguments(opts.Revision) + + if opts.Not != "" { + gitCmd.AddOptionValues("--not", opts.Not) + } + + gitCmd.AddDashesAndList(opts.File) err := gitCmd.Run(&RunOpts{ Dir: repo.Path, Stdout: stdoutWriter, @@ -365,11 +382,18 @@ func (repo *Repository) CommitsBetweenIDs(last, before string) ([]*Commit, error // CommitsCountBetween return numbers of commits between two commits func (repo *Repository) CommitsCountBetween(start, end string) (int64, error) { - count, err := CommitsCountFiles(repo.Ctx, repo.Path, []string{start + ".." + end}, []string{}) + count, err := CommitsCount(repo.Ctx, CommitsCountOptions{ + RepoPath: repo.Path, + Revision: []string{start + ".." + end}, + }) + if err != nil && strings.Contains(err.Error(), "no merge base") { // future versions of git >= 2.28 are likely to return an error if before and last have become unrelated. // previously it would return the results of git rev-list before last so let's try that... - return CommitsCountFiles(repo.Ctx, repo.Path, []string{start, end}, []string{}) + return CommitsCount(repo.Ctx, CommitsCountOptions{ + RepoPath: repo.Path, + Revision: []string{start, end}, + }) } return count, err diff --git a/routers/api/v1/repo/commits.go b/routers/api/v1/repo/commits.go index 401403c83..a5e2e7bae 100644 --- a/routers/api/v1/repo/commits.go +++ b/routers/api/v1/repo/commits.go @@ -146,6 +146,7 @@ func GetAllCommits(ctx *context.APIContext) { sha := ctx.FormString("sha") path := ctx.FormString("path") + not := ctx.FormString("not") var ( commitsCountTotal int64 @@ -178,14 +179,18 @@ func GetAllCommits(ctx *context.APIContext) { } // Total commit count - commitsCountTotal, err = baseCommit.CommitsCount() + commitsCountTotal, err = git.CommitsCount(ctx.Repo.GitRepo.Ctx, git.CommitsCountOptions{ + RepoPath: ctx.Repo.GitRepo.Path, + Not: not, + Revision: []string{baseCommit.ID.String()}, + }) + if err != nil { ctx.Error(http.StatusInternalServerError, "GetCommitsCount", err) return } // Query commits - not := ctx.FormString("not") commits, err = baseCommit.CommitsByRange(listOptions.Page, listOptions.PageSize, not) if err != nil { ctx.Error(http.StatusInternalServerError, "CommitsByRange", err) @@ -196,7 +201,14 @@ func GetAllCommits(ctx *context.APIContext) { sha = ctx.Repo.Repository.DefaultBranch } - commitsCountTotal, err = ctx.Repo.GitRepo.FileCommitsCount(sha, path) + commitsCountTotal, err = git.CommitsCount(ctx, + git.CommitsCountOptions{ + RepoPath: ctx.Repo.GitRepo.Path, + Not: not, + Revision: []string{sha}, + RelPath: []string{path}, + }) + if err != nil { ctx.Error(http.StatusInternalServerError, "FileCommitsCount", err) return @@ -205,7 +217,14 @@ func GetAllCommits(ctx *context.APIContext) { return } - commits, err = ctx.Repo.GitRepo.CommitsByFileAndRange(sha, path, listOptions.Page) + commits, err = ctx.Repo.GitRepo.CommitsByFileAndRange( + git.CommitsByFileAndRangeOptions{ + Revision: sha, + File: path, + Not: not, + Page: listOptions.Page, + }) + if err != nil { ctx.Error(http.StatusInternalServerError, "CommitsByFileAndRange", err) return diff --git a/routers/api/v1/repo/wiki.go b/routers/api/v1/repo/wiki.go index 0b9a36ec4..e33790a37 100644 --- a/routers/api/v1/repo/wiki.go +++ b/routers/api/v1/repo/wiki.go @@ -429,7 +429,12 @@ func ListPageRevisions(ctx *context.APIContext) { } // get Commit Count - commitsHistory, err := wikiRepo.CommitsByFileAndRange("master", pageFilename, page) + commitsHistory, err := wikiRepo.CommitsByFileAndRange( + git.CommitsByFileAndRangeOptions{ + Revision: "master", + File: pageFilename, + Page: page, + }) if err != nil { ctx.Error(http.StatusInternalServerError, "CommitsByFileAndRange", err) return diff --git a/routers/web/feed/file.go b/routers/web/feed/file.go index 6a8d0c454..56a9c54dd 100644 --- a/routers/web/feed/file.go +++ b/routers/web/feed/file.go @@ -10,6 +10,7 @@ import ( "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/context" + "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/util" "github.com/gorilla/feeds" @@ -21,7 +22,12 @@ func ShowFileFeed(ctx *context.Context, repo *repo.Repository, formatType string if len(fileName) == 0 { return } - commits, err := ctx.Repo.GitRepo.CommitsByFileAndRange(ctx.Repo.RefName, fileName, 1) + commits, err := ctx.Repo.GitRepo.CommitsByFileAndRange( + git.CommitsByFileAndRangeOptions{ + Revision: ctx.Repo.RefName, + File: fileName, + Page: 1, + }) if err != nil { ctx.ServerError("ShowBranchFeed", err) return diff --git a/routers/web/repo/commit.go b/routers/web/repo/commit.go index 93294f8dd..e88f1139f 100644 --- a/routers/web/repo/commit.go +++ b/routers/web/repo/commit.go @@ -230,7 +230,12 @@ func FileHistory(ctx *context.Context) { page = 1 } - commits, err := ctx.Repo.GitRepo.CommitsByFileAndRange(ctx.Repo.RefName, fileName, page) + commits, err := ctx.Repo.GitRepo.CommitsByFileAndRange( + git.CommitsByFileAndRangeOptions{ + Revision: ctx.Repo.RefName, + File: fileName, + Page: page, + }) if err != nil { ctx.ServerError("CommitsByFileAndRange", err) return diff --git a/routers/web/repo/wiki.go b/routers/web/repo/wiki.go index 374d1bf2e..a335c114b 100644 --- a/routers/web/repo/wiki.go +++ b/routers/web/repo/wiki.go @@ -376,7 +376,12 @@ func renderRevisionPage(ctx *context.Context) (*git.Repository, *git.TreeEntry) } // get Commit Count - commitsHistory, err := wikiRepo.CommitsByFileAndRange(wiki_service.DefaultBranch, pageFilename, page) + commitsHistory, err := wikiRepo.CommitsByFileAndRange( + git.CommitsByFileAndRangeOptions{ + Revision: wiki_service.DefaultBranch, + File: pageFilename, + Page: page, + }) if err != nil { if wikiRepo != nil { wikiRepo.Close() diff --git a/tests/integration/api_repo_git_commits_test.go b/tests/integration/api_repo_git_commits_test.go index 17750a279..d9c3d7b95 100644 --- a/tests/integration/api_repo_git_commits_test.go +++ b/tests/integration/api_repo_git_commits_test.go @@ -58,6 +58,29 @@ func TestAPIReposGitCommitList(t *testing.T) { session := loginUser(t, user.Name) token := getTokenForLoggedInUser(t, session) + // Test getting commits (Page 1) + req := NewRequestf(t, "GET", "/api/v1/repos/%s/repo20/commits?token="+token+"¬=master&sha=remove-files-a", user.Name) + resp := MakeRequest(t, req, http.StatusOK) + + var apiData []api.Commit + DecodeJSON(t, resp, &apiData) + + assert.Len(t, apiData, 2) + assert.EqualValues(t, "cfe3b3c1fd36fba04f9183287b106497e1afe986", apiData[0].CommitMeta.SHA) + compareCommitFiles(t, []string{"link_hi", "test.csv"}, apiData[0].Files) + assert.EqualValues(t, "c8e31bc7688741a5287fcde4fbb8fc129ca07027", apiData[1].CommitMeta.SHA) + compareCommitFiles(t, []string{"test.csv"}, apiData[1].Files) + + assert.EqualValues(t, resp.Header().Get("X-Total"), "2") +} + +func TestAPIReposGitCommitListNotMaster(t *testing.T) { + defer tests.PrepareTestEnv(t)() + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + // Login as User2. + session := loginUser(t, user.Name) + token := getTokenForLoggedInUser(t, session) + // Test getting commits (Page 1) req := NewRequestf(t, "GET", "/api/v1/repos/%s/repo16/commits?token="+token, user.Name) resp := MakeRequest(t, req, http.StatusOK) @@ -72,6 +95,8 @@ func TestAPIReposGitCommitList(t *testing.T) { compareCommitFiles(t, []string{"readme.md"}, apiData[1].Files) assert.EqualValues(t, "5099b81332712fe655e34e8dd63574f503f61811", apiData[2].CommitMeta.SHA) compareCommitFiles(t, []string{"readme.md"}, apiData[2].Files) + + assert.EqualValues(t, resp.Header().Get("X-Total"), "3") } func TestAPIReposGitCommitListPage2Empty(t *testing.T) { @@ -148,4 +173,26 @@ func TestGetFileHistory(t *testing.T) { assert.Len(t, apiData, 1) assert.Equal(t, "f27c2b2b03dcab38beaf89b0ab4ff61f6de63441", apiData[0].CommitMeta.SHA) compareCommitFiles(t, []string{"readme.md"}, apiData[0].Files) + + assert.EqualValues(t, resp.Header().Get("X-Total"), "1") +} + +func TestGetFileHistoryNotOnMaster(t *testing.T) { + defer tests.PrepareTestEnv(t)() + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + // Login as User2. + session := loginUser(t, user.Name) + token := getTokenForLoggedInUser(t, session) + + req := NewRequestf(t, "GET", "/api/v1/repos/%s/repo20/commits?path=test.csv&token="+token+"&sha=add-csv¬=master", user.Name) + resp := MakeRequest(t, req, http.StatusOK) + + var apiData []api.Commit + DecodeJSON(t, resp, &apiData) + + assert.Len(t, apiData, 1) + assert.Equal(t, "c8e31bc7688741a5287fcde4fbb8fc129ca07027", apiData[0].CommitMeta.SHA) + compareCommitFiles(t, []string{"test.csv"}, apiData[0].Files) + + assert.EqualValues(t, resp.Header().Get("X-Total"), "1") }