Fix FilterFS when skipping lazy parent directories
#183
Conversation
When using include/exclude options in a file walk, we lazily store the
parents, and only later call the target function with them when we find
a file inside. This handles the case where `**/*.go` should only walk
directories that contain `.go` files, and not every directory in the
tree.
However, when storing lazy parents, we weren't appropriately handling
the `SkipDir` value that could be returned. We were making two incorrect
assumptions:
1. An error value from `fn()` was always an error case - this isn't
true, since we have control error values like `SkipDir`. Because of
this, we need to ensure that `calledFn` is *always* set to true when
calling `fn` (even in an error case).
2. We can return `SkipDir` to skip this parent directory. This isn't
*quite* right, since returning `SkipDir` would return it only for the
child directory - we need to make sure it's called everytime for each
child directory of this parent directory, by storing it as a boolean
`skipFn` field.
With these changes, the new test passes, and we don't end up with
duplicate parent calls, as was happening before:
Error: Not equal:
expected: []string{"foo"}
actual : []string{"foo", "foo"}
Signed-off-by: Justin Chadwell <[email protected]>
Similar to the previous commit, we weren't properly respecting MapResult return values for parent directories. To resolve this, we just store into `skipFn` just like previously, and return `SkipDir` instead of `continue`ing on. After these changes, the new test passes, and we don't end up including entries that were in excluded directories, as was happening before: Error: Not equal: expected: "dir y\nfile y/b.txt\n" actual : "file x/a.txt\ndir y\nfile y/b.txt\n" Signed-off-by: Justin Chadwell <[email protected]>
| }) | ||
| assert.NoError(t, err) | ||
|
|
||
| assert.Equal(t, []string{"foo"}, found) |
There was a problem hiding this comment.
Why would this return anything at all. "foo" does not match the IncludePatterns.
There was a problem hiding this comment.
I assume because the Walk recurses into foo to see if anything matches the IncludePatterns...
There was a problem hiding this comment.
Walk finds a file that matches the include patterns, then starts calling Walk on parent directories, and the first Walk callback func returns SkipDir...
There was a problem hiding this comment.
I don't understand how this makes sense. IncludePatterns is an option to Walk. Callback should not be called on paths that don't match IncludePatterns.
There was a problem hiding this comment.
If it's not included there then it isn't hooked up to Send/Recv - which means that we don't send the file stats for parent directories.
e.g. foo as this parent should have it's permissions/etc transferred through these methods. Potentially Send shouldn't use Walk at all, but that seems out of scope for this IMO, as it's a larger refactor and architectural change (not really convinced by it either).
| continue | ||
| } else if result == MapResultSkipDir { |
There was a problem hiding this comment.
nit: don't need else if after a continue.
Original issue found by @vito in dagger/dagger#6687.
Split into two commits, both with slightly different issues:
filter: allow SkipDir return with lazy parents. Ensures that walker functions that returnfilepath.SkipDirare always respected, even when evaluated as a lazy parent function (see commit for more details).filter: ensure MapResult is followed for parent directories. Ensures that theFilterFSMap option forMapResultSkipDiris always respected, even when evaluated as a lazy parent function (see commit for more details).These issues seem to have been present for a while (including prior to #167, which introduced
FilterFS, where the issue was still present infsutil.Walk).