diff --git a/cmd/limactl/shell.go b/cmd/limactl/shell.go index 362514a1..705b585a 100644 --- a/cmd/limactl/shell.go +++ b/cmd/limactl/shell.go @@ -440,13 +440,16 @@ func shellAction(cmd *cobra.Command, args []string) error { destRsyncDir = shellescape.Quote(destRsyncDir) } + // The trailing slash makes rsync mirror the contents of the source directory into + // the destination directory, instead of copying the source directory into it like + // `limactl copy -r` does. Options.Recursive is deliberately left unset, so that + // the paths reach rsync as they are; `-a` already implies `-r`. paths := []string{ - hostCurrentDir, - fmt.Sprintf("%s:%s", inst.Name, destRsyncDir), + hostCurrentDir + "/", + fmt.Sprintf("%s:%s/", inst.Name, destRsyncDir), } rsync, err = copytool.New(ctx, string(copytool.BackendRsync), paths, ©tool.Options{ - Recursive: true, - Verbose: false, + Verbose: false, AdditionalArgs: []string{ "--delete", }, @@ -537,13 +540,15 @@ func windowsQuoteShell(shell string) string { func askUserForRsyncBack(ctx context.Context, cmd *cobra.Command, inst *limatype.Instance, sshCmd *exec.Cmd, hostCurrentDir, destRsyncDir string, rsync copytool.CopyTool, tty bool) error { remoteSource := fmt.Sprintf("%s:%s", inst.Name, destRsyncDir) + // See the comment on the trailing slash where the rsync tool is created. + remoteContents := remoteSource + "/" clean := filepath.Clean(hostCurrentDir) dirForCleanup := shellescape.Quote(filepath.Join(*inst.Config.User.Home, clean)) cleanupGuestWorkdir := false rsyncBack := func() error { paths := []string{ - remoteSource, + remoteContents, hostCurrentDir, } @@ -630,7 +635,7 @@ func askUserForRsyncBack(ctx context.Context, cmd *cobra.Command, inst *limatype diffCmd = exec.CommandContext(ctx, "diff", "-ruN", "--color=always", hostCurrentDir, hostTmpDest) if !rsyncToTempDir { paths := []string{ - remoteSource, + remoteContents, hostTmpDest, } diff --git a/hack/bats/tests/copy.bats b/hack/bats/tests/copy.bats index bffe7466..115c66d1 100644 --- a/hack/bats/tests/copy.bats +++ b/hack/bats/tests/copy.bats @@ -39,6 +39,13 @@ test_copy_dir_from_host_to_instance() { limactl copy --backend="$backend" -r "$BATS_TEST_TMPDIR/foo/" "$INSTANCE":/tmp/test_limactl_copy/foo_src_dst_with_slash/ limactl shell "$INSTANCE" -- test -d /tmp/test_limactl_copy/foo_src_dst_with_slash/foo limactl shell "$INSTANCE" -- rm -rf /tmp/test_limactl_copy/foo_src_dst_with_slash + + # SRC DST, with DST existing: SRC has to be copied into DST, like `cp -r` does. + # https://github.com/lima-vm/lima/issues/5500 + limactl shell "$INSTANCE" -- mkdir -p /tmp/test_limactl_copy/existing_dst + limactl copy --backend="$backend" -r "$BATS_TEST_TMPDIR/foo" "$INSTANCE":/tmp/test_limactl_copy/existing_dst + limactl shell "$INSTANCE" -- test -d /tmp/test_limactl_copy/existing_dst/foo/bar + limactl shell "$INSTANCE" -- rm -rf /tmp/test_limactl_copy/existing_dst } @test "copy directory from host to Lima instance (scp)" { @@ -70,6 +77,12 @@ test_copy_dir_from_instance_to_host() { # SRC/ DST/ limactl copy --backend="$backend" -r "$INSTANCE":/tmp/test_limactl_copy/foo/ "$BATS_TEST_TMPDIR/foo_src_dst_with_slash/" assert_dir_exists "$BATS_TEST_TMPDIR/foo_src_dst_with_slash/bar" + + # SRC DST, with DST existing: SRC has to be copied into DST, like `cp -r` does. + # https://github.com/lima-vm/lima/issues/5500 + mkdir -p "$BATS_TEST_TMPDIR/existing_dst" + limactl copy --backend="$backend" -r "$INSTANCE":/tmp/test_limactl_copy/foo "$BATS_TEST_TMPDIR/existing_dst" + assert_dir_exists "$BATS_TEST_TMPDIR/existing_dst/foo/bar" } @test "copy directory from Lima instance to host (scp)" { diff --git a/pkg/copytool/copytool_test.go b/pkg/copytool/copytool_test.go index 939ded0f..d098e4fa 100644 --- a/pkg/copytool/copytool_test.go +++ b/pkg/copytool/copytool_test.go @@ -4,6 +4,9 @@ package copytool import ( + "os" + "path/filepath" + "runtime" "slices" "testing" @@ -44,3 +47,140 @@ func TestRsyncCommandEndsOptionParsing(t *testing.T) { assert.Assert(t, sep != -1, "rsync args must contain the %#q separator: %v", "--", cmd.Args) assert.Assert(t, slices.Index(cmd.Args, dashPath) > sep, "path %#q must come after %#q: %v", dashPath, "--", cmd.Args) } + +// TestRsyncCommandPaths verifies that the paths of a recursive copy are passed to rsync in +// the form that makes it behave like `cp -r` and `scp -r`, regardless of how the trailing +// slashes were spelled by the caller. +// https://github.com/lima-vm/lima/issues/4468 +// https://github.com/lima-vm/lima/issues/5500 +func TestRsyncCommandPaths(t *testing.T) { + if _, err := newRsyncTool(&Options{}); err != nil { + t.Skip("rsync not found:", err) + } + + // Only local paths are used, so that no guest is involved. + tmpDir := t.TempDir() + dir := filepath.Join(tmpDir, "dir") + assert.NilError(t, os.Mkdir(dir, 0o755)) + dir2 := filepath.Join(tmpDir, "dir2") + assert.NilError(t, os.Mkdir(dir2, 0o755)) + file := filepath.Join(tmpDir, "file.txt") + assert.NilError(t, os.WriteFile(file, []byte("hello"), 0o644)) + existing := filepath.Join(tmpDir, "existing") + assert.NilError(t, os.Mkdir(existing, 0o755)) + missing := filepath.Join(tmpDir, "missing") + + tests := []struct { + name string + opts Options + args []string + expected []string + }{ + { + // The destination becomes a copy of the source directory. + name: "dir into missing dst", opts: Options{Recursive: true}, + args: []string{dir, missing}, expected: []string{dir + "/", missing}, + }, + { + // The source directory is copied into the destination. + name: "dir into existing dst", opts: Options{Recursive: true}, + args: []string{dir, existing}, expected: []string{dir, existing}, + }, + { + // A trailing slash on the source is not significant. + name: "dir with slash into existing dst", opts: Options{Recursive: true}, + args: []string{dir + "/", existing}, expected: []string{dir, existing}, + }, + { + name: "dir with slash into missing dst", opts: Options{Recursive: true}, + args: []string{dir + "/", missing}, expected: []string{dir + "/", missing}, + }, + { + // A trailing slash would just make rsync fail on a non-directory. + name: "file into missing dst", opts: Options{Recursive: true}, + args: []string{file, missing}, expected: []string{file, missing}, + }, + { + // Multiple sources are never merged into the destination. + name: "multiple sources", opts: Options{Recursive: true}, + args: []string{dir + "/", dir2, missing}, expected: []string{dir, dir2, missing}, + }, + { + // Without -r the paths reach rsync as they are, trailing slash included. + // `limactl shell --sync` relies on it to mirror the directories. + name: "non-recursive", opts: Options{}, + args: []string{dir + "/", existing}, expected: []string{dir + "/", existing}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tool, err := newRsyncTool(&tt.opts) + assert.NilError(t, err) + cmd, err := tool.Command(t.Context(), tt.args, nil) + assert.NilError(t, err) + + // The paths are everything after the "--" separator. + sep := slices.Index(cmd.Args, "--") + assert.Assert(t, sep != -1, "rsync args must contain the %#q separator: %v", "--", cmd.Args) + assert.DeepEqual(t, cmd.Args[sep+1:], tt.expected) + }) + } +} + +// TestRsyncCommandUndeterminedPath verifies that a path that can neither be confirmed nor +// ruled out to be a directory fails the copy, rather than falling back to copying the +// contents of the source, which would put the files in the wrong place. +func TestRsyncCommandUndeterminedPath(t *testing.T) { + if runtime.GOOS == "windows" { + // The test needs a path that can neither be confirmed nor ruled out to be a + // directory. A file used as a directory component fails with ENOTDIR on POSIX + // systems, but Windows reports it as a missing path instead, which os.Stat + // turns into fs.ErrNotExist. See the comment in $GOROOT/src/os/root_windows.go. + t.Skip("ENOTDIR for a file used as a directory is POSIX-specific") + } + tool, err := newRsyncTool(&Options{Recursive: true}) + if err != nil { + t.Skip("rsync not found:", err) + } + + tmpDir := t.TempDir() + dir := filepath.Join(tmpDir, "dir") + assert.NilError(t, os.Mkdir(dir, 0o755)) + file := filepath.Join(tmpDir, "file.txt") + assert.NilError(t, os.WriteFile(file, []byte("hello"), 0o644)) + + // Stat fails with ENOTDIR, rather than reporting the destination as missing. + _, err = tool.Command(t.Context(), []string{dir, filepath.Join(file, "dst")}, nil) + assert.ErrorContains(t, err, "not a directory") +} + +func TestTrimTrailingSlashes(t *testing.T) { + tests := []struct{ path, expected string }{ + {"/tmp/foo", "/tmp/foo"}, + {"/tmp/foo/", "/tmp/foo"}, + {"/tmp/foo//", "/tmp/foo"}, + {"/", "/"}, + {"", ""}, + } + for _, tt := range tests { + assert.Equal(t, trimTrailingSlashes(tt.path), tt.expected, "path=%q", tt.path) + } +} + +func TestQuoteRemotePath(t *testing.T) { + tests := []struct{ path, expected string }{ + {"/tmp/foo", "/tmp/foo"}, + {"foo bar", "'foo bar'"}, + {"/tmp/$(touch pwned)", `'/tmp/$(touch pwned)'`}, + // A leading "~" or "~user" has to stay expandable by the shell of the guest. + {"~", "~"}, + {"~/foo bar", "~/'foo bar'"}, + {"~foo", "~foo"}, + {"~foo/bar", "~foo/bar"}, + {"~foo/bar baz", "~foo/'bar baz'"}, + {"~$(touch pwned)/foo", `'~$(touch pwned)/foo'`}, + } + for _, tt := range tests { + assert.Equal(t, quoteRemotePath(tt.path), tt.expected, "path=%q", tt.path) + } +} diff --git a/pkg/copytool/rsync.go b/pkg/copytool/rsync.go index f3c2069f..19d731cf 100644 --- a/pkg/copytool/rsync.go +++ b/pkg/copytool/rsync.go @@ -5,8 +5,11 @@ package copytool import ( "context" + "errors" "fmt" + "os" "os/exec" + "regexp" "strings" "al.essio.dev/pkg/shellescape" @@ -58,28 +61,33 @@ func (t *rsyncTool) IsAvailableOnGuest(ctx context.Context, paths []string) bool } func checkRsyncOnGuest(ctx context.Context, inst *limatype.Instance) bool { + checkCmd, err := sshCommandOnGuest(ctx, inst, "command -v rsync >/dev/null 2>&1") + if err != nil { + logrus.Debugf("failed to prepare the SSH command for the rsync check: %v", err) + return false + } + return checkCmd.Run() == nil +} + +// sshCommandOnGuest returns a command that executes script with the shell of the guest. +func sshCommandOnGuest(ctx context.Context, inst *limatype.Instance, script string) (*exec.Cmd, error) { sshExe, err := sshutil.NewSSHExe() if err != nil { - logrus.Debugf("failed to create SSH executable: %v", err) - return false + return nil, err } sshOpts, err := sshutil.SSHOpts(ctx, sshExe, inst.Dir, *inst.Config.User.Name, false, false, false, false) if err != nil { - logrus.Debugf("failed to get SSH options for rsync check: %v", err) - return false + return nil, err } sshArgs := append([]string{}, sshExe.Args...) sshArgs = append(sshArgs, sshutil.SSHArgsFromOpts(sshOpts)...) - checkCmd := exec.CommandContext(ctx, sshExe.Exe, sshArgs...) - checkCmd.Args = append(checkCmd.Args, + sshArgs = append(sshArgs, "-p", fmt.Sprintf("%d", inst.SSHLocalPort), *inst.Config.User.Name+"@"+inst.SSHAddress, - "command -v rsync >/dev/null 2>&1", + script, ) - - err = checkCmd.Run() - return err == nil + return exec.CommandContext(ctx, sshExe.Exe, sshArgs...), nil } func (t *rsyncTool) Command(ctx context.Context, paths []string, opts *Options) (*exec.Cmd, error) { @@ -146,25 +154,13 @@ func (t *rsyncTool) Command(ctx context.Context, paths []string, opts *Options) rsyncArgs = append(rsyncArgs, "-e", sshCmd) } - // Handle trailing slash for directory copies to keep consistent behavior with scp, - // which was the original implementation of `limactl copy -r`. + // Adjust the trailing slashes of the paths so that a recursive copy behaves like + // `cp -r` and `scp -r`, which was the original implementation of `limactl copy -r`. // https://github.com/lima-vm/lima/issues/4468 - if effectiveOpts.Recursive { - for i, cp := range copyPaths { - //nolint:modernize // stringscutprefix: HasSuffix + TrimSuffix can be simplified to CutSuffix - if strings.HasSuffix(cp.Path, "/") { - if cp.IsRemote { - for j, cp2 := range copyPaths { - if i != j { - cp2.Path = strings.TrimSuffix(cp2.Path, "/") - } - } - } else { - cp.Path = strings.TrimSuffix(cp.Path, "/") - } - } else { - cp.Path += "/" - } + // https://github.com/lima-vm/lima/issues/5500 + if effectiveOpts.Recursive && len(copyPaths) > 1 { + if err := adjustTrailingSlashes(ctx, paths, copyPaths); err != nil { + return nil, err } } @@ -182,3 +178,114 @@ func (t *rsyncTool) Command(ctx context.Context, paths []string, opts *Options) return exec.CommandContext(ctx, t.toolPath, rsyncArgs...), nil } + +// adjustTrailingSlashes rewrites the source paths of a recursive copy so that rsync +// behaves like `cp -r` and `scp -r`: the source directory is copied into the destination +// when the destination is an existing directory (DST/SRC), and becomes the destination +// when it does not exist yet. rsync decides this from a trailing slash on the source +// instead: "SRC/" copies the *contents* of SRC, while "SRC" always creates DST/SRC. +// +// paths are the original command line arguments, corresponding to copyPaths by index. +// The last one is the destination; the preceding ones are the sources. +func adjustTrailingSlashes(ctx context.Context, paths []string, copyPaths []*Path) error { + last := len(copyPaths) - 1 + srcs, dst := copyPaths[:last], copyPaths[last] + + // A trailing slash on a source is not significant for `limactl copy`. + for _, src := range srcs { + src.Path = trimTrailingSlashes(src.Path) + } + + // Multiple sources always require the destination to be a directory. + if len(srcs) != 1 { + return nil + } + // Otherwise rsync has to be told to copy the contents of the source when the + // destination is not an existing directory, so that the destination becomes a copy of + // the source directory rather than its parent. + dstIsDir, err := isDir(ctx, paths[last], dst) + if err != nil || dstIsDir { + return err + } + // A source that is not a directory is already copied by name, and a trailing slash + // would just make rsync fail on it. + srcIsDir, err := isDir(ctx, paths[0], srcs[0]) + if err != nil { + return err + } + if srcIsDir { + srcs[0].Path += "/" + } + return nil +} + +// trimTrailingSlashes removes the trailing slashes from path, unless the path is made of +// slashes only (e.g. "/"), as trimming those would turn it into an empty path. +func trimTrailingSlashes(path string) string { + if trimmed := strings.TrimRight(path, "/"); trimmed != "" { + return trimmed + } + return path +} + +// isDir reports whether the path of cp is an existing directory. A path that does not +// exist is not an error, but failing to determine the answer is: guessing it would copy +// the files to the wrong place. +// +// rawPath is the corresponding command line argument. It is used instead of cp.Path for +// local paths, because cp.Path may have been rewritten to a subsystem path (e.g. +// "/cygdrive/c/...") that only the SSH executable understands on Windows hosts. +func isDir(ctx context.Context, rawPath string, cp *Path) (bool, error) { + if !cp.IsRemote { + st, err := os.Stat(rawPath) + if errors.Is(err, os.ErrNotExist) { + return false, nil + } + if err != nil { + return false, err + } + return st.IsDir(), nil + } + path := cp.Path + if path == "" { + // `limactl copy foo inst:` copies into the login directory of the guest. + path = "." + } + cmd, err := sshCommandOnGuest(ctx, cp.Instance, "test -d "+quoteRemotePath(path)) + if err != nil { + return false, err + } + // `test` writes nothing, so Output() only serves to capture the stderr of SSH. + if _, err = cmd.Output(); err == nil { + return true, nil + } + // `test` exits with 1 when the path is not a directory. Any other status means that + // the answer is unknown; SSH itself exits with 255 when it fails to connect. + if exitErr, ok := errors.AsType[*exec.ExitError](err); ok { + if exitErr.ExitCode() == 1 { + return false, nil + } + if stderr := strings.TrimSpace(string(exitErr.Stderr)); stderr != "" { + err = fmt.Errorf("%w (%s)", err, stderr) + } + } + return false, fmt.Errorf("failed to check whether %#q is a directory on instance %#q: %w", + path, cp.InstanceName, err) +} + +// tildePrefixRegexp matches a "~" or "~user" prefix that the shell of the guest expands. +var tildePrefixRegexp = regexp.MustCompile(`^~[A-Za-z0-9._-]*$`) + +// quoteRemotePath quotes path for the shell of the guest, keeping a leading "~" or +// "~user" unquoted so that the shell still expands it, like it does for the paths that +// rsync passes to the guest. +func quoteRemotePath(path string) string { + prefix, rest, found := strings.Cut(path, "/") + if !tildePrefixRegexp.MatchString(prefix) { + return shellescape.Quote(path) + } + if !found { + return prefix + } + return prefix + "/" + shellescape.Quote(rest) +}