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

GetChanges is returning files that didn't change in the PR #486

Open
dpordomingo opened this issue Jan 23, 2019 · 2 comments · May be fixed by #600
Open

GetChanges is returning files that didn't change in the PR #486

dpordomingo opened this issue Jan 23, 2019 · 2 comments · May be fixed by #600
Assignees
Labels
bug Something isn't working

Comments

@dpordomingo
Copy link
Contributor

dpordomingo commented Jan 23, 2019

Given this history:

* 7450de7 (origin/master) Change 'README.md' file
|
| * dd8c39e (fork/candidate) Change 'golang.go' file
| |
| /   
* 83f7e4d
| 
* 7ccf9be

For a pull request to merge fork/candidate against origin/master, GetChanges rpc is returning golang.go and README.md.

I guess it's because DataService is fetching changes with kind of:
git diff 7450de7..dd8c39e
that returns all differences between 7450de7 and dd8c39e (what is wrong)

instead of doing:
git diff 7450de7...dd8c39e
that returns the differences between common ancestor (83f7e4d) and dd8c39e, representing the changes made by the branch being pullrequested.

Keeping this, forces the analyzer to avoid posting comments about files that were not modified in the PR being reviewed (ie. README.md), because otherwise GitHub API will return an unhandled Path is invalid error:

{
  "message": "Unprocessable Entity",
  "errors": []github.Error{}[
    github.Error{
      Resource:"Review",
      Field:"",
      Code:"custom",
      Message:"Path is invalid"
    }
  ],
}

(described by #487)

@dpordomingo dpordomingo added the bug Something isn't working label Jan 23, 2019
@dpordomingo
Copy link
Contributor Author

I think GitHub API is returning an unhandled Path is invalid error due to google/go-github#540

@carlosms
Copy link
Contributor

Let's make sure we include a a test with this scenario in https://github.com/src-d/lookout-test-fixtures

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants