Only check for conflicts/merging if the PR has not been merged in the interim #10132

zeripath · 2020-02-03T22:15:05Z

The current code for checking PRs suffers from a race with being merged in the interim. This PR reduces the likelihood of this race occurring.

(We would need temporary locks and to extend the state counter for PRs to include merging etc. for this to be completely fixed.)

…terim

services/pull/check.go

models/pull.go

codecov-io · 2020-02-04T21:49:17Z

Codecov Report

Merging #10132 into master will increase coverage by <.01%.
The diff coverage is 7.4%.

@@            Coverage Diff             @@
##           master   #10132      +/-   ##
==========================================
+ Coverage   43.63%   43.64%   +<.01%     
==========================================
  Files         576      576              
  Lines       79716    79718       +2     
==========================================
+ Hits        34788    34789       +1     
- Misses      40631    40635       +4     
+ Partials     4297     4294       -3

Impacted Files	Coverage Δ
routers/user/profile.go	`50.69% <0%> (-0.97%)`	⬇️
routers/routes/routes.go	`86.47% <100%> (-0.05%)`	⬇️
models/pull.go	`42.03% <50%> (+0.1%)`	⬆️
models/unit.go	`37.03% <0%> (-2.47%)`	⬇️
modules/queue/unique_queue_disk_channel.go	`52.56% <0%> (-1.93%)`	⬇️
routers/repo/view.go	`39.13% <0%> (-0.87%)`	⬇️
modules/log/event.go	`65.64% <0%> (+1.02%)`	⬆️
services/pull/pull.go	`33.93% <0%> (+1.5%)`	⬆️
services/pull/check.go	`35.97% <0%> (+3.04%)`	⬆️

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 0140315...7955006. Read the comment docs.

models/pull.go

lafriks · 2020-02-06T21:31:36Z

Still CI error :P

This might improve the chance that the race does not affect us but does not prevent it.

zeripath · 2020-02-09T17:23:09Z

Finally!! Ready for re-review @lafriks @guillep2k

guillep2k

Wow! This thing was very difficult! Sorry about the "affected rows" count not working. 😞

Here's a couple of nits. I think the pr.Issue = nil suggestion is important.

models/pull.go

Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com>

guillep2k · 2020-02-09T21:36:00Z

Nope that wasn't it - I think the issue is the count doesn't seem to be returned on MySQL.

This is probably a bug on xorm or the MySQL driver.

lafriks · 2020-02-09T21:56:22Z

@guillep2k no, MS SQL Server does not allow updating ID key

models/pull.go

Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com>

guillep2k · 2020-02-09T22:57:23Z

@guillep2k no, MS SQL Server does not allow updating ID key

Oh, right, because it's auto-increment. I forgot about that.

… interim (go-gitea#10132) * Only check for merging if the PR has not been merged in the interim * fixup! Only check for merging if the PR has not been merged in the interim * Try to fix test failure * Use PR2 not PR1 in tests as PR1 merges automatically * return already merged error * enforce locking * move pullrequest checking to after merge This might improve the chance that the race does not affect us but does not prevent it. * Remove minor race with getting merge commit id move check pr after merge * Remove unnecessary prepareTestEnv - onGiteaRun does this for us * Add information about when merging occuring * More logging Co-authored-by: Lauris BH <lauris@nix.lv> Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com>

6543 · 2020-02-10T11:59:59Z

@zeripath send backport (had to do some reorder)

… interim (#10132) (#10206) * Only check for conflicts/merging if the PR has not been merged in the interim (#10132) * Only check for merging if the PR has not been merged in the interim * fixup! Only check for merging if the PR has not been merged in the interim * Try to fix test failure * Use PR2 not PR1 in tests as PR1 merges automatically * return already merged error * enforce locking * move pullrequest checking to after merge This might improve the chance that the race does not affect us but does not prevent it. * Remove minor race with getting merge commit id move check pr after merge * Remove unnecessary prepareTestEnv - onGiteaRun does this for us * Add information about when merging occuring * More logging Co-authored-by: Lauris BH <lauris@nix.lv> Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com> * re order Co-authored-by: zeripath <art27@cantab.net> Co-authored-by: Lauris BH <lauris@nix.lv> Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com>

Only check for merging if the PR has not been merged in the interim

ef1a8a5

zeripath added the type/bug label Feb 3, 2020

zeripath added this to the 1.12.0 milestone Feb 3, 2020

fixup! Only check for merging if the PR has not been merged in the in…

e29a6de

…terim

lafriks reviewed Feb 3, 2020

View reviewed changes

services/pull/check.go Show resolved Hide resolved

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 3, 2020

zeripath commented Feb 3, 2020

View reviewed changes

models/pull.go Outdated Show resolved Hide resolved

zeripath added 4 commits February 3, 2020 22:45

Try to fix test failure

1205165

Use PR2 not PR1 in tests as PR1 merges automatically

ee7d5f7

Merge branch 'master' into improve-pr-check

4c00eb6

return already merged error

9a90443

Merge branch 'master' into improve-pr-check

ad20cd2

lafriks approved these changes Feb 5, 2020

View reviewed changes

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 Feb 5, 2020

lafriks added the backport/v1.11 label Feb 5, 2020

guillep2k reviewed Feb 5, 2020

View reviewed changes

models/pull.go Outdated Show resolved Hide resolved

zeripath added 3 commits February 6, 2020 20:04

enforce locking

fdc055e

enforce locking - fix-test

0f9e498

enforce locking - fix-testx2

5551c6d

lafriks approved these changes Feb 6, 2020

View reviewed changes

enforce locking - fix-testx3

7d5772f

zeripath added 6 commits February 9, 2020 08:15

move pullrequest checking to after merge

76f7eef

This might improve the chance that the race does not affect us but does not prevent it.

Remove minor race with getting merge commit id

a60c5b8

fixup

87f5793

move check pr after merge

f8f886f

Remove unnecessary prepareTestEnv - onGiteaRun does this for us

389342c

Add information about when merging occuring

203ff71

zeripath added 6 commits February 9, 2020 16:22

remove the count - perhaps that will help

441ff1a

next remove the update id

52cd67c

next remove the update id - make it updated_unix instead

8373df4

On failure to merge ensure that the pr is rechecked for conflict errors

c8c5809

On failure to merge ensure that the pr is rechecked for conflict errors

f505332

Merge branch 'master' into improve-pr-check

0140315

lafriks approved these changes Feb 9, 2020

View reviewed changes

6543 approved these changes Feb 9, 2020

View reviewed changes

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 Feb 9, 2020

Merge branch 'master' into improve-pr-check

bd7c2eb

guillep2k reviewed Feb 9, 2020

View reviewed changes

models/pull.go Show resolved Hide resolved

models/pull.go Outdated Show resolved Hide resolved

models/pull.go Outdated Show resolved Hide resolved

zeripath and others added 2 commits February 9, 2020 20:57

Update models/pull.go

7486e16

Update models/pull.go

7955006

Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com>

guillep2k reviewed Feb 9, 2020

View reviewed changes

models/pull.go Outdated Show resolved Hide resolved

models/pull.go Outdated Show resolved Hide resolved

models/pull.go Outdated Show resolved Hide resolved

models/pull.go Outdated Show resolved Hide resolved

zeripath and others added 2 commits February 9, 2020 22:13

Apply suggestions from code review

6bd8699

Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com>

Merge branch 'master' into improve-pr-check

1ffd6bc

guillep2k approved these changes Feb 9, 2020

View reviewed changes

lafriks merged commit 875c5e1 into go-gitea:master Feb 9, 2020

zeripath deleted the improve-pr-check branch February 10, 2020 07:47

6543 mentioned this pull request Feb 10, 2020

Only check for conflicts/merging if the PR has not been merged in the interim (#10132) #10206

Merged

lafriks added the backport/done All backports for this PR have been created label Feb 10, 2020

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only check for conflicts/merging if the PR has not been merged in the interim #10132

Only check for conflicts/merging if the PR has not been merged in the interim #10132

zeripath commented Feb 3, 2020

codecov-io commented Feb 4, 2020 •

edited

lafriks commented Feb 6, 2020

zeripath commented Feb 9, 2020

guillep2k left a comment

guillep2k commented Feb 9, 2020

lafriks commented Feb 9, 2020

guillep2k commented Feb 9, 2020

6543 commented Feb 10, 2020

Only check for conflicts/merging if the PR has not been merged in the interim #10132

Only check for conflicts/merging if the PR has not been merged in the interim #10132

Conversation

zeripath commented Feb 3, 2020

codecov-io commented Feb 4, 2020 • edited

Codecov Report

lafriks commented Feb 6, 2020

zeripath commented Feb 9, 2020

guillep2k left a comment

Choose a reason for hiding this comment

guillep2k commented Feb 9, 2020

lafriks commented Feb 9, 2020

guillep2k commented Feb 9, 2020

6543 commented Feb 10, 2020

codecov-io commented Feb 4, 2020 •

edited