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.

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

Use merge-base in pull requests to determine the common ancestor #95

Merged
merged 1 commit into from
May 28, 2017

Conversation

bradleyfalzon
Copy link
Owner

GopherCI generates its own diffs, not using GitHubs, for the main
reason that GitHub did not have a uniform way to fetch diffs and
we would like to support other platforms (GitLab) potentially later.
So whilst we could generate a diff perfectly fine, we kept doing so.

It's become apparent that we're not generating the diffs correctly
at all. We're creating a diff between HEAD of the PR and the latest
commit in the base branch, this isn't what GitHub is doing, so when
we post a comment to a particular line, the line numbers could be
wrong.

Initially it was thought we could just create a diff interface,
passing this into the Analyser, and never generating the diffs
ourselves, but apicompat needs to know the exact base ref. Previously
it was using FETCH_HEAD, as was git diff.

So, instead of using GitHub's diffs, we'd learn how to accurately
generate a diff by knowing the exact point where the branch for the
PR was created. We'd then pass this to apicompat as well.

To do this, we'll create a refReader interface, because a push already
knows the base ref, but a pull request does not. When invoking the
Analyse method, the caller will determine whether it should use
one of two new types implementing refReader.

A push will use a hard coded base ref that it obtains from the API
and a pull request will use git merge-tool to compare the two branches
FETCH_HEAD and HEAD to find the common ancestor.

Git diff and apicompat will then use the result of refReader.

In doing this, it requires the pull request cloner to clone a much
larger history, to ensure it captures enough history to find the
last common commit. This might be able to be optimised at a later
stage if it could know how many commits are in the pull request.

Fixes #84.

GopherCI generates its own diffs, not using GitHubs, for the main
reason that GitHub did not have a uniform way to fetch diffs and
we would like to support other platforms (GitLab) potentially later.
So whilst we could generate a diff perfectly fine, we kept doing so.

It's become apparent that we're not generating the diffs correctly
at all. We're creating a diff between HEAD of the PR and the latest
commit in the base branch, this isn't what GitHub is doing, so when
we post a comment to a particular line, the line numbers could be
wrong.

Initially it was thought we could just create a diff interface,
passing this into the Analyser, and never generating the diffs
ourselves, but apicompat needs to know the exact base ref. Previously
it was using FETCH_HEAD, as was git diff.

So, instead of using GitHub's diffs, we'd learn how to accurately
generate a diff by knowing the exact point where the branch for the
PR was created. We'd then pass this to apicompat as well.

To do this, we'll create a refReader interface, because a push already
knows the base ref, but a pull request does not. When invoking the
Analyse method, the caller will determine whether it should use
one of two new types implementing refReader.

A push will use a hard coded base ref that it obtains from the API
and a pull request will use git merge-tool to compare the two branches
FETCH_HEAD and HEAD to find the common ancestor.

Git diff and apicompat will then use the result of refReader.

In doing this, it requires the pull request cloner to clone a much
larger history, to ensure it captures enough history to find the
last common commit. This might be able to be optimised at a later
stage if it could know how many commits are in the pull request.

Fixes #84.
@coveralls
Copy link

Coverage Status

Coverage increased (+1.02%) to 61.034% when pulling 60739d2 on refreader into 03badd2 on master.

@bradleyfalzon bradleyfalzon merged commit d3b4fb7 into master May 28, 2017
@bradleyfalzon bradleyfalzon deleted the refreader branch May 28, 2017 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants