Merge pull request #1164 from aciidb0mb3r/remote-needless-fetch

Misc improvements to Repository Manager etc

Author: aciidgh

Sources/Commands/SwiftPackageTool.swift

@@ -82,16 +82,14 @@ public class SwiftPackageTool: SwiftTool<PackageToolOptions> {
 
         case .fetch:
             diagnostics.emit(data: FetchDeprecatedDiagnostic())
-            let workspace = try getActiveWorkspace()
-            workspace.resolve(root: try getWorkspaceRoot(), diagnostics: diagnostics)
+            try resolve()
 
         case .resolve:
             let resolveOptions = options.resolveOptions
-            let workspace = try getActiveWorkspace()
-            let root = try getWorkspaceRoot()
 
             // If a package is provided, use that to resolve the dependencies.
             if let packageName = resolveOptions.packageName {
+                let workspace = try getActiveWorkspace()
                 return try workspace.resolve(
                     packageName: packageName,
                     root: getWorkspaceRoot(),
@@ -102,18 +100,16 @@ public class SwiftPackageTool: SwiftTool<PackageToolOptions> {
             }
 
             // Otherwise, run a normal resolve.
-            workspace.resolve(root: root, diagnostics: diagnostics)
+            try resolve()
 
         case .edit:
             let packageName = options.editOptions.packageName!
-            // Load the package graph.
-            try loadPackageGraph()
-
-            // Get the current workspace.
+            try resolve()
             let workspace = try getActiveWorkspace()
 
             // Create revision object if provided by user.
             let revision = options.editOptions.revision.flatMap({ Revision(identifier: $0) })
+
             // Put the dependency in edit mode.
             workspace.edit(
                 packageName: packageName,
@@ -124,9 +120,7 @@ public class SwiftPackageTool: SwiftTool<PackageToolOptions> {
 
         case .unedit:
             let packageName = options.editOptions.packageName!
-
-            // Load the package graph.
-            try loadPackageGraph()
+            try resolve()
             let workspace = try getActiveWorkspace()
 
             try workspace.unedit(packageName: packageName, forceRemove: options.editOptions.shouldForceRemove)

Sources/Commands/SwiftTool.swift

@@ -279,6 +279,18 @@ public class SwiftTool<Options: ToolOptions> {
         fatalError("Must be implmented by subclasses")
     }
 
+    /// Resolve the dependencies.
+    func resolve() throws {
+        let workspace = try getActiveWorkspace()
+        workspace.resolve(root: try getWorkspaceRoot(), diagnostics: diagnostics)
+
+        // Throw if there were errors when loading the graph.
+        // The actual errors will be printed before exiting.
+        guard !diagnostics.hasErrors else {
+            throw Error.hasFatalDiagnostics
+        }
+    }
+
     /// Fetch and load the complete package graph.
     @discardableResult
     func loadPackageGraph() throws -> PackageGraph {

Sources/PackageGraph/RepositoryPackageContainerProvider.swift

@@ -53,9 +53,17 @@ public class RepositoryPackageContainerProvider: PackageContainerProvider {
     public func getContainer(
         for identifier: Container.Identifier,
         completion: @escaping (Result<Container, AnyError>) -> Void
+    ) {
+        getContainer(for: identifier, skipUpdate: false, completion: completion)
+    }
+
+    public func getContainer(
+        for identifier: Container.Identifier,
+        skipUpdate: Bool,
+        completion: @escaping (Result<Container, AnyError>) -> Void
     ) {
         // Resolve the container using the repository manager.
-        repositoryManager.lookup(repository: identifier) { result in
+        repositoryManager.lookup(repository: identifier, skipUpdate: skipUpdate) { result in
             // Create the container wrapper.
             let container = result.mapAny { handle -> Container in
                 // Open the repository.

Sources/SourceControl/RepositoryManager.swift

@@ -203,7 +203,16 @@ public class RepositoryManager {
     ///
     /// Note: Recursive lookups are not supported i.e. calling lookup inside
     /// completion block of another lookup will block.
-    public func lookup(repository: RepositorySpecifier, completion: @escaping LookupCompletion) {
+    ///
+    /// - Parameters:
+    ///   - repository: The repository to look up.
+    ///   - skipUpdate: If a repository is availble, skip updating it.
+    ///   - completion: The completion block that should be called after lookup finishes.
+    public func lookup(
+        repository: RepositorySpecifier,
+        skipUpdate: Bool = false,
+        completion: @escaping LookupCompletion
+    ) {
         operationQueue.addOperation {
             // First look for the handle.
             let handle = self.getHandle(repository: repository)
@@ -217,6 +226,11 @@ public class RepositoryManager {
                         // Update the repository when it is being looked up.
                         let repo = try handle.open()
 
+                        // Skip update if asked to.
+                        if skipUpdate {
+                            return handle
+                        }
+
                         self.callbacksQueue.async {
                             self.delegate.handleWillUpdate(handle: handle)
                         }
@@ -300,24 +314,6 @@ public class RepositoryManager {
         }
     }
 
-    /// Synchronous variation of lookup(repository:) method.
-    public func lookupSynchronously(repository: RepositorySpecifier) throws -> RepositoryHandle {
-        let lookupCondition = Condition()
-        var result: Result<RepositoryHandle, AnyError>? = nil
-        lookup(repository: repository) { theResult in
-            lookupCondition.whileLocked {
-                result = theResult
-                lookupCondition.signal()
-            }
-        }
-        lookupCondition.whileLocked {
-            while result == nil {
-                lookupCondition.wait()
-            }
-        }
-        return try result!.dematerialize()
-    }
-
     /// Open a repository from a handle.
     private func open(_ handle: RepositoryHandle) throws -> Repository {
         return try provider.open(

Sources/Workspace/Workspace.swift

@@ -621,7 +621,9 @@ extension Workspace {
             // Otherwise, create a checkout at the destination from our repository store.
             //
             // Get handle to the repository.
-            let handle = try repositoryManager.lookupSynchronously(repository: dependency.repository)
+            let handle = try await {
+                repositoryManager.lookup(repository: dependency.repository, skipUpdate: true, completion: $0)
+            }
             let repo = try handle.open()
 
             // Do preliminary checks on branch and revision, if provided.
@@ -1077,7 +1079,9 @@ extension Workspace {
 
             case .revision(let identifier):
                 // Get the latest revision from the container.
-                let container = try await { containerProvider.getContainer(for: specifier, completion: $0) }
+                let container = try await {
+                    containerProvider.getContainer(for: specifier, skipUpdate: true, completion: $0) 
+                }
                 var revision = try container.getRevision(forIdentifier: identifier)
                 let branch = identifier == revision.identifier ? nil : identifier
 
@@ -1256,7 +1260,9 @@ extension Workspace {
         }
 
         // If not, we need to get the repository from the checkouts.
-        let handle = try repositoryManager.lookupSynchronously(repository: repository)
+        let handle = try await {
+            repositoryManager.lookup(repository: repository, skipUpdate: true, completion: $0)
+        }
 
         // Clone the repository into the checkouts.
         let path = checkoutsPath.appending(component: repository.fileSystemIdentifier)
@@ -1329,7 +1335,7 @@ extension Workspace {
         // way to get it back out of the resolver which is very
         // annoying. Maybe we should make an SPI on the provider for
         // this?
-        let container = try await { containerProvider.getContainer(for: specifier, completion: $0) }
+        let container = try await { containerProvider.getContainer(for: specifier, skipUpdate: true, completion: $0) }
         let checkoutState: CheckoutState
 
         switch requirement {

Tests/SourceControlTests/RepositoryManagerTests.swift

@@ -17,6 +17,12 @@ import TestSupport
 
 @testable import class SourceControl.RepositoryManager
 
+extension RepositoryManager {
+    fileprivate func lookupSynchronously(repository: RepositorySpecifier) throws -> RepositoryHandle {
+        return try await { self.lookup(repository: repository, completion: $0) }
+    }
+}
+
 private enum DummyError: Swift.Error {
     case invalidRepository
 }
@@ -239,25 +245,6 @@ class RepositoryManagerTests: XCTestCase {
         }
     }
 
-    func testSyncLookup() throws {
-        mktmpdir { path in
-            let provider = DummyRepositoryProvider()
-            let delegate = DummyRepositoryManagerDelegate()
-            let manager = RepositoryManager(path: path, provider: provider, delegate: delegate)
-            let dummyRepo = RepositorySpecifier(url: "dummy")
-            let handle = try manager.lookupSynchronously(repository: dummyRepo)
-            // Relookup should return same instance.
-            XCTAssert(handle === (try? manager.lookupSynchronously(repository: dummyRepo)))
-            // And async lookup should also return same instance.
-            let lookupExpectation = expectation(description: "Repository lookup expectation")
-            manager.lookup(repository: dummyRepo) { result in
-                XCTAssert(handle === (try? result.dematerialize()))
-                lookupExpectation.fulfill()
-            }
-            waitForExpectations(timeout: 1)
-        }
-    }
-
     /// Check that the manager is persistent.
     func testPersistence() {
         mktmpdir { path in
@@ -343,11 +330,42 @@ class RepositoryManagerTests: XCTestCase {
         }
     }
 
+    func testSkipUpdate() throws {
+        mktmpdir { path in
+            let repos = path.appending(component: "repo")
+            let provider = DummyRepositoryProvider()
+            let delegate = DummyRepositoryManagerDelegate()
+            try localFileSystem.createDirectory(repos, recursive: true)
+
+            let manager = RepositoryManager(path: repos, provider: provider, delegate: delegate)
+            let dummyRepo = RepositorySpecifier(url: "dummy")
+
+            _ = try await { manager.lookup(repository: dummyRepo, completion: $0) }
+            XCTAssertEqual(delegate.willFetch.count, 1)
+            XCTAssertEqual(delegate.didFetch.count, 1)
+            XCTAssertEqual(delegate.willUpdate.count, 0)
+            XCTAssertEqual(delegate.didUpdate.count, 0)
+
+            _ = try await { manager.lookup(repository: dummyRepo, completion: $0) }
+            _ = try await { manager.lookup(repository: dummyRepo, completion: $0) }
+            XCTAssertEqual(delegate.willFetch.count, 1)
+            XCTAssertEqual(delegate.didFetch.count, 1)
+            XCTAssertEqual(delegate.willUpdate.count, 2)
+            XCTAssertEqual(delegate.didUpdate.count, 2)
+
+            _ = try await { manager.lookup(repository: dummyRepo, skipUpdate: true, completion: $0) }
+            XCTAssertEqual(delegate.willFetch.count, 1)
+            XCTAssertEqual(delegate.didFetch.count, 1)
+            XCTAssertEqual(delegate.willUpdate.count, 2)
+            XCTAssertEqual(delegate.didUpdate.count, 2)
+        }
+    }
+
     static var allTests = [
         ("testBasics", testBasics),
         ("testParallelLookups", testParallelLookups),
         ("testPersistence", testPersistence),
-        ("testSyncLookup", testSyncLookup),
         ("testReset", testReset),
+        ("testSkipUpdate", testSkipUpdate),
     ]
 }