Skip to content
This repository has been archived by the owner on Apr 12, 2019. It is now read-only.

Refactor CommitsBefore method to match github #88

Merged
merged 2 commits into from
Dec 10, 2017
Merged

Refactor CommitsBefore method to match github #88

merged 2 commits into from
Dec 10, 2017

Conversation

daviian
Copy link
Member

@daviian daviian commented Oct 11, 2017

The CommitsBefore method is only used to retrieve commits for the push webhook if a new branch is pushed.
Namely at https://github.com/go-gitea/gitea/blob/a4cd4616c6f8524f8eb79d6184e855b3a8164130/models/update.go#L229

The refactored method now returns exactly those commits which were added in the new branch and are not existing in any other branch.
This result now behaves exactly as github (compared to the webhook result) except for the order.

The order will be fixed in a separate PR as there is another method which needs order fixing.
All these changes are necessary to implement go-gitea/gitea#2548

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>
repo_commit.go Outdated
return repo.commitsBefore(id, num)
}

func (repo *Repository) getBranches(commit *Commit) ([]string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Add a limit int so we can do for-each-ref --count=2 for the above command (L295) (makes it slightly more resiliant to branch-heavy repo)

@bkcsoft
Copy link
Member

bkcsoft commented Oct 12, 2017

Overall I think this is a very bad way of getting the list of commits. But fixing it properly would be harder to do in a single PR so LGTM

@daviian
Copy link
Member Author

daviian commented Oct 12, 2017

@bkcsoft What would be a more sound approach in your opinion?
But do we agree about the expected result at least?

Or should I create a new method to retrieve commits from the new branch?

Signed-off-by: David Schneiderbauer <dschneiderbauer@gmail.com>
@bkcsoft
Copy link
Member

bkcsoft commented Oct 15, 2017

@daviian I'd check what branches we had before the push, and which ones we have after the push and then compare them. But that's a bit out-of-scope for this PR :)

@lafriks
Copy link
Member

lafriks commented Dec 10, 2017

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants