[xh123637]

Fixes: #7514

Summary

docker cp printed the Successfully copied ... summary unconditionally in
the non-quiet paths of both copyFromContainer and copyToContainer, even
when the copy itself failed. Copying a directory from a container onto an
existing local regular file therefore printed
Successfully copied 0B to ... before reporting cannot copy directory
and exiting with code 1.

This change only prints the summary when the copy succeeded, in both
directions. The quiet paths already returned the error without printing a
summary and are unchanged.

Release notes (optional)

Fix `docker cp` printing a "Successfully copied" message when the copy fails.

Testing

New regression tests in cli/command/container/cp_test.go:

• TestCopyFromContainerDoesNotReportSuccessOnFailure: copying a directory
  onto an existing regular file returns cannot copy directory, leaves the
  destination unchanged, and prints no success message.
• TestCopyToContainerDoesNotReportSuccessOnFailure: a failed upload prints
  no success message.

Both tests failed before this change (the success message was printed) and
pass after it. The existing TestCopy* / TestRunCopy* tests in
cli/command/container still pass, and gofmt and go vet are clean for
the changed package.

Note: TestContainerExportOutputToFile fails in this environment both with
and without this change (gid expectation mismatch), so it is unrelated to
this change. No end-to-end run against a real daemon was done.

[thaJeztah]

We should have a look at this code in general, because it's rather convoluted (definitely for a follow-up), but perhaps as part of this PR, we should consider renaming res to err - looking at the code didn't make it clear that res is just an error, not some other "result" return type.

[xh123637]

Done — renamed res to err in copyFromContainer (commit 317a13c). Since err was already declared in this function, the CopyTo call now assigns to it directly. I left the broader cleanup of this code for a follow-up, as you suggested.

[thaJeztah]

Thanks for updating; could you;

• squash the commits
• remove the Fixes #xxxx from the commit message (we try to avoid those as they can lead to pre-mature closing tickets, and produce noise on GitHub when merged in forks and branches)
• Your sign-off uses xh123637 (your GitHub handle) as name; could you use your real name? see https://github.com/docker/cli/blob/v29.10.0-rc.1/CONTRIBUTING.md#sign-your-work

Use a known identity (sorry, no anonymous contributions.)

[denlo-0]

Thanks for the fix! I tested 317a13c against a real Docker Engine and confirmed that it resolves the issue. Successful copies still work as expected.

[xh123637]

Thanks for the pointers — all three addressed in the new head commit ee70962:

• Squashed into a single commit.
• Removed the Fixes: line from the commit message (the issue link stays in the PR description).
• The sign-off now uses my real name: Signed-off-by: Xiong Han <[email protected]>, with the commit author/committer set to the same name. DCO is green.

No code changes — the tree is identical to the previous head.

[thaJeztah]

LGTM

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[thaJeztah]

Looks like a linter isn't happy now 😢

cli/command/container/cp.go:357:1: cyclomatic complexity 17 of func `copyToContainer` is high (> 16) (gocyclo)
81.65 func copyToContainer(ctx context.Context, dockerCLI command.Cli, copyConfig cpConfig) error {
81.65 ^

[thaJeztah]

Pushed a fix (added some //nolint comments for now)

[thaJeztah]

Derp; one //nolint too many; should be good now 😅