Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add pager to the branches page #14202
Changes from all commits
a699af2
748e95d
1d75b8e
3edc644
be8acf6
29c5745
9ab70a4
95a5b93
f2acab1
f866033
f42506d
6c7bcea
706f805
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This also should be paged
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.
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 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.
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.
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 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.
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.
may benefit from something that will load all the commits in one
git cat-file --batch
(That is replace the getCommit calls)
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.
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 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.
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.
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.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
You can add some tests on
integrations/branches_test.go
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.
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 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 .
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.
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:We need 'master' along and not included in the branches below, then we have this:
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):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 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.
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.
Could we don't ignore the default branch but mark it?
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.
as above
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.
Does this need some paging too?
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.
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 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.
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.
pprof data confirms this the slowest part