Skip to content

Commit a805a37

Browse files
neonichuneonichu
authored
isValidDirectory and isValidRefFormat should be throwing (#7033)
Currently, it is impossible to figure out any failures of these, despite them stopping any package operations entirely if they fail. Instead, we should make them throwing and bubble up any errors. rdar://117442702
1 parent b109f3c commit a805a37

File tree

10 files changed

+65
-53
lines changed

10 files changed

+65
-53
lines changed

‎Sources/Commands/PackageTools/ArchiveSource.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ extension SwiftPackageTool {
6767
) throws {
6868
let gitRepositoryProvider = GitRepositoryProvider()
6969
if gitRepositoryProvider.repositoryExists(at: packageDirectory) &&
70-
gitRepositoryProvider.isValidDirectory(packageDirectory){
70+
(try? gitRepositoryProvider.isValidDirectory(packageDirectory)) == true {
7171
let repository = GitRepository(path: packageDirectory, cancellator: cancellator)
7272
try repository.archive(to: archivePath)
7373
} else {

‎Sources/PackageLoading/ManifestLoader+Validation.swift

Lines changed: 37 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -221,12 +221,20 @@ public struct ManifestValidator {
221221
// validate source control ref
222222
switch dependency.requirement {
223223
case .branch(let name):
224-
if !self.sourceControlValidator.isValidRefFormat(name) {
225-
diagnostics.append(.invalidSourceControlBranchName(name))
224+
do {
225+
if try !self.sourceControlValidator.isValidRefFormat(name) {
226+
diagnostics.append(.invalidSourceControlBranchName(name))
227+
}
228+
} catch {
229+
diagnostics.append(.invalidSourceControlBranchName(name, underlyingError: error))
226230
}
227231
case .revision(let revision):
228-
if !self.sourceControlValidator.isValidRefFormat(revision) {
229-
diagnostics.append(.invalidSourceControlRevision(revision))
232+
do {
233+
if try !self.sourceControlValidator.isValidRefFormat(revision) {
234+
diagnostics.append(.invalidSourceControlRevision(revision))
235+
}
236+
} catch {
237+
diagnostics.append(.invalidSourceControlRevision(revision, underlyingError: error))
230238
}
231239
default:
232240
break
@@ -235,20 +243,24 @@ public struct ManifestValidator {
235243
// there is a case to be made to throw early (here) if the path does not exists
236244
// but many of our tests assume they can pass a non existent path
237245
if case .local(let localPath) = dependency.location, self.fileSystem.exists(localPath) {
238-
if !self.sourceControlValidator.isValidDirectory(localPath) {
239-
// Provides better feedback when mistakingly using url: for a dependency that
240-
// is a local package. Still allows for using url with a local package that has
241-
// also been initialized by git
242-
diagnostics.append(.invalidSourceControlDirectory(localPath))
246+
do {
247+
if try !self.sourceControlValidator.isValidDirectory(localPath) {
248+
// Provides better feedback when mistakingly using url: for a dependency that
249+
// is a local package. Still allows for using url with a local package that has
250+
// also been initialized by git
251+
diagnostics.append(.invalidSourceControlDirectory(localPath))
252+
}
253+
} catch {
254+
diagnostics.append(.invalidSourceControlDirectory(localPath, underlyingError: error))
243255
}
244256
}
245257
return diagnostics
246258
}
247259
}
248260

249261
public protocol ManifestSourceControlValidator {
250-
func isValidRefFormat(_ revision: String) -> Bool
251-
func isValidDirectory(_ path: AbsolutePath) -> Bool
262+
func isValidRefFormat(_ revision: String) throws -> Bool
263+
func isValidDirectory(_ path: AbsolutePath) throws -> Bool
252264
}
253265

254266
extension Basics.Diagnostic {
@@ -304,16 +316,24 @@ extension Basics.Diagnostic {
304316
""")
305317
}
306318

307-
static func invalidSourceControlBranchName(_ name: String) -> Self {
308-
.error("invalid branch name: '\(name)'")
319+
static func errorSuffix(_ error: Error?) -> String {
320+
if let error {
321+
return ": \(error.interpolationDescription)"
322+
} else {
323+
return ""
324+
}
325+
}
326+
327+
static func invalidSourceControlBranchName(_ name: String, underlyingError: Error? = nil) -> Self {
328+
.error("invalid branch name: '\(name)'\(errorSuffix(underlyingError))")
309329
}
310330

311-
static func invalidSourceControlRevision(_ revision: String) -> Self {
312-
.error("invalid revision: '\(revision)'")
331+
static func invalidSourceControlRevision(_ revision: String, underlyingError: Error? = nil) -> Self {
332+
.error("invalid revision: '\(revision)'\(errorSuffix(underlyingError))")
313333
}
314334

315-
static func invalidSourceControlDirectory(_ path: AbsolutePath) -> Self {
316-
.error("cannot clone from local directory \(path)\nPlease git init or use \"path:\" for \(path)")
335+
static func invalidSourceControlDirectory(_ path: AbsolutePath, underlyingError: Error? = nil) -> Self {
336+
.error("cannot clone from local directory \(path)\nPlease git init or use \"path:\" for \(path)\(errorSuffix(underlyingError))")
317337
}
318338
}
319339

‎Sources/PackageMetadata/PackageMetadata.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,7 @@ public struct PackageSearchClient {
253253
to: tempPath,
254254
progressHandler: nil
255255
)
256-
if self.repositoryProvider.isValidDirectory(tempPath),
256+
if try self.repositoryProvider.isValidDirectory(tempPath),
257257
let repository = try self.repositoryProvider.open(
258258
repository: repositorySpecifier,
259259
at: tempPath

‎Sources/SPMTestSupport/InMemoryGitRepository.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -482,11 +482,11 @@ public final class InMemoryGitRepositoryProvider: RepositoryProvider {
482482
return checkout
483483
}
484484

485-
public func isValidDirectory(_ directory: AbsolutePath) -> Bool {
485+
public func isValidDirectory(_ directory: AbsolutePath) throws -> Bool {
486486
return true
487487
}
488488

489-
public func isValidRefFormat(_ ref: String) -> Bool {
489+
public func isValidRefFormat(_ ref: String) throws -> Bool {
490490
return true
491491
}
492492

‎Sources/SourceControl/GitRepository.swift

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -205,23 +205,15 @@ public struct GitRepositoryProvider: RepositoryProvider, Cancellable {
205205
return localFileSystem.isDirectory(directory)
206206
}
207207

208-
public func isValidDirectory(_ directory: Basics.AbsolutePath) -> Bool {
209-
do {
210-
let result = try self.git.run(["-C", directory.pathString, "rev-parse", "--git-dir"])
211-
return result == ".git" || result == "." || result == directory.pathString
212-
} catch {
213-
return false
214-
}
208+
public func isValidDirectory(_ directory: Basics.AbsolutePath) throws -> Bool {
209+
let result = try self.git.run(["-C", directory.pathString, "rev-parse", "--git-dir"])
210+
return result == ".git" || result == "." || result == directory.pathString
215211
}
216212

217213
/// Returns true if the git reference name is well formed.
218-
public func isValidRefFormat(_ ref: String) -> Bool {
219-
do {
220-
_ = try self.git.run(["check-ref-format", "--allow-onelevel", ref])
221-
return true
222-
} catch {
223-
return false
224-
}
214+
public func isValidRefFormat(_ ref: String) throws -> Bool {
215+
_ = try self.git.run(["check-ref-format", "--allow-onelevel", ref])
216+
return true
225217
}
226218

227219
public func copy(from sourcePath: Basics.AbsolutePath, to destinationPath: Basics.AbsolutePath) throws {

‎Sources/SourceControl/Repository.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,10 +143,10 @@ public protocol RepositoryProvider: Cancellable {
143143
func copy(from sourcePath: AbsolutePath, to destinationPath: AbsolutePath) throws
144144

145145
/// Returns true if the directory is valid git location.
146-
func isValidDirectory(_ directory: AbsolutePath) -> Bool
146+
func isValidDirectory(_ directory: AbsolutePath) throws -> Bool
147147

148148
/// Returns true if the git reference name is well formed.
149-
func isValidRefFormat(_ ref: String) -> Bool
149+
func isValidRefFormat(_ ref: String) throws -> Bool
150150
}
151151

152152
/// Abstract repository operations.

‎Sources/SourceControl/RepositoryManager.swift

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,7 @@ public class RepositoryManager: Cancellable {
216216
self.delegate?.didUpdate(package: package, repository: handle.repository, duration: duration)
217217
}
218218
return handle
219-
} else if self.provider.isValidDirectory(repositoryPath) {
219+
} else if try self.provider.isValidDirectory(repositoryPath) {
220220
return handle
221221
}
222222
}
@@ -334,7 +334,7 @@ public class RepositoryManager: Cancellable {
334334
}
335335
} catch {
336336
// If we are offline and have a valid cached repository, use the cache anyway.
337-
if isOffline(error) && self.provider.isValidDirectory(cachedRepositoryPath) {
337+
if try isOffline(error) && self.provider.isValidDirectory(cachedRepositoryPath) {
338338
// For the first offline use in the lifetime of this repository manager, emit a warning.
339339
if self.emitNoConnectivityWarning.get(default: false) {
340340
self.emitNoConnectivityWarning.put(false)
@@ -422,13 +422,13 @@ public class RepositoryManager: Cancellable {
422422
}
423423

424424
/// Returns true if the directory is valid git location.
425-
public func isValidDirectory(_ directory: AbsolutePath) -> Bool {
426-
self.provider.isValidDirectory(directory)
425+
public func isValidDirectory(_ directory: AbsolutePath) throws -> Bool {
426+
try self.provider.isValidDirectory(directory)
427427
}
428428

429429
/// Returns true if the git reference name is well formed.
430-
public func isValidRefFormat(_ ref: String) -> Bool {
431-
self.provider.isValidRefFormat(ref)
430+
public func isValidRefFormat(_ ref: String) throws -> Bool {
431+
try self.provider.isValidRefFormat(ref)
432432
}
433433

434434
/// Reset the repository manager.

‎Tests/PackageLoadingTests/PDLoadingTests.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -190,11 +190,11 @@ final class ManifestTestDelegate: ManifestLoaderDelegate {
190190
}
191191

192192
fileprivate struct NOOPManifestSourceControlValidator: ManifestSourceControlValidator {
193-
func isValidRefFormat(_ revision: String) -> Bool {
193+
func isValidRefFormat(_ revision: String) throws -> Bool {
194194
true
195195
}
196196

197-
func isValidDirectory(_ path: AbsolutePath) -> Bool {
197+
func isValidDirectory(_ path: AbsolutePath) throws -> Bool {
198198
true
199199
}
200200
}

‎Tests/SourceControlTests/RepositoryManagerTests.swift

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -557,11 +557,11 @@ class RepositoryManagerTests: XCTestCase {
557557
fatalError("should not be called")
558558
}
559559

560-
func isValidDirectory(_ directory: AbsolutePath) -> Bool {
560+
func isValidDirectory(_ directory: AbsolutePath) throws -> Bool {
561561
fatalError("should not be called")
562562
}
563563

564-
func isValidRefFormat(_ ref: String) -> Bool {
564+
func isValidRefFormat(_ ref: String) throws -> Bool {
565565
fatalError("should not be called")
566566
}
567567

@@ -640,11 +640,11 @@ private class DummyRepository: Repository {
640640
fatalError("unexpected API call")
641641
}
642642

643-
func isValidDirectory(_ directory: AbsolutePath) -> Bool {
643+
func isValidDirectory(_ directory: AbsolutePath) throws -> Bool {
644644
fatalError("unexpected API call")
645645
}
646646

647-
func isValidRefFormat(_ ref: String) -> Bool {
647+
func isValidRefFormat(_ ref: String) throws -> Bool {
648648
fatalError("unexpected API call")
649649
}
650650

@@ -724,11 +724,11 @@ private class DummyRepositoryProvider: RepositoryProvider {
724724
return DummyWorkingCheckout(at: path)
725725
}
726726

727-
func isValidDirectory(_ directory: AbsolutePath) -> Bool {
727+
func isValidDirectory(_ directory: AbsolutePath) throws -> Bool {
728728
return true
729729
}
730730

731-
func isValidRefFormat(_ ref: String) -> Bool {
731+
func isValidRefFormat(_ ref: String) throws -> Bool {
732732
return true
733733
}
734734

‎Tests/WorkspaceTests/SourceControlPackageContainerTests.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,11 +146,11 @@ private class MockRepositories: RepositoryProvider {
146146
fatalError("unexpected API call")
147147
}
148148

149-
func isValidDirectory(_ directory: AbsolutePath) -> Bool {
149+
func isValidDirectory(_ directory: AbsolutePath) throws -> Bool {
150150
return true
151151
}
152152

153-
func isValidRefFormat(_ ref: String) -> Bool {
153+
func isValidRefFormat(_ ref: String) throws -> Bool {
154154
return true
155155
}
156156

0 commit comments

Comments
 (0)