FindMergeBase - Removing redundant commit node visits

pkg/graveler/ref/merge_base_finder.go

@@ -26,28 +26,57 @@ func FindMergeBase(ctx context.Context, getter CommitGetter, repositoryID gravel
 	reached := make(map[graveler.CommitID]reachedFlags)
 	reached[rightID] |= fromRight
 	reached[leftID] |= fromLeft
-	// create an hypothetical commit with given nodes as parents, and insert it to the queue
-	heap.Push(&queue, &graveler.CommitRecord{
-		Commit: &graveler.Commit{Parents: []graveler.CommitID{leftID, rightID}},
-	})
+	commit, err := getCommitAndEnqueue(ctx, getter, &queue, repositoryID, leftID)
+	if err != nil {
+		return nil, err
+	}
+	if leftID == rightID {
+		return commit, nil
+	}
+
+	_, err = getCommitAndEnqueue(ctx, getter, &queue, repositoryID, rightID)
+	if err != nil {
+		return nil, err
+	}
 	for {
 		if queue.Len() == 0 {
 			return nil, nil
 		}
 		commitRecord = heap.Pop(&queue).(*graveler.CommitRecord)
 		commitFlags := reached[commitRecord.CommitID]
-		if commitFlags&fromLeft != 0 && commitFlags&fromRight != 0 {
-			// commit was reached from both left and right nodes
-			return commitRecord.Commit, nil
-		}
 		for _, parent := range commitRecord.Parents {
-			parentCommit, err := getter.GetCommit(ctx, repositoryID, parent)
-			if err != nil {
-				return nil, err
+			if _, exist := reached[parent]; !exist {
+				// parent commit is queued only if it was not handled before. Otherwise it, and
+				// all its ancestors were already queued and so, will have entries in 'reached' map
+				_, err := getCommitAndEnqueue(ctx, getter, &queue, repositoryID, parent)
+				if err != nil {
+					return nil, err
+				}
 			}
-			heap.Push(&queue, &graveler.CommitRecord{CommitID: parent, Commit: parentCommit})
-			// mark the parent with the flag values from its descendents:
+			// mark the parent with the flag values from its descendents. This is done regardless
+			// of whether this parent commit is being queued in the current iteration or not. In
+			// both cases, if the 'reached' update signifies it was reached from both left and
+			// right nodes - it is the requested parent node
 			reached[parent] |= commitFlags
+			if reached[parent]&fromLeft != 0 && reached[parent]&fromRight != 0 {
+				// commit was reached from both left and right nodes
+				// Since it is not certain that the commit exist in the commits queue, a call
+				// to GetCommit is issued
+				commit, err := getter.GetCommit(ctx, repositoryID, parent)
+				if err != nil {
+					return nil, err
+				}
+				return commit, nil
+			}
 		}
 	}
 }
+
+func getCommitAndEnqueue(ctx context.Context, getter CommitGetter, queue *CommitsGenerationPriorityQueue, repositoryID graveler.RepositoryID, commitID graveler.CommitID) (*graveler.Commit, error) {
+	commit, err := getter.GetCommit(ctx, repositoryID, commitID)
+	if err != nil {
+		return nil, err
+	}
+	heap.Push(queue, &graveler.CommitRecord{CommitID: commitID, Commit: commit})
+	return commit, nil
+}