what happens if they pass a directory with -f

@jfrazelle Very good point. For destination, if a directory is specified, then it should try to copy the file to that directory, creating the path on its way(which is current behavior).
I believe,your point is more towards source, if the source is a directory, -f is ignored.

Two ways to change the implementation. 1. It will be ideal to know if the source is a file or dir on the container before sending the request to the container avoiding overhead of transferring data. A bit unnecessary new functionality. 2. Detect if this is the last file in the stream received, io.ReadCloser does not let me peek but if -f is specified I can fetch the next file after the first file is fetched and expect it to be EOF and if that is not true, we can error out.

I like the way code handles it today for directories(before and after my change). If at all I tend toward option 2
Please let me know your opinion.

Examples (tested):
docker cp -f abc:/srcdir/file1 test //copies file1 into a file ./test in the current dir
docker cp -f abc:/srcdir/file1 newdir/ //copies file1 into a ./newdir/file1 (creates newdir if needed)
docker cp -f abc:/srcdir/file1 newdir/newfile //copies file1 into ./newdir/newfile

Old behavior (-f is ignored)
docker cp -f abc:/srcdir/ test //creates ./test/srcdir ... copies entire contents from srcdir
docker cp -f abc:/srcdir test //creates ./test/srcdir ... copies entire contents from srcdir

rebased, please see if this can be merged.

I'm not sure about this. I thought that docker cp should be somehow similar to cp or scp.

@LK4D4 I understand the concern but we have less of a choice to revert the behavior we have today (make cp file centric). I see current behavior is widely used in Docker Build and the builder. I thought adding explicit flag to work with single files is the only option. Please suggest if you think of an alternative approach that I can use to implement this.

@SvenDowideit When running the python script to update docs I believe it probably adds the commiters name to the history.
When the container file is a directory, it behave like it does today. The directory will be tar'd and copied over. I can catch it but I do not want to regress that behavior we have today.

@brahmaroutu sorry, the documentation in this PR does not tell the reader what the new flag does that is different to without it - that needs to be added to this PR.

Should this be possible without having to add the flag? I.e.;

# copy file.txt to file foobar.txt
docker cp /some/file.txt foobar.txt

# if dest exists, copy the file.txt into dest, 
# otherwise save file.txt as a *file* called "dest"
docker cp /some/file.txt dest

# possibly (probably too magic) if dest ends with a /, 
# force it being a directory, create dest if it not exists
# and copy file *into* that directory
docker cp /some/file.txt dest/

I just think that this will act more according to user expectations; having to set a flag in order to copy a file just doesn't feel natural.

I wanted to separate the current behavior, docker cp is directory bound, by adding a flag. The other way is to detect that the tar received from the container has a single file and logic can deal with it, this require complex changes as the current logic cannot read the tar twice(once to detect content and then stream). My implementation is explicit that user can tell me that he is copying a file that way we simply extend current 'cp' command.

I'm with you on not trying to break BC. Would it be possible to have the daemon return a different response based on if the source was a file or a directory?

Only able to comment from a user perspective as I lack the technical skills to oversee the technical requirements to achieve this.

In short, if possible, I'd really prefer being able to copy a file without having to add a flag.

I see your concern, changing the current signature/behavior of cp could be quite sensitive. I will have to explore other alternatives.

Of course, given that the maintainers agree 😄

Thanks for investigating, much appreciated!

We got a little explanation about the reasons for the -f flag. It is to preserve backwards compat with the current functionality. I think that makes sense in this case.

I'll let @tiborvass and @unclejack look at the archive changes as they are the most familiar with that code.

@crosbymichael If we have the ability to seek(pos) and reset on the pkg/ioutils/reader we can probably peek and detect the number of objects in the stream. If it has only one file then we can change the logic accordingly. This is more of a golang issue I guess.

@brahmaroutu you can probably look at the tar headers to get the number of files that the tarstream contains.

@brahmaroutu @crosbymichael sorry but this one is difficult to agree on the design. Let's make sure we first agree, and then update the PR.

I don't like -f. cp -f is connotated with --force, not --file.
Moreover, we are adding a new flag because the default behavior is non-intuitive. But what makes me sad is that there doesn't seem to be a good solution...

To be honest, I really think docker cp needs a breaking change. We should assess who uses it and how, and maybe have a very clean path to updating it. With maybe just a warning in 1.4 to say that in 1.5 it will be different, I don't know.

@tiborvass I agree, given the design, we cannot be automatically detect a file copy. Willing to change the implementation if you have better ideas. The golang tar does not have any meta data to see if a single file is in the archive and hence we rely on user letting us know that this is a single file copy. Changing the command name(cpm) may be another option?
I need not hack my way through this as we know the new parser implementation can take care of this some how.

@brahmaroutu Like I said I don't want you to code unnecessarily, that's why I'd prefer we agree on the design first. Will add this to the list of things I'd like to discuss in the next design review session.

@tiborvass yep, i was just commenting on I can see how it make sense here as to not break backwards compat because our first reflex was to say "we shouldn't need a flag at all".

Just an FYI this totally works and I use it all the time.

docker cp binary:/go/src/github.com/docker/docker/bundles/1.3.1-dev/binary/docker-1.3.1-dev .

I get the binary, one file in my cwd.

I know the tar ball works fine for copying single files. We are addressing the scenario where
docker cp myapp:/my_app/app.py app1.py
this creates app1.py/app.py because of the tar ball effect instead of simply creating a new file app1.py.

Yes, from my example, i would mostly say that this is starting to look like a real bug and not something that we would need to work around to keep backwards compat.

@crosbymichael if we decide it's a bug, I'm all for it!

I vote for treating it like a bug and fixing it in a new API version bump. If the client wants to use the old version (<= 1.16) then keep the old behavior. If the client uses a new api version (>= 1.17) then match the behavior of standard tools like scp and cp.

Here's a summary of the current work I have at the other PR with general improvements to docker cp:

Improve docker cp

This command currently uses the following HTTP API endpoint:

POST /containers/(id)/copy

This endpoint only supports copying files/directories from a container to the
client. Its behavior is also odd in that DEST is always treated as a
directory. It would be preferrable to have the same behavior as standard Unix
utilities like cp and scp.

New API Endpoints

This endpoint may be split into two separate API endpoints to handle copying
files or directories to and from a container:

GET /containers/(id)/copy?path=/container/path

This endpoint has similar behavior to the current copy endpoint and is used
to copy data from the container to the client. The difference is that it will
correctly handle the case when the source is a file or is a directory and ends
with a trailing path separator, including the directory or only its contents.

PUT /containers/(id)/copy?srcPath=/local/path&dstPath=/container/path

This endpoint may be used for copying data from the client to a destination
path inside a container.

Use of these 2 endpoints together would allow for a client to copy files or
directories from one container to another, but this would require transfering
an archive through the client. For this purpose, a 3rd endpoint can be used:

PUT /containers/(id)/copy-across?dstContainer=id&srcPath=/path1&dstPath=/path2

Any container paths which are not absolute are considered relative to the
container's working directory.

CLI Usage

copy from a container to the client

docker cp CONTAINER:PATH LOCALPATH

Copies a file or directory at PATH in the specified CONTAINER to a
LOCALPATH on the client.

copy from the client to a container

docker cp LOCALPATH CONTAINER:PATH

Copies a file or directory at LOCALPATH to PATH in the specified
CONTAINER.

copy from one container to another

docker cp SRC_CONTAINER:SRC_PATH DST_CONTAINER:DST_PATH

Copies a file or directory at SRC_PATH in SRC_CONTAINER to DST_PATH in
the DST_CONTAINER.

Behavior of docker cp SOURCE DEST

The behavior has been changed to be similar to a combination of mkdir -p and
cp -r:

• SOURCE specifies a file
  • DEST does not exist
    • the file is saved to a file created at DEST
  • DEST exists and is a file
    • the file is overwritten with the contents of the source file
  • DEST exists and is a directory
    • the file is saved to this directory using the basename from PATH
• SOURCE specifies a directory (no trailing path separator)
  • DEST does not exist
    • DEST is created as a directory and the source directory is copied
      into this directory
  • DEST exists and is a file
    • Error condition: cannot copy a directory to a file
  • DEST exists and is a directory
    • the source directory is copied into this directory
• SOURCE specifies a directory (with trailing path separator)
  • DEST does not exist
    • DEST is created as a directory and the source directory's contents
      are copied into this directory
  • DEST exists and is a file
    • Error condition: cannot copy a directory to a file
  • DEST exists and is a directory
    • the source directory's contents are copied into this directory

@brahmaroutu would you mind checking if #9871 fixes the issue. Then I think we can close this. Sorry for letting this linger for so long.

@jfrazelle Yes, we should close this issue. I see #9871 is a complete solution, addresses many more missing pieces from the copy command. Great Job from @jlhawn.
I will post any concerns I have into #9871.

thanks so much :)