Re-implement the comparator to better perform in most common cases

Signed-off-by: Marcin Iwanicki <[email protected]>
bloomberg · Jan 8, 2020 · 54357f4 · 54357f4
1 parent 629f56e
commit 54357f4
Show file tree

Hide file tree

Showing 9 changed files with 328 additions and 167 deletions.
diff --git a/CommandTests/Generated/p1_p2_console_format_verbose.2.40a241bd.md b/CommandTests/Generated/p1_p2_console_format_verbose.2.40a241bd.md
diff --git a/CommandTests/Generated/p1_p2_json_format.2.e54c06ce.md b/CommandTests/Generated/p1_p2_json_format.2.e54c06ce.md
diff --git a/CommandTests/Generated/p1_p2_json_format_verbose.2.0e0a3eb6.md b/CommandTests/Generated/p1_p2_json_format_verbose.2.0e0a3eb6.md
diff --git a/CommandTests/Generated/p1_p2_markdown_format_verbose.2.ac528bab.md b/CommandTests/Generated/p1_p2_markdown_format_verbose.2.ac528bab.md
diff --git a/CommandTests/Generated/p1_p2_verbose.2.fe666557.md b/CommandTests/Generated/p1_p2_verbose.2.fe666557.md
diff --git a/Sources/XCDiffCore/Comparator/BuildPhasesComparator.swift b/Sources/XCDiffCore/Comparator/BuildPhasesComparator.swift
@@ -32,57 +32,144 @@ final class BuildPhasesComparator: Comparator {
         let context = ["\"\(first.name)\" target"]
         let firstDescriptors = first.buildPhases.map { $0.descriptor() }
         let secondDescriptors = second.buildPhases.map { $0.descriptor() }
-        let count = max(firstDescriptors.count, secondDescriptors.count)
-        let differentValues: [CompareResult.DifferentValues] = (0 ..< count).compactMap { index in
-            let context = "Build Phase \(index + 1)"
-            guard let firstDescriptor = firstDescriptors[safe: index] else {
-                return .init(context: context,
-                             first: nil,
-                             second: secondDescriptors[index].description)
+        let firstIdentifiers = firstDescriptors.map { $0.identifier }
+        let secondIdentifiers = secondDescriptors.map { $0.identifier }
+        let onlyInFirst = self.onlyInFirst(firstIdentifiers, secondIdentifiers)
+        let onlyInSecond = self.onlyInSecond(firstIdentifiers, secondIdentifiers)
+        let differentValues = self.differentValues(firstDescriptors, secondDescriptors)
+        return result(context: context,
+                      onlyInFirst: onlyInFirst,
+                      onlyInSecond: onlyInSecond,
+                      differentValues: differentValues)
+    }
+
+    private func differentValues(_ first: [BuildPhaseDescriptor],
+                                 _ second: [BuildPhaseDescriptor]) -> [CompareResult.DifferentValues] {
+        return differentOrder(first, second) + differentProperties(first, second)
+    }
+
+    private func differentOrder(_ first: [BuildPhaseDescriptor],
+                                _ second: [BuildPhaseDescriptor]) -> [CompareResult.DifferentValues] {
+        let firstIdentifiers = first.map { $0.identifier }
+        let secondIdentifiers = second.map { $0.identifier }
+        let commonInFirst = self.commonInFirst(firstIdentifiers, secondIdentifiers)
+        let commonInSecond = self.commonInSecond(firstIdentifiers, secondIdentifiers)
+        let count = min(commonInFirst.count, commonInSecond.count)
+        let commonFirstIdentifiers = commonInFirst[0 ..< count]
+        let commonSecondIdentifiers = commonInSecond[0 ..< count]
+        let firstValue = commonFirstIdentifiers.map { $0.description }.joined(separator: ", ")
+        let secondValue = commonSecondIdentifiers.map { $0.description }.joined(separator: ", ")
+        guard commonFirstIdentifiers == commonSecondIdentifiers else {
+            return [CompareResult.DifferentValues(context: "Different order",
+                                                  first: firstValue,
+                                                  second: secondValue)]
+        }
+        return []
+    }
+
+    private func differentProperties(_ first: [BuildPhaseDescriptor],
+                                     _ second: [BuildPhaseDescriptor]) -> [CompareResult.DifferentValues] {
+        let firstIdentifiers = Set(first.map { $0.identifier })
+        let secondIdentifiers = Set(second.map { $0.identifier })
+        let firstDescriptorsByIdentifier = Dictionary(grouping: first) { $0.identifier }
+        let secondDescriptorsByIdentifier = Dictionary(grouping: second) { $0.identifier }
+        let commonIdentifiers = firstIdentifiers.intersection(secondIdentifiers)
+        return commonIdentifiers.flatMap { identifier -> [CompareResult.DifferentValues] in
+            let firstDescriptors = firstDescriptorsByIdentifier[identifier]!
+            let secondDescriptors = secondDescriptorsByIdentifier[identifier]!
+            let count = min(firstDescriptors.count, secondDescriptors.count)
+            let result: [CompareResult.DifferentValues] = (0 ..< count).compactMap {
+                let firstDescriptor = firstDescriptors[$0]
+                let secondDescriptor = secondDescriptors[$0]
+                guard firstDescriptor == secondDescriptor else {
+                    let identifier = firstDescriptor.identifier.description
+                    let firstProperties = firstDescriptor.properties(compareTo: secondDescriptor)
+                    let secondProperties = secondDescriptor.properties(compareTo: firstDescriptor)
+                    return CompareResult.DifferentValues(context: "Different properties in \"\(identifier)\"",
+                                                         first: firstProperties,
+                                                         second: secondProperties)
+                }
+                return nil
             }
-            guard let secondDescriptor = secondDescriptors[safe: index] else {
-                return .init(context: context,
-                             first: firstDescriptors[index].description,
-                             second: nil)
+            return result
+        }
+    }
+
+    private func onlyInFirst(_ first: [BuildPhaseDescriptor.Identifier],
+                             _ second: [BuildPhaseDescriptor.Identifier]) -> [String] {
+        return second.reduce(first) { acc, value in
+            guard let index = acc.firstIndex(of: value) else {
+                return acc
             }
-            guard firstDescriptor == secondDescriptor else {
-                return .init(context: context,
-                             first: firstDescriptor.description,
-                             second: secondDescriptor.description)
+            var result = acc
+            result.remove(at: index)
+            return result
+        }.map { $0.description }
+    }
+
+    private func onlyInSecond(_ first: [BuildPhaseDescriptor.Identifier],
+                              _ second: [BuildPhaseDescriptor.Identifier]) -> [String] {
+        return onlyInFirst(second, first)
+    }
+
+    private func commonInFirst(_ first: [BuildPhaseDescriptor.Identifier],
+                               _ second: [BuildPhaseDescriptor.Identifier]) -> [BuildPhaseDescriptor.Identifier] {
+        var secondIdentifiers = second
+        var result = [BuildPhaseDescriptor.Identifier]()
+        for firstValue in first {
+            guard let index = secondIdentifiers.firstIndex(of: firstValue) else {
+                continue
             }
-            return nil
+            result.append(firstValue)
+            secondIdentifiers.remove(at: index)
         }
-        return result(context: context, differentValues: differentValues)
+        return result
+    }
+
+    private func commonInSecond(_ first: [BuildPhaseDescriptor.Identifier],
+                                _ second: [BuildPhaseDescriptor.Identifier]) -> [BuildPhaseDescriptor.Identifier] {
+        return commonInFirst(second, first)
     }
 }
 
 private extension PBXBuildPhase {
     func descriptor() -> BuildPhaseDescriptor {
-        return BuildPhaseDescriptor(name: name(),
-                                    type: buildPhase,
+        // looks like XcodeProj PBXBuildPhase.name() never returns nil
+        return BuildPhaseDescriptor(identifier: .init(name: name()!, type: buildPhase),
                                     inputFileListPaths: inputFileListPaths,
                                     outputFileListPaths: outputFileListPaths,
                                     runOnlyForDeploymentPostprocessing: runOnlyForDeploymentPostprocessing)
     }
 }
 
-private struct BuildPhaseDescriptor: Equatable, CustomStringConvertible {
-    let name: String?
-    let type: BuildPhase
+private struct BuildPhaseDescriptor: Equatable {
+    struct Identifier: Hashable, CustomStringConvertible {
+        let name: String
+        let type: BuildPhase
+
+        var description: String {
+            guard name.lowercased() != type.rawValue.lowercased() else {
+                return name
+            }
+            return "\(name) (\(type))"
+        }
+    }
+
+    let identifier: Identifier
     let inputFileListPaths: [String]?
     let outputFileListPaths: [String]?
     let runOnlyForDeploymentPostprocessing: Bool
 
-    var description: String {
+    func properties(compareTo second: BuildPhaseDescriptor) -> String {
         var elements = [String]()
-        elements.append("name = \(name ?? "nil")")
-        elements.append("type = \(type)")
-        elements.append("runOnlyForDeploymentPostprocessing = \(runOnlyForDeploymentPostprocessing)")
-        if let inputFileListPaths = inputFileListPaths {
-            elements.append("inputFileListPaths = \(inputFileListPaths.description)")
+        if runOnlyForDeploymentPostprocessing != second.runOnlyForDeploymentPostprocessing {
+            elements.append("runOnlyForDeploymentPostprocessing = \(runOnlyForDeploymentPostprocessing)")
+        }
+        if inputFileListPaths != second.inputFileListPaths {
+            elements.append("inputFileListPaths = \(inputFileListPaths?.description ?? "nil")")
         }
-        if let outputFileListPaths = outputFileListPaths {
-            elements.append("outputFileListPaths = \(outputFileListPaths.description)")
+        if outputFileListPaths != second.outputFileListPaths {
+            elements.append("outputFileListPaths = \(outputFileListPaths?.description ?? "nil")")
         }
         return elements.joined(separator: ", ")
     }