From 25552ba236b2dbcb1f9d3e8fa8f8fdeb7cde8410 Mon Sep 17 00:00:00 2001 From: Xiong Han <3324733895@qq.com> Date: Fri, 13 Nov 2026 01:33:21 +0800 Subject: [PATCH] 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 --- cli/command/container/cp.go | 26 ++++++++------ cli/command/container/cp_test.go | 62 ++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 10 deletions(-) diff --git a/cli/command/container/cp.go b/cli/command/container/cp.go index fbce9df646..52fc06ab3e 100644 --- a/cli/command/container/cp.go +++ b/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 } diff --git a/cli/command/container/cp_test.go b/cli/command/container/cp_test.go index e193ebbf41..d2ce73b0d3 100644 --- a/cli/command/container/cp_test.go +++ b/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()) +}