validate local repository has the correct remote (#7079)

motivation: in some edge cases, the local repository may be partially
valid (the containing directory is a legit git repo, but the repository
directory is not), or otherwise point to a remote different than the one
expected

changes:
* validate that the local repository remote aligns with the expected
one, not just that the directory is a valid git repo
* refactor validation flow to be more streamlined

Author: tomerd

Sources/SPMTestSupport/InMemoryGitRepository.swift

@@ -486,6 +486,10 @@ public final class InMemoryGitRepositoryProvider: RepositoryProvider {
         return true
     }
 
+    public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool {
+        return true
+    }
+
     public func cancel(deadline: DispatchTime) throws {
         // noop
     }

Sources/SourceControl/GitRepository.swift

@@ -210,6 +210,11 @@ public struct GitRepositoryProvider: RepositoryProvider, Cancellable {
         return result == ".git" || result == "." || result == directory.pathString
     }
 
+    public func isValidDirectory(_ directory: Basics.AbsolutePath, for repository: RepositorySpecifier) throws -> Bool {
+        let remoteURL = try self.git.run(["-C", directory.pathString, "remote", "get-url", "origin"])
+        return remoteURL == repository.url
+    }
+
     public func copy(from sourcePath: Basics.AbsolutePath, to destinationPath: Basics.AbsolutePath) throws {
         try localFileSystem.copy(from: sourcePath, to: destinationPath)
     }

Sources/SourceControl/Repository.swift

@@ -144,6 +144,9 @@ public protocol RepositoryProvider: Cancellable {
 
     /// Returns true if the directory is valid git location.
     func isValidDirectory(_ directory: AbsolutePath) throws -> Bool
+
+    /// Returns true if the directory is valid git location for the specified repository
+    func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool
 }
 
 /// Abstract repository operations.

Sources/SourceControl/RepositoryManager.swift

@@ -188,37 +188,42 @@ public class RepositoryManager: Cancellable {
     // sync func and roll the logic into the async version above
     private func lookup(
         package: PackageIdentity,
-        repository: RepositorySpecifier,
+        repository repositorySpecifier: RepositorySpecifier,
         updateStrategy: RepositoryUpdateStrategy,
         observabilityScope: ObservabilityScope,
         delegateQueue: DispatchQueue
     ) throws -> RepositoryHandle {
-        let relativePath = try repository.storagePath()
+        let relativePath = try repositorySpecifier.storagePath()
         let repositoryPath = self.path.appending(relativePath)
-        let handle = RepositoryHandle(manager: self, repository: repository, subpath: relativePath)
+        let handle = RepositoryHandle(manager: self, repository: repositorySpecifier, subpath: relativePath)
 
         // check if a repository already exists
         // errors when trying to check if a repository already exists are legitimate
         // and recoverable, and as such can be ignored
-        if (try? self.provider.repositoryExists(at: repositoryPath)) ?? false {
+        quick: if (try? self.provider.repositoryExists(at: repositoryPath)) ?? false {
             let repository = try handle.open()
+            
+            guard ((try? self.provider.isValidDirectory(repositoryPath, for: repositorySpecifier)) ?? false) else {
+                observabilityScope.emit(warning: "\(repositoryPath) is not valid git repository for '\(repositorySpecifier.location)', will fetch again.")
+                break quick
+            }
+
             // Update the repository if needed
             if self.fetchRequired(repository: repository, updateStrategy: updateStrategy) {
                 let start = DispatchTime.now()
-                
+
                 delegateQueue.async {
                     self.delegate?.willUpdate(package: package, repository: handle.repository)
                 }
-                
+
                 try repository.fetch()
                 let duration = start.distance(to: .now())
                 delegateQueue.async {
                     self.delegate?.didUpdate(package: package, repository: handle.repository, duration: duration)
                 }
-                return handle
-            } else if try self.provider.isValidDirectory(repositoryPath) {
-                return handle
             }
+
+            return handle
         }
 
         // inform delegate that we are starting to fetch
@@ -334,7 +339,7 @@ public class RepositoryManager: Cancellable {
                 }
             } catch {
                 // If we are offline and have a valid cached repository, use the cache anyway.
-                if try isOffline(error) && self.provider.isValidDirectory(cachedRepositoryPath) {
+                if try isOffline(error) && self.provider.isValidDirectory(cachedRepositoryPath, for: handle.repository) {
                     // For the first offline use in the lifetime of this repository manager, emit a warning.
                     if self.emitNoConnectivityWarning.get(default: false) {
                         self.emitNoConnectivityWarning.put(false)
@@ -426,6 +431,11 @@ public class RepositoryManager: Cancellable {
         try self.provider.isValidDirectory(directory)
     }
 
+    /// Returns true if the directory is valid git location for the specified repository
+    public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool {
+        try self.provider.isValidDirectory(directory, for: repository)
+    }
+
     /// Reset the repository manager.
     ///
     /// Note: This also removes the cloned repositories from the disk.

Tests/SourceControlTests/RepositoryManagerTests.swift

@@ -561,11 +561,134 @@ class RepositoryManagerTests: XCTestCase {
                 fatalError("should not be called")
             }
 
+            public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool {
+                fatalError("should not be called")
+            }
+
             func cancel(deadline: DispatchTime) throws {
                 print("cancel")
             }
         }
     }
+
+    func testInvalidRepositoryOnDisk() throws {
+        let fileSystem = localFileSystem
+        let observability = ObservabilitySystem.makeForTesting()
+
+        try testWithTemporaryDirectory { path in
+            let repositoriesDirectory = path.appending("repositories")
+            try fileSystem.createDirectory(repositoriesDirectory, recursive: true)
+
+            let testRepository = RepositorySpecifier(url: .init("test-\(UUID().uuidString)"))
+            let provider = MockRepositoryProvider(repository: testRepository)
+
+            let manager = RepositoryManager(
+                fileSystem: fileSystem,
+                path: repositoriesDirectory,
+                provider: provider,
+                delegate: nil
+            )
+
+            _ = try manager.lookup(repository: testRepository, observabilityScope: observability.topScope)
+            testDiagnostics(observability.diagnostics) { result in
+                result.check(
+                    diagnostic: .contains("is not valid git repository for '\(testRepository)', will fetch again"),
+                    severity: .warning
+                )
+            }
+        }
+
+        class MockRepositoryProvider: RepositoryProvider {
+            let repository: RepositorySpecifier
+            var fetch: Int = 0
+
+            init(repository: RepositorySpecifier) {
+                self.repository = repository
+            }
+
+            func fetch(repository: RepositorySpecifier, to path: AbsolutePath, progressHandler: ((FetchProgress) -> Void)?) throws {
+                assert(repository == self.repository)
+                self.fetch += 1
+            }
+
+            func repositoryExists(at path: AbsolutePath) throws -> Bool {
+                // the directory exists
+                return true
+            }
+
+            func open(repository: RepositorySpecifier, at path: AbsolutePath) throws -> Repository {
+                return MockRepository()
+            }
+
+            func createWorkingCopy(repository: RepositorySpecifier, sourcePath: AbsolutePath, at destinationPath: AbsolutePath, editable: Bool) throws -> WorkingCheckout {
+                fatalError("should not be called")
+            }
+
+            func workingCopyExists(at path: AbsolutePath) throws -> Bool {
+                fatalError("should not be called")
+            }
+
+            func openWorkingCopy(at path: AbsolutePath) throws -> WorkingCheckout {
+                fatalError("should not be called")
+            }
+
+            func copy(from sourcePath: AbsolutePath, to destinationPath: AbsolutePath) throws {
+                fatalError("should not be called")
+            }
+
+            func isValidDirectory(_ directory: AbsolutePath) throws -> Bool {
+                fatalError("should not be called")
+            }
+
+            public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool {
+                assert(repository == self.repository)
+                // the directory is not valid
+                return false
+            }
+
+            func cancel(deadline: DispatchTime) throws {
+                fatalError("should not be called")
+            }
+        }
+
+        class MockRepository: Repository {
+            func getTags() throws -> [String] {
+                fatalError("unexpected API call")
+            }
+
+            func resolveRevision(tag: String) throws -> Revision {
+                fatalError("unexpected API call")
+            }
+
+            func resolveRevision(identifier: String) throws -> Revision {
+                fatalError("unexpected API call")
+            }
+
+            func exists(revision: Revision) -> Bool {
+                fatalError("unexpected API call")
+            }
+
+            func isValidDirectory(_ directory: AbsolutePath) throws -> Bool {
+                fatalError("unexpected API call")
+            }
+
+            public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool {
+                fatalError("unexpected API call")
+            }
+
+            func fetch() throws {
+                // noop
+            }
+
+            func openFileView(revision: Revision) throws -> FileSystem {
+                fatalError("unexpected API call")
+            }
+
+            public func openFileView(tag: String) throws -> FileSystem {
+                fatalError("unexpected API call")
+            }
+        }
+    }
 }
 
 extension RepositoryManager {
@@ -613,46 +736,6 @@ private enum DummyError: Swift.Error {
     case invalidRepository
 }
 
-private class DummyRepository: Repository {
-    unowned let provider: DummyRepositoryProvider
-
-    init(provider: DummyRepositoryProvider) {
-        self.provider = provider
-    }
-
-    func getTags() throws -> [String] {
-        ["1.0.0"]
-    }
-
-    func resolveRevision(tag: String) throws -> Revision {
-        fatalError("unexpected API call")
-    }
-
-    func resolveRevision(identifier: String) throws -> Revision {
-        fatalError("unexpected API call")
-    }
-
-    func exists(revision: Revision) -> Bool {
-        fatalError("unexpected API call")
-    }
-
-    func isValidDirectory(_ directory: AbsolutePath) throws -> Bool {
-        fatalError("unexpected API call")
-    }
-
-    func fetch() throws {
-        self.provider.increaseFetchCount()
-    }
-
-    func openFileView(revision: Revision) throws -> FileSystem {
-        fatalError("unexpected API call")
-    }
-
-    public func openFileView(tag: String) throws -> FileSystem {
-        fatalError("unexpected API call")
-    }
-}
-
 private class DummyRepositoryProvider: RepositoryProvider {
     private let fileSystem: FileSystem
 
@@ -720,6 +803,10 @@ private class DummyRepositoryProvider: RepositoryProvider {
         return true
     }
 
+    func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool {
+        return true
+    }
+
     func cancel(deadline: DispatchTime) throws {
         // noop
     }
@@ -795,7 +882,7 @@ private class DummyRepositoryProvider: RepositoryProvider {
     }
 }
 
-private class DummyRepositoryManagerDelegate: RepositoryManager.Delegate {
+fileprivate class DummyRepositoryManagerDelegate: RepositoryManager.Delegate {
     private var _willFetch = ThreadSafeArrayStore<(repository: RepositorySpecifier, details: RepositoryManager.FetchDetails)>()
     private var _didFetch = ThreadSafeArrayStore<(repository: RepositorySpecifier, result: Result<RepositoryManager.FetchDetails, Error>)>()
     private var _willUpdate = ThreadSafeArrayStore<RepositorySpecifier>()
@@ -870,3 +957,47 @@ private class DummyRepositoryManagerDelegate: RepositoryManager.Delegate {
         self.group.leave()
     }
 }
+
+fileprivate class DummyRepository: Repository {
+    unowned let provider: DummyRepositoryProvider
+
+    init(provider: DummyRepositoryProvider) {
+        self.provider = provider
+    }
+
+    func getTags() throws -> [String] {
+        ["1.0.0"]
+    }
+
+    func resolveRevision(tag: String) throws -> Revision {
+        fatalError("unexpected API call")
+    }
+
+    func resolveRevision(identifier: String) throws -> Revision {
+        fatalError("unexpected API call")
+    }
+
+    func exists(revision: Revision) -> Bool {
+        fatalError("unexpected API call")
+    }
+
+    func isValidDirectory(_ directory: AbsolutePath) throws -> Bool {
+        fatalError("unexpected API call")
+    }
+
+    public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool {
+        fatalError("unexpected API call")
+    }
+
+    func fetch() throws {
+        self.provider.increaseFetchCount()
+    }
+
+    func openFileView(revision: Revision) throws -> FileSystem {
+        fatalError("unexpected API call")
+    }
+
+    public func openFileView(tag: String) throws -> FileSystem {
+        fatalError("unexpected API call")
+    }
+}

Tests/WorkspaceTests/SourceControlPackageContainerTests.swift

@@ -150,6 +150,10 @@ private class MockRepositories: RepositoryProvider {
         return true
     }
 
+    public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool {
+        return true
+    }
+
     func cancel(deadline: DispatchTime) throws {
         // noop
     }