-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Avoid issue info panic #29625
Avoid issue info panic #29625
Conversation
for len(ret) < 3 { | ||
ret = append(ret, "") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know in 2 years why that is needed? The whole delimiter concept is fragile because the values are not just numbers but user provided text which can contain the delimiter:
Line 232 in a2b0fb1
Content: fmt.Sprintf("%d|%s", review.Issue.Index, strings.Split(comm.Content, "\n")[0]), |
Line 278 in a2b0fb1
Content: fmt.Sprintf("%d|%s", pr.Issue.Index, pr.Issue.Title), |
Line 310 in a2b0fb1
Content: fmt.Sprintf("%d|%s|%s", review.Issue.Index, reviewerName, comment.Content), |
That field should be json or a struct which can be json serialized by xorm. Here for example it contains json text:
Line 344 in a2b0fb1
Content: string(data), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree with you, but this patch is done intentionally.
- The fragile
|
is as old 2014-07 (8dd07c0 Unknwon on 2014-07-26 at 12:24)- At that time, only
SplitN(2)
, it is generally safe. - Add dismiss review feature #12674 made it worse:
SplitN(3)
....
- At that time, only
- This patch fixes the panic, and is easy to backport
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like don't show the comment which not have enough data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like don't show the comment which not have enough data.
Then it needs a lot of new {{if}}
blocks in the templates. I don't think it's worth to make it that complex.
911cba9
to
da1bf58
Compare
da1bf58
to
858b76e
Compare
I know there might be some different opinions about this "fix", IMO, it is good enough:
|
Backport #29625 by wxiaoguang Fix #29624 Co-authored-by: wxiaoguang <[email protected]>
* origin/main: (29 commits) Tweak actions color and borders (go-gitea#29640) Add download URL for executable files (go-gitea#28260) Move all login and account creation page labels to be above inputs (go-gitea#29432) Avoid issue info panic (go-gitea#29625) Cache repository default branch commit status to reduce query on commit status table (go-gitea#29444) Avoid unexpected panic in graceful manager (go-gitea#29629) Add a link for the recently pushed branch notification (go-gitea#29627) Fix wrong header of org project view page (go-gitea#29626) Detect broken git hooks (go-gitea#29494) Sync branches to DB immediately when handle git hook calling (go-gitea#29493) Fix wrong line number in code search result (go-gitea#29260) Make wiki default branch name changable (go-gitea#29603) A small refactor for agit implementation (go-gitea#29614) Update Twitter Logo (go-gitea#29621) Fix 500 error when adding PR comment (go-gitea#29622) Run editorconfig-checker on `locale_en-US.ini` (go-gitea#29608) bump protobuf module (go-gitea#29617) Add ac claim for old docker/build-push-action@v3 / current buildx gha cache (go-gitea#29584) Skip email domain check when admins edit user emails (go-gitea#29609) Improve natural sort (go-gitea#29611) ...
* giteaofficial/main: Use strict protocol check when redirect (go-gitea#29642) Update various logos and unify their filenames (go-gitea#29637) Tweak actions color and borders (go-gitea#29640) Add download URL for executable files (go-gitea#28260) Move all login and account creation page labels to be above inputs (go-gitea#29432) Avoid issue info panic (go-gitea#29625) Cache repository default branch commit status to reduce query on commit status table (go-gitea#29444) Avoid unexpected panic in graceful manager (go-gitea#29629) Add a link for the recently pushed branch notification (go-gitea#29627) Fix wrong header of org project view page (go-gitea#29626) Detect broken git hooks (go-gitea#29494) Sync branches to DB immediately when handle git hook calling (go-gitea#29493) Fix wrong line number in code search result (go-gitea#29260) Make wiki default branch name changable (go-gitea#29603) A small refactor for agit implementation (go-gitea#29614) Update Twitter Logo (go-gitea#29621) Fix 500 error when adding PR comment (go-gitea#29622)
Fix #29624