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

Remove seen map from getLastCommitForPaths #6807

Merged
merged 1 commit into from
Apr 30, 2019

Conversation

filipnavara
Copy link
Contributor

@filipnavara filipnavara commented Apr 30, 2019

Ensures correctly traversing the commit graph for all path and avoids
erroneously skipping some. Also preallocate some arrays to correct size
to prevent unnecessary reallocations.

Fixes #6708.

Ensures correctly traversing the commit graph for all path and avoids
erroneously skipping some. Also preallocate some arrays to correct size
to prevent unnecessary reallocations.

Fixes go-gitea#6708.

Signed-off-by: Filip Navara <filip.navara@gmail.com>
@filipnavara
Copy link
Contributor Author

filipnavara commented Apr 30, 2019

Note that on specially corrupted repositories (cyclic commit graph) this could pose a problem since the algorithm could end up in a loop. It's non trivial to create such repository but it should probably be fixed anyway to prevent intentional DoS attacks.

It is no longer possible to prevent it with simple map but maybe it's possible to impose some other limit (time, depth). Previous code from Gitea 1.8.0 used to do the processing in parallel and give up after one minute. Not sure what would be rendered on output then.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 30, 2019
@codecov-io
Copy link

Codecov Report

Merging #6807 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6807      +/-   ##
==========================================
- Coverage   41.21%   41.21%   -0.01%     
==========================================
  Files         421      421              
  Lines       58064    58059       -5     
==========================================
- Hits        23931    23927       -4     
+ Misses      30966    30965       -1     
  Partials     3167     3167
Impacted Files Coverage Δ
modules/git/commit_info.go 69.37% <100%> (+0.28%) ⬆️
models/gpg_key.go 55.83% <0%> (-0.84%) ⬇️
models/repo_list.go 67.89% <0%> (+1.05%) ⬆️

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 9b9ec78...f37b081. Read the comment docs.

@lafriks lafriks added this to the 1.9.0 milestone Apr 30, 2019
@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 Apr 30, 2019
@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 Apr 30, 2019
@adelowo
Copy link
Member

adelowo commented Apr 30, 2019

We have to backport this to 1.8 ?

@filipnavara
Copy link
Contributor Author

1.8 is not affected

@lafriks lafriks merged commit 55daee8 into go-gitea:master Apr 30, 2019
@filipnavara filipnavara deleted the fix6708 branch April 30, 2019 13:29
@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
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Template exception when rendering file list
5 participants