Skip to content

Conversation

@ethantkoenig
Copy link
Member

Fixes #1186 and #1964.

If a PR is fast-forwarded, use the last commit of the PR as the "merge commit".

This ignores a more subtle problem, which is that we determine the "merger" of a PR by the author of the "merge commit", not by the user who actually pushed that commit to origin (which is how Github does it). Fixing this would require a much larger refactor, so for now I decided to just do a quick fix for the more glaring problem.

@lunny lunny added the type/bug label Jun 17, 2017
@lunny lunny added this to the 1.2.0 milestone Jun 17, 2017
@lunny
Copy link
Member

lunny commented Jun 17, 2017

Looks good. Do we have PR integration test?

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

lafriks commented Jun 17, 2017

@lunny
Copy link
Member

lunny commented Jun 17, 2017

@lafriks but the CI is not failed before this PR. So I think that test didn't hit the situation this PR wants to resolve. But anyhow 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 Jun 17, 2017
@ethantkoenig
Copy link
Member Author

ethantkoenig commented Jun 19, 2017

@lunny This is difficult to write an integration test for, because the error occurred in a background thread. The error wouldn't cause a test to fail, since the test has no way of checking whether the error occurred.

@ashkulz
Copy link

ashkulz commented Jun 20, 2017

Will this be backported to 1.1.x?

@lunny
Copy link
Member

lunny commented Jun 21, 2017

@ashkulz sure.

@Bwko
Copy link
Member

Bwko commented Jun 21, 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 Jun 21, 2017
@lunny lunny merged commit 902a749 into go-gitea:master Jun 22, 2017
@ethantkoenig ethantkoenig deleted the fix/merge_pr branch June 25, 2017 02:43
@bkcsoft bkcsoft added the backport/done All backports for this PR have been created label Jul 11, 2017
@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.

Labels

backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1.1.0 index error panic: runtime error: slice bounds out of range

7 participants