improve repository cache information emmited to delegate (#3705)

motivation: fix flaky test, more accurate representaiton of cache usage information

changes:
* dont fail the cache lookup when the parent directory for the target is not found, create it instead
* more reliabily report the status of when the repositories cache is updated and used
* change the test to not rely on a side-effect of parent directory not being found, which is no longer the case
* adjust the test to the new status reporting and add another step to test the behavior when cache is poopulated

Author: tomerd

Sources/SourceControl/RepositoryManager.swift

@@ -231,8 +231,8 @@ public class RepositoryManager {
     /// - Returns: Details about the performed fetch.
     @discardableResult
     func fetchAndPopulateCache(handle: RepositoryHandle, repositoryPath: AbsolutePath) throws -> FetchDetails {
-        var updatedCache = false
-        var fromCache = false
+        var cacheUsed = false
+        var cacheUpdated = false
 
         // We are expecting handle.repository.url to always be a resolved absolute path.
         let isLocal = (try? AbsolutePath(validating: handle.repository.url)) != nil
@@ -248,29 +248,29 @@ public class RepositoryManager {
                         if (fileSystem.exists(cachedRepositoryPath)) {
                             let repo = try self.provider.open(repository: handle.repository, at: cachedRepositoryPath)
                             try repo.fetch()
+                            cacheUsed = true
                         } else {
                             try self.provider.fetch(repository: handle.repository, to: cachedRepositoryPath)
                         }
-                        updatedCache = true
+                        cacheUpdated = true
                         // Copy the repository from the cache into the repository path.
+                        try fileSystem.createDirectory(repositoryPath.parentDirectory, recursive: true)
                         try self.provider.copy(from: cachedRepositoryPath, to: repositoryPath)
-                        fromCache = true
                     }
                 }
             } catch {
+                cacheUsed = false
                 // Fetch without populating the cache in the case of an error.
                 print("Skipping cache due to an error: \(error)")
                 // It is possible that we already created the directory before failing, so clear leftover data if present.
                 try fileSystem.removeFileTree(repositoryPath)
                 try self.provider.fetch(repository: handle.repository, to: repositoryPath)
-                fromCache = false
             }
         } else {
             // Fetch without populating the cache when no `cachePath` is set.
             try self.provider.fetch(repository: handle.repository, to: repositoryPath)
-            fromCache = false
         }
-        return FetchDetails(fromCache: fromCache, updatedCache: updatedCache)
+        return FetchDetails(fromCache: cacheUsed, updatedCache: cacheUpdated)
     }
 
     public func openWorkingCopy(at path: AbsolutePath) throws -> WorkingCheckout {

Tests/SourceControlTests/RepositoryManagerTests.swift

@@ -355,8 +355,6 @@ class RepositoryManagerTests: XCTestCase {
             // Avoid a crasher that seems to happen only on macOS 10.15, but leave an environment variable for testing.
             try XCTSkipUnless(ProcessEnv.vars["SWIFTPM_ENABLE_FLAKY_REPOSITORYMANAGERTESTS"] == "1", "skipping test that sometimes crashes in CI (rdar://70540298)")
         }
-
-        try XCTSkipIf(true, "test became racy")
 
         fixture(name: "DependencyResolution/External/Simple") { prefix in
             let cachePath = prefix.appending(component: "cache")
@@ -388,6 +386,7 @@ class RepositoryManagerTests: XCTestCase {
             XCTAssertEqual(delegate.didFetch[0].fetchDetails,
                            RepositoryManager.FetchDetails(fromCache: false, updatedCache: true))
 
+            // removing the repositories path to force re-fetch
             try localFileSystem.removeFileTree(repositoriesPath)
 
             // fetch packages from the cache
@@ -398,10 +397,11 @@ class RepositoryManagerTests: XCTestCase {
             XCTAssertEqual(delegate.willFetch[1].fetchDetails,
                            RepositoryManager.FetchDetails(fromCache: true, updatedCache: false))
             XCTAssertEqual(delegate.didFetch[1].fetchDetails,
-                           RepositoryManager.FetchDetails(fromCache: false, updatedCache: true))
+                           RepositoryManager.FetchDetails(fromCache: true, updatedCache: true))
 
-            try localFileSystem.removeFileTree(repositoriesPath)
+            //  reset the state on disk
             try localFileSystem.removeFileTree(cachePath)
+            try localFileSystem.removeFileTree(repositoriesPath)
 
             // fetch packages and populate cache
             delegate.willFetchGroup?.enter()
@@ -413,6 +413,13 @@ class RepositoryManagerTests: XCTestCase {
                            RepositoryManager.FetchDetails(fromCache: false, updatedCache: false))
             XCTAssertEqual(delegate.didFetch[2].fetchDetails,
                            RepositoryManager.FetchDetails(fromCache: false, updatedCache: true))
+
+            // update packages from the cache
+            delegate.willUpdateGroup?.enter()
+            delegate.willUpdateGroup?.enter()
+            _ = try manager.lookup(repository: repo)
+            XCTAssertEqual(delegate.willUpdate[0].fileSystemIdentifier, repo.fileSystemIdentifier)
+            XCTAssertEqual(delegate.didUpdate[0].fileSystemIdentifier, repo.fileSystemIdentifier)
         }
     }