Add Contains #23
Add Contains #23
Conversation
hasprefix.go
Outdated
| // HasPrefix returns true if p or any parent of p is the same file as prefix. | ||
| // prefix must exist, but p may not. It is an expensive but accurate alternative | ||
| // to the deprecated filepath.HasPrefix. | ||
| func HasPrefix(fs Stater, p, prefix string) (bool, error) { |
There was a problem hiding this comment.
IMO "HasPrefix" is a bit misleading. To me, it suggests that you're operating on paths, but you're actually operating on actual FS datastructures. "IsIn" or "Contains" or something like that would be more accurate.
There was a problem hiding this comment.
Good point. Renamed to Contains.
hasprefix.go
Outdated
| case os.IsNotExist(err): | ||
| // Do nothing and skip ahead to trying p's parent directory. | ||
| default: | ||
| return false, err |
There was a problem hiding this comment.
IMO this is not correct. If the error is another error (say, a permission denied error), p can still be in a subdirectory of prefix.
There was a problem hiding this comment.
Good point again. I've update the code to handle more error cases.
hasprefix.go
Outdated
| default: | ||
| return false, err | ||
| } | ||
| parentDir := filepath.Dir(p) |
There was a problem hiding this comment.
There needs to be some thought about how you want to deal with symlinks etc. For example, if p denotes a symlink into prefix (say prefix == "/home/mero" and p is /tmp/foo, pointing at /home/mero/foo), should this function return true, or false? And on the other hand, if p is a symlink inside prefix, pointing out of it (say prefix == /home/mero and p is /home/mero/foo pointing at /tmp/foo)? It really depends on the semantic of what you want to achieve.
Just as food for thought: It would be possible to use Open in combination with Openat to walk the actual inodes. That should make this totally, 100% referring to the on-disk structures, respecting all permissions and symlinks and the like. Would obviously only work on posix operating systems. I think that mainly excludes Plan 9 currently? Dunno.
Again, depends on the semantics that you want to achieve.
There was a problem hiding this comment.
That's really insightful. I've updated the comment:
// Contains returns true if p is reachable by traversing through prefix. prefix
// must exist, but p may not. It is an expensive but accurate alternative to the
// deprecated filepath.HasPrefix.I think this is enough for now, but you're right that there's a lot of subtlety here.
hasprefix.go
Outdated
| fi, err := fs.Stat(p) | ||
| switch { | ||
| case err == nil: | ||
| if os.SameFile(fi, prefixFI) { |
There was a problem hiding this comment.
Note that os.SameFile only works if the os.FileInfo was returned by the os-package. Meaning, your Stater interface doesn't fully capture the requirements of this function - if I implement it myself, with a custom os.FileInfo, this will probably fail.
There was a problem hiding this comment.
Good point, but I suggest leaving this unchanged. Reasoning:
-
This package does not abstract
*os.File(by design) and all operations fall back to the underlyingospackage. Therefore theos.FileInforeturned byfs.Statshould always be compatible withos.SameFile. -
Note that we have similar problems with methods like
os.IsNotExistandos.IsPermission: these make assumptions about theerrorargument being in some way compatible with the underlyingospackage. -
At this point, I don't want to make
os.SameFile,os.IsNotExist, andos.IsPermissionmethods of theFSinterface to avoid cluttering it too much. But you're right, technically they should be.
Thanks again for the deep insight :)
There was a problem hiding this comment.
I added a comment about assumptions about fs.Stat.
|
Thank you @Merovius for one of the most through and insightful code reviews that I've ever had! |
This PR adds a expensive but (hopefully) accurate implementation of the deprecated
filepath.HasPrefix, following discussions with @Merovius and @stapelberg at the Zurich Golang meetup organized by @SchumacherFM and independently proposed by @hirochachacha two years ago. Compared to @hirochachacha's implementation, it should correctly handle non-existent paths.Refs golang/go#18358.
Reviews welcome.