child diff simplify check - compare delete and content first (#845)

treeverse · Oct 21, 2020 · 6c6351b · 6c6351b
1 parent 36a5a8d
commit 6c6351b
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 39 deletions.
diff --git a/catalog/cataloger_diff.go b/catalog/cataloger_diff.go
@@ -398,7 +398,13 @@ func (c *cataloger) diffFromChild(ctx context.Context, tx db.Tx, params *diffPar
 			return fmt.Errorf("scan next parent element: %w", err)
 		}
 
-		diffType := evaluateFromChildElementDiffType(effectiveCommits, params.RightBranchID, childEnt, parentEnt)
+		// point to matched parent entry
+		var matchedParent *DBScannerEntry
+		if parentEnt != nil && parentEnt.Path == childEnt.Path {
+			matchedParent = parentEnt
+		}
+
+		diffType := evaluateFromChildElementDiffType(effectiveCommits, childEnt, matchedParent)
 		if diffType == DifferenceTypeNone {
 			continue
 		}
@@ -446,48 +452,38 @@ func createDiffResultsTable(ctx context.Context, executor sq.Execer) (string, er
 	return diffResultsTableName, nil
 }
 
-func evaluateFromChildElementDiffType(effectiveCommits *diffEffectiveCommits, parentBranchID int64, childEnt *DBScannerEntry, parentEnt *DBScannerEntry) DifferenceType {
-	var matchedParent *DBScannerEntry
-	if parentEnt != nil && parentEnt.Path == childEnt.Path {
-		matchedParent = parentEnt
+func evaluateFromChildElementDiffType(effectiveCommits *diffEffectiveCommits, childEnt *DBScannerEntry, matchedParent *DBScannerEntry) DifferenceType {
+	// both deleted - none
+	if childEnt.IsDeleted() && (matchedParent == nil || matchedParent.IsDeleted()) {
+		return DifferenceTypeNone
 	}
-	// when the entry was deleted
-	if childEnt.IsDeleted() {
-		if matchedParent == nil || matchedParent.IsDeleted() {
-			return DifferenceTypeNone
-		}
-		if matchedParent.BranchID == parentBranchID {
-			// check if parent did any change to the entity
-			if matchedParent.MinCommit > effectiveCommits.ParentEffectiveCommit {
-				return DifferenceTypeConflict
-			}
-			return DifferenceTypeRemoved
-		}
-		// check if the parent saw this entry at the time
-		if matchedParent.MinCommit >= effectiveCommits.ParentEffectiveCommitByBranchID(matchedParent.BranchID) {
-			return DifferenceTypeRemoved
-		}
+
+	// same entry - none
+	if matchedParent != nil && childEnt.IsDeleted() == matchedParent.IsDeleted() && childEnt.Checksum == matchedParent.Checksum {
 		return DifferenceTypeNone
 	}
-	// when the entry was not deleted
+
+	// both entries are not deleted or point to the same content
 	if matchedParent != nil {
-		if matchedParent.IsDeleted() {
-			return DifferenceTypeAdded
-		}
-		if matchedParent.BranchID == parentBranchID {
-			// check if parent did any change to the entity
-			if matchedParent.MinCommit > effectiveCommits.ParentEffectiveCommit {
-				return DifferenceTypeConflict
-			}
-			return DifferenceTypeChanged
-		}
-		if matchedParent.MinCommit >= effectiveCommits.ParentEffectiveCommitByBranchID(matchedParent.BranchID) {
-			return DifferenceTypeChanged
+		// matched target was updated after client - conflict
+		effectiveCommitID := effectiveCommits.ParentEffectiveCommitByBranchID(matchedParent.BranchID)
+		if effectiveCommitID > UncommittedID && matchedParent.MinCommit > effectiveCommitID {
+			return DifferenceTypeConflict
 		}
+	}
+
+	// source deleted - removed
+	if childEnt.IsDeleted() {
+		return DifferenceTypeRemoved
+	}
+
+	// if target deleted - added
+	if matchedParent == nil || matchedParent.IsDeleted() {
 		return DifferenceTypeAdded
 	}
 
-	return DifferenceTypeAdded
+	// if target found - changed
+	return DifferenceTypeChanged
 }
 
 func insertDiffResultsBatch(exerciser sq.Execer, tableName string, batch []*diffResultRecord) error {

diff --git a/catalog/cataloger_merge_test.go b/catalog/cataloger_merge_test.go
@@ -701,9 +701,7 @@ func TestCataloger_Merge_FromChildNewEntrySameEntry(t *testing.T) {
 		t.Fatal("merge commit log should have two parents")
 	}
 
-	if diff := deep.Equal(res.Summary, map[DifferenceType]int{
-		DifferenceTypeChanged: 1,
-	}); diff != nil {
+	if diff := deep.Equal(res.Summary, map[DifferenceType]int{}); diff != nil {
 		t.Fatal("Merge Summary", diff)
 	}
 	// TODO(barak): enable test after diff between commits is supported
@@ -1020,7 +1018,7 @@ func TestCataloger_Merge_FromParentThreeBranchesExtended1(t *testing.T) {
 	_, err = c.Commit(ctx, repository, "branch2", "commit file0 creation", "tester", nil)
 	testutil.MustDo(t, "commit file0 creation to branch2", err)
 
-	testCatalogerCreateEntry(t, ctx, c, repository, "master", "/file111", nil, "seed1")
+	testCatalogerCreateEntry(t, ctx, c, repository, "master", "/file111", nil, "seed2")
 	_, err = c.Merge(ctx, repository, "branch2", "branch1", "tester", "pushing /file111 down", nil)
 	testutil.MustDo(t, "merge branch2 to branch1", err)