fix: skip sub-directories when copying a plugin directory - #582
fix: skip sub-directories when copying a plugin directory#582arpitjain099 wants to merge 1 commit into
Conversation
CopyDirToDir is documented to copy only the top-level regular files of a directory, but its WalkDir callback names its path parameter "path", which shadows nothing useful and makes the skip check d.Name() != filepath.Base(path) always false (both sides resolve to the current entry's base name). As a result the walk descended into every sub-directory and copied nested files, flattening them into the destination. Compare the walked path against the source root instead, matching the pattern already used in parsePluginFromDir. Add a regression test. Signed-off-by: Arpit Jain <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR fixes internal/file.CopyDirToDir so it truly copies only the top-level regular files of a directory by preventing filepath.WalkDir from descending into subdirectories. It also adds a regression test to ensure nested files in subdirectories are not copied during plugin installation.
Changes:
- Fix subdirectory skipping logic in
CopyDirToDirby comparing walked paths against the walk root (src). - Add a regression test verifying only top-level regular files are copied and that non-directory sources return
ErrNotDirectory.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/file/file.go | Corrects the WalkDir callback logic so subdirectories are properly skipped when copying. |
| internal/file/file_test.go | Adds a regression test to prevent nested files from being copied and validates non-directory error behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This PR is stale because it has been opened for 45 days with no activity. Remove stale label or comment. Otherwise, it will be closed in 30 days. |
|
Commenting to keep this off the stale path, and to say the change is still current. Re-checked // skip sub-directories
if d.IsDir() && d.Name() != filepath.Base(path) {
return fs.SkipDir
}
The branch is rebased and mergeable, CI is green, and the diff is two files: the one-line comparison against the walk root, matching the No rush from my side, but a look from anyone on the review list would be appreciated, or tell me what you would like changed and I will turn it around. |
While reading the plugin install path I noticed that
internal/file.CopyDirToDirdoesn't behave the way its comment describes. It's meant to copy only the top-level regular files of a directory, but the skip-subdirectories guard never fires.The
WalkDircallback names its path parameterpath, so the checkd.Name() != filepath.Base(path)compares the current entry's base name against itself and is always false. The walk therefore descends into every sub-directory and copies nested files, flattening them into the destination.parsePluginFromDirin the same package gets this right by comparing against the walk root, so the two disagree on what a plugin directory contains:Installvalidates the top level but then copies nested files too.The fix compares the walked path against the source root, matching the existing
parsePluginFromDirpattern. I added a regression test that fails before the change (a nested file gets copied) and passes after. Verified withgo test ./internal/file/... ./plugin/...on linux/amd64.Thanks for taking a look.