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
Merged

Add pager to the branches page #14202

merged 13 commits into from
Jan 19, 2021

Conversation

skyline75489
Copy link
Contributor

Supports #14180

This adds a pager control in the 'branches' page, which will speed up the page loading time significantly. For projects like PyTorch which has 4000+ branches, the current branches page in gitea will take forever to load.

I'm new to the project. Also new to golang programming. I think I should be adding tests but I don't really know how.

Demo screenshot (with DarkReader enabled so the CSS is darker):

图片

@a1012112796
Copy link
Member

@skyline75489 Please fix lint.

routers/repo/branch.go:195:2: SA4006: this value of `err` is never used (staticcheck)
--
7 | defaultBranch, err := repo_module.GetBranch(ctx.Repo.Repository, ctx.Repo.Repository.DefaultBranch)
8 | ^

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 31, 2020
@a1012112796 a1012112796 added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Dec 31, 2020
@codecov-io
Copy link

codecov-io commented Jan 1, 2021

Codecov Report

Merging #14202 (f42506d) into master (74a0481) will decrease coverage by 0.00%.
The diff coverage is 55.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14202      +/-   ##
==========================================
- Coverage   41.79%   41.79%   -0.01%     
==========================================
  Files         744      744              
  Lines       79538    79571      +33     
==========================================
+ Hits        33246    33259      +13     
- Misses      40818    40830      +12     
- Partials     5474     5482       +8     
Impacted Files Coverage Δ
modules/git/repo_commit.go 45.66% <ø> (ø)
routers/repo/branch.go 58.05% <54.08%> (+1.69%) ⬆️
modules/setting/git.go 71.42% <100.00%> (+3.00%) ⬆️
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
modules/indexer/stats/db.go 48.00% <0.00%> (-12.00%) ⬇️
modules/git/repo_commit_nogogit.go 63.33% <0.00%> (-1.67%) ⬇️
modules/log/file.go 73.60% <0.00%> (-1.61%) ⬇️
services/pull/pull.go 42.57% <0.00%> (-0.50%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9659808...f42506d. Read the comment docs.

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?

}

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

routers/repo/branch.go Show resolved Hide resolved
@@ -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) {
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.

routers/repo/branch.go Outdated Show resolved Hide resolved

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)

}

// 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

branches = append(branches, loadOneBranch(ctx, defaultBranch, protectedBranches, repoIDToRepo, repoIDToGitRepo))

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?

}
}

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

@techknowlogick techknowlogick added this to the 1.14.0 milestone Jan 1, 2021
@zeripath
Copy link
Contributor

zeripath commented Jan 2, 2021

#14180 (comment)

Seems to confirm part of the problem is the number of branches. Therefore paging would be helpful.

routers/repo/branch.go Outdated Show resolved Hide resolved
6543 and others added 3 commits January 3, 2021 16:34
Co-authored-by: silverwind <me@silverwind.io>
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor

zeripath commented Jan 4, 2021

Tests now fail.

I personally don't find these kind of tests that helpful - I'd much prefer creating enough branches programmatically either through pushing or API

This reverts commit 95a5b93.
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

I'm gonna approve as I think this is still a decent improvement - there is more that can be done here - and elsewhere with branches to make things more efficient but this is certainly better.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 10, 2021
Co-authored-by: 6543 <6543@obermui.de>
@lunny
Copy link
Member

lunny commented Jan 14, 2021

I think https://github.com/go-gitea/gitea/pull/14202/files#r555065843 should be a better UI.

@6543
Copy link
Member

6543 commented Jan 15, 2021

I would only show the Default branch on the first page ...

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

I think there is more in this area who could be improved but this is a big one already ...

so lets get it in and move forward

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 19, 2021
@6543 6543 merged commit 0c0445c into go-gitea:master Jan 19, 2021
a1012112796 added a commit to a1012112796/gitea that referenced this pull request Jan 19, 2021
* master:
  Add pager to the branches page (go-gitea#14202)
  Removed invalid form tag (go-gitea#14391)
  Update back-up restore example for 1.13 changes (go-gitea#14374)
  It seems vet on windows is unnecessary (go-gitea#14302)
@go-gitea go-gitea locked and limited conversation to collaborators Mar 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants