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

table pull_request wasn't updated correctly #2649

Merged
merged 3 commits into from
Oct 5, 2017
Merged

table pull_request wasn't updated correctly #2649

merged 3 commits into from
Oct 5, 2017

Conversation

kaz
Copy link
Contributor

@kaz kaz commented Oct 4, 2017 •
edited
Loading

Since this commit (dd55534), table pull_request wasn't updated correctly.

Because of this, merge description says such wrong thing:
Ghost merged 1 commits from user/branch into user/branch 48 years ago

Example https://try.gitea.io/amama/haipro/pulls/1

@codecov-io
Copy link

codecov-io commented Oct 4, 2017 •
edited
Loading

Codecov Report

Merging #2649 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2649   +/-   ##
=======================================
  Coverage   27.13%   27.13%           
=======================================
  Files          86       86           
  Lines       17061    17061           
=======================================
  Hits         4629     4629           
  Misses      11754    11754           
  Partials      678      678
Impacted Files Coverage Δ
models/pull.go 20.38% <0%> (ø) ⬆️

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 6b62f04...df432e7. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 4, 2017
@ethantkoenig
Copy link
Member

LGTM

@tboerger tboerger 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 Oct 4, 2017
daviian
daviian reviewed Oct 4, 2017
View reviewed changes
models/pull.go Outdated
@@ -422,7 +422,7 @@ func (pr *PullRequest) setMerged() (err error) {
if err = pr.Issue.changeStatus(sess, pr.Merger, pr.Issue.Repo, true); err != nil {
return fmt.Errorf("Issue.changeStatus: %v", err)
}
if _, err = sess.Id(pr.ID).Cols("has_merged").Update(pr); err != nil {
if _, err = sess.Id(pr.ID).Cols("has_merged, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be sess.ID ?

Copy link
Member

Choose a reason for hiding this comment

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

They are aliases but by GoLang naming ID is more correct

Copy link
Member

@ethantkoenig ethantkoenig Oct 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Id(..) is deprecated, so I think we should prefer ID(..) (https://godoc.org/github.com/go-xorm/xorm#Engine.Id)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Umm ... but Id(..) is used in many other places.
It seems to be reasonable to fix all of them at once as an other issue.

Copy link
Member

Choose a reason for hiding this comment

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

To not to accidently break something it's better to change them one by one when touching old code like in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll fix it

@lafriks lafriks added type/bug issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP labels Oct 4, 2017
@lafriks lafriks added this to the 1.3.0 milestone Oct 4, 2017
@tonivj5
Copy link
Contributor

tonivj5 commented Oct 4, 2017

https://try.gitea.io/amama/haipro/pulls/1 Why if do I enter into commits or files it shows a 500 error? 😕

@kaz
Copy link
Contributor Author

kaz commented Oct 5, 2017

@xxxTonixxx

It is because merged_commit_id column won't be updated.
Due to this, GetCommit() and GetDiffRange() fail trying to open the commit with empty id.

This PR fixes this bug too.

@kaz
Copy link
Contributor Author

kaz commented Oct 5, 2017

rebased

@lafriks
Copy link
Member

lafriks commented Oct 5, 2017

LGTM

@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 Oct 5, 2017
@lafriks lafriks merged commit ffab139 into go-gitea:master Oct 5, 2017
@lafriks lafriks mentioned this pull request Oct 5, 2017
Closed
7 tasks
@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

@ethantkoenig ethantkoenig ethantkoenig left review comments

@daviian daviian daviian left review comments

@lafriks lafriks lafriks left review comments

Assignees
No one assigned
Labels
issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.
7 participants
@kaz @codecov-io @ethantkoenig @tonivj5 @lafriks @daviian @tboerger