Why is the fs.WalkDirFunc error parameter being ignored?

This seems related to #56172. There's a similar question in these about how to handle broken writes and missing directories.

Maybe for this and #56172 there could be a "create directories" argument. Maybe it could be a mode and if the mode is non-zero directories are created. For os.CopyFS, the mode could be &'d with the mode of the source directory.

Maybe there should also be os.CopyFSFile to just copy one fs.File.

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

In #56172 AFAICT the consensus was that the copy operation should attempt to use FICLONE (or equivalent) whenever possible; this does not currently seem to be captured in this superseding proposal though.

There are questions about whether to create directories, modes, and so on. If we go down that road, it should be a third-party package (lots of them) so that everyone can use the combination they want. The basic os.CopyFS should be the simple one that's good enough the other 99% of the time.

In theory os.CopyFS could detect an os.DirFS source and use its own special-case code to take advantage of FICLONE if that was an important optimization.

Have all remaining concerns about this proposal been addressed?

The sample code has os.MkdirAll(targ, 0777). Should that use info, err := d.Info() and os.MkdirAll(targ, info.Mode()) instead?

I think it should use 0777, the same way that os.OpenFile uses 0666. Let the umask set the permissions, same as if we were using the shell's mkdir and 'cat >file'. The only bits being copied out are the execute bits on regular files.

I think the complete docs for CopyFS are:

// CopyFS copies the file system fsys into the directory dir,
// creating dir if necessary.
//
// Newly created directories and files have their default modes
// according to the current umask, except that the execute bits
// are copied from the file in fsys when creating a local file.
//
// If a file name in fsys does not satisfy filepath.IsLocal,
// an error is returned for that file.
//
// Copying stops at and returns the first error encountered.
func CopyFS(dir string, fsys fs.FS) error

Should CopyFS also copy symlinks? Something like this:

switch m := d.Mode(); {
    case m.IsRegular():
        ...
    case m&os.ModeSymlink != 0:
        link, err := os.Readlink(path)
        if err != nil {
           return err
        }
        if err := os.Symlink(link, targ); err != nil {
	    return err
        }
    }

If so, we should make sure that it works reasonable well with Windows symbolic links, or at least that the outcome is well understood and consistent. This is related to #63703.

@qmuntal I imagine that would need #49580 to land first.

It's very unclear to me what CopyFS should do with symlinks. Perhaps we should un-accept #49580. :-)
Seriously, if the symlink is to something outside dir, that's probably an error, right?
Creating a symlink to /etc/passwd seems like it violates the spirit of using filepath.IsLocal for the rest of the files.

Symbolic links are so awful. I am going to come back to this after Go 1.22 is frozen.

Getting back to symlinks, if we don't want to unaccept #49580 (which would be fine with me but I'm not proposing it), then it seems like we should only accept symlinks in CopyFS that stay within the fs. Symlinks pointing outside the FS tree would result in an error from the overall CopyFS operation. So for example you cannot CopyFS and create a symlink to /etc/passwd.

Does that seem reasonable?

That seems reasonable to me. Leaving #49580 as accepted also seems like the pragmatic option - even if it's awkward and adds lots more edge cases, the reality is that users will want symlink support. Either we add support ourselves, or soon enough there will be external packages like go-symlinkfs that don't have good interoperability with the rest of the ecosystem.

I think the revised complete docs for CopyFS are:

// CopyFS copies the file system fsys into the directory dir,
// creating dir if necessary.
//
// Newly created directories and files have their default modes
// according to the current umask, except that the execute bits
// are copied from the file in fsys when creating a local file.
//
// If a file name in fsys does not satisfy filepath.IsLocal,
// an error is returned for that file.
//
// If a symbolic link would lexically point outside dir or
// cannot be created, an error is returned.
//
// Copying stops at and returns the first error encountered.
func CopyFS(dir string, fsys fs.FS) error

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

Adding to package os:

// CopyFS copies the file system fsys into the directory dir,
// creating dir if necessary.
//
// Newly created directories and files have their default modes
// according to the current umask, except that the execute bits
// are copied from the file in fsys when creating a local file.
//
// If a file name in fsys does not satisfy filepath.IsLocal,
// an error is returned for that file.
//
// Copying stops at and returns the first error encountered.
func CopyFS(dir string, fsys fs.FS) error

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Adding to package os:

// CopyFS copies the file system fsys into the directory dir,
// creating dir if necessary.
//
// Newly created directories and files have their default modes
// according to the current umask, except that the execute bits
// are copied from the file in fsys when creating a local file.
//
// If a file name in fsys does not satisfy filepath.IsLocal,
// an error is returned for that file.
//
// Copying stops at and returns the first error encountered.
func CopyFS(dir string, fsys fs.FS) error

Change https://go.dev/cl/558995 mentions this issue: os: implement CopyFS

Change https://go.dev/cl/564295 mentions this issue: internal/safefilepath: add IsLocal