Skip to content
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

fix: use _comp_compgen for COMPREPLY=($(compgen ...)) #955

Merged
merged 15 commits into from
May 8, 2023

Conversation

akinomyoga
Copy link
Collaborator

@akinomyoga akinomyoga commented May 3, 2023

These are relatively trivial ones for the _comp_compgen rewriting.

edit: #952 is blocking more, so could you see #952 before this? Merged

Note: The commits "fix: use _comp_compgen for COMPREPLY=(...)" are essentially generated by sed:

sed -E "s/$path/$rep/g"

with

pat='(COMPREPLY)=\(\$\(compgen ([^()]*) -- "\$(cur|\{cur-?\})"[[:space:]]*\)\)'   rep='_comp_compgen -- \2'
pat='(COMPREPLY)\+=\(\$\(compgen ([^()]*) -- "\$(cur|\{cur-?\})"[[:space:]]*\)\)' rep='_comp_compgen -a -- \2'
pat='(COMPREPLY)=\(\$\(compgen ([^()]*) -- ("[^"()]+")[[:space:]]*\)\)'           rep='_comp_compgen -c \3 -- \2'
pat='(COMPREPLY)\+=\(\$\(compgen ([^()]*) -- ("[^"()]+")[[:space:]]*\)\)'         rep='_comp_compgen -ac \3 -- \2'
pat='(COMPREPLY)=\(\$\(compgen ([^()]*)\)\)'                                      rep='_comp_compgen -R -- \2'
pat='(COMPREPLY)\+=\(\$\(compgen ([^()]*)\)\)'                                    rep='_comp_compgen -aR -- \2'

@akinomyoga
Copy link
Collaborator Author

  • d660f8a In rebasing, I already found four oversights of -a that should not have been removed...

@akinomyoga
Copy link
Collaborator Author

akinomyoga commented May 5, 2023

The cases in f00dfd0 and ac88694 are the ones originally without specifying any -- "...". I suspect some of them were unintentionally forgotten to add -- "$cur".

  • The completion generation for single characters such as -W '{0..9}' is fine without -- "$cur" because any filtering using "$cur" would generate all or nothing.
  • The generated completions containing $cur as a prefix would also be fine (feh, tar, wget, rcs), though some of them might need proper quoting against the -W splitting.
  • xgamma appears to try to remove the beginning : intensionally.
  • suspect hping2 (1[TAB] would not complete)
  • sus lintian, useradd, usermod

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.

LGTM! Pre-approved with some comments addressed.

completions/gnome-mplayer Outdated Show resolved Hide resolved
completions/hping2 Outdated Show resolved Hide resolved
completions/tune2fs Outdated Show resolved Hide resolved
completions/useradd Outdated Show resolved Hide resolved
completions/usermod Outdated Show resolved Hide resolved
akinomyoga and others added 13 commits May 8, 2023 02:27
The IFS separators specified by the options `-s SEP` or `-l` option is
intended to take effect in evaluating `compgen`, such as the word
splitting in `-W text`.

However, if we set IFS in a separate command from `compgen`, the
arguments specified to `compgen` would also be unexpectedly affected.
Here, ${_cur:+-- "$_cur"} is intended to be expanded to two words if
`_cur` is non-empty.  However, when the separators do not contain a
whitespace, it unexpectedly results in a single word.

IFS can be specified as a temporary environment of the `compgen`
command so that it does not affect the arguments of `compgen`.
There are also more non-trivial cases where COMPREPLY is first
constructed and then filtered by `_comp_compgen -- -W
'"${COMPREPLY[@]}"'`, but they would conflict with forthcoming other
PRs for _comp_compgen rewrite.  We will address them after finishing
the _comp_compgen-related PRs.

Co-authored-by: Ville Skyttä <[email protected]>
@akinomyoga akinomyoga force-pushed the _comp_compgen-1 branch 2 times, most recently from 3736999 to 70d0595 Compare May 7, 2023 22:24
@scop scop merged commit a805a2f into scop:master May 8, 2023
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