Skip to content

fix(bash_completion): use _comp_split and new _comp_compgen for safer splitting #919

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 16, 2023

Conversation

akinomyoga
Copy link
Collaborator

Now we have a utility function _comp_split to safely split strings (including the output of the compgen builtin and other commands) without being affected by pathname expansions, unexpected word splitting, etc. I would like to switch from the repeated local IFS=... reset=$(shopt -po noglob); set -o noglob; ...; IFS=$' \t\n'; $reset to _comp_split.

  • b46b71e I modified _comp_split to return whether nonzero elements are produced in the exit status. This is because ((${#arr[@]})) is tested in many cases after splitting strings.
  • f8a8f9f Also, there are many cases of _comp_split -l arr "$(compgen ARGS)", so it is useful to define a function _comp_compgen arr ARGS (in particular to reduce nested quoting and command substitutions).
  • bf37c0d and a765cc5 are independent fixes of the problems I noticed in testing _comp_compgen, etc.
  • d015b5c is the main rewrite for the switching

The changes suggested in this PR are actually the ones that I mentioned in the following PRs before.

The changes to completions/* are left for later PRs.

Copy link
Owner

@scop scop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_tilde and _known_hosts_real fixes cherry-picked, will revisit the main content of this later. (As a first impression, I like the idea.)

Copy link
Owner

@scop scop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, this is great, thanks! Left some nitty comments, pre-approved with them addressed/not as you like.

Co-authored-by: Ville Skyttä <ville.skytta@iki.fi>
@akinomyoga akinomyoga force-pushed the _comp_split branch 2 times, most recently from cdeda74 to eaeb53e Compare April 15, 2023 13:01
akinomyoga and others added 4 commits April 15, 2023 22:09
Co-authored-by: Ville Skyttä <ville.skytta@iki.fi>
Co-authored-by: Ville Skyttä <ville.skytta@iki.fi>
We allow positional parameters ($1...) in the current word specified
as `-- CUR` at the end of the argument list.
We needed to adjust IFS for splitting before, but we do not need it
now because we switched to `_comp_split`.  IFS is anyway adjusted by
callers `_parse_help` and `_part_usage`.

Co-authored-by: Ville Skyttä <ville.skytta@iki.fi>
@scop scop merged commit b061c76 into scop:master Apr 16, 2023
@akinomyoga akinomyoga deleted the _comp_split branch April 16, 2023 18:23
@scop
Copy link
Owner

scop commented May 1, 2023

Added https://github.com/scop/bash-completion/projects/2#card-89052206 for the completions/* work, perhaps that's something we could try to do for 2.12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants