Preserve writeability with `hackpadfs.Sub`

[duckbrain]

When calling hackpadfs.Sub, any of the hackpadfs extensions become unavailable on the sub FS. (unless the FS implements SubFS) This is because it calls out to fs.Sub, which wraps it in a type that "covers" up the hackpadfs extension methods.

Minimal Reproduction

https://go.dev/play/p/89L4QJs-Qf3

[duckbrain]

I was able to implement a fix by copying the fs.Sub implementation from the standard lib and extending it.

Implementing MountFS may be able to provide a more elegant solution.

[JohnStarich]

Hey @duckbrain, thanks for opening this one. That's a good point about the implementation essentially disappearing inside a standard library wrapper.

I like your idea of trying to use MountFS for a clean solution. Since MountFS must implement every interface in hackpadfs anyway, that would make an improved Sub() easier to maintain. There's trouble though: The root hackpadfs package can't import hackpadfs/mount without an import cycle, which looks tough to unravel. Bummer.

I have some thoughts below, and I'll take a look at your PR soon too.

One option is to add a new method *mount.FS.Sub(), where calling hackpadfs.Sub(mountFS, "foo") would return another *mount.FS with a base path of "foo". Downside here is it only works when used directly on a *mount.FS.

Further, maybe it'd be useful to add a new Sub() constructor for *mount.FS that automatically wraps a generic hackpadfs.FS. The downside is it's not natively supported in the root package, but it's pretty close. Maybe something a bit like this:

package mount

// Sub implements hackpadfs.SubFS and is compatible with
// hackpadfs mount lookups for standard FS operations.
func Sub(fs hackpadfs.FS, dir string) (*FS, error) { return &FS{fs, dir} }

This one could insert a new internal basePath field (or similar) and keep track of that while mounting and reading mounts.

Neither of these options is quite as nice as a native root package implementation though. We might need to look at mount.FS a little harder to see if it can be extracted to an ./internal/mount package where it could be shared.

[JohnStarich]

Just musing here, but maybe this would be sufficient.

package hackpadfs

// Sub attempts to call an optimized fs.Sub() if available. Falls back to a small MountFS implementation.
func Sub(fs FS, dir string) (FS, error) {
	if fs, ok := fs.(SubFS); ok {
		return fs.Sub(dir)
	}
	if fs, ok := fs.(MountFS); ok {
		return Sub(fs.Mount(dir))
	}
	if !gofs.ValidPath(dir) {
		return nil, &PathError{Op: "sub", Path: dir, Err: ErrInvalid}
	}
	return &subFS{
		basePath: dir,
		rootFS:   fs,
	}, nil
}

var _ interface {
	FS
	MountFS
} = &subFS{}

type subFS struct {
	basePath string
	rootFS   FS
}

func (fs *subFS) Open(name string) (File, error) {
	mount, subPath := fs.Mount(name)
	return OpenFile(mount, subPath, 0, 0)
}

func (fs *subFS) Mount(p string) (mount FS, subPath string) {
	return fs.rootFS, path.Join(fs.basePath, p)
}

[duckbrain]

Yeah, you only need to implement the interface. The way I'm seeing it, MountFS is closer to an "UnwrapFS", which mount.FS implements.

fstest

The main issue I've found with this approach is fstest. Because it's doing direct type assertions instead of using the hackpadfs.* helper functions, it doesn't handle "unwrapping" MountFS. I was able to work around this by creating a helper type that wraps the FS and uses the hackpadfs.* helper functions, but t.Skp()'s on ErrNotImplemented.

type FS struct {
	fs.FS
	tb testing.TB
	s  *Suite

	SkipNotImplemented bool
	StatViaFile        bool
}

func (f *FS) Mkdir(name string, perm fs.FileMode) error {
	f.tb.Helper()
	err := fs.Mkdir(f.FS, name, perm)
	f.skipNotImplemented(err)
	return err
}

My version of fstest isn't remotely compatible, but I can push a branch as a more full example.

hackpadfs.* helper functions

The other issue is that the helper functions don't handle file path alterations in error messages when unwrapping. I've played around with something akin to errors.As, using type parameters that handles that case.

func As[T FS](fsys FS, name string, fn func(fsys T, name string) error) error {
	origName := name
	for {
		if x, ok := fsys.(T); ok {
			err := fn(x, name)
			if !errors.Is(err, ErrNotImplemented) {
				var pErr *PathError
				if errors.As(err, &pErr) {
					pErr.Path = origName
					return pErr
				}
				return err
			}
		}
		if x, ok := fsys.(MountFS); ok {
			fsys, name = x.Mount(name)
			continue
		}
		return ErrNotImplemented
	}
}

// Used something like this:

func Create(fs FS, name string) (File, error) {
	var file File
	err := As(fs, name, func(fsys CreateFS, name string) error {
		var err error
		file, err = fsys.Create(name)
		return err
	})
	if errors.Is(err, ErrNotImplemented) {
		// Fallback to OpenFile if Create is not implemented
		return OpenFile(fs, name, FlagReadWrite|FlagCreate|FlagTruncate, 0666)
	}
	return file, err
}

This makes it so when hackpadfs.Create is used on an FS that needs to be unwrapped, the error will be acceptable to fstest.

[duckbrain]

In any case, these seem like separate issues that are prerequisites to a simpler/better hackpadfs.Sub

[JohnStarich]

The main issue I've found with this approach is fstest. Because it's doing direct type assertions instead of using the hackpadfs.* helper functions, it doesn't handle "unwrapping" MountFS.

That's a good point and I agree. MountFS was added later, and clearly wasn't given its due in fstest.

I like the error handling and Skip approach you've suggested, especially now I understand where you're coming from. I'll take a deeper look at this and your PR this weekend, then see where to go from there.

[JohnStarich]

I spent the weekend updating hackpadfs to Go 1.19, which took longer than expected. Will return to this one soon.

[JohnStarich]

It took some time to rehash everything, but I think it's nailed now. @duckbrain How's the latest release v0.1.12 looking for you?

[JohnStarich]

Ack, I realized just now your PR actually did many similar changes. In particular the same bug fixes encountered when improving fstest 🤦 I'll be more careful next time, you deserve some credit there.

[JohnStarich]

Hey @duckbrain, is the latest release working well for you? I'll go ahead and close, but we can reopen if you hit issues.

[duckbrain]

Hey, sorry for just dropping off the face of the Earth for a few months. That shouldn't happen again.

Looked at the PR and it looks very similar to what I have. I just tried swapping out my Sub with the hackpad one, but my tests are failing. Looks like mine are expecting the mutation methods instead of using Mount. I'll have to fix them, but the PR looks like what I was asking about. Thanks! :tada: