Skip to content
This repository was archived by the owner on Jul 18, 2025. It is now read-only.

Don't use extglob in bash/docker-machine.bash#4141

Merged
shin- merged 1 commit into
docker-archive-public:masterfrom
Empact:extglob
Jul 11, 2017
Merged

Don't use extglob in bash/docker-machine.bash#4141
shin- merged 1 commit into
docker-archive-public:masterfrom
Empact:extglob

Conversation

@Empact
Copy link
Copy Markdown
Contributor

@Empact Empact commented Jun 16, 2017

extglob is not needed here - the only call is:
_docker_machine_map_key_of_current_option '--filter'

So $glob is always '--filter', so there isn't every a pattern-list to match.

According to my testing completion for docker-machine
ls --filter completion works for both driver and state

Fixes #4126
Introduced in fd9a0a6

@GordonTheTurtle
Copy link
Copy Markdown

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "extglob" git@github.com:Empact/machine.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

extglob is not needed here - the only call is:
_docker_machine_map_key_of_current_option '--filter'

So $glob is always '--filter', so there isn't every a pattern-list to match.

According to my testing completion for docker-machine
ls --filter completion works for both driver and state

Fixes docker-archive-public#4126
Introduced in fd9a0a6

Signed-off-by: Ben Woosley <ben.woosley@gmail.com>
@Empact Empact changed the title Don't use extglob in bash/docker-machine.bash #4126 Don't use extglob in bash/docker-machine.bash Jun 16, 2017
@Empact
Copy link
Copy Markdown
Contributor Author

Empact commented Jun 16, 2017

cc @albers

@albers
Copy link
Copy Markdown
Contributor

albers commented Jun 17, 2017 via email

@albers
Copy link
Copy Markdown
Contributor

albers commented Jun 20, 2017

Ah, Mac OS again with its ancient bash.
This function was taken from Docker's bash completion which takes care of temporarily setting the extglob option while loading the script. I forgot to take this into account here.

@Empact You're right: no extglob is needed here, so it's probably best to keep the code simple. Thanks very much for fixing this.

To verify without a Mac:
Original code fails to load with bash 3.2.57:

$ docker run --rm --volume $(pwd)/contrib/completion/bash/docker-machine.bash:/docker-completion albers/bash-completion-mac bash -c ". /docker-completion" && echo OK
/docker-completion: line 86: syntax error in conditional expression: unexpected token `('
/docker-completion: line 86: syntax error near `@($'
/docker-completion: line 86: `    [[ ${words[$glob_pos]} == @($glob) ]] && echo "$key"'

With the fix it loads properly:

$ docker run --rm --volume $(pwd)/contrib/completion/bash/docker-machine.bash:/docker-completion albers/bash-completion-mac bash -c ". /docker-completion" && echo OK
OK

LGTM

@shin- This is a bugfix and should go into 1.12.0.

@jakirkham
Copy link
Copy Markdown

Would it be reasonable to test against bash 3.2.57 to catch errors like this during development?

@albers
Copy link
Copy Markdown
Contributor

albers commented Jul 9, 2017

@jakirkham I'm working on this. Similar to docker/cli#266, a test like docker run --rm --volume $(pwd)/contrib/completion/bash/docker-machine.bash:/docker-completion albers/bash-completion-mac bash -c ". /docker-completion" should be added.
I'm not familiar with the build tooling, though, so this will take some time.

@shin- shin- added this to the 0.12.2 milestone Jul 11, 2017
@shin-
Copy link
Copy Markdown
Contributor

shin- commented Jul 11, 2017

Thank you both!

@shin- shin- merged commit 08e7a5f into docker-archive-public:master Jul 11, 2017
@Empact Empact deleted the extglob branch July 11, 2017 20:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants