expand: Config stubs out ReadDir but Config.glob calls os.Stat directly

[theclapp]

So I can give a custom ReadDir that talks to a remote server, or references a map (for caching or unit testing), but os.Stat breaks all of those ideas.

[theclapp]

I don't know if you'd want to reimplement Stat in terms of ReadDir of the containing directory, or stub out Stat like you have ReadDir.

[mvdan]

Indeed, @alecthomas already pointed out that stat calls aren't configurable in https://github.com/mvdan/sh/pull/783. I had a mental TODO about this but failed to follow through on filing an issue again :)

We decided against adding a generic fs.FS API to control access to the filesystem, because it's read-only, so we can't possibly use a FS to replace the existing "open handler" in the interpreter.

With that in mind, I think adding more narrowly-focused handlers seems fine. Right now, we've got these:

$ git grep -E 'os\.(S|Ls)tat' expand interp
expand/expand.go:               info, err := os.Stat(match)
interp/api.go:      info, err := os.Stat(path)
interp/handler.go:  info, err := os.Stat(file)
interp/runner.go:   return os.Stat(r.absPath(name))
interp/test.go:     info, err := os.Lstat(r.absPath(x))

For the interpreter, we could reuse OpenHandlerFunc, but that does mean an extra syscall per stat operation, and possibly confusion as we're opening files we don't really need to. The expand package is in a similar situation - like you point out, we could reuse ReadDir, but I lean towards not doing that as it's an extra syscall, extra work, and possibly confusing to the end user.

I think both should gain a handler/option akin to https://pkg.go.dev/io/fs#StatFS. If we stick to that precise API, then v4 transitioning further into io/fs APIs should be smoother.

[mvdan]

Note that the interpreter needs Lstat in one instance, to tell if a file is a symlink or not - without following symlinks. This is a bit of a problem, because io/fs only exposes a Stat method, and no Lstat. The interface doesn't explicitly document that the behavior should be to follow symlinks (https://github.com/golang/go/issues/45470), but in practice, the os.DirFS implementation (and likely many others) do follow them: https://cs.opensource.google/go/go/+/refs/tags/go1.17.8:src/os/file.go;l=658

So we might need to do something else to replace os.Lstat. Perhaps the interpreter should take a "handler" interface with both methods, Stat and Lstat, whereas the expand handler would make do with just Stat. An alternative is what you mentioned earlier - reuse ReadDirHandlerFunc, because it will presumably not follow symlinks, though it will be relatively wasteful.

[mvdan]

I also realise you just want the expand API here, so I'm more than happy to review a patch to add a Stat func(name string) (FileInfo, error) field. We can then leave this issue open to also improve the interp API.

[theclapp]

We decided against adding a generic fs.FS API to control access to the filesystem, because it's read-only, so we can't possibly use a FS to replace the existing "open handler" in the interpreter

I understand this comment, but I also kind of don't. fs.FS is just an interface. It's augmented by several other interfaces that add other functionality beyond the basic fs.FS. So why can't you add other interfaces that add Write functionality?

Failing that, the whole io/fs package is just 815 lines of Go, not including tests. It seems like you could just clone the package wholesale and add the functionality you want, without too much trouble.

Of course, you might just not want to do that. Which is fine. :)

In any case, yeah, I only want the expand API. I'll see what I can do.

[mvdan]

The tricky part for the interpreter is that, in the likely scenario that fs.FS gains read-write methods, it's relatively likely that their signature would be different than ours. That incompatibility would be rather unfortunate and confusing for end users.

[mvdan]

I propose an alternative approach in https://github.com/mvdan/sh/pull/826, after taking a second look while on the computer :)

I think the interpreter probably still wants a proper Stat call, because its use of that syscall is more than just one edge case like in expand's globbing with symlinks.