Move the 'GitRepositoryProvider' to compare urls using their canonical representation, correctly accepting results where repositories only differ by '.git' (#7741)

francescomikulis · francescomikulis · commit fa04bac4df5c · 2024-07-23T09:43:00.000-07:00
Resolving local package dependencies often outputs a warning:
&lt;path&gt; is not valid git repository for '&lt;repo&gt;', will fetch again.

The underlying issue is that updating packages with a local SCM path
dependency always fail, as the shell-based repository URL has a file://
scheme, while the absolute path string does not.

After this change we only compare the canonical URLs, which will resolve
the inconsistency of comparing strings without considering the scheme
and / or path extension.

Validating the origin of checked-out repositories no longer ensures that
the path extension (often .git) matches the repository's specifier, as
different git clients inconsistently preserve the path extension of the
remote. Moving to the CanonicalPackageURL also ensures that absolute
paths are treated as urls with a file:// scheme, matching git's
behavior.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,10 @@ Note: This is in reverse chronological order, so newer entries are added to the
 Swift 6.0
 -----------
 
+* [#7741]
+
+  Fixed an issue where repositories would be re-cloned each build rather than using the cache due to git validation errors.
+
 * [#7530]
 
   Starting from tools-version 6.0 makes it possible for packages to depend on each other if such dependency doesn't form any target-level cycles.
diff --git a/Sources/Commands/PackageCommands/ArchiveSource.swift b/Sources/Commands/PackageCommands/ArchiveSource.swift
@@ -66,8 +66,7 @@ extension SwiftPackageCommand {
         cancellator: Cancellator?
     ) throws  {
         let gitRepositoryProvider = GitRepositoryProvider()
-        if gitRepositoryProvider.repositoryExists(at: packageDirectory) &&
-            (try? gitRepositoryProvider.isValidDirectory(packageDirectory)) == true {
+        if (try? gitRepositoryProvider.isValidDirectory(packageDirectory)) == true {
             let repository = GitRepository(path: packageDirectory, cancellator: cancellator)
             try repository.archive(to: archivePath)
         } else {
diff --git a/Sources/PackageModel/PackageIdentity.swift b/Sources/PackageModel/PackageIdentity.swift
@@ -480,6 +480,7 @@ private func computeCanonicalLocation(_ string: String) -> (description: String,
 
     // Prepend a leading slash for file URLs and paths
     if detectedScheme == "file" || string.first.flatMap(isSeparator) ?? false {
+        scheme = "file"
         description.insert("/", at: description.startIndex)
     }
 
diff --git a/Sources/SPMTestSupport/InMemoryGitRepository.swift b/Sources/SPMTestSupport/InMemoryGitRepository.swift
@@ -14,6 +14,7 @@ import Basics
 import Dispatch
 import Foundation
 import SourceControl
+import struct PackageModel.CanonicalPackageURL
 
 import struct TSCBasic.ByteString
 import enum TSCBasic.FileMode
@@ -68,7 +69,7 @@ public final class InMemoryGitRepository {
     fileprivate let path: AbsolutePath
 
     /// The file system in which this repository should be installed.
-    private let fs: InMemoryFileSystem
+    fileprivate let fs: InMemoryFileSystem
 
     private let lock = NSLock()
 
@@ -438,10 +439,6 @@ public final class InMemoryGitRepositoryProvider: RepositoryProvider {
         add(specifier: RepositorySpecifier(path: path), repository: repo)
     }
 
-    public func repositoryExists(at path: AbsolutePath) throws -> Bool {
-        return fetchedMap[path] != nil
-    }
-
     public func copy(from sourcePath: AbsolutePath, to destinationPath: AbsolutePath) throws {
         guard let repo = fetchedMap[sourcePath] else {
             throw InternalError("unknown repo at \(sourcePath)")
@@ -482,11 +479,11 @@ public final class InMemoryGitRepositoryProvider: RepositoryProvider {
     }
 
     public func isValidDirectory(_ directory: AbsolutePath) throws -> Bool {
-        return true
+        return fetchedMap[directory] != nil || specifierMap.get().values.map(\.path).contains(directory)
     }
 
     public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool {
-        return true
+        return fetchedMap[directory] != nil || specifierMap[repository] != nil
     }
 
     public func cancel(deadline: DispatchTime) throws {
diff --git a/Sources/SourceControl/GitRepository.swift b/Sources/SourceControl/GitRepository.swift
@@ -15,6 +15,8 @@ import Basics
 import Dispatch
 import class Foundation.NSLock
 
+import struct PackageModel.CanonicalPackageURL
+
 import struct TSCBasic.ByteString
 import protocol TSCBasic.DiagnosticLocation
 import struct TSCBasic.FileInfo
@@ -201,18 +203,14 @@ public struct GitRepositoryProvider: RepositoryProvider, Cancellable {
         )
     }
 
-    public func repositoryExists(at directory: Basics.AbsolutePath) -> Bool {
-        return localFileSystem.isDirectory(directory)
-    }
-
     public func isValidDirectory(_ directory: Basics.AbsolutePath) throws -> Bool {
         let result = try self.git.run(["-C", directory.pathString, "rev-parse", "--git-dir"])
         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, "config", "--get", "remote.origin.url"])
-        return remoteURL == repository.url
+        return CanonicalPackageURL(remoteURL) == CanonicalPackageURL(repository.url)
     }
 
     public func copy(from sourcePath: Basics.AbsolutePath, to destinationPath: Basics.AbsolutePath) throws {
diff --git a/Sources/SourceControl/Repository.swift b/Sources/SourceControl/Repository.swift
@@ -89,9 +89,6 @@ public protocol RepositoryProvider: Cancellable {
     /// - Throws: If there is any error fetching the repository.
     func fetch(repository: RepositorySpecifier, to path: AbsolutePath, progressHandler: FetchProgress.Handler?) throws
 
-    /// Returns true if a  repository exists at `path`
-    func repositoryExists(at path: AbsolutePath) throws -> Bool
-
     /// Open the given repository.
     ///
     /// - Parameters:
diff --git a/Sources/SourceControl/RepositoryManager.swift b/Sources/SourceControl/RepositoryManager.swift
@@ -220,7 +220,7 @@ public class RepositoryManager: Cancellable {
         // 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
-        quick: if (try? self.provider.repositoryExists(at: repositoryPath)) ?? false {
+        quick: if (try? self.provider.isValidDirectory(repositoryPath)) ?? false {
             let repository = try handle.open()
 
             guard ((try? self.provider.isValidDirectory(repositoryPath, for: repositorySpecifier)) ?? false) else {
diff --git a/Tests/SourceControlTests/GitRepositoryProviderTests.swift b/Tests/SourceControlTests/GitRepositoryProviderTests.swift
@@ -16,28 +16,28 @@ import SPMTestSupport
 import XCTest
 
 class GitRepositoryProviderTests: XCTestCase {
-    func testRepositoryExists() throws {
+    func testIsValidDirectory() throws {
         try testWithTemporaryDirectory { sandbox in
             let provider = GitRepositoryProvider()
 
             // standard repository
             let repositoryPath = sandbox.appending("test")
             try localFileSystem.createDirectory(repositoryPath)
             initGitRepo(repositoryPath)
-            XCTAssertTrue(provider.repositoryExists(at: repositoryPath))
+            XCTAssertTrue(try provider.isValidDirectory(repositoryPath))
 
             // no-checkout bare repository
             let noCheckoutRepositoryPath = sandbox.appending("test-no-checkout")
             try localFileSystem.copy(from: repositoryPath.appending(".git"), to: noCheckoutRepositoryPath)
-            XCTAssertTrue(provider.repositoryExists(at: noCheckoutRepositoryPath))
+            XCTAssertTrue(try provider.isValidDirectory(noCheckoutRepositoryPath))
 
             // non-git directory
             let notGitPath = sandbox.appending("test-not-git")
-            XCTAssertFalse(provider.repositoryExists(at: notGitPath))
+            XCTAssertThrowsError(try provider.isValidDirectory(notGitPath))
 
             // non-git child directory of a git directory
             let notGitChildPath = repositoryPath.appending("test-not-git")
-            XCTAssertFalse(provider.repositoryExists(at: notGitChildPath))
+            XCTAssertThrowsError(try provider.isValidDirectory(notGitChildPath))
         }
     }
 }
diff --git a/Tests/SourceControlTests/GitRepositoryTests.swift b/Tests/SourceControlTests/GitRepositoryTests.swift
@@ -768,4 +768,142 @@ class GitRepositoryTests: XCTestCase {
             XCTAssertNoThrow(try checkoutRepo.getCurrentRevision())
         }
     }
+    
+    func testValidDirectoryLocalRelativeOrigin() async throws {
+        try testWithTemporaryDirectory { tmpDir in
+            // Create a repository.
+            let packageDir = tmpDir.appending("SomePackage")
+            try localFileSystem.createDirectory(packageDir)
+            
+            // Create a repository manager for it.
+            let repoProvider = GitRepositoryProvider()
+            let repositoryManager = RepositoryManager(
+                fileSystem: localFileSystem,
+                path: packageDir,
+                provider: repoProvider,
+                delegate: .none
+            )
+            
+            let customRemote = "../OriginOfSomePackage.git"
+            
+            // Before initializing the directory with a git repo, it is never valid.
+            XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir))
+            XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(packageDir.pathString))))
+            XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemote))))
+            
+            initGitRepo(packageDir)
+            // Set the remote.
+            try systemQuietly([Git.tool, "-C", packageDir.pathString, "remote", "add", "origin", customRemote])
+            XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir))
+            
+            let customRemoteWithoutPathExtension = (customRemote as NSString).deletingPathExtension
+            XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemote))))
+            // We consider the directory valid even if the remote does not have the same path extension - in this case we expected '.git'.
+            XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemoteWithoutPathExtension))))
+            // We consider the directory valid even if the remote does not have the same path extension - in this case we expected '.git'.
+            XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for:  RepositorySpecifier(url: SourceControlURL((customRemote as NSString).deletingPathExtension + "/"))))
+            
+            // The following ensure that are actually checking the remote's origin.
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(path: AbsolutePath(validating: "/"))))
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("/"))))
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(path: packageDir)))
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(packageDir.pathString))))
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(path: packageDir.appending(extension: "git"))))
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(packageDir.pathString.appending(".git")))))
+            
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("https://mycustomdomain/some-package.git"))))
+        }
+    }
+    
+    func testValidDirectoryLocalAbsoluteOrigin() async throws {
+        try testWithTemporaryDirectory { tmpDir in
+            // Create a repository.
+            let packageDir = tmpDir.appending("SomePackage")
+            try localFileSystem.createDirectory(packageDir)
+            
+            // Create a repository manager for it.
+            let repoProvider = GitRepositoryProvider()
+            let repositoryManager = RepositoryManager(
+                fileSystem: localFileSystem,
+                path: packageDir,
+                provider: repoProvider,
+                delegate: .none
+            )
+            
+            let customRemote = tmpDir.appending("OriginOfSomePackage.git")
+            
+            // Before initializing the directory with a git repo, it is never valid.
+            XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir))
+            XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(packageDir.pathString))))
+            XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemote.pathString))))
+            
+            initGitRepo(packageDir)
+            // Set the remote.
+            try systemQuietly([Git.tool, "-C", packageDir.pathString, "remote", "add", "origin", customRemote.pathString])
+            XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir))
+            
+            let customRemotePath = customRemote.pathString
+            let customRemotePathWithoutPathExtension = (customRemotePath as NSString).deletingPathExtension
+            XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(path: customRemote)))
+            XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemotePath))))
+            // We consider the directory valid even if the remote does not have the same path extension - in this case we expected '.git'.
+            XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(path: try AbsolutePath(validating: customRemotePathWithoutPathExtension))))
+            XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemotePathWithoutPathExtension))))
+            // We consider the directory valid even if the remote does not have the same path extension - in this case we expected '.git'.
+            XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for:  RepositorySpecifier(path: try AbsolutePath(validating: customRemotePathWithoutPathExtension + "/"))))
+            XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for:  RepositorySpecifier(url: SourceControlURL((customRemotePath as NSString).deletingPathExtension + "/"))))
+            
+            // The following ensure that are actually checking the remote's origin.
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(path: AbsolutePath(validating: "/"))))
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("/"))))
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(path: packageDir)))
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(packageDir.pathString))))
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(path: packageDir.appending(extension: "git"))))
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(packageDir.pathString.appending(".git")))))
+            
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("https://mycustomdomain/some-package.git"))))
+        }
+    }
+    
+    func testValidDirectoryRemoteOrigin() async throws {
+        try testWithTemporaryDirectory { tmpDir in
+            // Create a repository.
+            let packageDir = tmpDir.appending("SomePackage")
+            try localFileSystem.createDirectory(packageDir)
+            
+            // Create a repository manager for it.
+            let repoProvider = GitRepositoryProvider()
+            let repositoryManager = RepositoryManager(
+                fileSystem: localFileSystem,
+                path: packageDir,
+                provider: repoProvider,
+                delegate: .none
+            )
+            
+            let customRemote = try XCTUnwrap(URL(string: "https://mycustomdomain/some-package.git"))
+            
+            // Before initializing the directory with a git repo, it is never valid.
+            XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir))
+            XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemote))))
+            
+            initGitRepo(packageDir)
+            // Set the remote.
+            try systemQuietly([Git.tool, "-C", packageDir.pathString, "remote", "add", "origin", customRemote.absoluteString])
+            XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir))
+            
+            XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemote))))
+            // We consider the directory valid even if the remote does not have the same path extension - in this case we expected '.git'.
+            XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("https://mycustomdomain/some-package"))))
+            // We consider the directory valid even if the remote does not have the same path extension - in this case we expected '.git'.
+            XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for:  RepositorySpecifier(url: SourceControlURL("https://mycustomdomain/some-package/"))))
+            
+            // The following ensure that are actually checking the remote's origin.
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(path: AbsolutePath(validating: "/"))))
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("/"))))
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(path: packageDir)))
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(packageDir.pathString))))
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(path: packageDir.appending(extension: "git"))))
+            XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(packageDir.pathString.appending(".git")))))
+        }
+    }
 }
diff --git a/Tests/SourceControlTests/RepositoryManagerTests.swift b/Tests/SourceControlTests/RepositoryManagerTests.swift
diff --git a/Tests/WorkspaceTests/SourceControlPackageContainerTests.swift b/Tests/WorkspaceTests/SourceControlPackageContainerTests.swift

Original file line number	Diff line number	Diff line change
`@@ -480,6 +480,7 @@ private func computeCanonicalLocation(_ string: String) -> (description: String,`
`480`	`480`
`481`	`481`	`// Prepend a leading slash for file URLs and paths`
`482`	`482`	`if detectedScheme == "file" \|\| string.first.flatMap(isSeparator) ?? false {`
	`483`	`+ scheme = "file"`
`483`	`484`	`description.insert("/", at: description.startIndex)`
`484`	`485`	`}`
`485`	`486`
Original file line number	Diff line number	Diff line change
`@@ -15,6 +15,8 @@ import Basics`
`15`	`15`	`import Dispatch`
`16`	`16`	`import class Foundation.NSLock`
`17`	`17`
	`18`	`+import struct PackageModel.CanonicalPackageURL`
	`19`	`+`
`18`	`20`	`import struct TSCBasic.ByteString`
`19`	`21`	`import protocol TSCBasic.DiagnosticLocation`
`20`	`22`	`import struct TSCBasic.FileInfo`
`@@ -201,18 +203,14 @@ public struct GitRepositoryProvider: RepositoryProvider, Cancellable {`
`201`	`203`	`)`
`202`	`204`	`}`
`203`	`205`
`204`		`- public func repositoryExists(at directory: Basics.AbsolutePath) -> Bool {`
`205`		`- return localFileSystem.isDirectory(directory)`
`206`		`- }`
`207`		`-`
`208`	`206`	`public func isValidDirectory(_ directory: Basics.AbsolutePath) throws -> Bool {`
`209`	`207`	`let result = try self.git.run(["-C", directory.pathString, "rev-parse", "--git-dir"])`
`210`	`208`	`return result == ".git" \|\| result == "." \|\| result == directory.pathString`
`211`	`209`	`}`
`212`	`210`
`213`	`211`	`public func isValidDirectory(_ directory: Basics.AbsolutePath, for repository: RepositorySpecifier) throws -> Bool {`
`214`	`212`	`let remoteURL = try self.git.run(["-C", directory.pathString, "config", "--get", "remote.origin.url"])`
`215`		`- return remoteURL == repository.url`
	`213`	`+ return CanonicalPackageURL(remoteURL) == CanonicalPackageURL(repository.url)`
`216`	`214`	`}`
`217`	`215`
`218`	`216`	`public func copy(from sourcePath: Basics.AbsolutePath, to destinationPath: Basics.AbsolutePath) throws {`