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 wrong log when push to a new branch #1220

Merged
merged 1 commit into from
Mar 17, 2017
Merged

Fix wrong log when push to a new branch #1220

merged 1 commit into from
Mar 17, 2017

Conversation

lunny
Copy link
Member

@lunny lunny commented Mar 11, 2017

fix #1111

@lunny lunny added this to the 1.2.0 milestone Mar 11, 2017
strk
strk reviewed Mar 11, 2017
View reviewed changes
Copy link
Member

@strk strk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM beside the comment

models/action.go
@@ -360,7 +360,7 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit) err

issue, err := GetIssueByRef(ref)
if err != nil {
if IsErrIssueNotExist(err) {
if IsErrIssueNotExist(err) || err == errMissingIssueNumber {
continue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if GetIssueByRef should just return an ErrIssueNotExist even when an issue number is missing...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me check could we do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetIssueByRef will return many kinds of error besides ErrIssueNotExist and errMissingIssueNumber. Don't know how to fix this.

@strk
Copy link
Member

strk commented Mar 11, 2017

Does the LGTM bot not look into reviews ?

@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 11, 2017
@appleboy
Copy link
Member

@strk L-G-T-M bot doesn't look into review comments. :(

@pgaskin
Copy link
Contributor

pgaskin commented Mar 14, 2017

LGTM except for @strk's comment

@tboerger tboerger 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 14, 2017
@lunny lunny merged commit 3cf0e51 into go-gitea:master Mar 17, 2017
@lunny lunny mentioned this pull request Mar 17, 2017
Closed
@lunny lunny deleted the lunny/fix_wrong_err_log branch April 19, 2017 05:52
agrn pushed a commit to agrn/gitea that referenced this pull request May 19, 2017
@lunny @agrn
fix wrong log when push to a new branch (go-gitea#1220)
deb1e34
This was referenced May 19, 2017
Merged
Merged
lunny pushed a commit that referenced this pull request May 20, 2017
@agrn @lunny
Backport bugfixes #1220 and #1393 to v1.1 (#1758)
4fd55d8 
* Fix "Error: No issue number specified"  when pushing

* fix wrong log when push to a new branch (#1220)

* Removing empty file modules/github/issues.go
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@strk strk strk left review comments

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.

Push success but display an error log
5 participants
@lunny @strk @appleboy @pgaskin @tboerger