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

refactor(completions/*): ensure quoting for compgen -W '${arr[@]}' #887

Merged
merged 6 commits into from
Apr 5, 2023

Conversation

akinomyoga
Copy link
Collaborator

There are also other fixes I noticed while rewriting compgen -W '${arr[@]}'. The descriptions are found in the commit messages.

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.

This seems the right thing to do in a sense, but given the uses are mostly in cases like COMPREPLY=($(...)) that just end up splitting everything to the array by whitespace anyway, I feel in practice this doesn't make much of a practical difference at the moment, or at least it doesn't generally protect against whitespace embedded in individual completion items. But perhaps I'm missing something?

completions/minicom Show resolved Hide resolved
@akinomyoga
Copy link
Collaborator Author

This seems the right thing to do in a sense, but given the uses are mostly in cases like COMPREPLY=($(...)) that just end up splitting everything to the array by whitespace anyway, I feel in practice this doesn't make much of a practical difference at the moment, or at least it doesn't generally protect against whitespace embedded in individual completion items. But perhaps I'm missing something?

Ah, yes. I actually intend to submit a separate PR for the mass change of COMPREPLY=($(...)) for the protection from random IFS (025736d is a WIP commit), but it's blocked by #870 and even another planned PR.

Another reason is that it is not evident at a glance whether these codes are kept unquoted because they are harmless or just not yet checked. It is noisy for me because I tend to check whether it is safe or not every time I witness such a code.

@scop
Copy link
Owner

scop commented Mar 23, 2023

Just a ping here about the conflict so it doesn't go unnoticed, no rush.

@akinomyoga
Copy link
Collaborator Author

Thank you! I'll update it.

With the current implementation, an empty completion would be
generated when there is a file whose basename is `minirc.`.  In this
patch, we filter such files in the pathname-expansion phase by
requiring at least one character after `minirc.` in the filenames.
* fix a leaked variable `filedir` (written directly in the global
  context)
* fix a problem that filenames containing IFS would be splitted
  `$(/bin/ls ...)`.
* fix a problem that the fixed path `/bin/ls' always uses the system
  `ls' even when the user wants to replace it with another version for
  virtual environments, NixOS, etc.
* fix a problem that elements of tmplist would be added multiple times
  with `tmplist+=(${tmplist[@]-} new_elem)`
* fix a problem that word splitting and pathname expansions are
  performed for `${tmplist[@]-}.`
* fix a problem that the package names can be splitted by (unusual)
  IFS
* fix localvar_inherit for `pkginst_list`
@akinomyoga
Copy link
Collaborator Author

I have rebased it.

@scop scop merged commit 6806708 into scop:master Apr 5, 2023
@akinomyoga akinomyoga deleted the failglob-2 branch April 5, 2023 19:39
akinomyoga added a commit to akinomyoga/bash-completion that referenced this pull request Sep 18, 2023
Since the removal of elements are performed by "${params[@]/$i/}" in
the above loops, the removed elements remain in the array `params` as
empty elements.  Therefore, we need to remove the empty elements by
specifying `-X ''`.  This is a regression by
scop#887.  These empty
elements were originally automatically dropped by unquoted
${params[@]}.

Reference:
scop@13c28e9#diff-62e0a63a79957c52fe6b79be0a66a987da044839c6bcaeaa725fcd199c4e528fR160
akinomyoga added a commit to akinomyoga/bash-completion that referenced this pull request Sep 23, 2023
Since the removal of elements are performed by "${params[@]/$i/}" in
the above loops, the removed elements remain in the array `params` as
empty elements.  Therefore, we need to remove the empty elements by
specifying `-X ''`.  This is a regression by
scop#887.  These empty
elements were originally automatically dropped by unquoted
${params[@]}.

Reference:
scop@13c28e9#diff-62e0a63a79957c52fe6b79be0a66a987da044839c6bcaeaa725fcd199c4e528fR160
akinomyoga added a commit to akinomyoga/bash-completion that referenced this pull request Sep 23, 2023
Since the removal of elements are performed by "${params[@]/$i/}" in
the above loops, the removed elements remain in the array `params` as
empty elements.  Therefore, we need to remove the empty elements by
specifying `-X ''`.  This is a regression by
scop#887.  These empty
elements were originally automatically dropped by unquoted
${params[@]}.

Reference:
scop@13c28e9#diff-62e0a63a79957c52fe6b79be0a66a987da044839c6bcaeaa725fcd199c4e528fR160
akinomyoga added a commit to akinomyoga/bash-completion that referenced this pull request Sep 25, 2023
Since the removal of elements are performed by "${params[@]/$i/}" in
the above loops, the removed elements remain in the array `params` as
empty elements.  Therefore, we need to remove the empty elements by
specifying `-X ''`.  This is a regression by
scop#887.  These empty
elements were originally automatically dropped by unquoted
${params[@]}.

Reference:
scop@13c28e9#diff-62e0a63a79957c52fe6b79be0a66a987da044839c6bcaeaa725fcd199c4e528fR160
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