LGTM

The problem with this is that now we say:
Error response from daemon: Prefix can't be empty
which isn't very meaningful to an end-user.

At least for delete, I prefer the current msg:
Error response from daemon: Container name cannot be empty

@duglin So does deleteContainers not fall under the same scenario as stated in the issue "Check is not needed because the http handler didn't match that route with an empty string" ?

Why shouldn't it? There's another check in GetFullContainerName in ContainerRm that says Container name cannot be emtpy. @duglin when is printing Prefix can't be empty?

Actually, I would almost prefer if we checked for these on the CLI - and printed a nice error message from there. Then on the daemon side I'd expect both to return 404's since I think they map to:

diff: ../containers//changes - which gets a 301 redirect to ../containers/changes - which then gives a 404
del: ../containers/

I like 404 from a rest view..

@runcom with this PR I get this:

$docker rm ""
Error response from daemon: Prefix can't be empty
FATA[0000] Error: failed to remove one or more containers 

which isn't very friendly as the old way.

@duglin wasn't working before also then https://github.com/docker/docker/pull/12197/files

because there was no check before, and that just maintained the old behaviour

antonio-laptop :: ~ » docker rm ""
Error response from daemon: Prefix can't be empty
FATA[0000] Error: failed to remove one or more containers

I get this error even on 1.6 rc5

In my master (just refreshed) I get:

$ docker rm ""
Error response from daemon: Container name cannot be empty
FATA[0000] Error: failed to remove one or more containers 

Regardless, I think the "Container name cannot be empty" is the right one - so, let's make that come out.... somehow (and on purpose) :-)

@duglin So change back deleteContainers, but getContainersChanges is fine as is?

you get that on master AND on 1.5.0+... however yes Container name cannot be empty is more meaningful

@kostickm let's add checks to the CLI code to stop "" even before it tries to hit the daemon - for both.

I think the daemon side changes for "changes"/diff are ok.

For "delete" though, we have an interesting situation where:
DELETE ../containers/ returns Prefix can't be empty
but
DELETE ../containers returns a 404.
and
DELETE ../containers/foo returns a 404.
I would prefer if the "../containers/" acted like the other 2 and returned a 404.

I'll work on making those changes.

@duglin For delete, the place where it returns the 404 or the "Prefix can't be empty" is found in pkg/truncindex/truncindex.go. I didn't want to change it at that level in case it is needed in other cases. Though from the CLI with the "" check in place we won't run into this error. I tested it without the CLI check though to make sure it returns the 404 in all cases now.

Let me know what you think :)

@estesp Let me change that!

LGTM
But I will note that I'm pretty sure the 404 is coming back because of an interesting behavior on the daemon. In both cases the incoming URL will be something like .../containers// or ../containers//changes which forces a 301 to be returned with with ../container/ or ../containers/changes. And that generates a 404 from a different spot in the code. But that an issue for this PR.

Actually, I take that back - in the rm case it is hitting the new code so that's good :-) (I had a typo in my test)

LGTM