copytool: make copy -r into an existing directory consistent across backends

For a recursive copy whose destination is an existing directory, the rsync
backend copied the *contents* of the source directory, while the scp backend
copied the directory itself, like `cp -r` and `scp -r` do. Since `--backend=auto`
prefers rsync and only falls back to scp when the guest has no rsync, the
outcome of the very same command depended on the guest image.

The rsync backend appended a trailing slash to every path of a recursive copy,
and a trailing slash on the source is what tells rsync to copy the contents.
Derive the form to pass to rsync from the destination instead, so that a
trailing slash spelled by the user is no longer significant:

- the destination is an existing directory: `SRC DST`, copying SRC into DST;
- the destination does not exist yet: `SRC/ DST`, making DST a copy of SRC.

A destination that can neither be confirmed nor ruled out to be a directory
fails the copy, instead of being assumed to be missing, which would put the
files in the wrong place again.

This also fixes two related defects of the previous code: `copy -r` of a
non-directory into a destination that does not exist made rsync fail on the
appended slash, and multiple sources were merged into the destination instead
of being copied by name.

Add the missing case to copy.bats (both backends, both directions), which only
covered a plain `SRC DST` copy into a destination that does not exist yet, plus
unit tests for the path rewriting, which needs no VM.

`limactl shell --sync` does want the form that copies the contents, so let it
spell its own paths with a trailing slash and stop setting Options.Recursive,
which for rsync only added a `-r` that `-a` already implies.

Fixes #5500

Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

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, &copytool.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,
 					}
 

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)" {

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)
+	}
+}

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)
+}