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

Add pager to the branches page #14202

Merged
merged 13 commits into from
Jan 19, 2021
3 changes: 3 additions & 0 deletions modules/git/repo_commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ func (repo *Repository) GetCommitByPath(relpath string) (*Commit, error) {
// CommitsRangeSize the default commits range size
var CommitsRangeSize = 50

// BranchesRangeSize the default branches range size
var BranchesRangeSize = 20

func (repo *Repository) commitsByRange(id SHA1, page, pageSize int) (*list.List, error) {
stdout, err := NewCommand("log", id.String(), "--skip="+strconv.Itoa((page-1)*pageSize),
"--max-count="+strconv.Itoa(pageSize), prettyLogFormat).RunInDirBytes(repo.Path)
Expand Down
205 changes: 125 additions & 80 deletions routers/repo/branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,22 @@ func Branches(ctx *context.Context) {
ctx.Data["PageIsViewCode"] = true
ctx.Data["PageIsBranches"] = true

ctx.Data["Branches"] = loadBranches(ctx)
page := ctx.QueryInt("page")
if page <= 1 {
page = 1
}

pageSize := ctx.QueryInt("limit")
if pageSize <= 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need a max here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not allow this to be > our max

6543 marked this conversation as resolved.
Show resolved Hide resolved
pageSize = git.BranchesRangeSize
zeripath marked this conversation as resolved.
Show resolved Hide resolved
}

branches, branchesCount := loadBranches(ctx, page, pageSize)
ctx.Data["Branches"] = branches
skyline75489 marked this conversation as resolved.
Show resolved Hide resolved
pager := context.NewPagination(int(branchesCount), git.BranchesRangeSize, page, 5)
pager.SetDefaultParams(ctx)
ctx.Data["Page"] = pager

ctx.HTML(200, tplBranch)
}

Expand Down Expand Up @@ -176,17 +191,18 @@ func deleteBranch(ctx *context.Context, branchName string) error {
return nil
}

func loadBranches(ctx *context.Context) []*Branch {
func loadBranches(ctx *context.Context, page, pageSize int) ([]*Branch, int) {
zeripath marked this conversation as resolved.
Show resolved Hide resolved
defaultBranch, err := repo_module.GetBranch(ctx.Repo.Repository, ctx.Repo.Repository.DefaultBranch)
rawBranches, err := repo_module.GetBranches(ctx.Repo.Repository)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also should be paged

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have a think about how the git command could be paged if it is not obvious

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I failed to find a way to make the git command pagable, be it 'show-ref' or 'branch/tag'. So yeah this is not good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok it looks like we should page by ignoring lines etc. from the output of the git process.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like enforcing this could be a little involved. Think you skip this if don't fancy it.

if err != nil {
ctx.ServerError("GetBranches", err)
return nil
return nil, 0
}

protectedBranches, err := ctx.Repo.Repository.GetProtectedBranches()
if err != nil {
ctx.ServerError("GetProtectedBranches", err)
return nil
return nil, 0
}

repoIDToRepo := map[int64]*models.Repository{}
Expand All @@ -195,100 +211,129 @@ func loadBranches(ctx *context.Context) []*Branch {
repoIDToGitRepo := map[int64]*git.Repository{}
repoIDToGitRepo[ctx.Repo.Repository.ID] = ctx.Repo.GitRepo

branches := make([]*Branch, len(rawBranches))
for i := range rawBranches {
commit, err := rawBranches[i].GetCommit()
var totalNumOfBranches = len(rawBranches)
var startIndex = (page - 1) * pageSize
if startIndex > totalNumOfBranches {
startIndex = totalNumOfBranches - 1
}
var endIndex = startIndex + pageSize
if endIndex > totalNumOfBranches {
endIndex = totalNumOfBranches - 1
}

var branches []*Branch
for i := startIndex; i < endIndex; i++ {
var branch = loadOneBranch(ctx, rawBranches[i], protectedBranches, repoIDToRepo, repoIDToGitRepo)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may benefit from something that will load all the commits in one git cat-file --batch

(That is replace the getCommit calls)

if branch == nil {
return nil, 0
}

if branch.Name == ctx.Repo.Repository.DefaultBranch {
Copy link
Member

@lunny lunny Jan 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then

if endIndex < totalNumOfBranches {
  endIndex++
}

Otherwise, we will get 19 branches every page except default branch.

I think it's better to get 20 branches every page except default branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s actually my original logic. But I found that if the current page is the last page, and ‘master’ is also in this page, adding endIndex++ will make the index out of bound.

In fact, the entire pager mechanism is still based on the assumption that ‘master’ in the list. If we need to remove master from the list, the startIndex calculation also needs to be modified. I did try to change this, but it turns out to be too much work. I decided to left that for future work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I added if endIndex < totalNumOfBranches {.

The default branch will always be kept on the top of the branches list table. That make the user feel, the paging is only for the list table but not all the branches on the page. So that, I think keep 20 branches on the table but not 19 is less confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me add some tests to clarify this. Could you please point me where i should be adding test? Does this affect the public documentation? I think there is still a lot work that needs to be done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add some tests on integrations/branches_test.go

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be easier to just always have the default branch on the page in a separate block.

that way you could simplify the logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to add some mock repos. It makes the PR really big, though. And I still get 404 instead of 200. Anything I'm missing? @lunny @zeripath .

Copy link
Contributor Author

@skyline75489 skyline75489 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I reverted the "Mock data" PR. The problem with endIndex++ is that I can not just skip 'master' without breaking the content on the next page. Say we have 60 branches (including 'master'). If we do not need 'master' along, we have this:

Page0:  Branch0  ...  Branch19
Page1:  Branch20 ... master ... Branch39
Page2:  Branch40 ...  Branch59

We need 'master' along and not included in the branches below, then we have this:

Page0:  Branch0  ...  Branch19
Page1:  Branch20 ... Branch39 (There is only 19 branches)
Page2:  Branch40 ...  Branch59

This is the current solution. @lunny I'm using for i := startIndex; i < endIndex; i++ so there is indeed 20 branches instead of 19 branches. Ideally we should be doing this (and I think this is also what @lunny want me to do with skipping):

Page0:  Branch0  ...  Branch19
Page1:  Branch20 ... Branch40
Page2:  Branch41 ...  Branch59 (How do I know this should start at 41?)

But I cannot know that the third page should start at Branch41 unless I loop through all branches and find out where 'master' is, which seems inefficient.

Copy link
Member

@lunny lunny Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we can still display default branch in the list but give it a mark that means it's default. And the buttons on this line is different from others but the same as the top default branch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we don't ignore the default branch but mark it?

// Skip defult branch
zeripath marked this conversation as resolved.
Show resolved Hide resolved
continue
}

branches = append(branches, branch)
}

// Always add the default branch
branches = append(branches, loadOneBranch(ctx, defaultBranch, protectedBranches, repoIDToRepo, repoIDToGitRepo))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above


if ctx.Repo.CanWrite(models.UnitTypeCode) {
deletedBranches, err := getDeletedBranches(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need some paging too?

if err != nil {
ctx.ServerError("GetCommit", err)
return nil
ctx.ServerError("getDeletedBranches", err)
return nil, 0
}
branches = append(branches, deletedBranches...)
}

var isProtected bool
branchName := rawBranches[i].Name
for _, b := range protectedBranches {
if b.BranchName == branchName {
isProtected = true
break
}
return branches, len(rawBranches) - 1
}

func loadOneBranch(ctx *context.Context, rawBranch *git.Branch, protectedBranches []*models.ProtectedBranch,
repoIDToRepo map[int64]*models.Repository,
repoIDToGitRepo map[int64]*git.Repository) *Branch {

commit, err := rawBranch.GetCommit()
if err != nil {
ctx.ServerError("GetCommit", err)
return nil
}

branchName := rawBranch.Name
var isProtected bool
for _, b := range protectedBranches {
if b.BranchName == branchName {
isProtected = true
break
}
}

divergence, divergenceError := repofiles.CountDivergingCommits(ctx.Repo.Repository, git.BranchPrefix+branchName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be the most expensive part of this call. Did you do any pprof analysis to check where most time was spent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No actually. You see before the PR I cannot open the branches page at all. It takes forever to load. Adding this PR changes makes loading the branches page possible. And i'm currently OK with it. So i didn't dig further.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pprof data confirms this the slowest part

if divergenceError != nil {
ctx.ServerError("CountDivergingCommits", divergenceError)
return nil
}

divergence, divergenceError := repofiles.CountDivergingCommits(ctx.Repo.Repository, git.BranchPrefix+branchName)
if divergenceError != nil {
ctx.ServerError("CountDivergingCommits", divergenceError)
pr, err := models.GetLatestPullRequestByHeadInfo(ctx.Repo.Repository.ID, branchName)
if err != nil {
ctx.ServerError("GetLatestPullRequestByHeadInfo", err)
return nil
}
headCommit := commit.ID.String()

mergeMovedOn := false
if pr != nil {
pr.HeadRepo = ctx.Repo.Repository
if err := pr.LoadIssue(); err != nil {
ctx.ServerError("pr.LoadIssue", err)
return nil
}

pr, err := models.GetLatestPullRequestByHeadInfo(ctx.Repo.Repository.ID, branchName)
if err != nil {
ctx.ServerError("GetLatestPullRequestByHeadInfo", err)
if repo, ok := repoIDToRepo[pr.BaseRepoID]; ok {
pr.BaseRepo = repo
} else if err := pr.LoadBaseRepo(); err != nil {
ctx.ServerError("pr.LoadBaseRepo", err)
return nil
} else {
repoIDToRepo[pr.BaseRepoID] = pr.BaseRepo
}
headCommit := commit.ID.String()
pr.Issue.Repo = pr.BaseRepo

mergeMovedOn := false
if pr != nil {
pr.HeadRepo = ctx.Repo.Repository
if err := pr.LoadIssue(); err != nil {
ctx.ServerError("pr.LoadIssue", err)
return nil
if pr.HasMerged {
baseGitRepo, ok := repoIDToGitRepo[pr.BaseRepoID]
if !ok {
baseGitRepo, err = git.OpenRepository(pr.BaseRepo.RepoPath())
if err != nil {
ctx.ServerError("OpenRepository", err)
return nil
}
defer baseGitRepo.Close()
repoIDToGitRepo[pr.BaseRepoID] = baseGitRepo
}
if repo, ok := repoIDToRepo[pr.BaseRepoID]; ok {
pr.BaseRepo = repo
} else if err := pr.LoadBaseRepo(); err != nil {
ctx.ServerError("pr.LoadBaseRepo", err)
pullCommit, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName())
if err != nil && !git.IsErrNotExist(err) {
ctx.ServerError("GetBranchCommitID", err)
return nil
} else {
repoIDToRepo[pr.BaseRepoID] = pr.BaseRepo
}
pr.Issue.Repo = pr.BaseRepo

if pr.HasMerged {
baseGitRepo, ok := repoIDToGitRepo[pr.BaseRepoID]
if !ok {
baseGitRepo, err = git.OpenRepository(pr.BaseRepo.RepoPath())
if err != nil {
ctx.ServerError("OpenRepository", err)
return nil
}
defer baseGitRepo.Close()
repoIDToGitRepo[pr.BaseRepoID] = baseGitRepo
}
pullCommit, err := baseGitRepo.GetRefCommitID(pr.GetGitRefName())
if err != nil && !git.IsErrNotExist(err) {
ctx.ServerError("GetBranchCommitID", err)
return nil
}
if err == nil && headCommit != pullCommit {
// the head has moved on from the merge - we shouldn't delete
mergeMovedOn = true
}
if err == nil && headCommit != pullCommit {
// the head has moved on from the merge - we shouldn't delete
mergeMovedOn = true
}
}

isIncluded := divergence.Ahead == 0 && ctx.Repo.Repository.DefaultBranch != branchName

branches[i] = &Branch{
Name: branchName,
Commit: commit,
IsProtected: isProtected,
IsIncluded: isIncluded,
CommitsAhead: divergence.Ahead,
CommitsBehind: divergence.Behind,
LatestPullRequest: pr,
MergeMovedOn: mergeMovedOn,
}
}

if ctx.Repo.CanWrite(models.UnitTypeCode) {
deletedBranches, err := getDeletedBranches(ctx)
if err != nil {
ctx.ServerError("getDeletedBranches", err)
return nil
}
branches = append(branches, deletedBranches...)
isIncluded := divergence.Ahead == 0 && ctx.Repo.Repository.DefaultBranch != branchName
return &Branch{
Name: branchName,
Commit: commit,
IsProtected: isProtected,
IsIncluded: isIncluded,
CommitsAhead: divergence.Ahead,
CommitsBehind: divergence.Behind,
LatestPullRequest: pr,
MergeMovedOn: mergeMovedOn,
}

return branches
}

func getDeletedBranches(ctx *context.Context) ([]*Branch, error) {
Expand Down
3 changes: 3 additions & 0 deletions templates/repo/branch/list.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@
</tbody>
</table>
</div>

{{template "base/paginate" .}}

6543 marked this conversation as resolved.
Show resolved Hide resolved
{{end}}
</div>
</div>
Expand Down