Add pager to the branches page #14202

skyline75489 · 2020-12-31T10:19:26Z

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 · 2020-12-31T12:10:37Z

@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 | ^

codecov-io · 2021-01-01T01:03:59Z

Codecov Report

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

@@            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.

lunny · 2021-01-01T02:29:08Z

routers/repo/branch.go

+			return nil, 0
+		}
+
+		if branch.Name == ctx.Repo.Repository.DefaultBranch {


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.

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.

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.

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.

You can add some tests on integrations/branches_test.go

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.

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 .

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.

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.

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

zeripath · 2020-12-31T14:17:08Z

routers/repo/branch.go

+	}
+
+	pageSize := ctx.QueryInt("limit")
+	if pageSize <= 0 {


do we need a max here?

Do not allow this to be > our max

routers/repo/branch.go

zeripath · 2020-12-31T14:24:30Z

routers/repo/branch.go

@@ -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)


This also should be paged

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

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.

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

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

routers/repo/branch.go

zeripath · 2020-12-31T15:26:29Z

routers/repo/branch.go

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


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

(That is replace the getCommit calls)

zeripath · 2020-12-31T15:26:59Z

routers/repo/branch.go

+	}
+
+	// Always add the default branch
+	branches = append(branches, loadOneBranch(ctx, defaultBranch, protectedBranches, repoIDToRepo, repoIDToGitRepo))


zeripath · 2020-12-31T15:27:57Z

routers/repo/branch.go

+	branches = append(branches, loadOneBranch(ctx, defaultBranch, protectedBranches, repoIDToRepo, repoIDToGitRepo))
+
+	if ctx.Repo.CanWrite(models.UnitTypeCode) {
+		deletedBranches, err := getDeletedBranches(ctx)


Does this need some paging too?

zeripath · 2020-12-31T15:36:10Z

routers/repo/branch.go

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


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

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.

pprof data confirms this the slowest part

zeripath · 2021-01-02T13:59:30Z

#14180 (comment)

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

routers/repo/branch.go

templates/repo/branch/list.tmpl

Co-authored-by: silverwind <me@silverwind.io>

Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath · 2021-01-04T16:58:11Z

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.

zeripath

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.

routers/repo/branch.go

Co-authored-by: 6543 <6543@obermui.de>

lunny · 2021-01-14T02:24:24Z

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

6543 · 2021-01-15T00:50:24Z

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

6543

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

* 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)

Add pager to the branches page

a699af2

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 31, 2020

a1012112796 added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Dec 31, 2020

Lint

748e95d

lunny reviewed Jan 1, 2021

View reviewed changes

zeripath reviewed Jan 1, 2021

View reviewed changes

techknowlogick added this to the 1.14.0 milestone Jan 1, 2021

zeripath reviewed Jan 2, 2021

View reviewed changes

routers/repo/branch.go Show resolved Hide resolved

zeripath added 2 commits January 2, 2021 17:15

Update routers/repo/branch.go

1d75b8e

Update routers/repo/branch.go

3edc644

6543 reviewed Jan 2, 2021

View reviewed changes

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

override pageSize if bigger than max

be8acf6

silverwind reviewed Jan 3, 2021

View reviewed changes

templates/repo/branch/list.tmpl Outdated Show resolved Hide resolved

6543 and others added 3 commits January 3, 2021 16:34

Update templates/repo/branch/list.tmpl

29c5745

Co-authored-by: silverwind <me@silverwind.io>

Make branches commit range configurable

9ab70a4

Signed-off-by: Andrew Thornton <art27@cantab.net>

Mock data

95a5b93

Revert "Mock data"

f2acab1

This reverts commit 95a5b93.

zeripath approved these changes Jan 10, 2021

View reviewed changes

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

Merge branch 'master' into chesterliu/dev/branch-pages

f866033

6543 reviewed Jan 11, 2021

View reviewed changes

routers/repo/branch.go Show resolved Hide resolved

Update routers/repo/branch.go

f42506d

Co-authored-by: 6543 <6543@obermui.de>

Merge branch 'master' into chesterliu/dev/branch-pages

6c7bcea

Merge branch 'master' into chesterliu/dev/branch-pages

706f805

6543 approved these changes Jan 19, 2021

View reviewed changes

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 merged commit 0c0445c into go-gitea:master Jan 19, 2021

6543 mentioned this pull request Jan 30, 2021

[API] Add pagination to ListBranches #14524

Merged

go-gitea locked and limited conversation to collaborators Mar 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pager to the branches page #14202

Add pager to the branches page #14202

skyline75489 commented Dec 31, 2020

a1012112796 commented Dec 31, 2020

codecov-io commented Jan 1, 2021 •

edited

lunny Jan 1, 2021 •

edited

skyline75489 Jan 1, 2021

lunny Jan 1, 2021

skyline75489 Jan 1, 2021

lunny Jan 1, 2021

zeripath Jan 1, 2021

skyline75489 Jan 4, 2021

skyline75489 Jan 5, 2021 •

edited

lunny Jan 6, 2021 •

edited

lunny Jan 11, 2021

zeripath Dec 31, 2020

zeripath Jan 2, 2021

zeripath Dec 31, 2020

zeripath Dec 31, 2020

skyline75489 Jan 1, 2021

zeripath Jan 1, 2021

zeripath Jan 2, 2021

zeripath Dec 31, 2020

zeripath Dec 31, 2020

zeripath Dec 31, 2020

zeripath Dec 31, 2020

skyline75489 Jan 1, 2021

zeripath Jan 2, 2021

zeripath commented Jan 2, 2021

zeripath commented Jan 4, 2021

zeripath left a comment

lunny commented Jan 14, 2021

6543 commented Jan 15, 2021

6543 left a comment

Add pager to the branches page #14202

Add pager to the branches page #14202

Conversation

skyline75489 commented Dec 31, 2020

a1012112796 commented Dec 31, 2020

codecov-io commented Jan 1, 2021 • edited

Codecov Report

lunny Jan 1, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skyline75489 Jan 5, 2021 • edited

Choose a reason for hiding this comment

lunny Jan 6, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeripath commented Jan 2, 2021

zeripath commented Jan 4, 2021

zeripath left a comment

Choose a reason for hiding this comment

lunny commented Jan 14, 2021

6543 commented Jan 15, 2021

6543 left a comment

Choose a reason for hiding this comment

codecov-io commented Jan 1, 2021 •

edited

lunny Jan 1, 2021 •

edited

skyline75489 Jan 5, 2021 •

edited

lunny Jan 6, 2021 •

edited