-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Prevent nil dereference in mailIssueCommentToParticipants (#5891, #5895) #5894
Prevent nil dereference in mailIssueCommentToParticipants (#5891, #5895) #5894
Conversation
…itea#5891) Previous code could potentially dereference nil - this PR ensures that the poster is loaded before dereferencing it. Signed-off-by: Andrew Thornton <[email protected]>
Signed-off-by: Andrew Thornton <[email protected]>
@@ -88,6 +88,10 @@ func mailIssueCommentToParticipants(e Engine, issue *Issue, doer *User, content | |||
names = append(names, participants[i].Name) | |||
} | |||
|
|||
if err := issue.loadRepo(e); err != nil { |
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.
This changes are not in the original PR #5891
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.
Yup as per: #5891 (comment)
I noticed another nil dereference.
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.
Oh, I missed that :)
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.
Happy to remove and do another PR if preferred.
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.
Ideally, I'd prefer it in another PR since backports are basically cherrypicked.
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.
The only trouble is that the PR doesn't then completely fix the bug...
@techknowlogick Should we keep the repo fix in this PR or make another with it separately?
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.
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.
Yeah, preferably two PRs for the reasons @adelowo listed above. However in this case I think we can keep it as one PR.
The previous code could potentially dereference nil - this PR ensures
that the poster and the repo are loaded before dereferencing them.
#5891
Signed-off-by: Andrew Thornton [email protected]