ping @vbatts

LGTM

moved a defer up a little to a better spot - thanks to @MHBauer for noticing.

So interesting. It seems like we hardly need to do more than just return as if the read is done, but this seems fine.
LGTM

counting two LGTM's so moved to "merge"

let's wait for the tests to finish first, just want to make sure the move of the defer doesn't break something

@duglin that's why I applied the label, and didn't press the button 👍

Why do we do this is tar loading? If such images exist wouldn't they fail on pulls also?

edit: so isn't the bug in DecompressStream where it shouldn't fail for empty file but just mark it as uncompressed.

Pulls fine for me:

λ docker pull matthughes/library-centos:6-2015.03.20
6-2015.03.20: Pulling from matthughes/library-centos
17d1436ef796: Pull complete
9c7cb910d843: Pull complete
25445a0fc502: Pull complete
Digest: sha256:aaba82794dff22b95c649cdd03fcdf3929ce5ecc953403718e713336bc6f9a85
Status: Downloaded newer image for matthughes/library-centos:6-2015.03.20

λ docker images matthughes/library-centos:6-2015.03.20
REPOSITORY                  TAG                 IMAGE ID            CREATED             VIRTUAL SIZE
matthughes/library-centos   6-2015.03.20        25445a0fc502        8 months ago        215.7 MB

λ docker history matthughes/library-centos:6-2015.03.20
IMAGE               CREATED             CREATED BY                                      SIZE                COMMENT
25445a0fc502        8 months ago        /bin/sh -c #(nop) ADD file:2b2b26209d285cd1a9   215.7 MB
9c7cb910d843        13 months ago       /bin/sh -c #(nop) MAINTAINER The CentOS Proje   0 B
17d1436ef796        2 years ago                                                         0 B                 Imported from -

@tonistiigi I originally had this fix in that func but then the unit tests failed because there are cases where EOF is a valid return value (error cases) and the unit tests were looking for them. So I moved this logic up a level. Not 100% sure this is still the right spot but it did seem to fix it.

This change is better to understand now with all the pools gone but I still doubt it's the correct approach.

First, this isn't specific to load. The problem appears when you point empty file to DecompressStream and for this image, in the registry, the blob is compressed so the error doesn't appear. I pushed the same image uncompressed as tonistiigi/library-centos:6-2015.03.20 and it fails on pull just like load does.

Other problem is that if the tar was some random garbage less than 10 bytes, then with this fix we would completely ignore the error.

My suggestion would be that if DecompressStream cannot detect the compression because of an error on Peek() it should just redirect the data as uncompressed and not error by itself. This breaks a unit test TestUntarPathWithInvalidDest but if you look at what the test does, it should really be named TestUnpackingEmptyFileShouldFail, so I'm not sure which behavior we want.

If for some reason we want to have exceptions for pull/load etc. but not change the function we should define DecompressStreamAllowEmpty() or IsEmptyReader() and call that instead in pull/load code.

Also, I'm not sure if we should count empty file as a valid tar's at all. Maybe it's ok to error. Not sure what has caused them in this image.

Removed the "merge" status for now - needs more reviews/work.

@tonistiigi you were correct to push on the validity of the unit testcase. It was passing for the wrong reasons. Once I fixed that ignoring EOF in DecompressStream does seem to work. The test now, correctly, uses an invalid destination and will fail for that reason, not due to the source tar being empty.
See what you think.

LGTM

@vbatts and @LK4D4 can you guys re-review it, just to be sure? thanks.

@tonistiigi thanks for the help

LGTM on the updated fix

LGTM

Thanks so much @duglin !

@matthughes you're welcome - let me know if it doesn't fix it :-)