move manifest validation that requires source control to Workspace (#3955) (#3956)

motivation: all access to git operations should be done via the repositoriesProvider handed to the workspace, but some was hard wired to using Git

changes:
* move git related validation from manfeist JSON parser and graph loader to workspace and use the workspace repositoryProvider to perform them
* add methods on RepositoryProvider to hande validation of directory and ref formats.
* remove PackageLoading depedency on SourceControl which was a problem for a while and only needed for these validations
* add and adjust tests

rdar://84393257

Author: tomerd

Fixtures/Miscellaneous/InvalidRefs/InvalidBranch/Package.swift

@@ -0,0 +1,10 @@
+// swift-tools-version:5.5
+import PackageDescription
+
+let package = Package(
+    name: "Foo",
+    dependencies: [
+        .package(url: "https://localhost/foo/bar", branch: "#!~")
+    ],
+    targets: []
+)

Fixtures/Miscellaneous/InvalidRefs/InvalidRevision/Package.swift

@@ -0,0 +1,10 @@
+// swift-tools-version:5.5
+import PackageDescription
+
+let package = Package(
+    name: "Foo",
+    dependencies: [
+        .package(url: "https://localhost/foo/bar", revision: "#!~")
+    ],
+    targets: []
+)

Package.swift

@@ -190,8 +190,7 @@ let package = Package(
             name: "PackageLoading",
             dependencies: [
                 "Basics",
-                "PackageModel",
-                "SourceControl"
+                "PackageModel"
             ],
             exclude: ["CMakeLists.txt", "README.md"]
         ),

Sources/PackageGraph/PackageGraphRoot.swift

@@ -116,17 +116,9 @@ extension PackageDependency.SourceControl.Requirement {
         case .range(let range):
             return .versionSet(.range(range))
         case .revision(let identifier):
-            // FIXME: this validation could/should move somewhere more appropriate
-            guard Git.checkRefFormat(ref: identifier) else {
-                throw StringError("Could not find revision: '\(identifier)'")
-            }
-            return .revision(identifier)
-        case .branch(let identifier):
-            // FIXME: this validation could/should move somewhere more appropriate
-            guard Git.checkRefFormat(ref: identifier) else {
-                throw StringError("Could not find branch: '\(identifier)'")
-            }
             return .revision(identifier)
+        case .branch(let name):
+            return .revision(name)
         case .exact(let version):
             return .versionSet(.exact(version))
         }

Sources/PackageLoading/ManifestJSONParser.swift

@@ -11,7 +11,6 @@
 import Basics
 import Foundation
 import PackageModel
-import SourceControl // FIXME: remove this dependency
 import TSCBasic
 import TSCUtility
 
@@ -188,15 +187,6 @@ enum ManifestJSONParser {
         location = identityResolver.mappedLocation(for: location)
         // a package in a git location, may be a remote URL or on disk
         if let localPath = try? AbsolutePath(validating: location) {
-            // if exists, validate location is in fact a git repo
-            // 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 fileSystem.exists(localPath) {
-                let gitRepoProvider = GitRepositoryProvider()
-                guard gitRepoProvider.isValidDirectory(location) else {
-                    throw StringError("Cannot clone from local directory \(localPath)\nPlease git init or use \"path:\" for \(location)")
-                }
-            }
             // in the future this will check with the registries for the identity of the URL
             let identity = try identityResolver.resolveIdentity(for: localPath)
             return .localSourceControl(

Sources/SPMTestSupport/InMemoryGitRepository.swift

@@ -430,4 +430,12 @@ public final class InMemoryGitRepositoryProvider: RepositoryProvider {
     public func openWorkingCopy(at path: AbsolutePath) throws -> WorkingCheckout {
         return checkoutsMap[path]!
     }
+
+    public func isValidDirectory(_ directory: AbsolutePath) -> Bool {
+        return true
+    }
+
+    public func isValidRefFormat(_ ref: String) -> Bool {
+        return true
+    }
 }

Sources/SourceControl/GitRepository.swift

@@ -111,12 +111,19 @@ public struct GitRepositoryProvider: RepositoryProvider {
                          progress: progressHandler)
     }
 
-    public func isValidDirectory(_ directory: String) -> Bool {
-        // Provides better feedback when mistakingly 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
+    public func isValidDirectory(_ directory: AbsolutePath) -> Bool {
         do {
-            _ = try self.git.run(["-C", directory, "rev-parse", "--git-dir"])
+            _ = try self.git.run(["-C", directory.pathString, "rev-parse", "--git-dir"])
+            return true
+        } catch {
+            return false
+        }
+    }
+
+    /// Returns true if the git reference name is well formed.
+    public func isValidRefFormat(_ ref: String) -> Bool {
+        do {
+            _ = try self.git.run(["check-ref-format", "--allow-onelevel", ref])
             return true
         } catch {
             return false

Sources/SourceControl/Repository.swift

@@ -142,6 +142,12 @@ public protocol RepositoryProvider {
     ///   - sourcePath: the source path.
     ///   - destinationPath: the destination  path.
     func copy(from sourcePath: AbsolutePath, to destinationPath: AbsolutePath) throws
+
+    /// Returns true if the directory is valid git location.
+    func isValidDirectory(_ directory: AbsolutePath) -> Bool
+
+    /// Returns true if the git reference name is well formed.
+    func isValidRefFormat(_ ref: String) -> Bool
 }
 
 /// Abstract repository operations.

Sources/SourceControl/RepositoryManager.swift

@@ -351,6 +351,16 @@ public class RepositoryManager {
         }
     }
 
+    /// Returns true if the directory is valid git location.
+    public func isValidDirectory(_ directory: AbsolutePath) -> Bool {
+        self.provider.isValidDirectory(directory)
+    }
+
+    /// Returns true if the git reference name is well formed.
+    public func isValidRefFormat(_ ref: String) -> Bool {
+        self.provider.isValidRefFormat(ref)
+    }
+
     /// Reset the repository manager.
     ///
     /// Note: This also removes the cloned repositories from the disk.

Sources/Workspace/Workspace.swift

@@ -1874,6 +1874,7 @@ extension Workspace {
                         manifestLoadingScope.emit(error)
                         self.delegate?.didLoadManifest(packagePath: packagePath, url: packageLocation, version: version, packageKind: packageKind, manifest: nil, diagnostics: manifestLoadingDiagnostics.get())
                     case .success(let manifest):
+                        manifestLoadingScope.trap { try self.validateManifest(manifest) }
                         self.delegate?.didLoadManifest(packagePath: packagePath, url: packageLocation, version: version, packageKind: packageKind, manifest: manifest, diagnostics: manifestLoadingDiagnostics.get())
                     }
                     completion(result)
@@ -1885,6 +1886,48 @@ extension Workspace {
         //}
     }
 
+    // TODO: move more manifest validation in here from other parts of the code, e.g. from ManifestLoader
+    private func validateManifest(_ manifest: Manifest) throws {
+        // validate dependency requirements
+        for dependency in manifest.dependencies {
+            switch dependency {
+            case .sourceControl(let sourceControl):
+                try validateSourceControlDependency(sourceControl)
+            default:
+                break
+            }
+        }
+
+        func validateSourceControlDependency(_ dependency: PackageDependency.SourceControl) throws {
+            // validate source control ref
+            switch dependency.requirement {
+            case .branch(let name):
+                guard self.repositoryManager.isValidRefFormat(name) else {
+                    throw StringError("Invalid branch name: '\(name)'")
+                }
+            case .revision(let revision):
+                guard self.repositoryManager.isValidRefFormat(revision) else {
+                    throw StringError("Invalid revision: '\(revision)'")
+                }
+            default:
+                break
+            }
+            // if a location is on file system, validate it is in fact a git repo
+            // 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) {
+                guard self.repositoryManager.isValidDirectory(localPath) else {
+                    // Provides better feedback when mistakingly 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
+                    throw StringError("Cannot clone from local directory \(localPath)\nPlease git init or use \"path:\" for \(location)")
+                }
+            }
+        }
+
+    }
+
+
     /// Validates that all the edited dependencies are still present in the file system.
     /// If some checkout dependency is removed form the file system, clone it again.
     /// If some edited dependency is removed from the file system, mark it as unedited and

Tests/FunctionalTests/MiscellaneousTests.swift

@@ -416,22 +416,34 @@ class MiscellaneousTestCase: XCTestCase {
         }
     }
 
-    func testLocalPackageUsedAsURL() throws {
-        fixture(name: "Miscellaneous/LocalPackageAsURL", createGitRepo: false) { prefix in
+    func testLocalPackageUsedAsURLValidation() throws {
+        fixture(name: "Miscellaneous/LocalPackageAsURL", createGitRepo: false) { path in
             // This fixture has a setup that is trying to use a local package
             // as a url that hasn't been initialized as a repo
-
-            // Launch swift-build.
-            let app = prefix.appending(component: "Bar")
-
-            let result = try SwiftPMProduct.SwiftBuild.executeProcess([], packagePath: app)
-
+            let result = try SwiftPMProduct.SwiftBuild.executeProcess([], packagePath: path.appending(component: "Bar"))
             XCTAssert(result.exitStatus != .terminated(code: 0))
             let output = try result.utf8stderrOutput()
             XCTAssert(output.contains("Cannot clone from local directory"), "Didn't find expected output: \(output)")
         }
     }
 
+    func testInvalidRefsValidation() throws {
+        fixture(name: "Miscellaneous/InvalidRefs", createGitRepo: false) { path in
+            do {
+                let result = try SwiftPMProduct.SwiftBuild.executeProcess([], packagePath: path.appending(component: "InvalidBranch"))
+                XCTAssert(result.exitStatus != .terminated(code: 0))
+                let output = try result.utf8stderrOutput()
+                XCTAssert(output.contains("Invalid branch name: "), "Didn't find expected output: \(output)")
+            }
+            do {
+                let result = try SwiftPMProduct.SwiftBuild.executeProcess([], packagePath: path.appending(component: "InvalidRevision"))
+                XCTAssert(result.exitStatus != .terminated(code: 0))
+                let output = try result.utf8stderrOutput()
+                XCTAssert(output.contains("Invalid revision: "), "Didn't find expected output: \(output)")
+            }
+        }
+    }
+
     func testUnicode() {
         #if !os(Linux) && !os(Android) // TODO: - Linux has trouble with this and needs investigation.
         fixture(name: "Miscellaneous/Unicode") { prefix in

Tests/SourceControlTests/RepositoryManagerTests.swift

@@ -42,6 +42,14 @@ private class DummyRepository: Repository {
         fatalError("unexpected API call")
     }
 
+    func isValidDirectory(_ directory: AbsolutePath) -> Bool {
+        fatalError("unexpected API call")
+    }
+
+    func isValidRefFormat(_ ref: String) -> Bool {
+        fatalError("unexpected API call")
+    }
+
     func fetch() throws {
         self.provider.increaseFetchCount()
     }
@@ -108,6 +116,14 @@ private class DummyRepositoryProvider: RepositoryProvider {
         return DummyWorkingCheckout(at: path)
     }
 
+    func isValidDirectory(_ directory: AbsolutePath) -> Bool {
+        return true
+    }
+
+    func isValidRefFormat(_ ref: String) -> Bool {
+        return true
+    }
+
     func increaseFetchCount() {
         self.lock.withLock {
             self._numFetches += 1

Tests/WorkspaceTests/SourceControlPackageContainerTests.swift

@@ -132,6 +132,14 @@ private class MockRepositories: RepositoryProvider {
     func openWorkingCopy(at path: AbsolutePath) throws -> WorkingCheckout {
         fatalError("unexpected API call")
     }
+
+    func isValidDirectory(_ directory: AbsolutePath) -> Bool {
+        return true
+    }
+
+    func isValidRefFormat(_ ref: String) -> Bool {
+        return true
+    }
 }
 
 private class MockResolverDelegate: RepositoryManagerDelegate {