Skip to content

[BugFix] [API] Pull.API.Convert: Only try to get HeadBranch if HeadRepo exist - #10029

Merged
techknowlogick merged 8 commits into
go-gitea:masterfrom
6543-forks:fix-pull-api-convert
Jan 31, 2020
Merged

techknowlogick merged 8 commits into
go-gitea:masterfrom
6543-forks:fix-pull-api-convert

Conversation

@6543

@6543 6543 commented Jan 28, 2020 •

Copy link
Copy Markdown
Member

close #10025

  • remove one pr.Issue.LoadRepo (it is ececuted 2 times)
  • prevent nil-pointer exeption on user.APIFormat()
  • handle non existing HreadRepo

credits to @jolheiser @zeripath who found the witch part of the code i had to look at :)

@6543

6543 commented Jan 28, 2020 •

Copy link
Copy Markdown
Member Author

the diff looks weard on code bloc L101-129 but I only added a if statement before witch check if pr.HeadRepo != nil

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

codecov-io commented Jan 28, 2020 •

Copy link
Copy Markdown

Codecov Report

Merging #10029 into master will increase coverage by 0.07%.
The diff coverage is 60.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10029      +/-   ##
==========================================
+ Coverage   43.37%   43.45%   +0.07%     
==========================================
  Files         565      566       +1     
  Lines       78905    78987      +82     
==========================================
+ Hits        34226    34321      +95     
+ Misses      40469    40429      -40     
- Partials     4210     4237      +27
Impacted Files Coverage Δ
modules/structs/repo.go 36.36% <ø> (ø) ⬆️
routers/repo/setting.go 15.01% <0%> (+0.02%) ⬆️
routers/api/v1/api.go 75.07% <100%> (+0.61%) ⬆️
services/repository/transfer.go 55.55% <28.57%> (-11.12%) ⬇️
routers/api/v1/repo/transfer.go 72.85% <72.85%> (ø)
services/pull/temp_repo.go 31.62% <0%> (-2.57%) ⬇️
models/unit.go 37.03% <0%> (-2.47%) ⬇️
routers/repo/view.go 38.59% <0%> (-0.88%) ⬇️
services/pull/patch.go 67.92% <0%> (ø) ⬆️
... and 6 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 f2dd94f...10413ce. Read the comment docs.

Comment thread modules/convert/pull.go Resolved
@6543

6543 commented Jan 28, 2020

Copy link
Copy Markdown
Member Author

@lunny done

Comment thread modules/convert/pull.go Resolved
@lunny lunny added the type/bug label Jan 28, 2020
@lunny

lunny commented Jan 28, 2020

Copy link
Copy Markdown
Member

@6543 I think you could add a test append on function TestPullBranchDelete. I have tested a broken pull request there.

@6543
6543 force-pushed the fix-pull-api-convert branch from 81569f1 to 3c9ef56 Compare January 28, 2020 23:42
@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 Jan 29, 2020
Comment thread modules/convert/pull.go Resolved
@6543
6543 force-pushed the fix-pull-api-convert branch from 99eb9b2 to 7d132ed Compare January 30, 2020 05:00
@6543

6543 commented Jan 30, 2020 •

Copy link
Copy Markdown
Member Author

@lunny test added
@lafriks changed

general Info: while I was creating the test i roun in a nil-pointer-exeption so i added a check on user.APIFormat() <- the test now works without this fix but it might prevent som 500 in the future

Comment thread modules/convert/pull.go Resolved
@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 Jan 31, 2020
@lunny

lunny commented Jan 31, 2020

Copy link
Copy Markdown
Member

Only a nit #10029 (comment)

@6543
6543 force-pushed the fix-pull-api-convert branch from 36b2286 to 71c9391 Compare January 31, 2020 05:55
@6543

6543 commented Jan 31, 2020

Copy link
Copy Markdown
Member Author

by the way should we backport this?

@techknowlogick techknowlogick added this to the 1.12.0 milestone Jan 31, 2020
@techknowlogick
techknowlogick merged commit 8d43563 into go-gitea:master Jan 31, 2020
@techknowlogick

Copy link
Copy Markdown
Member

@6543 please backport :)

@6543
6543 deleted the fix-pull-api-convert branch January 31, 2020 21:17
6543 added a commit to 6543-forks/gitea that referenced this pull request Jan 31, 2020
@techknowlogick techknowlogick added the backport/done All backports for this PR have been created label Feb 1, 2020
@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.

Pull API returning null

7 participants