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.

Sign up for GitHub

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

Jump to bottom

Use commit graph files for listing pages #7314

Merged
merged 10 commits into from
Jul 2, 2019
Merged

Use commit graph files for listing pages #7314

merged 10 commits into from
Jul 2, 2019

Conversation

filipnavara
Copy link
Contributor

Complements #7313, supercedes #6701.

/cc @lunny

@lunny lunny added this to the 1.10.0 milestone Jun 27, 2019
@lunny lunny added the type/enhancement An improvement of existing functionality label Jun 27, 2019
@codecov-io
Copy link

codecov-io commented Jun 27, 2019 •
edited
Loading

Codecov Report

Merging #7314 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7314      +/-   ##
==========================================
+ Coverage   41.26%   41.26%   +<.01%     
==========================================
  Files         466      467       +1     
  Lines       63258    63291      +33     
==========================================
+ Hits        26101    26116      +15     
- Misses      33747    33760      +13     
- Partials     3410     3415       +5
Impacted Files Coverage Δ
modules/git/notes.go 40% <33.33%> (-3.75%) ⬇️
modules/git/repo_commitgraph.go 35.71% <35.71%> (ø)
modules/git/commit_info.go 68.23% <66.66%> (-1.58%) ⬇️
modules/log/event.go 64.61% <0%> (-1.03%) ⬇️
models/repo_list.go 73.09% <0%> (+1.01%) ⬆️
modules/avatar/avatar.go 54% <0%> (+6%) ⬆️

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 e728b55...0ca28c6. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 27, 2019
@zeripath
Copy link
Contributor

So we don't actually get git to make and store commit graphs at present - Do you think we should be setting core.commitGraph ? Is there anyway we can create the commit graph ourselves, say on push?

@filipnavara
Copy link
Contributor Author

@lunny's PR linked in the description actually sets the git settings to create the commit graphs. The intermediate commits in this PR contain code that can build it too but I didn't clean it up. The actual code that landed in go-git makes it much easier to produce the commit-graph files because it no longer needs one to build it in hierarchical order.

@zeripath
Copy link
Contributor

So can we build a commitgraph without #7313? We have a lot of users who are on old gits and it would be good if they could get some improvement from this.

@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 Jun 27, 2019
@filipnavara
Copy link
Contributor Author

Yes, absolutely, if we decide it's worth the effort. I will be happy to provide the code for doing so.

@lunny
Copy link
Member

lunny commented Jun 27, 2019

Maybe we should have a setting.git.EnableCommitGraph to enable or disable commitgraph feature.

@lunny
Copy link
Member

lunny commented Jun 27, 2019

@filipnavara #7313 (comment)

@zeripath
Copy link
Contributor

I think this should be 1.9

@filipnavara
Copy link
Contributor Author

It should be fairly safe to land in 1.9 but ultimately it's up to your decision. We were running various iterations of this code in production since last December. This last one has been deployed to our servers for close to a month with no reported failure.

@zeripath
Copy link
Contributor

I'm gonna change it to 1.9, if anyone disagrees please change back.

@zeripath zeripath modified the milestones: 1.10.0, 1.9.0 Jun 29, 2019
lunny
lunny requested changes Jun 29, 2019
View reviewed changes
modules/git/repo_commitgraph.go Show resolved Hide resolved
modules/git/repo_commitgraph.go
return cgobject.NewGraphCommitNodeIndex(index, r.gogitRepo.Storer), file
}
}

Copy link
Member

Choose a reason for hiding this comment

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

If err is not file not exist error, we should return it but not hide it.

Copy link
Contributor Author

@filipnavara filipnavara Jun 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should return it. This is quite graceful error handling with fallback to the regular git object storage. Worst case is a performance penalty but it will still yield the same results.

Copy link
Member

@lunny lunny Jun 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

OK. But at least, we should know commit graph is not using since something wrong. We need an error log or warning log if commitgraph file exist but load failed. That will be helpful when we investigate errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lunny is my change to lines 30-33 acceptable?

@GiteaBot GiteaBot removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 2, 2019
@GiteaBot GiteaBot added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Jul 2, 2019
@lunny
Copy link
Member

lunny commented Jul 2, 2019

@filipnavara @zeripath Thanks!

@lunny lunny merged commit 6e2a59e into go-gitea:master Jul 2, 2019
jeffliu27 pushed a commit to jeffliu27/gitea that referenced this pull request Jul 18, 2019
@filipnavara @jeffliu27
Use commit graph files for listing pages (go-gitea#7314)
42dc6fe 
* Experimental support for git commit graph files and bloom filter index

Signed-off-by: Filip Navara <filip.navara@gmail.com>

* Force vendor of commitgraph

Signed-off-by: Filip Navara <filip.navara@gmail.com>

* Remove bloom filter experiment and debug prints

* Remove old code for building commit graphs

* Remove unused function

* Remove mmap usage

* gofmt

* sort vendor/modules.txt

* Add copyright header and log commit-graph error
@davidsvantesson davidsvantesson mentioned this pull request Sep 13, 2019
Closed
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zeripath zeripath zeripath approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.9.0
Development

Successfully merging this pull request may close these issues.
6 participants
@filipnavara @codecov-io @zeripath @lunny @GiteaBot @sapk