fix same branch diff should compare by checksum (#878)

treeverse · Oct 29, 2020 · f85b6e8 · f85b6e8
1 parent 9f593f3
commit f85b6e8
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 7 deletions.
diff --git a/catalog/cataloger_diff.go b/catalog/cataloger_diff.go
@@ -382,6 +382,9 @@ func getDiffDifferences(ctx context.Context, tx db.Tx, limit int, after string,
 	if err != nil {
 		return nil, fmt.Errorf("select diff results: %w", err)
 	}
+	if result == nil {
+		result = Differences{}
+	}
 	return result, nil
 }
 
@@ -732,7 +735,7 @@ func evaluateSameBranchElementDiffType(sourceEnt *DBScannerEntry, targetEnt *DBS
 		return DifferenceTypeAdded
 	}
 	// source and target not matched - change
-	if targetEnt.BranchID == sourceEnt.BranchID && targetEnt.MinCommit != sourceEnt.MinCommit {
+	if targetEnt.IsDeleted() != sourceEnt.IsDeleted() || targetEnt.Checksum != sourceEnt.Checksum {
 		return DifferenceTypeChanged
 	}
 	// entries match - none

diff --git a/catalog/cataloger_diff_test.go b/catalog/cataloger_diff_test.go
@@ -130,7 +130,7 @@ func TestCataloger_Diff_FromChild(t *testing.T) {
 	res, more, err := c.Diff(ctx, repository, "branch1", DefaultBranchName, DiffParams{Limit: -1})
 	testutil.MustDo(t, "Diff changes between branch1 and master", err)
 	if more {
-		t.Fatal("Diff has more differences, expected none")
+		t.Fatal("Diff has more than expected differences")
 	}
 	if diff := deep.Equal(res, Differences{
 		Difference{Entry: Entry{Path: "file1"}, Type: DifferenceTypeRemoved},
@@ -173,7 +173,7 @@ func TestCataloger_Diff_SameBranch(t *testing.T) {
 	res, more, err := c.Diff(ctx, repository, secondCommit.Reference, firstCommit.Reference, DiffParams{Limit: -1})
 	testutil.MustDo(t, "Diff changes from second and first commits", err)
 	if more {
-		t.Fatal("Diff has more differences, expected none")
+		t.Fatal("Diff has more than expected differences")
 	}
 	if diff := deep.Equal(res, Differences{
 		Difference{Entry: Entry{Path: "file1-" + DefaultBranchName}, Type: DifferenceTypeRemoved},
@@ -187,7 +187,7 @@ func TestCataloger_Diff_SameBranch(t *testing.T) {
 	res, more, err = c.Diff(ctx, repository, firstCommit.Reference, secondCommit.Reference, DiffParams{Limit: -1})
 	testutil.MustDo(t, "Diff changes from first and second commits", err)
 	if more {
-		t.Fatal("Diff has more differences, expected none")
+		t.Fatal("Diff has more than expected differences")
 	}
 	if diff := deep.Equal(res, Differences{
 		Difference{Entry: Entry{Path: "file1-" + DefaultBranchName}, Type: DifferenceTypeAdded},
@@ -235,7 +235,7 @@ func TestCataloger_Diff_SameBranchDiffMergedChanges(t *testing.T) {
 	res, more, err := c.Diff(ctx, repository, secondCommit.Reference, firstCommit.Reference, DiffParams{Limit: -1})
 	testutil.MustDo(t, "Diff changes from second and first commits", err)
 	if more {
-		t.Fatal("Diff has more differences, expected none")
+		t.Fatal("Diff has more than expected differences")
 	}
 	if diff := deep.Equal(res, Differences{
 		Difference{Entry: Entry{Path: "file1-" + DefaultBranchName}, Type: DifferenceTypeRemoved},
@@ -249,14 +249,30 @@ func TestCataloger_Diff_SameBranchDiffMergedChanges(t *testing.T) {
 	res, more, err = c.Diff(ctx, repository, firstCommit.Reference, secondCommit.Reference, DiffParams{Limit: -1})
 	testutil.MustDo(t, "Diff changes from first and second commits", err)
 	if more {
-		t.Fatal("Diff has more differences, expected none")
+		t.Fatal("Diff has more than expected differences")
 	}
 	if diff := deep.Equal(res, Differences{
 		Difference{Entry: Entry{Path: "file1-" + DefaultBranchName}, Type: DifferenceTypeAdded},
 		Difference{Entry: Entry{Path: "file2-" + DefaultBranchName}, Type: DifferenceTypeChanged},
 	}); diff != nil {
 		t.Fatal("Diff unexpected differences:", diff)
 	}
+
+	// rewrite a file with different content and expect to find a change in diff
+	testCatalogerCreateEntry(t, ctx, c, repository, "branch1", "file2-"+DefaultBranchName, nil, DefaultBranchName+"mod2")
+	rewriteCommit, err := c.Commit(ctx, repository, "branch1", "rewrite file2", "tester", nil)
+	testutil.MustDo(t, "rewrite file2", err)
+
+	res, more, err = c.Diff(ctx, repository, rewriteCommit.Reference, secondCommit.Reference, DiffParams{Limit: -1})
+	testutil.MustDo(t, "Diff changes from rewrite and second commits", err)
+	if more {
+		t.Fatal("Diff has more than expected differences")
+	}
+	if diff := deep.Equal(res, Differences{
+		Difference{Entry: Entry{Path: "file2-" + DefaultBranchName}, Type: DifferenceTypeChanged},
+	}); diff != nil {
+		t.Fatal("Diff unexpected differences:", diff)
+	}
 }
 
 func TestCataloger_Diff_FromChildThreeBranches(t *testing.T) {
@@ -397,7 +413,7 @@ func TestCataloger_Diff_FromParentThreeBranches(t *testing.T) {
 	res, more, err := c.Diff(ctx, repository, "master", "branch0", DiffParams{Limit: -1})
 	testutil.MustDo(t, "Diff changes from master to branch0", err)
 	if more {
-		t.Fatal("Diff has more differences, expected none")
+		t.Fatal("Diff has more than expected differences")
 	}
 	if diff := deep.Equal(res, Differences{
 		Difference{Entry: Entry{Path: "file1-" + DefaultBranchName}, Type: DifferenceTypeRemoved},