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

Update go-git dependency to get performance fixes #7248

Closed
filipnavara opened this issue Jun 18, 2019 · 14 comments · Fixed by #7249
Closed

Update go-git dependency to get performance fixes #7248

filipnavara opened this issue Jun 18, 2019 · 14 comments · Fixed by #7249
Labels
type/enhancement An improvement of existing functionality
Milestone

Comments

@filipnavara
Copy link
Contributor

Go-git 4.12 (https://github.com/src-d/go-git/releases/tag/v4.12.0) was released now so Gitea should pick it up for the performance improvements.

@lunny lunny added the type/enhancement An improvement of existing functionality label Jun 19, 2019
@lunny lunny added this to the 1.9.0 milestone Jun 19, 2019
@filipnavara
Copy link
Contributor Author

Unfortunately the new commitgraph packages are still missing after the update (the plumbing/format/commitgraph and plumbing/object/commitgraph directories).

@zeripath
Copy link
Contributor

Oh that's odd they should be there - try make vendor and see if they appear and were missed in the commit.

@filipnavara
Copy link
Contributor Author

I cannot see them on GitHub web (for Gitea repo) so they must have been missed somewhere along the way. make vendor makes a mess on my Windows machine which is why I always ask others to do it for me.

@zeripath
Copy link
Contributor

Ok they definitely should be there - but they're missing because git doesn't complain about new untracked directories. So our make vendor test will miss that they weren't added.

Could you open an issue and either myself or someone else will be able to fix the missing vendor files.

(What happens when you run make vendor?)

@lunny
Copy link
Member

lunny commented Jun 21, 2019

So is this a bug of go?

@lunny
Copy link
Member

lunny commented Jun 21, 2019

It seems plumbing/format/commitgraph and plumbing/object/commitgraph have never been referenced by other packages on go-git. Is that the reason the directories missing when vendoring?

@zeripath
Copy link
Contributor

Nope I suspect the files get added when we do make vendor, but that we missed them when creating the pr and our make test-vendor case misses the fact that they're not added.

I suspect these commit graph functions are not used anywhere in go git at present so it doesn't cause compilation issues that they're not present.

@lunny
Copy link
Member

lunny commented Jun 21, 2019

@zeripath I have executed make vendor locally again. But it also didn't copy that two directories.

@zeripath
Copy link
Contributor

So just checking quickly it doesn't appear to be fixed using make vendor. Damn.

@sapk
Copy link
Member

sapk commented Jun 21, 2019

@filipnavara I forced the vendoring of package by using _ import of commitgraph packages. You can cherry-pick this commit ac4ebac from the PR #7276 to have the needed deps vendored. Let me know if you need more deps.

@filipnavara
Copy link
Contributor Author

@sapk Thanks! That's exactly what I needed.

@lunny
Copy link
Member

lunny commented Jun 21, 2019

@filipnavara @sapk So I think that #7276 is unnecessary.
@filipnavara You can just send a PR which reference the two directories and if you cannot run make vendor correctly, maintainers could help you with make vendor on that PR.

@sapk
Copy link
Member

sapk commented Jun 21, 2019

Yes @lunny it doesn't need to be merged (and it shouldn't). I just did it to verify that it only vendor referenced folder and it could ease the process of vendor for @filipnavara by just cherry-picking the commit (and removing _ import).

@zeripath
Copy link
Contributor

This is an interesting finding for me - it basically means that's whilst developing there may be more useful functionality in our already vendored packages that we could use but don't know that is there - I did wonder why go-git appeared so sparse when I was looking for functionality. I'll have to have another look at it.

jeffliu27 pushed a commit to jeffliu27/gitea that referenced this issue Jul 18, 2019
@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.
Labels
type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants