From 76008c332a57172969afd2fb81e7eb462ba3a938 Mon Sep 17 00:00:00 2001 From: Francesco Mikulis-Borsoi Date: Mon, 1 Jul 2024 09:51:32 -0700 Subject: [PATCH 01/11] Move the 'GitRepositoryProvider' to compare urls using their canonical representation, correctly accepting results where repositories only differ by '.git'. --- Sources/SourceControl/GitRepository.swift | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/Sources/SourceControl/GitRepository.swift b/Sources/SourceControl/GitRepository.swift index 1d8a8f7e09b..25159ff10ed 100644 --- a/Sources/SourceControl/GitRepository.swift +++ b/Sources/SourceControl/GitRepository.swift @@ -15,6 +15,9 @@ import Basics import Dispatch import class Foundation.NSLock +import struct PackageModel.CanonicalPackageLocation +import struct PackageModel.CanonicalPackageURL + import struct TSCBasic.ByteString import protocol TSCBasic.DiagnosticLocation import struct TSCBasic.FileInfo @@ -207,12 +210,21 @@ public struct GitRepositoryProvider: RepositoryProvider, Cancellable { 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 + return result == ".git" || result == "." + // Compare the canonical representation, which will drop any suffix + || CanonicalPackageLocation(result) == CanonicalPackageLocation(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 + switch repository.location { + case .url(let url): + // Compare the canonical representation, which will drop any suffix and canonicalize scp-style urls + return CanonicalPackageURL(remoteURL) == CanonicalPackageURL(url.absoluteString) + case .path(let absolutePath): + // Compare the canonical representation, which will drop any suffix + return CanonicalPackageLocation(remoteURL) == CanonicalPackageLocation(absolutePath.pathString) + } } public func copy(from sourcePath: Basics.AbsolutePath, to destinationPath: Basics.AbsolutePath) throws { From 27a47a2b162d04bb2d1756238b7283c7b6d82547 Mon Sep 17 00:00:00 2001 From: Francesco Mikulis-Borsoi Date: Fri, 5 Jul 2024 16:41:12 -0700 Subject: [PATCH 02/11] Added tests for the 'isValidDirectory' function of the RepositoryProvider. Clarified the API surface and reduced the change to only ignore path extensions for local (path-based) dependencies. --- Sources/SourceControl/GitRepository.swift | 11 +-- Sources/SourceControl/Repository.swift | 2 +- Sources/SourceControl/RepositoryManager.swift | 11 ++- .../InMemoryGitRepository.swift | 2 +- .../PackageLoadingTests/PDLoadingTests.swift | 1 + .../GitRepositoryTests.swift | 62 +++++++++++++++++ .../RepositoryManagerTests.swift | 12 ++-- .../SourceControlPackageContainerTests.swift | 68 ------------------- 8 files changed, 83 insertions(+), 86 deletions(-) diff --git a/Sources/SourceControl/GitRepository.swift b/Sources/SourceControl/GitRepository.swift index 25159ff10ed..f445d57e88b 100644 --- a/Sources/SourceControl/GitRepository.swift +++ b/Sources/SourceControl/GitRepository.swift @@ -215,16 +215,9 @@ public struct GitRepositoryProvider: RepositoryProvider, Cancellable { || CanonicalPackageLocation(result) == CanonicalPackageLocation(directory.pathString) } - public func isValidDirectory(_ directory: Basics.AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { + public func isValidDirectory(_ directory: Basics.AbsolutePath, for repository: SourceControlURL) throws -> Bool { let remoteURL = try self.git.run(["-C", directory.pathString, "config", "--get", "remote.origin.url"]) - switch repository.location { - case .url(let url): - // Compare the canonical representation, which will drop any suffix and canonicalize scp-style urls - return CanonicalPackageURL(remoteURL) == CanonicalPackageURL(url.absoluteString) - case .path(let absolutePath): - // Compare the canonical representation, which will drop any suffix - return CanonicalPackageLocation(remoteURL) == CanonicalPackageLocation(absolutePath.pathString) - } + return remoteURL == repository.absoluteString } public func copy(from sourcePath: Basics.AbsolutePath, to destinationPath: Basics.AbsolutePath) throws { diff --git a/Sources/SourceControl/Repository.swift b/Sources/SourceControl/Repository.swift index 0f4bd4eaf75..b4c892d62ea 100644 --- a/Sources/SourceControl/Repository.swift +++ b/Sources/SourceControl/Repository.swift @@ -146,7 +146,7 @@ public protocol RepositoryProvider: Cancellable { 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 + func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool } /// Abstract repository operations. diff --git a/Sources/SourceControl/RepositoryManager.swift b/Sources/SourceControl/RepositoryManager.swift index eee9a3e077b..ffaadacb97c 100644 --- a/Sources/SourceControl/RepositoryManager.swift +++ b/Sources/SourceControl/RepositoryManager.swift @@ -452,7 +452,7 @@ public class RepositoryManager: Cancellable { } /// Returns true if the directory is valid git location for the specified repository - public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { + public func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool { try self.provider.isValidDirectory(directory, for: repository) } @@ -512,6 +512,15 @@ public class RepositoryManager: Cancellable { } } +extension RepositoryProvider { + fileprivate func isValidDirectory(_ directory: AbsolutePath, for repositorySpecifier: RepositorySpecifier) throws -> Bool { + switch repositorySpecifier.location { + case .path: return try isValidDirectory(directory) + case .url(let url): return try isValidDirectory(directory, for: url) + } + } +} + extension RepositoryManager { /// Handle to a managed repository. public struct RepositoryHandle { diff --git a/Sources/_InternalTestSupport/InMemoryGitRepository.swift b/Sources/_InternalTestSupport/InMemoryGitRepository.swift index c7fc4c4abba..9a4d735f3c2 100644 --- a/Sources/_InternalTestSupport/InMemoryGitRepository.swift +++ b/Sources/_InternalTestSupport/InMemoryGitRepository.swift @@ -485,7 +485,7 @@ public final class InMemoryGitRepositoryProvider: RepositoryProvider { return true } - public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { + public func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool { return true } diff --git a/Tests/PackageLoadingTests/PDLoadingTests.swift b/Tests/PackageLoadingTests/PDLoadingTests.swift index b3970079313..6d2f5381661 100644 --- a/Tests/PackageLoadingTests/PDLoadingTests.swift +++ b/Tests/PackageLoadingTests/PDLoadingTests.swift @@ -13,6 +13,7 @@ import Basics import PackageLoading import PackageModel +import SourceControl import _InternalTestSupport import XCTest diff --git a/Tests/SourceControlTests/GitRepositoryTests.swift b/Tests/SourceControlTests/GitRepositoryTests.swift index 3ab25a71f65..7d1547be2e3 100644 --- a/Tests/SourceControlTests/GitRepositoryTests.swift +++ b/Tests/SourceControlTests/GitRepositoryTests.swift @@ -768,4 +768,66 @@ class GitRepositoryTests: XCTestCase { XCTAssertNoThrow(try checkoutRepo.getCurrentRevision()) } } + + func testValidDirectoryLocal() 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 + ) + + // Before initializing the directory with a git repo, it is never valid. + XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir)) + initGitRepo(packageDir) + + XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir)) + XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: SourceControlURL(packageDir.pathString))) + XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: SourceControlURL(URL(packageDir.pathString + "/")))) + XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: SourceControlURL("/"))) + XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: SourceControlURL("https://mycustomdomain/some-package.git"))) + } + } + + func testValidDirectoryRemote() 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: 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: SourceControlURL(customRemote))) + XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: SourceControlURL("/"))) + // We consider the directory invalid if the remote does not have the same path extension - in this case we expect '.git'. + XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: SourceControlURL("https://mycustomdomain/some-package"))) + // We consider the directory invalid if the remote does not have the same path extension - in this case we expect '.git'. + XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: SourceControlURL("https://mycustomdomain/some-package/"))) + } + } } diff --git a/Tests/SourceControlTests/RepositoryManagerTests.swift b/Tests/SourceControlTests/RepositoryManagerTests.swift index 00381fd9367..a90ef11d7d6 100644 --- a/Tests/SourceControlTests/RepositoryManagerTests.swift +++ b/Tests/SourceControlTests/RepositoryManagerTests.swift @@ -556,7 +556,7 @@ class RepositoryManagerTests: XCTestCase { fatalError("should not be called") } - public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { + public func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool { fatalError("should not be called") } @@ -635,8 +635,8 @@ class RepositoryManagerTests: XCTestCase { fatalError("should not be called") } - public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { - assert(repository == self.repository) + public func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool { + assert(RepositorySpecifier(url: repository) == self.repository) // the directory is not valid return false } @@ -667,7 +667,7 @@ class RepositoryManagerTests: XCTestCase { fatalError("unexpected API call") } - public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { + public func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool { fatalError("unexpected API call") } @@ -798,7 +798,7 @@ private class DummyRepositoryProvider: RepositoryProvider { return true } - func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { + func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool { return true } @@ -980,7 +980,7 @@ fileprivate class DummyRepository: Repository { fatalError("unexpected API call") } - public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { + public func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool { fatalError("unexpected API call") } diff --git a/Tests/WorkspaceTests/SourceControlPackageContainerTests.swift b/Tests/WorkspaceTests/SourceControlPackageContainerTests.swift index f21afdb77ec..abc5395e39a 100644 --- a/Tests/WorkspaceTests/SourceControlPackageContainerTests.swift +++ b/Tests/WorkspaceTests/SourceControlPackageContainerTests.swift @@ -89,74 +89,6 @@ private class MockRepository: Repository { } } -private class MockRepositories: RepositoryProvider { - /// The known repositories, as a map of URL to repository. - let repositories: [RepositorySpecifier.Location: MockRepository] - var fetchRepositories = ThreadSafeKeyValueStore() - - /// A mock manifest loader for all repositories. - let manifestLoader: MockManifestLoader - - init(repositories repositoryList: [MockRepository]) { - var allManifests: [MockManifestLoader.Key: Manifest] = [:] - var repositories: [RepositorySpecifier.Location: MockRepository] = [:] - for repository in repositoryList { - assert(repositories.index(forKey: repository.location) == nil) - repositories[repository.location] = repository - for (version, manifest) in repository.versions { - allManifests[MockManifestLoader.Key(url: repository.location.description, version: version)] = manifest - } - } - - self.repositories = repositories - self.manifestLoader = MockManifestLoader(manifests: allManifests) - } - - func fetch(repository: RepositorySpecifier, to path: AbsolutePath, progressHandler: FetchProgress.Handler? = nil) throws { - assert(self.repositories.index(forKey: repository.location) != nil) - self.fetchRepositories[repository] = path - } - - func repositoryExists(at path: AbsolutePath) throws -> Bool { - self.fetchRepositories.get().contains(where: { key, value in value == path }) - } - - func copy(from sourcePath: AbsolutePath, to destinationPath: AbsolutePath) throws { - // No-op. - } - - func workingCopyExists(at path: AbsolutePath) throws -> Bool { - return false - } - - func open(repository: RepositorySpecifier, at path: AbsolutePath) throws -> Repository { - guard let repository = self.repositories[repository.location] else { - throw InternalError("unknown repository at \(repository.location)") - } - return repository - } - - func createWorkingCopy(repository: RepositorySpecifier, sourcePath: AbsolutePath, at destinationPath: AbsolutePath, editable: Bool) throws -> WorkingCheckout { - fatalError("unexpected API call") - } - - func openWorkingCopy(at path: AbsolutePath) throws -> WorkingCheckout { - fatalError("unexpected API call") - } - - func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { - return true - } - - public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { - return true - } - - func cancel(deadline: DispatchTime) throws { - // noop - } -} - private class MockRepositoryManagerDelegate: RepositoryManager.Delegate { var fetched = ThreadSafeArrayStore() var updated = ThreadSafeArrayStore() From 096755c0c2392f048f4cbd4bdc96b63060517a58 Mon Sep 17 00:00:00 2001 From: Francesco Mikulis-Borsoi Date: Thu, 11 Jul 2024 15:45:35 -0700 Subject: [PATCH 03/11] Removed 'repositoryExists(at:)' in favor of 'isValidDirectory(_:)'. Reintroduced the 'RepositorySpecifier' and moved to using 'CanonicalPackageURL', where paths now have a scheme of 'file'. --- .../PackageCommands/ArchiveSource.swift | 3 +- .../ManifestLoader+Validation.swift | 16 ++--- Sources/PackageMetadata/PackageMetadata.swift | 2 +- Sources/PackageModel/PackageIdentity.swift | 1 + Sources/SourceControl/GitRepository.swift | 20 +++---- Sources/SourceControl/Repository.swift | 16 +++-- Sources/SourceControl/RepositoryManager.swift | 24 ++------ .../InMemoryGitRepository.swift | 12 ++-- .../PackageLoadingTests/PDLoadingTests.swift | 2 +- .../GitRepositoryProviderTests.swift | 10 ++-- .../GitRepositoryTests.swift | 31 +++++----- .../RepositoryManagerTests.swift | 58 +++++++------------ 12 files changed, 77 insertions(+), 118 deletions(-) diff --git a/Sources/Commands/PackageCommands/ArchiveSource.swift b/Sources/Commands/PackageCommands/ArchiveSource.swift index 01d2b7695ef..d962b9b10bb 100644 --- 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 gitRepositoryProvider.isValidDirectory(packageDirectory) { let repository = GitRepository(path: packageDirectory, cancellator: cancellator) try repository.archive(to: archivePath) } else { diff --git a/Sources/PackageLoading/ManifestLoader+Validation.swift b/Sources/PackageLoading/ManifestLoader+Validation.swift index 61cd9f934bb..802e34d111a 100644 --- a/Sources/PackageLoading/ManifestLoader+Validation.swift +++ b/Sources/PackageLoading/ManifestLoader+Validation.swift @@ -267,15 +267,11 @@ public struct ManifestValidator { // there is a case to be made to throw early (here) if the path does not exists // but many of our tests assume they can pass a non existent path if case .local(let localPath) = dependency.location, self.fileSystem.exists(localPath) { - do { - if try !self.sourceControlValidator.isValidDirectory(localPath) { - // Provides better feedback when mistakenly using url: for a dependency that - // is a local package. Still allows for using url with a local package that has - // also been initialized by git - diagnostics.append(.invalidSourceControlDirectory(localPath)) - } - } catch { - diagnostics.append(.invalidSourceControlDirectory(localPath, underlyingError: error)) + if !self.sourceControlValidator.isValidDirectory(localPath) { + // Provides better feedback when mistakenly using url: for a dependency that + // is a local package. Still allows for using url with a local package that has + // also been initialized by git + diagnostics.append(.invalidSourceControlDirectory(localPath)) } } return diagnostics @@ -283,7 +279,7 @@ public struct ManifestValidator { } public protocol ManifestSourceControlValidator { - func isValidDirectory(_ path: AbsolutePath) throws -> Bool + func isValidDirectory(_ path: AbsolutePath) -> Bool } extension Basics.Diagnostic { diff --git a/Sources/PackageMetadata/PackageMetadata.swift b/Sources/PackageMetadata/PackageMetadata.swift index ea2249a2623..5435ecb47a3 100644 --- a/Sources/PackageMetadata/PackageMetadata.swift +++ b/Sources/PackageMetadata/PackageMetadata.swift @@ -253,7 +253,7 @@ public struct PackageSearchClient { to: tempPath, progressHandler: nil ) - if try self.repositoryProvider.isValidDirectory(tempPath), + if self.repositoryProvider.isValidDirectory(tempPath), let repository = try self.repositoryProvider.open( repository: repositorySpecifier, at: tempPath diff --git a/Sources/PackageModel/PackageIdentity.swift b/Sources/PackageModel/PackageIdentity.swift index d34aca1a797..54f53a35da7 100644 --- 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/SourceControl/GitRepository.swift b/Sources/SourceControl/GitRepository.swift index f445d57e88b..c945ac7af52 100644 --- a/Sources/SourceControl/GitRepository.swift +++ b/Sources/SourceControl/GitRepository.swift @@ -204,20 +204,18 @@ 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 == "." - // Compare the canonical representation, which will drop any suffix - || CanonicalPackageLocation(result) == CanonicalPackageLocation(directory.pathString) + public func isValidDirectory(_ directory: Basics.AbsolutePath) -> Bool { + do { + _ = try self.git.run(["-C", directory.pathString, "rev-parse", "--git-dir"]) + return true + } catch { + return false + } } - public func isValidDirectory(_ directory: Basics.AbsolutePath, for repository: SourceControlURL) throws -> Bool { + 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.absoluteString + return CanonicalPackageURL(remoteURL) == 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 index b4c892d62ea..ba7ce4729da 100644 --- a/Sources/SourceControl/Repository.swift +++ b/Sources/SourceControl/Repository.swift @@ -11,6 +11,7 @@ //===----------------------------------------------------------------------===// import Basics +import struct PackageModel.CanonicalPackageURL import Foundation /// Specifies a repository address. @@ -32,10 +33,10 @@ public struct RepositorySpecifier: Hashable, Sendable { } /// The location of the repository as string. - public var url: String { + public var url: CanonicalPackageURL { switch self.location { - case .path(let path): return path.pathString - case .url(let url): return url.absoluteString + case .path(let path): return CanonicalPackageURL(path.pathString) + case .url(let url): return CanonicalPackageURL(url.absoluteString) } } @@ -43,7 +44,7 @@ public struct RepositorySpecifier: Hashable, Sendable { public var basename: String { // FIXME: this might be wrong //var basename = self.url.pathComponents.dropFirst(1).last(where: { !$0.isEmpty }) ?? "" - var basename = (self.url as NSString).lastPathComponent + var basename = (self.url.description as NSString).lastPathComponent if basename.hasSuffix(".git") { basename = String(basename.dropLast(4)) } @@ -89,9 +90,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: @@ -143,10 +141,10 @@ public protocol RepositoryProvider: Cancellable { func copy(from sourcePath: AbsolutePath, to destinationPath: AbsolutePath) throws /// Returns true if the directory is valid git location. - func isValidDirectory(_ directory: AbsolutePath) throws -> Bool + func isValidDirectory(_ directory: AbsolutePath) -> Bool /// Returns true if the directory is valid git location for the specified repository - func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool + func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool } /// Abstract repository operations. diff --git a/Sources/SourceControl/RepositoryManager.swift b/Sources/SourceControl/RepositoryManager.swift index ffaadacb97c..98bcfd03269 100644 --- a/Sources/SourceControl/RepositoryManager.swift +++ b/Sources/SourceControl/RepositoryManager.swift @@ -220,14 +220,9 @@ 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 { + if ((try? self.provider.isValidDirectory(repositoryPath, for: repositorySpecifier)) ?? 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() @@ -244,6 +239,8 @@ public class RepositoryManager: Cancellable { } return handle + } else { + observabilityScope.emit(warning: "\(repositoryPath) is not valid git repository for '\(repositorySpecifier.location)', will fetch again.") } // inform delegate that we are starting to fetch @@ -447,12 +444,12 @@ public class RepositoryManager: Cancellable { } /// Returns true if the directory is valid git location. - public func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { - try self.provider.isValidDirectory(directory) + public func isValidDirectory(_ directory: AbsolutePath) -> Bool { + self.provider.isValidDirectory(directory) } /// Returns true if the directory is valid git location for the specified repository - public func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool { + public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { try self.provider.isValidDirectory(directory, for: repository) } @@ -512,15 +509,6 @@ public class RepositoryManager: Cancellable { } } -extension RepositoryProvider { - fileprivate func isValidDirectory(_ directory: AbsolutePath, for repositorySpecifier: RepositorySpecifier) throws -> Bool { - switch repositorySpecifier.location { - case .path: return try isValidDirectory(directory) - case .url(let url): return try isValidDirectory(directory, for: url) - } - } -} - extension RepositoryManager { /// Handle to a managed repository. public struct RepositoryHandle { diff --git a/Sources/_InternalTestSupport/InMemoryGitRepository.swift b/Sources/_InternalTestSupport/InMemoryGitRepository.swift index 9a4d735f3c2..5d7bb809555 100644 --- a/Sources/_InternalTestSupport/InMemoryGitRepository.swift +++ b/Sources/_InternalTestSupport/InMemoryGitRepository.swift @@ -438,10 +438,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)") @@ -481,12 +477,12 @@ public final class InMemoryGitRepositoryProvider: RepositoryProvider { return checkout } - public func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { - return true + public func isValidDirectory(_ directory: AbsolutePath) -> Bool { + return fetchedMap[directory] != nil } - public func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool { - return true + public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { + return fetchedMap[directory] != nil } public func cancel(deadline: DispatchTime) throws { diff --git a/Tests/PackageLoadingTests/PDLoadingTests.swift b/Tests/PackageLoadingTests/PDLoadingTests.swift index 6d2f5381661..1c8f2d8a1bc 100644 --- a/Tests/PackageLoadingTests/PDLoadingTests.swift +++ b/Tests/PackageLoadingTests/PDLoadingTests.swift @@ -176,7 +176,7 @@ final class ManifestTestDelegate: ManifestLoaderDelegate { } fileprivate struct NOOPManifestSourceControlValidator: ManifestSourceControlValidator { - func isValidDirectory(_ path: AbsolutePath) throws -> Bool { + func isValidDirectory(_ path: AbsolutePath) -> Bool { true } } diff --git a/Tests/SourceControlTests/GitRepositoryProviderTests.swift b/Tests/SourceControlTests/GitRepositoryProviderTests.swift index 4c2699a809d..3803312df54 100644 --- a/Tests/SourceControlTests/GitRepositoryProviderTests.swift +++ b/Tests/SourceControlTests/GitRepositoryProviderTests.swift @@ -16,7 +16,7 @@ import _InternalTestSupport import XCTest class GitRepositoryProviderTests: XCTestCase { - func testRepositoryExists() throws { + func testIsValidDirectory() throws { try testWithTemporaryDirectory { sandbox in let provider = GitRepositoryProvider() @@ -24,20 +24,20 @@ class GitRepositoryProviderTests: XCTestCase { let repositoryPath = sandbox.appending("test") try localFileSystem.createDirectory(repositoryPath) initGitRepo(repositoryPath) - XCTAssertTrue(provider.repositoryExists(at: repositoryPath)) + XCTAssertTrue(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(provider.isValidDirectory(noCheckoutRepositoryPath)) // non-git directory let notGitPath = sandbox.appending("test-not-git") - XCTAssertFalse(provider.repositoryExists(at: notGitPath)) + XCTAssertFalse(provider.isValidDirectory(notGitPath)) // non-git child directory of a git directory let notGitChildPath = repositoryPath.appending("test-not-git") - XCTAssertFalse(provider.repositoryExists(at: notGitChildPath)) + XCTAssertFalse(provider.isValidDirectory(notGitChildPath)) } } } diff --git a/Tests/SourceControlTests/GitRepositoryTests.swift b/Tests/SourceControlTests/GitRepositoryTests.swift index 7d1547be2e3..d6a74a40915 100644 --- a/Tests/SourceControlTests/GitRepositoryTests.swift +++ b/Tests/SourceControlTests/GitRepositoryTests.swift @@ -785,14 +785,14 @@ class GitRepositoryTests: XCTestCase { ) // Before initializing the directory with a git repo, it is never valid. - XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir)) + XCTAssertFalse(repositoryManager.isValidDirectory(packageDir)) initGitRepo(packageDir) - XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir)) - XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: SourceControlURL(packageDir.pathString))) - XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: SourceControlURL(URL(packageDir.pathString + "/")))) - XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: SourceControlURL("/"))) - XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: SourceControlURL("https://mycustomdomain/some-package.git"))) + XCTAssertTrue(repositoryManager.isValidDirectory(packageDir)) + XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(packageDir.pathString)))) + XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(URL(packageDir.pathString + "/"))))) + XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("/")))) + XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("https://mycustomdomain/some-package.git")))) } } @@ -814,20 +814,21 @@ class GitRepositoryTests: XCTestCase { 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: SourceControlURL(customRemote))) + XCTAssertFalse(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: SourceControlURL(customRemote))) - XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: SourceControlURL("/"))) - // We consider the directory invalid if the remote does not have the same path extension - in this case we expect '.git'. - XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: SourceControlURL("https://mycustomdomain/some-package"))) - // We consider the directory invalid if the remote does not have the same path extension - in this case we expect '.git'. - XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: SourceControlURL("https://mycustomdomain/some-package/"))) + XCTAssertTrue(repositoryManager.isValidDirectory(packageDir)) + XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemote)))) + XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("/")))) + + // 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/")))) } } } diff --git a/Tests/SourceControlTests/RepositoryManagerTests.swift b/Tests/SourceControlTests/RepositoryManagerTests.swift index a90ef11d7d6..f9de534adda 100644 --- a/Tests/SourceControlTests/RepositoryManagerTests.swift +++ b/Tests/SourceControlTests/RepositoryManagerTests.swift @@ -347,7 +347,8 @@ class RepositoryManagerTests: XCTestCase { provider: provider, delegate: delegate ) - let dummyRepo = RepositorySpecifier(path: "/dummy") + let dummyRepoPath = try AbsolutePath(validating: "/dummy") + let dummyRepo = RepositorySpecifier(path: dummyRepoPath) let results = ThreadSafeKeyValueStore() let concurrency = 10000 @@ -356,7 +357,7 @@ class RepositoryManagerTests: XCTestCase { group.addTask { delegate.prepare(fetchExpected: index == 0, updateExpected: index > 0) results[index] = try await manager.lookup( - package: .init(url: SourceControlURL(dummyRepo.url)), + package: PackageIdentity(path: dummyRepoPath), repository: dummyRepo, updateStrategy: .always, observabilityScope: observability.topScope, @@ -453,11 +454,12 @@ class RepositoryManagerTests: XCTestCase { let finishGroup = DispatchGroup() let results = ThreadSafeKeyValueStore>() for index in 0 ..< total { - let repository = RepositorySpecifier(path: try .init(validating: "/repo/\(index)")) + let path = try AbsolutePath(validating: "/repo/\(index)") + let repository = RepositorySpecifier(path: path) provider.startGroup.enter() finishGroup.enter() manager.lookup( - package: .init(urlString: repository.url), + package: PackageIdentity(path: path), repository: repository, updateStrategy: .never, observabilityScope: observability.topScope, @@ -528,10 +530,6 @@ class RepositoryManagerTests: XCTestCase { print("\(repository) okay") } - func repositoryExists(at path: AbsolutePath) throws -> Bool { - return false - } - func open(repository: RepositorySpecifier, at path: AbsolutePath) throws -> Repository { fatalError("should not be called") } @@ -552,11 +550,11 @@ class RepositoryManagerTests: XCTestCase { fatalError("should not be called") } - func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { - fatalError("should not be called") + func isValidDirectory(_ directory: AbsolutePath) -> Bool { + return false } - public func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool { + public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { fatalError("should not be called") } @@ -606,11 +604,6 @@ class RepositoryManagerTests: XCTestCase { 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() } @@ -631,12 +624,13 @@ class RepositoryManagerTests: XCTestCase { fatalError("should not be called") } - func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { - fatalError("should not be called") + func isValidDirectory(_ directory: AbsolutePath) -> Bool { + // the directory exists + return true } - public func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool { - assert(RepositorySpecifier(url: repository) == self.repository) + public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { + assert(repository.url == self.repository.url) // the directory is not valid return false } @@ -663,7 +657,7 @@ class RepositoryManagerTests: XCTestCase { fatalError("unexpected API call") } - func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { + func isValidDirectory(_ directory: AbsolutePath) -> Bool { fatalError("unexpected API call") } @@ -715,7 +709,7 @@ extension RepositoryManager { ) async throws -> RepositoryHandle { return try await safe_async { self.lookup( - package: .init(url: SourceControlURL(repository.url)), + package: .init(url: SourceControlURL(repository.location.description)), repository: repository, updateStrategy: updateStrategy, observabilityScope: observabilityScope, @@ -752,16 +746,12 @@ private class DummyRepositoryProvider: RepositoryProvider { } // We only support one dummy URL. - let basename = (repository.url as NSString).lastPathComponent + let basename = repository.basename if basename != "dummy" { throw DummyError.invalidRepository } } - public func repositoryExists(at path: AbsolutePath) throws -> Bool { - return self.fileSystem.isDirectory(path) - } - func copy(from sourcePath: AbsolutePath, to destinationPath: AbsolutePath) throws { try self.fileSystem.copy(from: sourcePath, to: destinationPath) @@ -794,11 +784,11 @@ private class DummyRepositoryProvider: RepositoryProvider { return DummyWorkingCheckout(at: path) } - func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { - return true + func isValidDirectory(_ directory: AbsolutePath) -> Bool { + return self.fileSystem.isDirectory(directory) } - func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool { + func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { return true } @@ -976,14 +966,6 @@ fileprivate class DummyRepository: Repository { fatalError("unexpected API call") } - func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { - fatalError("unexpected API call") - } - - public func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool { - fatalError("unexpected API call") - } - func fetch() throws { self.provider.increaseFetchCount() } From f588e25a10708904c42acfa59717abbe900d77de Mon Sep 17 00:00:00 2001 From: Francesco Mikulis-Borsoi Date: Fri, 12 Jul 2024 18:13:23 -0700 Subject: [PATCH 04/11] Removed some unnecessary changes, restoring the original implementation. --- Sources/Commands/PackageCommands/ArchiveSource.swift | 2 +- .../PackageLoading/ManifestLoader+Validation.swift | 4 ++-- Sources/PackageMetadata/PackageMetadata.swift | 2 +- Sources/SourceControl/GitRepository.swift | 12 ++++-------- Sources/SourceControl/Repository.swift | 9 ++++----- Sources/SourceControl/RepositoryManager.swift | 4 ++-- .../_InternalTestSupport/InMemoryGitRepository.swift | 2 +- Tests/PackageLoadingTests/PDLoadingTests.swift | 2 +- .../GitRepositoryProviderTests.swift | 8 ++++---- Tests/SourceControlTests/GitRepositoryTests.swift | 8 ++++---- .../SourceControlTests/RepositoryManagerTests.swift | 8 ++++---- 11 files changed, 28 insertions(+), 33 deletions(-) diff --git a/Sources/Commands/PackageCommands/ArchiveSource.swift b/Sources/Commands/PackageCommands/ArchiveSource.swift index d962b9b10bb..95bed5eb637 100644 --- a/Sources/Commands/PackageCommands/ArchiveSource.swift +++ b/Sources/Commands/PackageCommands/ArchiveSource.swift @@ -66,7 +66,7 @@ extension SwiftPackageCommand { cancellator: Cancellator? ) throws { let gitRepositoryProvider = GitRepositoryProvider() - if gitRepositoryProvider.isValidDirectory(packageDirectory) { + if (try? gitRepositoryProvider.isValidDirectory(packageDirectory)) ?? false { let repository = GitRepository(path: packageDirectory, cancellator: cancellator) try repository.archive(to: archivePath) } else { diff --git a/Sources/PackageLoading/ManifestLoader+Validation.swift b/Sources/PackageLoading/ManifestLoader+Validation.swift index 802e34d111a..3df9fb8f2fe 100644 --- a/Sources/PackageLoading/ManifestLoader+Validation.swift +++ b/Sources/PackageLoading/ManifestLoader+Validation.swift @@ -267,7 +267,7 @@ public struct ManifestValidator { // there is a case to be made to throw early (here) if the path does not exists // but many of our tests assume they can pass a non existent path if case .local(let localPath) = dependency.location, self.fileSystem.exists(localPath) { - if !self.sourceControlValidator.isValidDirectory(localPath) { + if !((try? self.sourceControlValidator.isValidDirectory(localPath)) ?? false) { // Provides better feedback when mistakenly using url: for a dependency that // is a local package. Still allows for using url with a local package that has // also been initialized by git @@ -279,7 +279,7 @@ public struct ManifestValidator { } public protocol ManifestSourceControlValidator { - func isValidDirectory(_ path: AbsolutePath) -> Bool + func isValidDirectory(_ path: AbsolutePath) throws -> Bool } extension Basics.Diagnostic { diff --git a/Sources/PackageMetadata/PackageMetadata.swift b/Sources/PackageMetadata/PackageMetadata.swift index 5435ecb47a3..f87d5a1b6a2 100644 --- a/Sources/PackageMetadata/PackageMetadata.swift +++ b/Sources/PackageMetadata/PackageMetadata.swift @@ -253,7 +253,7 @@ public struct PackageSearchClient { to: tempPath, progressHandler: nil ) - if self.repositoryProvider.isValidDirectory(tempPath), + if ((try? self.repositoryProvider.isValidDirectory(tempPath)) ?? false), let repository = try self.repositoryProvider.open( repository: repositorySpecifier, at: tempPath diff --git a/Sources/SourceControl/GitRepository.swift b/Sources/SourceControl/GitRepository.swift index c945ac7af52..56a15627ab8 100644 --- a/Sources/SourceControl/GitRepository.swift +++ b/Sources/SourceControl/GitRepository.swift @@ -204,18 +204,14 @@ public struct GitRepositoryProvider: RepositoryProvider, Cancellable { ) } - public func isValidDirectory(_ directory: Basics.AbsolutePath) -> Bool { - do { - _ = try self.git.run(["-C", directory.pathString, "rev-parse", "--git-dir"]) - return true - } catch { - return false - } + 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 CanonicalPackageURL(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 index ba7ce4729da..0acec2dd492 100644 --- a/Sources/SourceControl/Repository.swift +++ b/Sources/SourceControl/Repository.swift @@ -11,7 +11,6 @@ //===----------------------------------------------------------------------===// import Basics -import struct PackageModel.CanonicalPackageURL import Foundation /// Specifies a repository address. @@ -33,10 +32,10 @@ public struct RepositorySpecifier: Hashable, Sendable { } /// The location of the repository as string. - public var url: CanonicalPackageURL { + public var url: String { switch self.location { - case .path(let path): return CanonicalPackageURL(path.pathString) - case .url(let url): return CanonicalPackageURL(url.absoluteString) + case .path(let path): path.pathString + case .url(let url): url.absoluteString } } @@ -141,7 +140,7 @@ public protocol RepositoryProvider: Cancellable { func copy(from sourcePath: AbsolutePath, to destinationPath: AbsolutePath) throws /// Returns true if the directory is valid git location. - func isValidDirectory(_ directory: AbsolutePath) -> Bool + 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 diff --git a/Sources/SourceControl/RepositoryManager.swift b/Sources/SourceControl/RepositoryManager.swift index 98bcfd03269..703a33ed9fe 100644 --- a/Sources/SourceControl/RepositoryManager.swift +++ b/Sources/SourceControl/RepositoryManager.swift @@ -444,8 +444,8 @@ public class RepositoryManager: Cancellable { } /// Returns true if the directory is valid git location. - public func isValidDirectory(_ directory: AbsolutePath) -> Bool { - self.provider.isValidDirectory(directory) + public func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { + try self.provider.isValidDirectory(directory) } /// Returns true if the directory is valid git location for the specified repository diff --git a/Sources/_InternalTestSupport/InMemoryGitRepository.swift b/Sources/_InternalTestSupport/InMemoryGitRepository.swift index 5d7bb809555..5c30ec53c09 100644 --- a/Sources/_InternalTestSupport/InMemoryGitRepository.swift +++ b/Sources/_InternalTestSupport/InMemoryGitRepository.swift @@ -477,7 +477,7 @@ public final class InMemoryGitRepositoryProvider: RepositoryProvider { return checkout } - public func isValidDirectory(_ directory: AbsolutePath) -> Bool { + public func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { return fetchedMap[directory] != nil } diff --git a/Tests/PackageLoadingTests/PDLoadingTests.swift b/Tests/PackageLoadingTests/PDLoadingTests.swift index 1c8f2d8a1bc..6d2f5381661 100644 --- a/Tests/PackageLoadingTests/PDLoadingTests.swift +++ b/Tests/PackageLoadingTests/PDLoadingTests.swift @@ -176,7 +176,7 @@ final class ManifestTestDelegate: ManifestLoaderDelegate { } fileprivate struct NOOPManifestSourceControlValidator: ManifestSourceControlValidator { - func isValidDirectory(_ path: AbsolutePath) -> Bool { + func isValidDirectory(_ path: AbsolutePath) throws -> Bool { true } } diff --git a/Tests/SourceControlTests/GitRepositoryProviderTests.swift b/Tests/SourceControlTests/GitRepositoryProviderTests.swift index 3803312df54..7c032eef297 100644 --- a/Tests/SourceControlTests/GitRepositoryProviderTests.swift +++ b/Tests/SourceControlTests/GitRepositoryProviderTests.swift @@ -24,20 +24,20 @@ class GitRepositoryProviderTests: XCTestCase { let repositoryPath = sandbox.appending("test") try localFileSystem.createDirectory(repositoryPath) initGitRepo(repositoryPath) - XCTAssertTrue(provider.isValidDirectory(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.isValidDirectory(noCheckoutRepositoryPath)) + XCTAssertTrue(try provider.isValidDirectory(noCheckoutRepositoryPath)) // non-git directory let notGitPath = sandbox.appending("test-not-git") - XCTAssertFalse(provider.isValidDirectory(notGitPath)) + XCTAssertThrowsError(try provider.isValidDirectory(notGitPath)) // non-git child directory of a git directory let notGitChildPath = repositoryPath.appending("test-not-git") - XCTAssertFalse(provider.isValidDirectory(notGitChildPath)) + XCTAssertThrowsError(try provider.isValidDirectory(notGitChildPath)) } } } diff --git a/Tests/SourceControlTests/GitRepositoryTests.swift b/Tests/SourceControlTests/GitRepositoryTests.swift index d6a74a40915..3ec1725a8dd 100644 --- a/Tests/SourceControlTests/GitRepositoryTests.swift +++ b/Tests/SourceControlTests/GitRepositoryTests.swift @@ -785,10 +785,10 @@ class GitRepositoryTests: XCTestCase { ) // Before initializing the directory with a git repo, it is never valid. - XCTAssertFalse(repositoryManager.isValidDirectory(packageDir)) + XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir)) initGitRepo(packageDir) - XCTAssertTrue(repositoryManager.isValidDirectory(packageDir)) + XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir)) XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(packageDir.pathString)))) XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(URL(packageDir.pathString + "/"))))) XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("/")))) @@ -814,14 +814,14 @@ class GitRepositoryTests: XCTestCase { let customRemote = try XCTUnwrap(URL(string: "https://mycustomdomain/some-package.git")) // Before initializing the directory with a git repo, it is never valid. - XCTAssertFalse(repositoryManager.isValidDirectory(packageDir)) + 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(repositoryManager.isValidDirectory(packageDir)) + XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir)) XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemote)))) XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("/")))) diff --git a/Tests/SourceControlTests/RepositoryManagerTests.swift b/Tests/SourceControlTests/RepositoryManagerTests.swift index f9de534adda..495cab04dd6 100644 --- a/Tests/SourceControlTests/RepositoryManagerTests.swift +++ b/Tests/SourceControlTests/RepositoryManagerTests.swift @@ -550,7 +550,7 @@ class RepositoryManagerTests: XCTestCase { fatalError("should not be called") } - func isValidDirectory(_ directory: AbsolutePath) -> Bool { + func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { return false } @@ -624,7 +624,7 @@ class RepositoryManagerTests: XCTestCase { fatalError("should not be called") } - func isValidDirectory(_ directory: AbsolutePath) -> Bool { + func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { // the directory exists return true } @@ -657,7 +657,7 @@ class RepositoryManagerTests: XCTestCase { fatalError("unexpected API call") } - func isValidDirectory(_ directory: AbsolutePath) -> Bool { + func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { fatalError("unexpected API call") } @@ -784,7 +784,7 @@ private class DummyRepositoryProvider: RepositoryProvider { return DummyWorkingCheckout(at: path) } - func isValidDirectory(_ directory: AbsolutePath) -> Bool { + func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { return self.fileSystem.isDirectory(directory) } From b3bdde3dcb877187760edf73a6fd952938a89cd3 Mon Sep 17 00:00:00 2001 From: Francesco Mikulis-Borsoi Date: Mon, 15 Jul 2024 12:46:01 -0700 Subject: [PATCH 05/11] Removed more unnecessary changes. --- .../Commands/PackageCommands/ArchiveSource.swift | 2 +- .../PackageLoading/ManifestLoader+Validation.swift | 14 +++++++++----- Sources/PackageMetadata/PackageMetadata.swift | 2 +- Sources/SourceControl/Repository.swift | 6 +++--- Sources/SourceControl/RepositoryManager.swift | 9 ++++++--- Tests/PackageLoadingTests/PDLoadingTests.swift | 1 - .../RepositoryManagerTests.swift | 12 ++---------- 7 files changed, 22 insertions(+), 24 deletions(-) diff --git a/Sources/Commands/PackageCommands/ArchiveSource.swift b/Sources/Commands/PackageCommands/ArchiveSource.swift index 95bed5eb637..d2911dce87e 100644 --- a/Sources/Commands/PackageCommands/ArchiveSource.swift +++ b/Sources/Commands/PackageCommands/ArchiveSource.swift @@ -66,7 +66,7 @@ extension SwiftPackageCommand { cancellator: Cancellator? ) throws { let gitRepositoryProvider = GitRepositoryProvider() - if (try? gitRepositoryProvider.isValidDirectory(packageDirectory)) ?? false { + if try gitRepositoryProvider.isValidDirectory(packageDirectory) { let repository = GitRepository(path: packageDirectory, cancellator: cancellator) try repository.archive(to: archivePath) } else { diff --git a/Sources/PackageLoading/ManifestLoader+Validation.swift b/Sources/PackageLoading/ManifestLoader+Validation.swift index 3df9fb8f2fe..61cd9f934bb 100644 --- a/Sources/PackageLoading/ManifestLoader+Validation.swift +++ b/Sources/PackageLoading/ManifestLoader+Validation.swift @@ -267,11 +267,15 @@ public struct ManifestValidator { // there is a case to be made to throw early (here) if the path does not exists // but many of our tests assume they can pass a non existent path if case .local(let localPath) = dependency.location, self.fileSystem.exists(localPath) { - if !((try? self.sourceControlValidator.isValidDirectory(localPath)) ?? false) { - // Provides better feedback when mistakenly using url: for a dependency that - // is a local package. Still allows for using url with a local package that has - // also been initialized by git - diagnostics.append(.invalidSourceControlDirectory(localPath)) + do { + if try !self.sourceControlValidator.isValidDirectory(localPath) { + // Provides better feedback when mistakenly using url: for a dependency that + // is a local package. Still allows for using url with a local package that has + // also been initialized by git + diagnostics.append(.invalidSourceControlDirectory(localPath)) + } + } catch { + diagnostics.append(.invalidSourceControlDirectory(localPath, underlyingError: error)) } } return diagnostics diff --git a/Sources/PackageMetadata/PackageMetadata.swift b/Sources/PackageMetadata/PackageMetadata.swift index f87d5a1b6a2..ea2249a2623 100644 --- a/Sources/PackageMetadata/PackageMetadata.swift +++ b/Sources/PackageMetadata/PackageMetadata.swift @@ -253,7 +253,7 @@ public struct PackageSearchClient { to: tempPath, progressHandler: nil ) - if ((try? self.repositoryProvider.isValidDirectory(tempPath)) ?? false), + if try self.repositoryProvider.isValidDirectory(tempPath), let repository = try self.repositoryProvider.open( repository: repositorySpecifier, at: tempPath diff --git a/Sources/SourceControl/Repository.swift b/Sources/SourceControl/Repository.swift index 0acec2dd492..a86227b7271 100644 --- a/Sources/SourceControl/Repository.swift +++ b/Sources/SourceControl/Repository.swift @@ -34,8 +34,8 @@ public struct RepositorySpecifier: Hashable, Sendable { /// The location of the repository as string. public var url: String { switch self.location { - case .path(let path): path.pathString - case .url(let url): url.absoluteString + case .path(let path): return path.pathString + case .url(let url): return url.absoluteString } } @@ -43,7 +43,7 @@ public struct RepositorySpecifier: Hashable, Sendable { public var basename: String { // FIXME: this might be wrong //var basename = self.url.pathComponents.dropFirst(1).last(where: { !$0.isEmpty }) ?? "" - var basename = (self.url.description as NSString).lastPathComponent + var basename = (self.url as NSString).lastPathComponent if basename.hasSuffix(".git") { basename = String(basename.dropLast(4)) } diff --git a/Sources/SourceControl/RepositoryManager.swift b/Sources/SourceControl/RepositoryManager.swift index 703a33ed9fe..2acc271b2b9 100644 --- a/Sources/SourceControl/RepositoryManager.swift +++ b/Sources/SourceControl/RepositoryManager.swift @@ -220,9 +220,14 @@ 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 - if ((try? self.provider.isValidDirectory(repositoryPath, for: repositorySpecifier)) ?? false) { + quick: if (try? self.provider.isValidDirectory(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() @@ -239,8 +244,6 @@ public class RepositoryManager: Cancellable { } return handle - } else { - observabilityScope.emit(warning: "\(repositoryPath) is not valid git repository for '\(repositorySpecifier.location)', will fetch again.") } // inform delegate that we are starting to fetch diff --git a/Tests/PackageLoadingTests/PDLoadingTests.swift b/Tests/PackageLoadingTests/PDLoadingTests.swift index 6d2f5381661..b3970079313 100644 --- a/Tests/PackageLoadingTests/PDLoadingTests.swift +++ b/Tests/PackageLoadingTests/PDLoadingTests.swift @@ -13,7 +13,6 @@ import Basics import PackageLoading import PackageModel -import SourceControl import _InternalTestSupport import XCTest diff --git a/Tests/SourceControlTests/RepositoryManagerTests.swift b/Tests/SourceControlTests/RepositoryManagerTests.swift index 495cab04dd6..31ae7369764 100644 --- a/Tests/SourceControlTests/RepositoryManagerTests.swift +++ b/Tests/SourceControlTests/RepositoryManagerTests.swift @@ -630,7 +630,7 @@ class RepositoryManagerTests: XCTestCase { } public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { - assert(repository.url == self.repository.url) + assert(repository == self.repository) // the directory is not valid return false } @@ -657,14 +657,6 @@ class RepositoryManagerTests: XCTestCase { fatalError("unexpected API call") } - func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { - fatalError("unexpected API call") - } - - public func isValidDirectory(_ directory: AbsolutePath, for repository: SourceControlURL) throws -> Bool { - fatalError("unexpected API call") - } - func fetch() throws { // noop } @@ -709,7 +701,7 @@ extension RepositoryManager { ) async throws -> RepositoryHandle { return try await safe_async { self.lookup( - package: .init(url: SourceControlURL(repository.location.description)), + package: .init(url: SourceControlURL(repository.url)), repository: repository, updateStrategy: updateStrategy, observabilityScope: observabilityScope, From 363272c454ce22779df2e386c2f75ea90dba53f1 Mon Sep 17 00:00:00 2001 From: Francesco Mikulis-Borsoi Date: Mon, 15 Jul 2024 13:49:32 -0700 Subject: [PATCH 06/11] Clarified and expanded the tests. --- .../GitRepositoryTests.swift | 45 +++++++++++++++---- 1 file changed, 37 insertions(+), 8 deletions(-) diff --git a/Tests/SourceControlTests/GitRepositoryTests.swift b/Tests/SourceControlTests/GitRepositoryTests.swift index 3ec1725a8dd..766a359707a 100644 --- a/Tests/SourceControlTests/GitRepositoryTests.swift +++ b/Tests/SourceControlTests/GitRepositoryTests.swift @@ -784,15 +784,38 @@ class GitRepositoryTests: XCTestCase { 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)) - initGitRepo(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)) - XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(packageDir.pathString)))) - XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(URL(packageDir.pathString + "/"))))) - XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("/")))) - XCTAssertThrowsError(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("https://mycustomdomain/some-package.git")))) + + 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")))) } } @@ -820,15 +843,21 @@ class GitRepositoryTests: XCTestCase { 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)))) - XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("/")))) - // 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"))))) } } } From f5a8a65d3684e159a3d97540d7b09a9d2599ca74 Mon Sep 17 00:00:00 2001 From: Francesco Mikulis-Borsoi Date: Mon, 15 Jul 2024 13:52:09 -0700 Subject: [PATCH 07/11] Removed more unnecessary changes. --- Sources/Commands/PackageCommands/ArchiveSource.swift | 2 +- Sources/SourceControl/GitRepository.swift | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/Sources/Commands/PackageCommands/ArchiveSource.swift b/Sources/Commands/PackageCommands/ArchiveSource.swift index d2911dce87e..3c55008cf28 100644 --- a/Sources/Commands/PackageCommands/ArchiveSource.swift +++ b/Sources/Commands/PackageCommands/ArchiveSource.swift @@ -66,7 +66,7 @@ extension SwiftPackageCommand { cancellator: Cancellator? ) throws { let gitRepositoryProvider = GitRepositoryProvider() - if try gitRepositoryProvider.isValidDirectory(packageDirectory) { + if (try? gitRepositoryProvider.isValidDirectory(packageDirectory)) == true { let repository = GitRepository(path: packageDirectory, cancellator: cancellator) try repository.archive(to: archivePath) } else { diff --git a/Sources/SourceControl/GitRepository.swift b/Sources/SourceControl/GitRepository.swift index 56a15627ab8..feccd0dccb2 100644 --- a/Sources/SourceControl/GitRepository.swift +++ b/Sources/SourceControl/GitRepository.swift @@ -15,7 +15,6 @@ import Basics import Dispatch import class Foundation.NSLock -import struct PackageModel.CanonicalPackageLocation import struct PackageModel.CanonicalPackageURL import struct TSCBasic.ByteString From 51592d78d4b802042c5e90184cfcdcaa042339d8 Mon Sep 17 00:00:00 2001 From: Francesco Mikulis-Borsoi Date: Mon, 15 Jul 2024 14:24:39 -0700 Subject: [PATCH 08/11] Added tests for relative-path based origin. --- .../GitRepositoryTests.swift | 50 ++++++++++++++++++- 1 file changed, 48 insertions(+), 2 deletions(-) diff --git a/Tests/SourceControlTests/GitRepositoryTests.swift b/Tests/SourceControlTests/GitRepositoryTests.swift index 766a359707a..5dc0a57ab5c 100644 --- a/Tests/SourceControlTests/GitRepositoryTests.swift +++ b/Tests/SourceControlTests/GitRepositoryTests.swift @@ -769,7 +769,53 @@ class GitRepositoryTests: XCTestCase { } } - func testValidDirectoryLocal() async throws { + 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") @@ -819,7 +865,7 @@ class GitRepositoryTests: XCTestCase { } } - func testValidDirectoryRemote() async throws { + func testValidDirectoryRemoteOrigin() async throws { try testWithTemporaryDirectory { tmpDir in // Create a repository. let packageDir = tmpDir.appending("SomePackage") From 8e2691e333dfbcda4b9bda23f281737d96f3db47 Mon Sep 17 00:00:00 2001 From: Francesco Mikulis-Borsoi Date: Mon, 15 Jul 2024 15:44:30 -0700 Subject: [PATCH 09/11] Feedback - added back the validation on the matching path extension. --- Sources/SourceControl/GitRepository.swift | 3 ++ .../GitRepositoryTests.swift | 28 +++++++++---------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/Sources/SourceControl/GitRepository.swift b/Sources/SourceControl/GitRepository.swift index feccd0dccb2..6bbc6396a90 100644 --- a/Sources/SourceControl/GitRepository.swift +++ b/Sources/SourceControl/GitRepository.swift @@ -14,6 +14,7 @@ import Basics import Dispatch import class Foundation.NSLock +import class Foundation.NSString import struct PackageModel.CanonicalPackageURL @@ -211,6 +212,8 @@ public struct GitRepositoryProvider: RepositoryProvider, Cancellable { 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 CanonicalPackageURL(remoteURL) == CanonicalPackageURL(repository.url) + // NOTE: We require that the path extensions match (e.g. consistency for presence or absence of '.git') + && (remoteURL as NSString).pathExtension == (repository.url as NSString).pathExtension } public func copy(from sourcePath: Basics.AbsolutePath, to destinationPath: Basics.AbsolutePath) throws { diff --git a/Tests/SourceControlTests/GitRepositoryTests.swift b/Tests/SourceControlTests/GitRepositoryTests.swift index 5dc0a57ab5c..c2767e48869 100644 --- a/Tests/SourceControlTests/GitRepositoryTests.swift +++ b/Tests/SourceControlTests/GitRepositoryTests.swift @@ -798,10 +798,10 @@ class GitRepositoryTests: XCTestCase { 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 + "/")))) + // We consider the directory invalid if the remote does not have the same path extension - in this case we expected '.git'. + XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemoteWithoutPathExtension)))) + // We consider the directory invalid if the remote does not have the same path extension - in this case we expected '.git'. + XCTAssertFalse(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: "/")))) @@ -846,12 +846,12 @@ class GitRepositoryTests: XCTestCase { 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 + "/")))) + // We consider the directory invalid if the remote does not have the same path extension - in this case we expected '.git'. + XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(path: try AbsolutePath(validating: customRemotePathWithoutPathExtension)))) + XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemotePathWithoutPathExtension)))) + // We consider the directory invalid if the remote does not have the same path extension - in this case we expected '.git'. + XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(path: try AbsolutePath(validating: customRemotePathWithoutPathExtension + "/")))) + XCTAssertFalse(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: "/")))) @@ -892,10 +892,10 @@ class GitRepositoryTests: XCTestCase { 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/")))) + // We consider the directory invalid if the remote does not have the same path extension - in this case we expected '.git'. + XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("https://mycustomdomain/some-package")))) + // We consider the directory invalid if the remote does not have the same path extension - in this case we expected '.git'. + XCTAssertFalse(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: "/")))) From 0ead74db9315906bde911013f9205a4d2b5c95e9 Mon Sep 17 00:00:00 2001 From: Francesco Mikulis-Borsoi Date: Thu, 18 Jul 2024 13:10:19 -0700 Subject: [PATCH 10/11] Revert "Feedback - added back the validation on the matching path extension." This reverts commit 8e2691e333dfbcda4b9bda23f281737d96f3db47. --- Sources/SourceControl/GitRepository.swift | 3 -- .../GitRepositoryTests.swift | 28 +++++++++---------- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/Sources/SourceControl/GitRepository.swift b/Sources/SourceControl/GitRepository.swift index 6bbc6396a90..feccd0dccb2 100644 --- a/Sources/SourceControl/GitRepository.swift +++ b/Sources/SourceControl/GitRepository.swift @@ -14,7 +14,6 @@ import Basics import Dispatch import class Foundation.NSLock -import class Foundation.NSString import struct PackageModel.CanonicalPackageURL @@ -212,8 +211,6 @@ public struct GitRepositoryProvider: RepositoryProvider, Cancellable { 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 CanonicalPackageURL(remoteURL) == CanonicalPackageURL(repository.url) - // NOTE: We require that the path extensions match (e.g. consistency for presence or absence of '.git') - && (remoteURL as NSString).pathExtension == (repository.url as NSString).pathExtension } public func copy(from sourcePath: Basics.AbsolutePath, to destinationPath: Basics.AbsolutePath) throws { diff --git a/Tests/SourceControlTests/GitRepositoryTests.swift b/Tests/SourceControlTests/GitRepositoryTests.swift index c2767e48869..5dc0a57ab5c 100644 --- a/Tests/SourceControlTests/GitRepositoryTests.swift +++ b/Tests/SourceControlTests/GitRepositoryTests.swift @@ -798,10 +798,10 @@ class GitRepositoryTests: XCTestCase { let customRemoteWithoutPathExtension = (customRemote as NSString).deletingPathExtension XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemote)))) - // We consider the directory invalid if the remote does not have the same path extension - in this case we expected '.git'. - XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemoteWithoutPathExtension)))) - // We consider the directory invalid if the remote does not have the same path extension - in this case we expected '.git'. - XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL((customRemote as NSString).deletingPathExtension + "/")))) + // 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: "/")))) @@ -846,12 +846,12 @@ class GitRepositoryTests: XCTestCase { 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 invalid if the remote does not have the same path extension - in this case we expected '.git'. - XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(path: try AbsolutePath(validating: customRemotePathWithoutPathExtension)))) - XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemotePathWithoutPathExtension)))) - // We consider the directory invalid if the remote does not have the same path extension - in this case we expected '.git'. - XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(path: try AbsolutePath(validating: customRemotePathWithoutPathExtension + "/")))) - XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL((customRemotePath as NSString).deletingPathExtension + "/")))) + // 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: "/")))) @@ -892,10 +892,10 @@ class GitRepositoryTests: XCTestCase { XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir)) XCTAssertTrue(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL(customRemote)))) - // We consider the directory invalid if the remote does not have the same path extension - in this case we expected '.git'. - XCTAssertFalse(try repositoryManager.isValidDirectory(packageDir, for: RepositorySpecifier(url: SourceControlURL("https://mycustomdomain/some-package")))) - // We consider the directory invalid if the remote does not have the same path extension - in this case we expected '.git'. - XCTAssertFalse(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")))) + // 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: "/")))) From e1c678fee49eb69f082528c8c1dd9af160fb1aaa Mon Sep 17 00:00:00 2001 From: Francesco Mikulis-Borsoi Date: Fri, 19 Jul 2024 13:12:26 -0700 Subject: [PATCH 11/11] Updated the test InMemoryRepositoryProvider to explicitly handle repositories that aren't yet checked out, even though the InMemoryFileSystem claims the directories exist. --- CHANGELOG.md | 4 ++++ Sources/_InternalTestSupport/InMemoryGitRepository.swift | 7 ++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b9f709af37f..e8f6c840248 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ Swift Next 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. + * [#7507] `swift experimental-sdk` command is deprecated with `swift sdk` command replacing it. `--experimental-swift-sdk` and diff --git a/Sources/_InternalTestSupport/InMemoryGitRepository.swift b/Sources/_InternalTestSupport/InMemoryGitRepository.swift index 5c30ec53c09..6de8a0eec0a 100644 --- a/Sources/_InternalTestSupport/InMemoryGitRepository.swift +++ b/Sources/_InternalTestSupport/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() @@ -478,11 +479,11 @@ public final class InMemoryGitRepositoryProvider: RepositoryProvider { } public func isValidDirectory(_ directory: AbsolutePath) throws -> Bool { - return fetchedMap[directory] != nil + return fetchedMap[directory] != nil || specifierMap.get().values.map(\.path).contains(directory) } public func isValidDirectory(_ directory: AbsolutePath, for repository: RepositorySpecifier) throws -> Bool { - return fetchedMap[directory] != nil + return fetchedMap[directory] != nil || specifierMap[repository] != nil } public func cancel(deadline: DispatchTime) throws {