-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Changes from 1 commit
a699af2
748e95d
1d75b8e
3edc644
be8acf6
29c5745
9ab70a4
95a5b93
f2acab1
f866033
f42506d
6c7bcea
706f805
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
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) | ||
} | ||
|
||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also should be paged There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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{} | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. may benefit from something that will load all the commits in one (That is replace the getCommit calls) |
||
if branch == nil { | ||
return nil, 0 | ||
} | ||
|
||
if branch.Name == ctx.Repo.Repository.DefaultBranch { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. then
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's why I added 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can add some tests on There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK I reverted the "Mock data" PR. The problem with
We need 'master' along and not included in the branches below, then we have this:
This is the current solution. @lunny I'm using
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as above |
||
|
||
if ctx.Repo.CanWrite(models.UnitTypeCode) { | ||
deletedBranches, err := getDeletedBranches(ctx) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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