Fix GetChanges issue with changes in both sides

cmd/server-test/only_pr_changes_test.go

@@ -0,0 +1,61 @@
+// +build integration
+
+package server_test
+
+import (
+	"testing"
+	"time"
+
+	fixtures "github.com/src-d/lookout-test-fixtures"
+	"gopkg.in/src-d/lookout-sdk.v0/pb"
+
+	"github.com/stretchr/testify/suite"
+)
+
+func TestReviewOnlyPrChangesIntegrationSuite(t *testing.T) {
+	suite.Run(t, new(reviewOnlyPrChangesIntegrationSuite))
+}
+
+type reviewOnlyPrChangesIntegrationSuite struct {
+	IntegrationSuite
+}
+
+func (suite *reviewOnlyPrChangesIntegrationSuite) SetupTest() {
+	suite.ResetDB()
+
+	suite.StoppableCtx()
+	suite.r, suite.w = suite.StartLookoutd(dummyConfigFile)
+
+	suite.StartDummy("--files")
+	suite.GrepTrue(suite.r, `msg="connection state changed to 'READY'" addr="ipv4://localhost:9930" analyzer=Dummy`)
+}
+
+func (suite *reviewOnlyPrChangesIntegrationSuite) TearDownTest() {
+	// TODO: for integration tests with RabbitMQ we wait a bit so the queue
+	// is depleted. Ideally this would be done with something similar to ResetDB
+	time.Sleep(5 * time.Second)
+	suite.Stop()
+}
+
+func (suite *reviewOnlyPrChangesIntegrationSuite) TestAnalyzerErr() {
+	fixtures := fixtures.GetByName("get-changes-from-outdated-pr")
+	jsonReviewEvent := &jsonReviewEvent{
+		ReviewEvent: &pb.ReviewEvent{
+			InternalID:     "1",
+			Number:         1,
+			CommitRevision: *fixtures.GetCommitRevision(),
+		},
+	}
+
+	expectedComments := []string{
+		`{"analyzer-name":"Dummy","file":"javascript.js",`,
+		`status=success`,
+	}
+	notExpectedComments := []string{
+		`{"analyzer-name":"Dummy","file":"golang.go",`,
+	}
+
+	suite.sendEvent(jsonReviewEvent.String())
+	suite.GrepAndNotAll(suite.r, expectedComments, notExpectedComments)
+}
+

service/git/service.go

@@ -8,6 +8,7 @@ import (
 
 	errors "gopkg.in/src-d/go-errors.v1"
 	"gopkg.in/src-d/go-git.v4/plumbing/object"
+	"gopkg.in/src-d/go-git.v4"
 )
 
 // Service implements data service interface on top of go-git
@@ -50,6 +51,49 @@ func validateReferences(ctx context.Context, validateRefName bool, refs ...*look
 	return nil
 }
 
+func (r *Service) getFrom(
+	ctx context.Context,
+	base, head *lookout.ReferencePointer,
+) (
+	*lookout.ReferencePointer, error,
+) {
+	commits, err := r.loader.LoadCommits(ctx, *base, *head)
+	if err != nil {
+		return nil, err
+	}
+
+	var commonAncestor *object.Commit
+	res, err := git.MergeBase(commits[0], commits[1])
+	if err != nil {
+		return nil, err
+	}
+
+	if len(res) == 0 {
+		// If there is no common ancestor between two commits it means that they
+		// don't have a common history. That PR won't be able to be created in
+		// GitHub, but in other situations (ie. with lookout-sdk), an analyzer
+		// could be interested in analyzing the difference between both commints.
+		return base, nil
+	}
+
+	commonAncestor = res[0]
+
+	/*
+		// TODO(dpordomingo): uncomment this after testing that it's not a problem
+		// returning commits without ReferenceName (see comment below)
+		if base.Hash == commonAncestor.Hash.String() {
+			return base, nil
+		}
+	*/
+
+	return &lookout.ReferencePointer{
+		InternalRepositoryURL: base.InternalRepositoryURL,
+		// ReferenceName can be undefined for a random commit inside that repository
+		ReferenceName: "",
+		Hash:          commonAncestor.Hash.String(),
+	}, nil
+}
+
 // GetChanges returns a ChangeScanner that scans all changes according to the request.
 func (r *Service) GetChanges(ctx context.Context, req *lookout.ChangesRequest) (
 	lookout.ChangeScanner, error) {
@@ -58,7 +102,25 @@ func (r *Service) GetChanges(ctx context.Context, req *lookout.ChangesRequest) (
 		return nil, err
 	}
 
-	base, head, err := r.loadTrees(ctx, req.Base, req.Head)
+	var from *lookout.ReferencePointer
+
+	// The standard behavior for getting the changes between two commits is like doing
+	// `git diff base...head`, also `git diff $(git merge-base base head) head`
+	// (as it appears in `Changes` tab in GitHub PRs)
+	// If it is desired to get all changes between `base` and `head`,
+	// (as done by `git diff base..head`) it must be sent `req.TwoDotsMode` as true
+	if req.TwoDotsMode {
+		from = req.Base
+	} else {
+		if req.Base != nil {
+			from, err = r.getFrom(ctx, req.Base, req.Head)
+			if err != nil {
+				return nil, err
+			}
+		}
+	}
+
+	base, head, err := r.loadTrees(ctx, from, req.Head)
 	if err != nil {
 		return nil, err
 	}
@@ -111,8 +173,6 @@ func (r *Service) GetFiles(ctx context.Context, req *lookout.FilesRequest) (
 	return scanner, nil
 }
 
-const maxResolveLength = 20
-
 func (r *Service) loadTrees(ctx context.Context,
 	base, head *lookout.ReferencePointer) (*object.Tree, *object.Tree, error) {