proposal: os: add CopyFS

[rsc]

After discussion on #45757 (comment) and #61386, it would help many different use cases to add os.CopyFS that copies an fsys.FS into the local file system, safely. I propose to add that function.

The signature and implementation are roughly:

func CopyFS(dir string, fsys fs.FS) error {
	return fs.WalkDir(fsys, ".", func(path string, d fs.DirEntry, err error) error {
		targ := filepath.Join(dir, filepath.FromSlash(path))
		if d.IsDir() {
			if err := os.MkdirAll(targ, 0777); err != nil {
				return err
			}
			return nil
		}
		r, err := fsys.Open(path)
		if err != nil {
			return err
		}
		defer r.Close()
		info, err := r.Stat()
		if err != nil {
			return err
		}
		w, err := os.OpenFile(targ, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0666|info.Mode()&0777)
		if err != nil {
			return err
		}
		if _, err := io.Copy(w, r); err != nil {
			w.Close()
			return fmt.Errorf("copying %s: %v", path, err)
		}
		if err := w.Close(); err != nil {
			return err
		}
		return nil
	})
}

with perhaps some extra checks using filepath.IsLocal to guard against bad file names.

[icholy]

Why is the fs.WalkDirFunc error parameter being ignored?

[carlmjohnson]

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

[carlmjohnson]

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.

[rsc]

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

[CAFxX]

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.

[rsc]

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.

[rsc]

Have all remaining concerns about this proposal been addressed?

[carlmjohnson]

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

[rsc]

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.

[rsc]

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

[qmuntal]

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.

[mvdan]

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

[rsc]

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.

[rsc]

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