Add IncludePatterns and ExcludePatterns options for Copy #101

aaronlehmann · 2021-04-21T23:08:31Z

Allow include patterns and exclude patterns to be specified, similarly
to Walker.

There is a bit of extra complexity to handle the case of a pattern like
a/*/c. In this case, creating the directories a and a/b may need
to be delayed until we encounter a/b/c.

cc @hinshun

Allow include patterns and exclude patterns to be specified, similarly to Walker. There is a bit of extra complexity to handle the case of a pattern like `a/*/c`. In this case, creating the directories `a` and `a/b` may need to be delayed until we encounter `a/b/c`.

Allow include and exclude patterns to be specified for the "copy" op, similarly to "local". Depends on tonistiigi/fsutil#101

Allow include and exclude patterns to be specified for the "copy" op, similarly to "local". Depends on tonistiigi/fsutil#101 Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

tonistiigi · 2021-04-28T03:44:21Z

prefix/match.go

+)
+
+func Match(pattern, name string) (bool, bool) {
+	count := strings.Count(name, string(filepath.Separator))


Unclear if filepath is correct in here. Iiuc the old code only uses this function for local paths but I see this imported in buildkit/contenthash where all paths should be normalized to unix. At least the test should not pass in non-unix I think.

I think you are right about this. I guess https://github.com/moby/buildkit/pull/2082/files#diff-364fe775b55b3eec6b3ea0859ab920f0981c61d377e47830a46de5ec04058c44R437 does not fail on Windows because there are no test cases that depend on parent dirs being included in the checksum. Not sure if there's a way to simulate a change in parent dir metadata in that test so we can capture that scenario.

tonistiigi · 2021-04-28T03:47:20Z

copy/copy_test.go

@@ -10,6 +10,7 @@ import (

 	"github.com/containerd/continuity/fs/fstest"
 	"github.com/pkg/errors"
+	"github.com/stretchr/testify/assert"


is this assert intentional? Maybe a comment in the check if true.

Not sure I follow, do you prefer require over assert? I generally use assert unless the failed assertion would put the test in a bad state where it doesn't make sense to continue.

We usually use require exclusively so it is a bit out of place and should have a comment so that it is not reverted in the future for consistency.

tonistiigi · 2021-04-28T04:01:18Z

copy/copy.go

 	if !fi.IsDir() {
+		if include {
+			if err := c.createParentDirs(src, srcComponents, target, overwriteTargetMetadata); err != nil {


Isn't this quite a significant added performance overhead to rerun these checks where most of the time directories exist? If true then I think we need some caching. As everything is sorted shouldn't take too much memory.

This does add overhead, but only to the case where an include pattern is matched - so it won't regress performance of any existing cases.

Happy to add caching if you think it's worth the complexity.

I think this is needed. The number of syscalls per file is quite important for copy.

tonistiigi · 2021-04-28T04:04:25Z

copy/copy.go

 			return err
-		} else if !overwriteTargetMetadata {
+		} else if !overwriteTargetMetadata || !includeAll {


bit confused about the includeAll check in here as exclude patterns seem to be not affected by this.

created will always be false if includeAll is false (because in that case we delay creation of parent dirs). So this is shorter version of:

if !includeAll { // directory may not have been created yet, so don't try to set its metadata copyFileInfo = false }

tonistiigi · 2021-04-28T04:06:19Z

copy/copy.go

+			pattern = strings.TrimSuffix(pattern, string(filepath.Separator))
+		}
+
+		if ok, p := prefix.Match(pattern, path); ok {


Why do IncludePatterns/ExcludePatterns use different format/algorithm? Afaics only ExcludePatterns support !. Is there a difference of ExcludePatterns and potentially doing ! in IncludePatterns.

I wanted to match the logic in

fsutil/walker.go

Line 87 in 8599091

if opt != nil {

I think I'm following the same logic (it has separate handling for IncludePatterns and ExcludePatterns and doesn't appear to handle ! in IncludePatterns), but maybe I'm missing something.

I see. I don't remember why it is like this. I guess for .dockerignore is the only use-case atm that required ! and that is the excludes list. IncludePatterns is not used at all anymore as well and FollowPaths is used instead. I guess the FollowPaths case where symlinks need to be followed does not apply here?

I think it is important that something like COPY --filter can be implemented with this. I'm not quite sure if atm client would need to convert some of the patterns to a negative version. But I guess if we would only use one input then with current implementation depending on if user wants to mostly define include or exclude paths one of them would be much slower. Matching with the options of walker is not the most important here so if you think there is a better/simpler logic we can use instead lmk.

tonistiigi · 2021-04-28T04:08:16Z

copy/copy.go

+		return false, errors.Wrap(err, "failed to match excludepatterns")
+	}
+	if m {
+		if fi.IsDir() && c.excludePatternMatcher.Exclusions() {


Not for this PR but this looks non-optimal. I guess ! is rarely used in practice.

Modeled on this code from walker.go:

fsutil/walker.go

Line 131 in 8599091

for _, pat := range pm.Patterns() {

tonistiigi · 2021-04-28T04:08:59Z

copy/copy.go

@@ -109,7 +114,8 @@ func Copy(ctx context.Context, srcRoot, src, dstRoot, dst string, opts ...Opt) e
 		if err != nil {
 			return err
 		}
-		if err := c.copy(ctx, srcFollowed, dst, false); err != nil {
+		includeAll := len(c.includePatterns) == 0


Excludepatterns don't matter in here?

includeAll means we can skip evaluating the include patterns. Usually it's used when a parent dir matches an include pattern. It doesn't skip evaluating exclude patterns.

Maybe skipIncludePatterns would be a better name?

hinshun · 2021-04-29T17:02:18Z

prefix/match.go

+	"strings"
+)
+
+func Match(pattern, name string, slashSeparator bool) (bool, bool) {


Since the return value is (bool, bool) can we use named return values and a godoc for this function?

hinshun · 2021-04-29T17:08:20Z

copy/copy.go

+			pattern = strings.TrimSuffix(pattern, string(filepath.Separator))
+		}
+
+		if ok, p := prefix.Match(pattern, path, false); ok {


This can be simplified:

matched, partial = prefix.Match(pattern, path, false) if matched && !partial { break }

That could potentially overwrite a true value of matched with false

Ah missed that.

hinshun · 2021-04-29T17:55:04Z

copy/copy.go

+	if err != nil {
+		return false, errors.Wrap(err, "failed to match excludepatterns")
+	}
+	if m {


Nit, prefer:

if !m { return false, nil } dirSlash...

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

aaronlehmann · 2021-04-30T21:41:55Z

@tonistiigi: Any more feedback on this PR?

tonistiigi

As commented earlier, different types for include/exclude is a bit of a problem but don't have a specific alternative proposal atm.

aaronlehmann · 2021-05-03T15:36:05Z

Thanks!

Allow include and exclude patterns to be specified for the "copy" op, similarly to "local". Depends on tonistiigi/fsutil#101 Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

aaronlehmann added a commit to aaronlehmann/buildkit that referenced this pull request Apr 22, 2021

Add IncludePatterns and ExcludePatterns options for Copy

e9a93e4

Allow include and exclude patterns to be specified for the "copy" op, similarly to "local". Depends on tonistiigi/fsutil#101

aaronlehmann mentioned this pull request Apr 22, 2021

Add IncludePatterns and ExcludePatterns options for Copy moby/buildkit#2082

Merged

tonistiigi reviewed Apr 28, 2021

View reviewed changes

aaronlehmann added 4 commits April 28, 2021 13:24

Rename includeAll to skipIncludePatterns

08e5241

Use "require" instead of "assert"

e5bab58

Add slashSeparator argument to Match

b39921c

Cache whether parent dir entries have been copied yet

accc0fa

aaronlehmann mentioned this pull request Apr 28, 2021

Add includePatterns and excludePatterns support for copy openllb/hlb#239

Merged

hinshun reviewed Apr 29, 2021

View reviewed changes

aaronlehmann added 2 commits April 30, 2021 10:46

Use named returns and add godoc comment for Match

34d3aed

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>

Code style review suggestion

10412d7

tonistiigi approved these changes May 3, 2021

View reviewed changes

aaronlehmann merged commit 5c8be85 into tonistiigi:master May 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IncludePatterns and ExcludePatterns options for Copy #101

Add IncludePatterns and ExcludePatterns options for Copy #101

aaronlehmann commented Apr 21, 2021

tonistiigi Apr 28, 2021

aaronlehmann Apr 28, 2021

tonistiigi Apr 28, 2021

aaronlehmann Apr 28, 2021

tonistiigi Apr 28, 2021

tonistiigi Apr 28, 2021

aaronlehmann Apr 28, 2021

tonistiigi Apr 28, 2021

tonistiigi Apr 28, 2021

aaronlehmann Apr 28, 2021

tonistiigi Apr 28, 2021

aaronlehmann Apr 28, 2021

tonistiigi Apr 28, 2021

tonistiigi Apr 28, 2021

aaronlehmann Apr 28, 2021

tonistiigi Apr 28, 2021

aaronlehmann Apr 28, 2021

hinshun Apr 29, 2021

hinshun Apr 29, 2021

aaronlehmann Apr 29, 2021

hinshun Apr 29, 2021

hinshun Apr 29, 2021

aaronlehmann commented Apr 30, 2021

tonistiigi left a comment

aaronlehmann commented May 3, 2021

Add IncludePatterns and ExcludePatterns options for Copy #101

Add IncludePatterns and ExcludePatterns options for Copy #101

Conversation

aaronlehmann commented Apr 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Apr 30, 2021

tonistiigi left a comment

Choose a reason for hiding this comment

aaronlehmann commented May 3, 2021