Skip to content

Ignore symlinks and hidden (dot) files during --recursive. #644

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,10 @@ let package = Package(
.product(name: "SwiftSyntaxBuilder", package: "swift-syntax"),
]
),
.testTarget(
name: "swift-formatTests",
dependencies: ["swift-format"]
),
]
)

Expand Down
9 changes: 7 additions & 2 deletions Sources/swift-format/Frontend/Frontend.swift
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,17 @@ class Frontend {
"processURLs(_:) should only be called when 'urls' is non-empty.")

if parallel {
let filesToProcess = FileIterator(urls: urls).compactMap(openAndPrepareFile)
let filesToProcess =
FileIterator(urls: urls, followSymlinks: lintFormatOptions.followSymlinks)
.compactMap(openAndPrepareFile)
DispatchQueue.concurrentPerform(iterations: filesToProcess.count) { index in
processFile(filesToProcess[index])
}
} else {
FileIterator(urls: urls).lazy.compactMap(openAndPrepareFile).forEach(processFile)
FileIterator(urls: urls, followSymlinks: lintFormatOptions.followSymlinks)
.lazy
.compactMap(openAndPrepareFile)
.forEach(processFile)
}
}

Expand Down
7 changes: 7 additions & 0 deletions Sources/swift-format/Subcommands/LintFormatOptions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,13 @@ struct LintFormatOptions: ParsableArguments {
""")
var colorDiagnostics: Bool?

/// Whether symlinks should be followed.
@Flag(help: """
Follow symbolic links passed on the command line, or found during directory traversal when \
using `-r/--recursive`.
""")
var followSymlinks: Bool = false

/// The list of paths to Swift source files that should be formatted or linted.
@Argument(help: "Zero or more input filenames.")
var paths: [String] = []
Expand Down
115 changes: 84 additions & 31 deletions Sources/swift-format/Utilities/FileIterator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,54 +14,82 @@ import Foundation

/// Iterator for looping over lists of files and directories. Directories are automatically
/// traversed recursively, and we check for files with a ".swift" extension.
struct FileIterator: Sequence, IteratorProtocol {
@_spi(Testing)
public struct FileIterator: Sequence, IteratorProtocol {

/// List of file and directory URLs to iterate over.
let urls: [URL]
private let urls: [URL]

/// If true, symlinks will be followed when iterating over directories and files. If not, they
/// will be ignored.
private let followSymlinks: Bool

/// Iterator for the list of URLs.
var urlIterator: Array<URL>.Iterator
private var urlIterator: Array<URL>.Iterator

/// Iterator for recursing through directories.
var dirIterator: FileManager.DirectoryEnumerator? = nil
private var dirIterator: FileManager.DirectoryEnumerator? = nil

/// The current working directory of the process, which is used to relativize URLs of files found
/// during iteration.
let workingDirectory = URL(fileURLWithPath: ".")
private let workingDirectory = URL(fileURLWithPath: ".")

/// Keep track of the current directory we're recursing through.
var currentDirectory = URL(fileURLWithPath: "")
private var currentDirectory = URL(fileURLWithPath: "")

/// Keep track of files we have visited to prevent duplicates.
var visited: Set<String> = []
private var visited: Set<String> = []

/// The file extension to check for when recursing through directories.
let fileSuffix = ".swift"
private let fileSuffix = ".swift"

/// Create a new file iterator over the given list of file URLs.
///
/// The given URLs may be files or directories. If they are directories, the iterator will recurse
/// into them.
init(urls: [URL]) {
public init(urls: [URL], followSymlinks: Bool) {
self.urls = urls
self.urlIterator = self.urls.makeIterator()
self.followSymlinks = followSymlinks
}

/// Iterate through the "paths" list, and emit the file paths in it. If we encounter a directory,
/// recurse through it and emit .swift file paths.
mutating func next() -> URL? {
public mutating func next() -> URL? {
var output: URL? = nil
while output == nil {
// Check if we're recursing through a directory.
if dirIterator != nil {
output = nextInDirectory()
} else {
guard let next = urlIterator.next() else { return nil }
var isDir: ObjCBool = false
if FileManager.default.fileExists(atPath: next.path, isDirectory: &isDir), isDir.boolValue {
dirIterator = FileManager.default.enumerator(at: next, includingPropertiesForKeys: nil)
guard var next = urlIterator.next() else {
// If we've reached the end of all the URLs we wanted to iterate over, exit now.
return nil
}

guard let fileType = fileType(at: next) else {
continue
}

switch fileType {
case .typeSymbolicLink:
guard
followSymlinks,
let destination = try? FileManager.default.destinationOfSymbolicLink(atPath: next.path)
else {
break
}
next = URL(fileURLWithPath: destination)
fallthrough

case .typeDirectory:
dirIterator = FileManager.default.enumerator(
at: next,
includingPropertiesForKeys: nil,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could prefetch the fileResourceTypeKey here. I'm not sure how much it'll speed up the iteration, but I think it'll combine the later look-up into whenever the iterator does file IO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately it looks like the whole URL.resourceValues API is busted on Linux. Switched over to FileManager.attributesOfItem, which doesn't have the issue (but also doesn't benefit from pre-fetching).

options: [.skipsHiddenFiles])
currentDirectory = next
} else {

default:
// We'll get here if the path is a file, or if it doesn't exist. In the latter case,
// return the path anyway; we'll turn the error we get when we try to open the file into
// an appropriate diagnostic instead of trying to handle it here.
Expand All @@ -82,23 +110,41 @@ struct FileIterator: Sequence, IteratorProtocol {
private mutating func nextInDirectory() -> URL? {
var output: URL? = nil
while output == nil {
if let item = dirIterator?.nextObject() as? URL {
if item.lastPathComponent.hasSuffix(fileSuffix) {
var isDir: ObjCBool = false
if FileManager.default.fileExists(atPath: item.path, isDirectory: &isDir)
&& !isDir.boolValue
{
// We can't use the `.producesRelativePathURLs` enumeration option because it isn't
// supported yet on Linux, so we need to relativize the URL ourselves.
let relativePath =
item.path.hasPrefix(workingDirectory.path)
? String(item.path.dropFirst(workingDirectory.path.count + 1))
: item.path
output =
URL(fileURLWithPath: relativePath, isDirectory: false, relativeTo: workingDirectory)
}
guard let item = dirIterator?.nextObject() as? URL else {
break
}

guard item.lastPathComponent.hasSuffix(fileSuffix), let fileType = fileType(at: item) else {
continue
}

var path = item.path
switch fileType {
case .typeSymbolicLink where followSymlinks:
guard
let destination = try? FileManager.default.destinationOfSymbolicLink(atPath: path)
else {
break
}
} else { break }
path = destination
fallthrough

case .typeRegular:
// We attempt to relativize the URLs based on the current working directory, not the
// directory being iterated over, so that they can be displayed better in diagnostics. Thus,
// if the user passes paths that are relative to the current working diectory, they will
// be displayed as relative paths. Otherwise, they will still be displayed as absolute
// paths.
let relativePath =
path.hasPrefix(workingDirectory.path)
? String(path.dropFirst(workingDirectory.path.count + 1))
: path
output =
URL(fileURLWithPath: relativePath, isDirectory: false, relativeTo: workingDirectory)

default:
break
}
}
// If we've exhausted the files in the directory recursion, unset the directory iterator.
if output == nil {
Expand All @@ -107,3 +153,10 @@ struct FileIterator: Sequence, IteratorProtocol {
return output
}
}

/// Returns the type of the file at the given URL.
private func fileType(at url: URL) -> FileAttributeType? {
// We cannot use `URL.resourceValues(forKeys:)` here because it appears to behave incorrectly on
// Linux.
return try? FileManager.default.attributesOfItem(atPath: url.path)[.type] as? FileAttributeType
}
91 changes: 91 additions & 0 deletions Tests/swift-formatTests/Utilities/FileIteratorTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import XCTest

@_spi(Testing) import swift_format

final class FileIteratorTests: XCTestCase {
private var tmpdir: URL!

override func setUpWithError() throws {
tmpdir = try FileManager.default.url(
for: .itemReplacementDirectory,
in: .userDomainMask,
appropriateFor: FileManager.default.temporaryDirectory,
create: true)

// Create a simple file tree used by the tests below.
try touch("project/real1.swift")
try touch("project/real2.swift")
try touch("project/.hidden.swift")
try touch("project/.build/generated.swift")
try symlink("project/link.swift", to: "project/.hidden.swift")
}

override func tearDownWithError() throws {
try FileManager.default.removeItem(at: tmpdir)
}

func testNoFollowSymlinks() {
let seen = allFilesSeen(iteratingOver: [tmpdir], followSymlinks: false)
XCTAssertEqual(seen.count, 2)
XCTAssertTrue(seen.contains { $0.hasSuffix("project/real1.swift") })
XCTAssertTrue(seen.contains { $0.hasSuffix("project/real2.swift") })
}

func testFollowSymlinks() {
let seen = allFilesSeen(iteratingOver: [tmpdir], followSymlinks: true)
XCTAssertEqual(seen.count, 3)
XCTAssertTrue(seen.contains { $0.hasSuffix("project/real1.swift") })
XCTAssertTrue(seen.contains { $0.hasSuffix("project/real2.swift") })
// Hidden but found through the visible symlink project/link.swift
XCTAssertTrue(seen.contains { $0.hasSuffix("project/.hidden.swift") })
}

func testTraversesHiddenFilesIfExplicitlySpecified() {
let seen = allFilesSeen(
iteratingOver: [tmpURL("project/.build"), tmpURL("project/.hidden.swift")],
followSymlinks: false)
XCTAssertEqual(seen.count, 2)
XCTAssertTrue(seen.contains { $0.hasSuffix("project/.build/generated.swift") })
XCTAssertTrue(seen.contains { $0.hasSuffix("project/.hidden.swift") })
}

func testDoesNotFollowSymlinksIfFollowSymlinksIsFalseEvenIfExplicitlySpecified() {
// Symlinks are not traversed even if `followSymlinks` is false even if they are explicitly
// passed to the iterator. This is meant to avoid situations where a symlink could be hidden by
// shell expansion; for example, if the user writes `swift-format --no-follow-symlinks *`, if
// the current directory contains a symlink, they would probably *not* expect it to be followed.
let seen = allFilesSeen(iteratingOver: [tmpURL("project/link.swift")], followSymlinks: false)
XCTAssertTrue(seen.isEmpty)
}
}

extension FileIteratorTests {
/// Returns a URL to a file or directory in the test's temporary space.
private func tmpURL(_ path: String) -> URL {
return tmpdir.appendingPathComponent(path, isDirectory: false)
}

/// Create an empty file at the given path in the test's temporary space.
private func touch(_ path: String) throws {
let fileURL = tmpURL(path)
try FileManager.default.createDirectory(
at: fileURL.deletingLastPathComponent(), withIntermediateDirectories: true)
FileManager.default.createFile(atPath: fileURL.path, contents: Data())
}

/// Create a symlink between files or directories in the test's temporary space.
private func symlink(_ source: String, to target: String) throws {
try FileManager.default.createSymbolicLink(
at: tmpURL(source), withDestinationURL: tmpURL(target))
}

/// Computes the list of all files seen by using `FileIterator` to iterate over the given URLs.
private func allFilesSeen(iteratingOver urls: [URL], followSymlinks: Bool) -> [String] {
let iterator = FileIterator(urls: urls, followSymlinks: followSymlinks)
var seen: [String] = []
for next in iterator {
seen.append(next.path)
}
return seen
}
}