container/cp: don't print success message when copy fails

The non-quiet copy paths printed the "Successfully copied ..." summary
unconditionally, even when the copy failed. Copying a directory onto an
existing regular file therefore printed "Successfully copied 0B" before
reporting "cannot copy directory". Only print the summary when the copy
succeeded, in both the from-container and to-container directions.

Also rename the res variable in copyFromContainer to err: it only holds
the error returned by archive.CopyTo, so name it err to make that clear
instead of suggesting some other "result" return type.

Signed-off-by: Xiong Han <3324733895@qq.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

cli/command/container/cp.go

@@ -335,23 +335,27 @@ func copyFromContainer(ctx context.Context, dockerCLI command.Cli, copyConfig cp
 	}
 
 	restore, done := copyProgress(ctx, dockerCLI.Err(), copyFromContainerHeader, &copiedSize)
-	res := archive.CopyTo(preArchive, srcInfo, dstPath)
+	err = archive.CopyTo(preArchive, srcInfo, dstPath)
 	cancel()
 	<-done
 	restore()
-	reportedSize := copiedSize
-	if !cpRes.Stat.Mode.IsDir() {
-		reportedSize = cpRes.Stat.Size
+	if err == nil {
+		reportedSize := copiedSize
+		if !cpRes.Stat.Mode.IsDir() {
+			reportedSize = cpRes.Stat.Size
+		}
+		_, _ = fmt.Fprint(dockerCLI.Err(), copySummary(reportedSize, copiedSize, dstPath))
 	}
-	_, _ = fmt.Fprint(dockerCLI.Err(), copySummary(reportedSize, copiedSize, dstPath))
 
-	return res
+	return err
 }
 
 // In order to get the copy behavior right, we need to know information
 // about both the source and destination. The API is a simple tar
 // archive/extract API but we can use the stat info header about the
 // destination to be more informed about exactly what the destination is.
+//
+//nolint:gocyclo
 func copyToContainer(ctx context.Context, dockerCLI command.Cli, copyConfig cpConfig) error {
 	srcPath := copyConfig.sourcePath
 	dstPath := copyConfig.destPath
@@ -474,11 +478,13 @@ func copyToContainer(ctx context.Context, dockerCLI command.Cli, copyConfig cpCo
 	cancel()
 	<-done
 	restore()
-	reportedSize := copiedSize
-	if sizeErr == nil {
-		reportedSize = contentSize
+	if err == nil {
+		reportedSize := copiedSize
+		if sizeErr == nil {
+			reportedSize = contentSize
+		}
+		_, _ = fmt.Fprint(dockerCLI.Err(), copySummary(reportedSize, copiedSize, copyConfig.container+":"+dstInfo.Path))
 	}
-	_, _ = fmt.Fprint(dockerCLI.Err(), copySummary(reportedSize, copiedSize, copyConfig.container+":"+dstInfo.Path))
 
 	return err
 }

cli/command/container/cp_test.go

@@ -2,6 +2,7 @@ package container
 
 import (
 	"context"
+	"errors"
 	"io"
 	"os"
 	"runtime"
@@ -446,3 +447,64 @@ func TestCopyToContainerStdinReportsTransferredSize(t *testing.T) {
 	// "(transferred ...)" should not appear.
 	assert.Check(t, !strings.Contains(errOut, "(transferred"))
 }
+
+func TestCopyFromContainerDoesNotReportSuccessOnFailure(t *testing.T) {
+	// Copying a directory onto an existing regular file fails while
+	// extracting ("cannot copy directory"). The failure must not be
+	// preceded by a "Successfully copied" message.
+	srcDir := fs.NewDir(t, "cp-test-fromdir-fail",
+		fs.WithFile("file1", "hello"))
+	destFile := fs.NewFile(t, "cp-test-fromdir-fail-dest", fs.WithContent("original\n"))
+
+	fakeCli := test.NewFakeCli(&fakeClient{
+		containerCopyFromFunc: func(ctr, srcPath string) (client.CopyFromContainerResult, error) {
+			readCloser, err := archive.Tar(srcDir.Path(), compression.None)
+			return client.CopyFromContainerResult{
+				Content: readCloser,
+				Stat: container.PathStat{
+					Name: "mydir",
+					Mode: os.ModeDir | 0o755,
+				},
+			}, err
+		},
+	})
+	err := runCopy(context.TODO(), fakeCli, copyOptions{
+		source:      "container:/mydir",
+		destination: destFile.Path(),
+	})
+	assert.ErrorContains(t, err, "cannot copy directory")
+	assert.Check(t, !strings.Contains(fakeCli.ErrBuffer().String(), "Successfully copied"),
+		"unexpected success message in output: %q", fakeCli.ErrBuffer().String())
+
+	content, err := os.ReadFile(destFile.Path())
+	assert.NilError(t, err)
+	assert.Check(t, is.Equal("original\n", string(content)))
+}
+
+func TestCopyToContainerDoesNotReportSuccessOnFailure(t *testing.T) {
+	// A failed upload must not be followed by a "Successfully copied"
+	// message.
+	srcFile := fs.NewFile(t, "cp-test-to-fail", fs.WithContent("hello"))
+
+	fakeCli := test.NewFakeCli(&fakeClient{
+		containerStatPathFunc: func(containerID, path string) (client.ContainerStatPathResult, error) {
+			return client.ContainerStatPathResult{
+				Stat: container.PathStat{
+					Name: "tmp",
+					Mode: os.ModeDir | 0o755,
+				},
+			}, nil
+		},
+		containerCopyToFunc: func(containerID string, options client.CopyToContainerOptions) (client.CopyToContainerResult, error) {
+			_, _ = io.Copy(io.Discard, options.Content)
+			return client.CopyToContainerResult{}, errors.New("copy to container failed")
+		},
+	})
+	err := runCopy(context.TODO(), fakeCli, copyOptions{
+		source:      srcFile.Path(),
+		destination: "container:/tmp",
+	})
+	assert.ErrorContains(t, err, "copy to container failed")
+	assert.Check(t, !strings.Contains(fakeCli.ErrBuffer().String(), "Successfully copied"),
+		"unexpected success message in output: %q", fakeCli.ErrBuffer().String())
+}