From 072c6d9aed08ecdf48e1d93eea09073089d32dc2 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 11 Mar 2020 17:17:30 -0700 Subject: [PATCH] internal/copydir: Factor out our recursive directory copy for reuse We've previously been copying this function around so it could remain unexported while being used in various packages. However, it's a non-trivial function with lots of specific assumptions built into it, so here we'll put it somewhere that other packages can depend on it _and_ document the assumptions it seems to be making for future reference. As a bonus, this now uses os.SameFile to detect when two paths point to the same physical file, instead of the slightly buggy local implementation we had before which only worked on Unix systems and did not correctly handle when the paths were on different physical devices. The copy of the function I extracted here is the one from internal/initwd, so this commit also includes the removal of that unexported version and updating the callers in that package to use at at this new location. --- internal/{initwd => copydir}/copy_dir.go | 51 +++++++++++++------ internal/{initwd => copydir}/copy_dir_test.go | 6 +-- internal/initwd/from_module.go | 8 +-- internal/initwd/getter.go | 3 +- internal/initwd/inode.go | 21 -------- internal/initwd/inode_freebsd.go | 21 -------- internal/initwd/inode_windows.go | 8 --- internal/initwd/module_install_test.go | 3 +- 8 files changed, 48 insertions(+), 73 deletions(-) rename internal/{initwd => copydir}/copy_dir.go (50%) rename internal/{initwd => copydir}/copy_dir_test.go (96%) delete mode 100644 internal/initwd/inode.go delete mode 100644 internal/initwd/inode_freebsd.go delete mode 100644 internal/initwd/inode_windows.go diff --git a/internal/initwd/copy_dir.go b/internal/copydir/copy_dir.go similarity index 50% rename from internal/initwd/copy_dir.go rename to internal/copydir/copy_dir.go index 7096ff74f8..1f156430a1 100644 --- a/internal/initwd/copy_dir.go +++ b/internal/copydir/copy_dir.go @@ -1,4 +1,4 @@ -package initwd +package copydir import ( "io" @@ -7,9 +7,32 @@ import ( "strings" ) -// copyDir copies the src directory contents into dst. Both directories -// should already exist. -func copyDir(dst, src string) error { +// CopyDir recursively copies all of the files within the directory given in +// src to the directory given in dst. +// +// Both directories should already exist. If the destination directory is +// non-empty then the new files will merge in with the old, overwriting any +// files that have a relative path in common between source and destination. +// +// Recursive copying of directories is inevitably a rather opinionated sort of +// operation, so this function won't be appropriate for all use-cases. Some +// of the "opinions" it has are described in the following paragraphs: +// +// Symlinks in the source directory are recreated with the same target in the +// destination directory. If the symlink is to a directory itself, that +// directory is not recursively visited for further copying. +// +// File and directory modes are not preserved exactly, but the executable +// flag is preserved for files on operating systems where it is significant. +// +// Any "dot files" it encounters along the way are skipped, even on platforms +// that do not normally ascribe special meaning to files with names starting +// with dots. +// +// Callers may rely on the above details and other undocumented details of +// this function, so if you intend to change it be sure to review the callers +// first and make sure they are compatible with the change you intend to make. +func CopyDir(dst, src string) error { src, err := filepath.EvalSymlinks(src) if err != nil { return err @@ -38,7 +61,7 @@ func copyDir(dst, src string) error { dstPath := filepath.Join(dst, path[len(src):]) // we don't want to try and copy the same file over itself. - if eq, err := sameFile(path, dstPath); eq { + if eq, err := SameFile(path, dstPath); eq { return nil } else if err != nil { return err @@ -94,14 +117,16 @@ func copyDir(dst, src string) error { return filepath.Walk(src, walkFn) } -// sameFile tried to determine if to paths are the same file. -// If the paths don't match, we lookup the inode on supported systems. -func sameFile(a, b string) (bool, error) { +// SameFile returns true if the two given paths refer to the same physical +// file on disk, using the unique file identifiers from the underlying +// operating system. For example, on Unix systems this checks whether the +// two files are on the same device and have the same inode. +func SameFile(a, b string) (bool, error) { if a == b { return true, nil } - aIno, err := inode(a) + aInfo, err := os.Lstat(a) if err != nil { if os.IsNotExist(err) { return false, nil @@ -109,7 +134,7 @@ func sameFile(a, b string) (bool, error) { return false, err } - bIno, err := inode(b) + bInfo, err := os.Lstat(b) if err != nil { if os.IsNotExist(err) { return false, nil @@ -117,9 +142,5 @@ func sameFile(a, b string) (bool, error) { return false, err } - if aIno > 0 && aIno == bIno { - return true, nil - } - - return false, nil + return os.SameFile(aInfo, bInfo), nil } diff --git a/internal/initwd/copy_dir_test.go b/internal/copydir/copy_dir_test.go similarity index 96% rename from internal/initwd/copy_dir_test.go rename to internal/copydir/copy_dir_test.go index 06e02f824b..41e4332280 100644 --- a/internal/initwd/copy_dir_test.go +++ b/internal/copydir/copy_dir_test.go @@ -1,4 +1,4 @@ -package initwd +package copydir import ( "io/ioutil" @@ -52,7 +52,7 @@ func TestCopyDir_symlinks(t *testing.T) { targetDir := filepath.Join(tmpdir, "target") os.Mkdir(targetDir, os.ModePerm) - err = copyDir(targetDir, moduleDir) + err = CopyDir(targetDir, moduleDir) if err != nil { t.Fatal(err) } @@ -92,7 +92,7 @@ func TestCopyDir_symlink_file(t *testing.T) { targetDir := filepath.Join(tmpdir, "target") os.Mkdir(targetDir, os.ModePerm) - err = copyDir(targetDir, moduleDir) + err = CopyDir(targetDir, moduleDir) if err != nil { t.Fatal(err) } diff --git a/internal/initwd/from_module.go b/internal/initwd/from_module.go index 6b40d08d69..e944cac981 100644 --- a/internal/initwd/from_module.go +++ b/internal/initwd/from_module.go @@ -2,7 +2,6 @@ package initwd import ( "fmt" - "github.com/hashicorp/terraform/internal/earlyconfig" "io/ioutil" "log" "os" @@ -10,8 +9,11 @@ import ( "sort" "strings" + "github.com/hashicorp/terraform/internal/earlyconfig" + version "github.com/hashicorp/go-version" "github.com/hashicorp/terraform-config-inspect/tfconfig" + "github.com/hashicorp/terraform/internal/copydir" "github.com/hashicorp/terraform/internal/modsdir" "github.com/hashicorp/terraform/registry" "github.com/hashicorp/terraform/tfdiags" @@ -172,7 +174,7 @@ func DirFromModule(rootDir, modulesDir, sourceAddr string, reg *registry.Client, // We've found the module the user requested, which we must // now copy into rootDir so it can be used directly. log.Printf("[TRACE] copying new root module from %s to %s", record.Dir, rootDir) - err := copyDir(rootDir, record.Dir) + err := copydir.CopyDir(rootDir, record.Dir) if err != nil { diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, @@ -292,7 +294,7 @@ func DirFromModule(rootDir, modulesDir, sourceAddr string, reg *registry.Client, // We copy rather than "rename" here because renaming between directories // can be tricky in edge-cases like network filesystems, etc. log.Printf("[TRACE] copying new module %s from %s to %s", newKey, record.Dir, instPath) - err := copyDir(instPath, tempPath) + err := copydir.CopyDir(instPath, tempPath) if err != nil { diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, diff --git a/internal/initwd/getter.go b/internal/initwd/getter.go index 85574f7ca3..79bb7d7b62 100644 --- a/internal/initwd/getter.go +++ b/internal/initwd/getter.go @@ -9,6 +9,7 @@ import ( cleanhttp "github.com/hashicorp/go-cleanhttp" getter "github.com/hashicorp/go-getter" + "github.com/hashicorp/terraform/internal/copydir" "github.com/hashicorp/terraform/registry/regsrc" ) @@ -112,7 +113,7 @@ func (g reusingGetter) getWithGoGetter(instPath, addr string) (string, error) { if err != nil { return "", fmt.Errorf("failed to create directory %s: %s", instPath, err) } - err = copyDir(instPath, prevDir) + err = copydir.CopyDir(instPath, prevDir) if err != nil { return "", fmt.Errorf("failed to copy from %s to %s: %s", prevDir, instPath, err) } diff --git a/internal/initwd/inode.go b/internal/initwd/inode.go deleted file mode 100644 index 1150b093cc..0000000000 --- a/internal/initwd/inode.go +++ /dev/null @@ -1,21 +0,0 @@ -// +build linux darwin openbsd netbsd solaris dragonfly - -package initwd - -import ( - "fmt" - "os" - "syscall" -) - -// lookup the inode of a file on posix systems -func inode(path string) (uint64, error) { - stat, err := os.Stat(path) - if err != nil { - return 0, err - } - if st, ok := stat.Sys().(*syscall.Stat_t); ok { - return st.Ino, nil - } - return 0, fmt.Errorf("could not determine file inode") -} diff --git a/internal/initwd/inode_freebsd.go b/internal/initwd/inode_freebsd.go deleted file mode 100644 index 30532f54ac..0000000000 --- a/internal/initwd/inode_freebsd.go +++ /dev/null @@ -1,21 +0,0 @@ -// +build freebsd - -package initwd - -import ( - "fmt" - "os" - "syscall" -) - -// lookup the inode of a file on posix systems -func inode(path string) (uint64, error) { - stat, err := os.Stat(path) - if err != nil { - return 0, err - } - if st, ok := stat.Sys().(*syscall.Stat_t); ok { - return uint64(st.Ino), nil - } - return 0, fmt.Errorf("could not determine file inode") -} diff --git a/internal/initwd/inode_windows.go b/internal/initwd/inode_windows.go deleted file mode 100644 index 3ed58e4bf9..0000000000 --- a/internal/initwd/inode_windows.go +++ /dev/null @@ -1,8 +0,0 @@ -// +build windows - -package initwd - -// no syscall.Stat_t on windows, return 0 for inodes -func inode(path string) (uint64, error) { - return 0, nil -} diff --git a/internal/initwd/module_install_test.go b/internal/initwd/module_install_test.go index 239014990b..d4d7eb1a78 100644 --- a/internal/initwd/module_install_test.go +++ b/internal/initwd/module_install_test.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/terraform/configs" "github.com/hashicorp/terraform/configs/configload" "github.com/hashicorp/terraform/helper/logging" + "github.com/hashicorp/terraform/internal/copydir" "github.com/hashicorp/terraform/registry" "github.com/hashicorp/terraform/tfdiags" ) @@ -543,7 +544,7 @@ func tempChdir(t *testing.T, sourceDir string) (string, func()) { return "", nil } - if err := copyDir(tmpDir, sourceDir); err != nil { + if err := copydir.CopyDir(tmpDir, sourceDir); err != nil { t.Fatalf("failed to copy fixture to temporary directory: %s", err) return "", nil }