Merge pull request #2925 from abertelrud/eng/better-git-diagnostics

Improve the diagnostics that are emitted when a package dependency specifies a non-existent branch or commit

Author: abertelrud

Sources/PackageGraph/RepositoryPackageContainerProvider.swift

@@ -1,7 +1,7 @@
 /*
  This source file is part of the Swift.org open source project
 
- Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
+ Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors
  Licensed under Apache License v2.0 with Runtime Library Exception
 
  See http://swift.org/LICENSE.txt for license information
@@ -251,16 +251,28 @@ public class RepositoryPackageContainer: BasePackageContainer, CustomStringConve
 
     // A wrapper for getDependencies() errors. This adds additional information
     // about the container to identify it for diagnostics.
-    public struct GetDependenciesErrorWrapper: Swift.Error {
+    public struct GetDependenciesError: Error, CustomStringConvertible {
 
-        /// The container which had this error.
+        /// The container (repository) that encountered the error.
         public let containerIdentifier: String
 
-        /// The source control reference i.e. version, branch, revsion etc.
+        /// The source control reference (version, branch, revision, etc) that was involved.
         public let reference: String
 
         /// The actual error that occurred.
-        public let underlyingError: Swift.Error
+        public let underlyingError: Error
+        
+        /// Optional suggestion for how to resolve the error.
+        public let suggestion: String?
+        
+        /// Description shown for errors of this kind.
+        public var description: String {
+            var desc = "\(underlyingError) in \(containerIdentifier)"
+            if let suggestion = suggestion {
+                desc += " (\(suggestion))"
+            }
+            return desc
+        }
     }
 
     /// This is used to remember if tools version of a particular version is
@@ -370,8 +382,8 @@ public class RepositoryPackageContainer: BasePackageContainer, CustomStringConve
                 return try getDependencies(at: revision, version: version, productFilter: productFilter)
             }.1
         } catch {
-            throw GetDependenciesErrorWrapper(
-                containerIdentifier: identifier.repository.url, reference: version.description, underlyingError: error)
+            throw GetDependenciesError(
+                containerIdentifier: identifier.repository.url, reference: version.description, underlyingError: error, suggestion: nil)
         }
     }
 
@@ -383,8 +395,28 @@ public class RepositoryPackageContainer: BasePackageContainer, CustomStringConve
                 return try getDependencies(at: revision, productFilter: productFilter)
             }.1
         } catch {
-            throw GetDependenciesErrorWrapper(
-                containerIdentifier: identifier.repository.url, reference: revision, underlyingError: error)
+            // Examine the error to see if we can come up with a more informative and actionable error message.  We know that the revision is expected to be a branch name or a hash (tags are handled through a different code path).
+            if let gitInvocationError = error as? ProcessResult.Error, gitInvocationError.description.contains("Needed a single revision") {
+                // It was a Git process invocation error.  Take a look at the repository to see if we can come up with a reasonable diagnostic.
+                if let rev = try? repository.resolveRevision(identifier: revision), repository.exists(revision: rev) {
+                    // Revision does exist, so something else must be wrong.
+                    throw GetDependenciesError(
+                        containerIdentifier: identifier.repository.url, reference: revision, underlyingError: error, suggestion: nil)
+                }
+                else {
+                    // Revision does not exist, so we customize the error.
+                    let sha1RegEx = try! RegEx(pattern: #"\A[:xdigit:]{40}\Z"#)
+                    let isBranchRev = sha1RegEx.matchGroups(in: revision).compactMap{ $0 }.isEmpty
+                    let errorMessage = "could not find " + (isBranchRev ? "a branch named ‘\(revision)’" : "the commit \(revision)")
+                    let mainBranchExists = (try? repository.resolveRevision(identifier: "main")) != nil
+                    let suggestion = (revision == "master" && mainBranchExists) ? "did you mean ‘main’?" : nil
+                    throw GetDependenciesError(
+                        containerIdentifier: identifier.repository.url, reference: revision,
+                        underlyingError: StringError(errorMessage), suggestion: suggestion)
+                }
+            }
+            // If we get this far without having thrown an error, we wrap and throw the underlying error.
+            throw GetDependenciesError(containerIdentifier: identifier.repository.url, reference: revision, underlyingError: error, suggestion: nil)
         }
     }
 

Sources/Workspace/Workspace.swift

@@ -2064,16 +2064,7 @@ extension Workspace {
             return bindings
 
         case .error(let error):
-            switch error {
-            // Emit proper error if we were not able to parse some manifest during dependency resolution.
-            case let error as RepositoryPackageContainer.GetDependenciesErrorWrapper:
-                let location = PackageLocation.Remote(url: error.containerIdentifier, reference: error.reference)
-                diagnostics.emit(error.underlyingError, location: location)
-
-            default:
-                diagnostics.emit(error)
-            }
-
+            diagnostics.emit(error)
             return []
         }
     }

Tests/PackageGraphTests/RepositoryPackageContainerProviderTests.swift

@@ -1,7 +1,7 @@
 /*
  This source file is part of the Swift.org open source project
 
- Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
+ Copyright (c) 2014 - 2020 Apple Inc. and the Swift project authors
  Licensed under Apache License v2.0 with Runtime Library Exception
 
  See http://swift.org/LICENSE.txt for license information
@@ -11,6 +11,7 @@
 import XCTest
 
 import TSCBasic
+import TSCUtility
 import PackageLoading
 import PackageModel
 import PackageGraph
@@ -274,7 +275,7 @@ class RepositoryPackageContainerProviderTests: XCTestCase {
             let revision = try container.getRevision(forTag: "1.0.0")
             do {
                 _ = try container.getDependencies(at: revision.identifier, productFilter: .specific([]))
-            } catch let error as RepositoryPackageContainer.GetDependenciesErrorWrapper {
+            } catch let error as RepositoryPackageContainer.GetDependenciesError {
                 let error = error.underlyingError as! UnsupportedToolsVersion
                 XCTAssertMatch(error.description, .and(.prefix("package at '/' @"), .suffix("is using Swift tools version 3.1.0 which is no longer supported; consider using '// swift-tools-version:4.0' to specify the current tools version")))
             }
@@ -495,4 +496,62 @@ class RepositoryPackageContainerProviderTests: XCTestCase {
             )
         }
     }
+    
+    
+    func testMissingBranchDiagnostics() {
+        mktmpdir { tmpDir in
+            // Create a repository.
+            let packageDir = tmpDir.appending(component: "SomePackage")
+            try localFileSystem.createDirectory(packageDir)
+            initGitRepo(packageDir)
+            let packageRepo = GitRepository(path: packageDir)
+            
+            // Create a package manifest in it (it only needs the `swift-tools-version` part, because we'll supply the manifest later).
+            let manifestFile = packageDir.appending(component: "Package.swift")
+            try localFileSystem.writeFileContents(manifestFile, bytes: ByteString("// swift-tools-version:4.2"))
+            
+            // Commit it and tag it.
+            try packageRepo.stage(file: "Package.swift")
+            try packageRepo.commit(message: "Initial")
+            try packageRepo.tag(name: "1.0.0")
+            
+            // Rename the `master` branch to `main`.
+            try systemQuietly([Git.tool, "-C", packageDir.pathString, "branch", "-m", "main"])
+
+            // Create a repository manager for it.
+            let repoProvider = GitRepositoryProvider()
+            let repositoryManager = RepositoryManager(path: packageDir, provider: repoProvider, delegate: nil, fileSystem: localFileSystem)
+            
+            // Create a container provider, configured with a mock manifest loader that will return the package manifest.
+            let manifest = Manifest.createV4Manifest(
+                name: packageDir.basename,
+                path: packageDir.pathString,
+                url: packageDir.pathString,
+                packageKind: .root,
+                targets: [
+                    TargetDescription(name: packageDir.basename, path: packageDir.pathString)
+                ]
+            )
+            let containerProvider = RepositoryPackageContainerProvider(repositoryManager: repositoryManager, manifestLoader: MockManifestLoader(manifests: [.init(url: packageDir.pathString, version: nil) : manifest]))
+            
+            // Get a hold of the container for the test package.
+            let packageRef = PackageReference(identity: "somepackage", path: packageDir.pathString)
+            let container = try await { containerProvider.getContainer(for: packageRef, completion: $0) } as! RepositoryPackageContainer
+            
+            // Simulate accessing a fictitious dependency on the `master` branch, and check that we get back the expected error.
+            do { _ = try container.getDependencies(at: "master", productFilter: .everything) }
+            catch let error as RepositoryPackageContainer.GetDependenciesError {
+                // We expect to get an error message that mentions main.
+                XCTAssertMatch(error.description, .and(.prefix("could not find a branch named ‘master’"), .suffix("(did you mean ‘main’?)")))
+            }
+            
+            // Simulate accessing a fictitious dependency on some random commit that doesn't exist, and check that we get back the expected error.
+            do { _ = try container.getDependencies(at: "535f4cb5b4a0872fa691473e82d7b27b9894df00", productFilter: .everything) }
+            catch let error as RepositoryPackageContainer.GetDependenciesError {
+                // We expect to get an error message that mentions main.
+                XCTAssertMatch(error.description, .prefix("could not find the commit 535f4cb5b4a0872fa691473e82d7b27b9894df00"))
+            }
+        }
+    }
+
 }