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

Prevent deadlock in pull_service.GetDiverging(pr) #10905

Merged
merged 8 commits into from
Apr 1, 2020

Conversation

6543
Copy link
Member

@6543 6543 commented Mar 31, 2020

fix #10117 by using a temporary repo to compare

@6543 6543 changed the title [WIP] [BugFix] Fix get diverging 10117 [WIP] [BugFix] Fix GetDiverging(pull) Mar 31, 2020
@lafriks
Copy link
Member

lafriks commented Mar 31, 2020

Could you split cache to different PR, so that first part could be backported to 1.11.x?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 31, 2020
@6543 6543 changed the title [WIP] [BugFix] Fix GetDiverging(pull) [BugFix] Fix GetDiverging(pull) Mar 31, 2020
@6543
Copy link
Member Author

6543 commented Mar 31, 2020

@lafriks #9784 introduced function GetDiverging() so there is no need for backport

@lafriks lafriks added skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. and removed backport/v1.11 labels Mar 31, 2020
@lafriks lafriks added this to the 1.12.0 milestone Mar 31, 2020
@lafriks
Copy link
Member

lafriks commented Mar 31, 2020

You are right, sorry 🤦‍♂️

@codecov-io
Copy link

codecov-io commented Mar 31, 2020

Codecov Report

Merging #10905 into master will increase coverage by 0.01%.
The diff coverage is 44.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10905      +/-   ##
==========================================
+ Coverage   43.43%   43.45%   +0.01%     
==========================================
  Files         593      595       +2     
  Lines       83286    83467     +181     
==========================================
+ Hits        36178    36267      +89     
- Misses      42620    42717      +97     
+ Partials     4488     4483       -5     
Impacted Files Coverage Δ
models/error.go 30.50% <0.00%> (ø)
models/issue.go 51.17% <0.00%> (-0.25%) ⬇️
models/issue_label.go 61.60% <0.00%> (-1.45%) ⬇️
models/migrations/migrations.go 4.16% <ø> (ø)
models/migrations/v128.go 0.00% <0.00%> (ø)
models/migrations/v134.go 0.00% <0.00%> (ø)
models/migrations/v135.go 0.00% <0.00%> (ø)
modules/private/manager.go 0.00% <0.00%> (ø)
routers/repo/compare.go 40.80% <ø> (-0.14%) ⬇️
routers/repo/pull_review.go 0.00% <0.00%> (ø)
... and 21 more

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 36d9237...1b92b81. Read the comment docs.

@zeripath
Copy link
Contributor

zeripath commented Mar 31, 2020

Hmm... We should be using a temporary repo with shared object DBs to generate these divergences if we genuinely need to look between two repos. I don't like this add remote thing - it's bound to catch us out again - not least in that I think it could cause a repack of git references in the base repo to contain head git references which are later broken. (although we get a bit of that with pull references.)

However do we really need to do that? I don't remember when divergence is shown - if it's only down for actual PRs then can't we just use the refs/pull/ head to generate the divergence as they're then in the same repo.

@6543
Copy link
Member Author

6543 commented Apr 1, 2020

@zeripath do you have time to review :D

services/pull/check.go Outdated Show resolved Hide resolved
modules/git/repo_branch.go Outdated Show resolved Hide resolved
@6543
Copy link
Member Author

6543 commented Apr 1, 2020

@guillep2k I'll split things up ...

@6543
Copy link
Member Author

6543 commented Apr 1, 2020

@lafriks in the end it got split up 😅 ...

Cache PR Divergence -> #10914

@guillep2k ready to review

@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 1, 2020
@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 1, 2020
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

may not have deleted things properly

@6543
Copy link
Member Author

6543 commented Apr 1, 2020

ping lgtm

@zeripath zeripath changed the title [BugFix] Fix GetDiverging(pull) Prevent deadlock in pull_service.GetDiverging(pr) Apr 1, 2020
@zeripath zeripath merged commit a291a0e into go-gitea:master Apr 1, 2020
@6543 6543 deleted the fix-GetDiverging-10117 branch April 1, 2020 19:30
@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. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] GetDiverging(pull) can lock pull in a 500 state
7 participants