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.

Sign up for GitHub

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

Jump to bottom

Fix migration panic due to an empty review comment diff #28334

Merged
merged 3 commits into from Dec 5, 2023
Merged

Fix migration panic due to an empty review comment diff #28334

merged 3 commits into from Dec 5, 2023

Conversation

lng2020
Copy link
Member

@lng2020 lng2020 commented Dec 4, 2023

Fix #28328

func (p *PullRequestComment) GetDiffHunk() string {
	if p == nil || p.DiffHunk == nil {
		return ""
	}
	return *p.DiffHunk
}

This function in the package go-github may return an empty diff. When it's empty, the following code will panic because it access ss[1]

gitea/services/migrations/gitea_uploader.go

Lines 861 to 867 in ec1feed

for _, comment := range review.Comments {
line := comment.Line
if line != 0 {
comment.Position = 1
} else {
_, _, line, _ = git.ParseDiffHunkString(comment.DiffHunk)
}

gitea/modules/git/diff.go

Lines 97 to 101 in ec1feed

func ParseDiffHunkString(diffhunk string) (leftLine, leftHunk, rightLine, righHunk int) {
ss := strings.Split(diffhunk, "@@")
ranges := strings.Split(ss[1][1:], " ")
leftRange := strings.Split(ranges[0], ",")
leftLine, _ = strconv.Atoi(leftRange[0][1:])

@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 4, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 4, 2023
@lng2020 lng2020 added type/bug backport/v1.21 This PR should be backported to Gitea 1.21 labels Dec 4, 2023
@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 Dec 4, 2023
@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 Dec 5, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Dec 5, 2023
@lunny lunny enabled auto-merge (squash) December 5, 2023 08:00
@lunny lunny merged commit 49b98e4 into go-gitea:main Dec 5, 2023
25 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Dec 5, 2023
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Dec 5, 2023
@lng2020 @GiteaBot
Fix migration panic due to an empty review comment diff (go-gitea#28334)
1906029 
Fix go-gitea#28328 
```
func (p *PullRequestComment) GetDiffHunk() string {
	if p == nil || p.DiffHunk == nil {
		return ""
	}
	return *p.DiffHunk
}
```
This function in the package `go-github` may return an empty diff. When
it's empty, the following code will panic because it access `ss[1]`

https://github.com/go-gitea/gitea/blob/ec1feedbf582b05b6a5e8c59fb2457f25d053ba2/services/migrations/gitea_uploader.go#L861-L867

https://github.com/go-gitea/gitea/blob/ec1feedbf582b05b6a5e8c59fb2457f25d053ba2/modules/git/diff.go#L97-L101
@GiteaBot GiteaBot mentioned this pull request Dec 5, 2023
Merged
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Dec 5, 2023
@lng2020 lng2020 deleted the fix-empty-diff branch December 5, 2023 08:45
lunny pushed a commit that referenced this pull request Dec 5, 2023
@GiteaBot @lng2020
Fix migration panic due to an empty review comment diff (#28334) (#28362
8b590de 
)

Backport #28334 by @lng2020

Fix #28328 
```
func (p *PullRequestComment) GetDiffHunk() string {
	if p == nil || p.DiffHunk == nil {
		return ""
	}
	return *p.DiffHunk
}
```
This function in the package `go-github` may return an empty diff. When
it's empty, the following code will panic because it access `ss[1]`

https://github.com/go-gitea/gitea/blob/ec1feedbf582b05b6a5e8c59fb2457f25d053ba2/services/migrations/gitea_uploader.go#L861-L867

https://github.com/go-gitea/gitea/blob/ec1feedbf582b05b6a5e8c59fb2457f25d053ba2/modules/git/diff.go#L97-L101

Co-authored-by: Nanguan Lin <70063547+lng2020@users.noreply.github.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 6, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
0d2c650 
* giteaofficial/main:
  Fix the runs will not be displayed bug when the main branch have no workflows but other branches have (go-gitea#28359)
  [skip ci] Updated translations via Crowdin
  Render PyPi long description as document (go-gitea#28272)
  handle repository.size column being NULL in migration v263 (go-gitea#28336)
  Fix migration panic due to an empty review comment diff (go-gitea#28334)
  Add `HEAD` support for rpm repo files (go-gitea#28309)
  Remove deprecated query condition in ListReleases (go-gitea#28339)
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
@lng2020 @fuxiaohei
Fix migration panic due to an empty review comment diff (go-gitea#28334)
38df736 
Fix go-gitea#28328 
```
func (p *PullRequestComment) GetDiffHunk() string {
	if p == nil || p.DiffHunk == nil {
		return ""
	}
	return *p.DiffHunk
}
```
This function in the package `go-github` may return an empty diff. When
it's empty, the following code will panic because it access `ss[1]`

https://github.com/go-gitea/gitea/blob/ec1feedbf582b05b6a5e8c59fb2457f25d053ba2/services/migrations/gitea_uploader.go#L861-L867

https://github.com/go-gitea/gitea/blob/ec1feedbf582b05b6a5e8c59fb2457f25d053ba2/modules/git/diff.go#L97-L101
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@lng2020 @silverwind
Fix migration panic due to an empty review comment diff (go-gitea#28334)
b97cb33 
Fix go-gitea#28328 
```
func (p *PullRequestComment) GetDiffHunk() string {
	if p == nil || p.DiffHunk == nil {
		return ""
	}
	return *p.DiffHunk
}
```
This function in the package `go-github` may return an empty diff. When
it's empty, the following code will panic because it access `ss[1]`

https://github.com/go-gitea/gitea/blob/ec1feedbf582b05b6a5e8c59fb2457f25d053ba2/services/migrations/gitea_uploader.go#L861-L867

https://github.com/go-gitea/gitea/blob/ec1feedbf582b05b6a5e8c59fb2457f25d053ba2/modules/git/diff.go#L97-L101
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Mar 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny approved these changes

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. type/bug
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.

GitHub Migration: PANIC whilst trying to do migrate task: runtime error: index out of range
4 participants
@lng2020 @lunny @delvh @GiteaBot