[5.9] improve validation of existing checkouts (#6973)

motivation: fix edge cases where checkouts point to the wrong repository

changes:
* update WorkingCheckout::isAlternateObjectStoreValid to take an
expected repository path
* create an API on RepositoryManager to make sure the checkout aligns
with the locall repository

rdar://112134161

Author: tomerd

Sources/SPMTestSupport/InMemoryGitRepository.swift

@@ -379,7 +379,7 @@ extension InMemoryGitRepository: WorkingCheckout {
         }
     }
 
-    public func isAlternateObjectStoreValid() -> Bool {
+    public func isAlternateObjectStoreValid(expected: AbsolutePath) -> Bool {
         return true
     }
 

Sources/SourceControl/GitRepository.swift

@@ -597,7 +597,7 @@ public final class GitRepository: Repository, WorkingCheckout {
     }
 
     /// Returns true if there is an alternative object store in the repository and it is valid.
-    public func isAlternateObjectStoreValid() -> Bool {
+    public func isAlternateObjectStoreValid(expected: AbsolutePath) -> Bool {
         let objectStoreFile = self.path.appending(components: ".git", "objects", "info", "alternates")
         guard let bytes = try? localFileSystem.readFileContents(objectStoreFile) else {
             return false
@@ -606,7 +606,11 @@ public final class GitRepository: Repository, WorkingCheckout {
         guard let firstLine = ByteString(split[0]).validDescription else {
             return false
         }
-        return (try? localFileSystem.isDirectory(AbsolutePath(validating: firstLine))) == true
+        guard let objectsPath = try? AbsolutePath(validating: firstLine), localFileSystem.isDirectory(objectsPath) else {
+            return false
+        }
+        let repositoryPath = objectsPath.parentDirectory
+        return expected == repositoryPath
     }
 
     /// Returns true if the file at `path` is ignored by `git`

Sources/SourceControl/Repository.swift

@@ -273,7 +273,7 @@ public protocol WorkingCheckout {
     func checkout(newBranch: String) throws
 
     /// Returns true if there is an alternative store in the checkout and it is valid.
-    func isAlternateObjectStoreValid() -> Bool
+    func isAlternateObjectStoreValid(expected: AbsolutePath) -> Bool
 
     /// Returns true if the file at `path` is ignored by `git`
     func areIgnored(_ paths: [AbsolutePath]) throws -> [Bool]

Sources/SourceControl/RepositoryManager.swift

@@ -361,10 +361,18 @@ public class RepositoryManager: Cancellable {
         return FetchDetails(fromCache: cacheUsed, updatedCache: cacheUpdated)
     }
 
+    /// Open a working copy checkout at a path
     public func openWorkingCopy(at path: AbsolutePath) throws -> WorkingCheckout {
         try self.provider.openWorkingCopy(at: path)
     }
 
+    /// Validate a working copy check is aligned with its repository setup
+    public func isValidWorkingCopy(_ workingCopy: WorkingCheckout, for repository: RepositorySpecifier) throws -> Bool {
+        let relativePath = try repository.storagePath()
+        let repositoryPath = self.path.appending(relativePath)
+        return workingCopy.isAlternateObjectStoreValid(expected: repositoryPath)
+    }
+
     /// Open a repository from a handle.
     private func open(_ handle: RepositoryHandle) throws -> Repository {
         try self.provider.open(

Sources/Workspace/Workspace.swift

@@ -3309,8 +3309,13 @@ extension Workspace {
         observabilityScope: ObservabilityScope
     ) throws -> AbsolutePath {
         let repository = try package.makeRepositorySpecifier()
-        // first fetch the repository.
-        let checkoutPath = try self.fetchRepository(package: package, observabilityScope: observabilityScope)
+
+        // first fetch the repository
+        let checkoutPath = try self.fetchRepository(
+            package: package,
+            at: checkoutState.revision,
+            observabilityScope: observabilityScope
+        )
 
         // Check out the given revision.
         let workingCopy = try self.repositoryManager.openWorkingCopy(at: checkoutPath)
@@ -3378,37 +3383,40 @@ extension Workspace {
     ///
     /// - Returns: The path of the local repository.
     /// - Throws: If the operation could not be satisfied.
-    private func fetchRepository(package: PackageReference, observabilityScope: ObservabilityScope) throws -> AbsolutePath {
+    private func fetchRepository(
+        package: PackageReference,
+        at revision: Revision,
+        observabilityScope: ObservabilityScope
+    ) throws -> AbsolutePath {
+        let repository = try package.makeRepositorySpecifier()
+
         // If we already have it, fetch to update the repo from its remote.
         // also compare the location as it may have changed
         if let dependency = self.state.dependencies[comparingLocation: package] {
-            let path = self.location.repositoriesCheckoutSubdirectory(for: dependency)
+            let checkoutPath = self.location.repositoriesCheckoutSubdirectory(for: dependency)
 
-            // Make sure the directory is not missing (we will have to clone again
-            // if not).
-            fetch: if self.fileSystem.isDirectory(path) {
+            // Make sure the directory is not missing (we will have to clone again if not).
+            // This can become invalid if the build directory is moved.
+            fetch: if self.fileSystem.isDirectory(checkoutPath) {
                 // Fetch the checkout in case there are updates available.
-                let workingCopy = try self.repositoryManager.openWorkingCopy(at: path)
+                let workingCopy = try self.repositoryManager.openWorkingCopy(at: checkoutPath)
 
                 // Ensure that the alternative object store is still valid.
-                //
-                // This can become invalid if the build directory is moved.
-                guard workingCopy.isAlternateObjectStoreValid() else {
+                guard try self.repositoryManager.isValidWorkingCopy(workingCopy, for: repository) else {
+                    observabilityScope.emit(debug: "working copy at '\(checkoutPath)' does not align with expected local path of '\(repository)'")
                     break fetch
                 }
 
-                // The fetch operation may update contents of the checkout, so
                 // we need do mutable-immutable dance.
-                try self.fileSystem.chmod(.userWritable, path: path, options: [.recursive, .onlyFiles])
+                try self.fileSystem.chmod(.userWritable, path: checkoutPath, options: [.recursive, .onlyFiles])
                 try workingCopy.fetch()
-                try? self.fileSystem.chmod(.userUnWritable, path: path, options: [.recursive, .onlyFiles])
+                try? self.fileSystem.chmod(.userUnWritable, path: checkoutPath, options: [.recursive, .onlyFiles])
 
-                return path
+                return checkoutPath
             }
         }
 
         // If not, we need to get the repository from the checkouts.
-        let repository = try package.makeRepositorySpecifier()
         // FIXME: this should not block
         let handle = try temp_await {
             self.repositoryManager.lookup(
@@ -3423,24 +3431,24 @@ extension Workspace {
         }
 
         // Clone the repository into the checkouts.
-        let path = self.location.repositoriesCheckoutsDirectory.appending(component: repository.basename)
+        let checkoutPath = self.location.repositoriesCheckoutsDirectory.appending(component: repository.basename)
 
         // Remove any existing content at that path.
-        try self.fileSystem.chmod(.userWritable, path: path, options: [.recursive, .onlyFiles])
-        try self.fileSystem.removeFileTree(path)
+        try self.fileSystem.chmod(.userWritable, path: checkoutPath, options: [.recursive, .onlyFiles])
+        try self.fileSystem.removeFileTree(checkoutPath)
 
         // Inform the delegate that we're about to start.
-        self.delegate?.willCreateWorkingCopy(package: package.identity, repository: handle.repository.location.description, at: path)
+        self.delegate?.willCreateWorkingCopy(package: package.identity, repository: handle.repository.location.description, at: checkoutPath)
         let start = DispatchTime.now()
 
         // Create the working copy.
-        _ = try handle.createWorkingCopy(at: path, editable: false)
-        
+        _ = try handle.createWorkingCopy(at: checkoutPath, editable: false)
+
         // Inform the delegate that we're done.
         let duration = start.distance(to: .now())
-        self.delegate?.didCreateWorkingCopy(package: package.identity, repository: handle.repository.location.description, at: path, duration: duration)
+        self.delegate?.didCreateWorkingCopy(package: package.identity, repository: handle.repository.location.description, at: checkoutPath, duration: duration)
 
-        return path
+        return checkoutPath
     }
 
     /// Removes the clone and checkout of the provided specifier.

Tests/SourceControlTests/GitRepositoryTests.swift

@@ -655,11 +655,14 @@ class GitRepositoryTests: XCTestCase {
             let checkoutRepo = try provider.createWorkingCopy(repository: repoSpec, sourcePath: testClonePath, at: checkoutPath, editable: false)
 
             // The object store should be valid.
-            XCTAssertTrue(checkoutRepo.isAlternateObjectStoreValid())
+            XCTAssertTrue(checkoutRepo.isAlternateObjectStoreValid(expected: testClonePath))
+
+            // Wrong path
+            XCTAssertFalse(checkoutRepo.isAlternateObjectStoreValid(expected: testClonePath.appending(UUID().uuidString)))
 
             // Delete the clone (alternative object store).
             try localFileSystem.removeFileTree(testClonePath)
-            XCTAssertFalse(checkoutRepo.isAlternateObjectStoreValid())
+            XCTAssertFalse(checkoutRepo.isAlternateObjectStoreValid(expected: testClonePath))
         }
     }
 

Tests/SourceControlTests/RepositoryManagerTests.swift

@@ -787,7 +787,7 @@ private class DummyRepositoryProvider: RepositoryProvider {
             fatalError("not implemented")
         }
 
-        func isAlternateObjectStoreValid() -> Bool {
+        func isAlternateObjectStoreValid(expected: AbsolutePath) -> Bool {
             fatalError("not implemented")
         }