refine location comparison when pre-loading package state from local state

motivation: when changing between canonical dependency URLs SwiftPM over-caches and ignores changes in manifest until local cache is deleted. this can lead to auth issues when switching between https and ssh for example

changes:
* take into account URL scheme when comparing dependency locations
* always prefer root dependencies when computing preferred URLs
* clean up call sites that compare dependecy location
* add and expand tests

rdar://105732543

Author: tomerd

Sources/Basics/SourceControlURL.swift

@@ -13,17 +13,21 @@
 import Foundation
 
 public struct SourceControlURL: Codable, Equatable, Hashable, Sendable {
+    private let underlying: URL?
     private let urlString: String
 
     public init(stringLiteral: String) {
+        self.underlying = URL(string: stringLiteral)
         self.urlString = stringLiteral
     }
 
     public init(_ urlString: String) {
+        self.underlying = URL(string: urlString)
         self.urlString = urlString
     }
 
     public init(_ url: URL) {
+        self.underlying = url
         self.urlString = url.absoluteString
     }
 
@@ -36,7 +40,11 @@ public struct SourceControlURL: Codable, Equatable, Hashable, Sendable {
     }
 
     public var url: URL? {
-        return URL(string: self.urlString)
+        return self.underlying
+    }
+
+    public var scheme: String? {
+        return self.underlying?.scheme
     }
 }
 

Sources/PackageGraph/PubGrub/ContainerProvider.swift

@@ -62,14 +62,14 @@ final class ContainerProvider {
         completion: @escaping (Result<PubGrubPackageContainer, Error>) -> Void
     ) {
         // Return the cached container, if available.
-        if let container = self.containersCache[package], package.equalsIncludingLocation(container.package) {
+        if let container = self.containersCache[comparingLocation: package] {
             return completion(.success(container))
         }
 
         if let prefetchSync = self.prefetches[package] {
             // If this container is already being prefetched, wait for that to complete
             prefetchSync.notify(queue: .sharedConcurrent) {
-                if let container = self.containersCache[package], package.equalsIncludingLocation(container.package) {
+                if let container = self.containersCache[comparingLocation: package] {
                     // should be in the cache once prefetch completed
                     return completion(.success(container))
                 } else {
@@ -125,3 +125,12 @@ final class ContainerProvider {
         }
     }
 }
+
+extension ThreadSafeKeyValueStore where Key == PackageReference, Value == PubGrubPackageContainer {
+    subscript(comparingLocation package: PackageReference) -> PubGrubPackageContainer? {
+        if let container = self[package], container.package.equalsIncludingLocation(package) {
+            return container
+        }
+        return .none
+    }
+}

Sources/PackageModel/PackageReference.swift

@@ -161,7 +161,19 @@ extension PackageReference: Equatable {
 
     // TODO: consider rolling into Equatable
     public func equalsIncludingLocation(_ other: PackageReference) -> Bool {
-        return self.identity == other.identity && self.canonicalLocation == other.canonicalLocation
+        if self.identity != other.identity {
+            return false
+        }
+        if self.canonicalLocation != other.canonicalLocation {
+            return false
+        }
+        switch (self.kind, other.kind) {
+        case (.remoteSourceControl(let lurl), .remoteSourceControl(let rurl)):
+            return lurl.scheme == rurl.scheme
+        default:
+            break
+        }
+        return true
     }
 }
 

Sources/Workspace/ManagedDependency.swift

@@ -196,7 +196,7 @@ extension Workspace {
 
         // When loading manifests in Workspace, there are cases where we must also compare the location
         // as it may attempt to load manifests for dependencies that have the same identity but from a different location
-        // (e.g. dependency is changed to  a fork with the same identity)
+        // (e.g. dependency is changed to a fork with the same identity)
         public subscript(comparingLocation package: PackageReference) -> ManagedDependency? {
             if let dependency = self.dependencies[package.identity], dependency.packageRef.equalsIncludingLocation(package) {
                 return dependency

Sources/Workspace/Workspace.swift

@@ -1716,8 +1716,8 @@ extension Workspace {
                 needsUpdate = true
             } else {
                 for dependency in dependenciesToPin {
-                    if let pin = storedPinStore.pins.first(where: { $0.value.packageRef.equalsIncludingLocation(dependency.packageRef) }) {
-                        if pin.value.state != PinsStore.Pin(dependency)?.state {
+                    if let pin = storedPinStore.pins[comparingLocation: dependency.packageRef] {
+                        if pin.state != PinsStore.Pin(dependency)?.state {
                             needsUpdate = true
                             break
                         }
@@ -1913,6 +1913,8 @@ extension Workspace {
                 }
             }
 
+            let topLevelDependencies = root.packages.flatMap { $1.manifest.dependencies.map { $0.packageRef } }
+
             var requiredIdentities: OrderedCollections.OrderedSet<PackageReference> = []
             _ = transitiveClosure(inputNodes) { node in
                 return node.manifest.dependenciesRequired(for: node.productFilter).compactMap{ dependency in
@@ -1924,17 +1926,26 @@ extension Workspace {
                         if existing.canonicalLocation == package.canonicalLocation {
                             // same literal location is fine
                             if existing.locationString != package.locationString {
-                                let preferred = [existing, package].sorted(by: {
-                                    $0.locationString > $1.locationString
-                                }).first! // safe
-                                observabilityScope.emit(debug: """
-                                similar variants of package '\(package.identity)' \
-                                found at '\(package.locationString)' and '\(existing.locationString)'. \
-                                using preferred variant '\(preferred.locationString)'
-                                """)
-                                if preferred.locationString != existing.locationString {
-                                    requiredIdentities.remove(existing)
-                                    requiredIdentities.insert(preferred, at: index)
+                                // we prefer the top level dependencies
+                                if topLevelDependencies.contains(where: { $0.locationString ==  existing.locationString }) {
+                                    observabilityScope.emit(debug: """
+                                    similar variants of package '\(package.identity)' \
+                                    found at '\(package.locationString)' and '\(existing.locationString)'. \
+                                    using preferred root variant '\(existing.locationString)'
+                                    """)
+                                } else {
+                                    let preferred = [existing, package].sorted(by: {
+                                        $0.locationString > $1.locationString
+                                    }).first! // safe
+                                    observabilityScope.emit(debug: """
+                                    similar variants of package '\(package.identity)' \
+                                    found at '\(package.locationString)' and '\(existing.locationString)'. \
+                                    using preferred variant '\(preferred.locationString)'
+                                    """)
+                                    if preferred.locationString != existing.locationString {
+                                        requiredIdentities.remove(existing)
+                                        requiredIdentities.insert(preferred, at: index)
+                                    }
                                 }
                             }
                         } else {
@@ -3179,9 +3190,8 @@ extension Workspace {
             // a required dependency that is already loaded (managed) should be represented in the pins store.
             // also comparing location as it may have changed at this point
             if requiredDependencies.contains(where: { $0.equalsIncludingLocation(dependency.packageRef) }) {
-                let pin = pinsStore.pins[dependency.packageRef.identity]
                 // if pin not found, or location is different (it may have changed at this point) pin it
-                if !(pin?.packageRef.equalsIncludingLocation(dependency.packageRef) ?? false) {
+                if pinsStore.pins[comparingLocation: dependency.packageRef] == .none {
                     pinsStore.pin(dependency)
                 }
             } else if let pin = pinsStore.pins[dependency.packageRef.identity]  {
@@ -3747,7 +3757,7 @@ extension Workspace {
 
                 // only update if necessary
                 if !workingCopy.exists(revision: revision) {
-                    // The fetch operation may update contents of the checkout, 
+                    // The fetch operation may update contents of the checkout,
                     // so we need to do mutable-immutable dance.
                     try self.fileSystem.chmod(.userWritable, path: checkoutPath, options: [.recursive, .onlyFiles])
                     try workingCopy.fetch()
@@ -4632,3 +4642,12 @@ extension Workspace.ManagedDependencies {
         })
     }
 }
+
+extension PinsStore.Pins {
+    subscript(comparingLocation package: PackageReference) -> PinsStore.Pin? {
+        if let pin = self[package.identity], pin.packageRef.equalsIncludingLocation(package) {
+            return pin
+        }
+        return .none
+    }
+}