Merge pull request #5501 from AkihiroSuda/fix-5500

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

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