Skip to content

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Mar 8, 2020

When a branch of a forked repository is merged as a PR and then deleted /api/v1/repos/owner/name/pulls and /api/v1/repos/owner/name/pulls/index will cause a 500 with a panic internally.

This is because:

  • modules/git/repo_commit.go:21:GetRefCommitID(name string) does not return a git.ErrNotExist error
  • modules/convert/pull.go:110:log.Error("GetBranch[%s]: %v", headBranch.Name, err) causes a NPE because headBranch is nil as determined a few lines earlier.

These are now fixed.

Fix #10639

Signed-off-by: Andrew Thornton [email protected]

@zeripath zeripath added this to the 1.12.0 milestone Mar 8, 2020
@zeripath
Copy link
Contributor Author

zeripath commented Mar 8, 2020

I'm fairly certain that 1.11 will be affected too.

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

codecov-io commented Mar 8, 2020

Codecov Report

Merging #10676 into master will decrease coverage by 0.03%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10676      +/-   ##
==========================================
- Coverage   43.59%   43.55%   -0.04%     
==========================================
  Files         589      589              
  Lines       82561    82565       +4     
==========================================
- Hits        35996    35965      -31     
- Misses      42104    42146      +42     
+ Partials     4461     4454       -7
Impacted Files Coverage Δ
routers/repo/branch.go 55.72% <0%> (ø) ⬆️
modules/convert/pull.go 68.69% <0%> (-3.48%) ⬇️
modules/git/repo_commit.go 53.67% <100%> (+0.27%) ⬆️
modules/context/panic.go 50% <0%> (-50%) ⬇️
services/pull/check.go 38.41% <0%> (-15.86%) ⬇️
models/repo_mirror.go 2.12% <0%> (-10.64%) ⬇️
modules/queue/unique_queue_disk_channel.go 53.84% <0%> (-1.93%) ⬇️
services/pull/pull.go 34.7% <0%> (-1.18%) ⬇️
services/mirror/mirror.go 18.75% <0%> (-0.66%) ⬇️
models/pull.go 42.3% <0%> (-0.56%) ⬇️
... and 5 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 a9f4489...0814219. Read the comment docs.

@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 Mar 8, 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 Mar 8, 2020
@zeripath zeripath merged commit 3fc4f36 into go-gitea:master Mar 9, 2020
@zeripath zeripath deleted the fix-10639-panics-in-api-pull branch March 9, 2020 07:18
zeripath added a commit to zeripath/gitea that referenced this pull request Mar 22, 2020
Backport go-gitea#10676

* Fix panic in API pulls when headbranch does not exist
* refix other reference to plumbing.ErrReferenceNotFound

Signed-off-by: Andrew Thornton <[email protected]>
@lafriks lafriks added the backport/done All backports for this PR have been created label Mar 22, 2020
lafriks added a commit that referenced this pull request Mar 23, 2020
…mitID (#10676) (#10797)

* Fix panic in API pulls when headbranch does not exist (#10676)

Backport #10676

* Fix panic in API pulls when headbranch does not exist
* refix other reference to plumbing.ErrReferenceNotFound

Signed-off-by: Andrew Thornton <[email protected]>

* Apply suggestions from code review

Co-Authored-By: Lauris BH <[email protected]>
@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
backport/done All backports for this PR have been created 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.

Bug: Api: ListPullRequests [500]
6 participants