-
Notifications
You must be signed in to change notification settings - Fork 258
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
Ensure nil
is accepted as FS config
#1182
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is cleaner to handle this at the call site. The adapter is too late imho, and nil there a bug not an intended behavior. otoh keeping the nil guard in adapt is also fine!
so main change I prefer is to check nil inside here:
// WithFS implements ModuleConfig.WithFS
func (c *moduleConfig) WithFS(fs fs.FS) ModuleConfig {
return c.WithFSConfig(NewFSConfig().WithFSMount(fs, ""))
}
to
// WithFS implements ModuleConfig.WithFS
func (c *moduleConfig) WithFS(fs fs.FS) ModuleConfig {
var config FSConfig
if fs != nil {
config = NewFSConfig().WithFSMount(fs, "")
}
return c.WithFSConfig(config)
}
I originally did it that way and figured that it would do the same if I handled at Adapt; but it's ok to be a tad more defensive. I pushed the following, which avoids invoking // WithFS implements ModuleConfig.WithFS
func (c *moduleConfig) WithFS(fs fs.FS) ModuleConfig {
if fs == nil {
return c
}
return c.WithFSConfig(NewFSConfig().WithFSMount(fs, ""))
} |
sorry to be a nitpicker, but any WithXXX(nil) should clear previous don't you think? |
oh right I didn't think about that case! |
In previous releases, passing a `nil` value as an FS config did not cause any error. However, in 1.0.0-rc.1 this leads to the creation of an invalid `adapter{fs: nil}`, which eventually leads to a panic (nil): (f *FileEntry) Stat(st *platform.Stat_t) => (r *lazyDir) file() => r.fs.OpenFile(".", os.O_RDONLY, 0) with fs == nil The backwards-compatible fix is to make Adapt() return UnimplementedFS, and ensuring `nil` is a valid value that config is able to handle. However, we may want to consider returning an error somewhere, because configuring a nil FS may be unintended. Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bearing with me!
In previous releases, passing a
nil
value as an FS config did not cause any error. However, in 1.0.0-rc.1 this leadsto the creation of an invalid
adapter{fs: nil}
, which eventually leads to a panic (nil):with fs == nil
The backwards-compatible fix is to make Adapt() return UnimplementedFS.
However, we may want to consider returning an error somewhere, because configuring a nil FS may be unintended.
Signed-off-by: Edoardo Vacchi evacchi@users.noreply.github.com