Add isGitIgnored method for checking if a file is ignored by git #1613

ViktorSimko · 2018-06-20T22:03:34Z

These are the Git related changes that were a part of #1507 .

Add getIgnoredFiles to GitRepository

This makes an improvement performance-wise, because it can be used to check only the files we are concerned about

…ocation This way there is no need to create the shell process for every path separately which can improve the performance

aciidgh · 2018-06-21T14:51:36Z

Sources/SourceControl/GitRepository.swift

+    public func isGitIgnored(_ paths: [AbsolutePath]) throws -> [Bool] {
+        return try queue.sync {
+            let stringPaths = paths.map({ $0.asString })
+            let args = [Git.tool, "-C", self.path.asString, "check-ignore"] + stringPaths


This might cause issues for two reasons:

Paths with spaces needs to be escaped (we should test this).

The command-line invocation might surpass ARG_MAX.

I recommend implementing this with git's -z and --stdlib options so we can use a file to provide the input list. See https://git-scm.com/docs/git-check-ignore

So we should write the paths to a file and then redirect the file to the stdin of the process and then finally remove the file?

Yep! We can use the TemporaryFile class from Basic module.

Ok, Thank you!

It seems like the redirection isn't working using Process with args. Maybe it's escaping the <?

Yeah, I think you'll have to use ["sh", "-c", "all args"]

This ensures that we won't surpass ARG_MAX with the arguments to check-ignore, and also that paths now can include spaces

ViktorSimko · 2018-06-21T19:36:45Z

@aciidb0mb3r Thank you, I've managed to get it working that way.

ViktorSimko · 2018-06-22T18:19:45Z

Should we rename the method to areFilesIgnored(at:)?

aciidgh · 2018-06-28T01:08:23Z

Sources/SourceControl/GitRepository.swift

@@ -387,6 +398,33 @@ public class GitRepository: Repository, WorkingCheckout {
        return localFileSystem.isDirectory(AbsolutePath(firstLine))
    }

+    /// Returns true if the file at `path` is ignored by `git`
+    public func isGitIgnored(_ paths: [AbsolutePath]) throws -> [Bool] {


How about arePathsIgnored? or just areIgnored is also fine with me.

aciidgh · 2018-06-28T01:09:48Z

Sources/SourceControl/GitRepository.swift

+
+            let pathsFile = try TemporaryFile()
+            let pathsData = Data(pathsFileContent.utf8)
+            pathsFile.fileHandle.write(pathsData)


We should use BufferedOutputByteStream and localFileSystem to APIs to write to the temp file.

aciidgh · 2018-06-28T01:10:52Z

Sources/SourceControl/GitRepository.swift

+            let pathsData = Data(pathsFileContent.utf8)
+            pathsFile.fileHandle.write(pathsData)
+
+            let args = [Git.tool, "-C", self.path.asString, "check-ignore", "-z", "--stdin", "<", "\(pathsFile.path.asString)"]


We need to shell quotes the arguments here as we'll be using sh -c.

We should have a test for this.

aciidgh

Thanks! This mostly looks great, I left some minor comments.

aciidgh · 2018-06-28T14:32:07Z

Sources/SourceControl/GitRepository.swift

+            let pathsFile = try TemporaryFile()
+            try localFileSystem.writeFileContents(pathsFile.path) { $0 <<< pathsFileContent }
+
+            let args = [Git.tool, "-C", self.path.asString, "check-ignore", "-z", "--stdin", "<", "\(pathsFile.path.asString)"]


We need to shell escape the args here, we should also have a test for it.

aciidgh · 2018-06-28T14:33:42Z

Sources/SourceControl/GitRepository.swift

+    public func areIgnored(_ paths: [AbsolutePath]) throws -> [Bool] {
+        return try queue.sync {
+            let stringPaths = paths.map({ $0.asString })
+            let pathsFileContent = stringPaths.joined(separator: "\0")


It'll be better for performance if you append to the stream directly:

try localFileSystem.writeFileContents(pathsFile.path) { for path in paths { $0 <<< path.asString <<< "\0" } }

aciidgh · 2018-06-28T14:34:50Z

Sources/SourceControl/GitRepository.swift

@@ -92,6 +92,13 @@ public class GitRepositoryProvider: RepositoryProvider {
        }
    }

+    public func checkoutExists(at path: AbsolutePath) throws -> Bool {
+        precondition(exists(path))


Shouldn't we return false if the path doesn't exist?

We could return false, but I think that the purpose of this method is to determine if the checkout exists, not the path itself, so in my opinion, it's more like a precondition.

aciidgh · 2018-06-28T17:02:00Z

Sources/SourceControl/GitRepository.swift

+                }
+            }
+
+            let args = [Git.tool, "-C", "\"\(self.path.asString)\"", "check-ignore", "-z", "--stdin", "<", "\"\(pathsFile.path.asString)\""]


This might not be enough. We can use this method to properly escape: https://github.com/apple/swift-package-manager/blob/master/Sources/Basic/StringConversions.swift#L45

aciidgh · 2018-06-28T17:21:56Z

Sources/SourceControl/GitRepository.swift

+                return String(cString: bytes + [0])
+            })
+
+            guard result.exitStatus == .terminated(code: 0) || result.exitStatus == .terminated(code: 1) else {


The documentation says check-ignore will return 1 when "None of the provided paths are ignored". We shouldn't throw fatal in that case, right?

This doesn't throw on exit code 1. Only on exit codes different than 0 or 1.
This case is also included in the unit test.

oh ya, right!

aciidgh · 2018-06-28T19:25:38Z

@swift-ci please smoke test

aciidgh · 2018-06-28T21:25:00Z

Thanks a lot for working on this @ViktorSimko!

ViktorSimko · 2018-06-29T05:23:09Z

@aciidb0mb3r Thank you for helping me to be able to get this done, I've learnt a lot from this!

ViktorSimko added 5 commits June 20, 2018 23:49

Ignore files from .gitignore

c1184a2

Add getIgnoredFiles to GitRepository

Use git check-ignore for filtering files

9881587

This makes an improvement performance-wise, because it can be used to check only the files we are concerned about

Fix checkoutExists and add test for it

7ee2d12

Fix isIgnored and add test for it

c24b081

Modify isGitIgnored to be able to process multiple paths in one inv…

970d0c0

…ocation This way there is no need to create the shell process for every path separately which can improve the performance

aciidgh reviewed Jun 21, 2018

View reviewed changes

Use file input for check-ignore

c75b92f

This ensures that we won't surpass ARG_MAX with the arguments to check-ignore, and also that paths now can include spaces

aciidgh reviewed Jun 28, 2018

View reviewed changes

aciidgh added the WIP Work in progress label Jun 28, 2018

ViktorSimko added 2 commits June 28, 2018 15:28

Rename isGitIgnored to areIgnored

a79e038

Use localFileSystem to write the temporary file with the paths

859f58e

aciidgh reviewed Jun 28, 2018

View reviewed changes

ViktorSimko added 2 commits June 28, 2018 16:42

Add quotes around the path arguments for git in areIgnored

dd335c5

Append paths directly to the output stream

9d037b4

aciidgh reviewed Jun 28, 2018

View reviewed changes

Use shellEscaped on path arguments in areIgnored

118b505

aciidgh reviewed Jun 28, 2018

View reviewed changes

aciidgh merged commit 228f1b0 into swiftlang:master Jun 28, 2018

Add isGitIgnored method for checking if a file is ignored by git #1613

Add isGitIgnored method for checking if a file is ignored by git #1613

Uh oh!

Conversation

ViktorSimko commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorSimko commented Jun 21, 2018

Uh oh!

ViktorSimko commented Jun 22, 2018

Uh oh!

aciidgh Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aciidgh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aciidgh commented Jun 28, 2018

Uh oh!

aciidgh commented Jun 28, 2018

Uh oh!

ViktorSimko commented Jun 29, 2018

Uh oh!

Uh oh!

aciidgh Jun 28, 2018 •

edited

Loading