feat(_comp_expand_glob): Add a utility to safely perform pathname expansions (Fix #705) #708

akinomyoga · 2022-02-25T03:07:15Z

Fixes #705 and similar problems

Description

Pathname expansions are used in many places in the codebase, but it seems only a small part of these pathname expansions considers the setting of nullglob, and only mysql considers the setting of failglob. All the pathname expansions except for that in mysql can possibly fail due to failglob. Actually, there are even other settings that we need to care about for the pathname expansions, such as nounset, dotglob, GLOBIGNORE, and nocaseglob.

We don't want to always write codes to check all these settings in-place, so I suggest introducing a shell function _comp_expand_glob to perform the pathname expansions.

Commits

This PR contains several changes.

ecc384d This is an extension to conftest.py (bash_env_saved) that allows an arbitrary change to the variables and settings within the bash_env context. This is used in a test of _comp_expand_glob.
f0a4028 This is also an extension to conftest.py (bash_env_saved) that allows saving/restoring the settings of set -o NAME and checking the unintended changes to them (just like the existing ones for shopt and variables). This is also used in a test of _comp_expand_glob.
59ec460 This is the function to perform the pathname expansions. (This function is actually a simplified version of my functions ble/util/eval-pathname-expansion and ble/complete/util/eval-pathname-expansion).
36a540f Fixes shopt failglob causes bash_completiion to give error in _included_ssh_config_files #705.
89dc92e This fixes the other pathname expansions that I could find (I guess there are still other places that use the pathname expansions).
3b85848 This is a fix of a problem of mysql completion uncovered by the new _comp_expand_glob, which has originally happened with shopt -s nullglob.
5d9d354 This is another fix to mysql for a problem that has existed for a long time.

Discussion

There are several things that I want to discuss for the detailed design.

1. `dotglob`

While performing the pathname expansions, should we turn dotglob on or turn it off? Currently, It's turned off as that is the default of Bash. But maybe it is better to be turned on in some cases.

2. `nocaseglob`

Currently, _comp_expand_glob doesn't change the settings for nocaseglob for its internal expansions, but maybe we should also normalize the behavior of the case matching regardless of the current user settings of nocaseglob.

3. Naming of `_comp_expand_glob`

Other possibilities include

_comp{,_util}_{,_}expand_{pathname{,s},glob}

4. `extglob`

I think we are requiring that extglob is turned on, but maybe we can explicitly adjust extglob as well as the other options.

scop

TBH, I wish we didn't have to do something like this :/ But I guess we do want it.

bash_completion

test/t/conftest.py

akinomyoga · 2022-03-17T22:58:16Z

I have added fixes 5d9d354...3f86a09 and rebased & squashed changes.

scop

Barring a few small comments, and resolving conflicts, looks good to me; feel free to merge after addressing (or as always, feel free to bounce back for more opinions if you want).

I'm not sure if the hooks stuff discussed elsewhere will eventually lead into a generic ability to make the environment/options/settings what we want and reset them back at start and end of all our completions (e.g. _init_completion and trap trickery). If they do, this could become obsolete, but that stuff might take some time and might never actually realize, so I think this is fair enough at least in the meantime.

scop · 2022-04-15T06:14:44Z

bash_completion


+    local IFS=$' \t\n'


I think this may have been added originally for $reset to work and thus might not be needed any longer -- at least this PR is removing it with another similar change in _xinetd_services above and elsewhere.

Sorry for the delay. I think I kept it so that it doesn't affect the later word splitting COMPREPLY+=($(...)).IFS removal in _xinetd_services is an oversight. 8b267eb. I have also added IFS=$'\n' to protect _service for the splitting of compgen results.

scop · 2022-04-15T06:20:42Z

completions/ipmitool

+                    printf '%s\n' "${files[@]}" |
+                        command sed -ne 's/^[^0-9]*\([0-9]\{1,\}\)/\1/p'


Now that we have the candidates in a bash array, I suppose we could loop over and filter them in pure bash instead of doing this. Not a biggie, leaving up to you to decide what to do about it or to leave as is.

Refactored in 24e095b. We can actually implement it using compgen -X without a loop.

scop · 2022-04-15T06:32:51Z

bash_completion

@@ -245,6 +245,53 @@ _upvars()
    done
 }

+# Get the list of filenames that match with the specified glob pattern.


I think a little reminder here why we should be using this this would be a nice touch.

Suggested change

# Get the list of filenames that match with the specified glob pattern.

# Get the list of filenames that match with the specified glob pattern.

# This function does the globbing in a controlled environment, avoiding

# interference from user's shell options/settings or environment variables.

Thank you! I have applied it 62e2440.

Co-authored-by: Ville Skyttä <[email protected]>

When there are no "charset" files, nothing has been generated although we intended to at least generate "utf8". The check for the number of generated candidates should be performed after generating "utf8". The wrong check has been introduced in commit 08dd2cd. Originally, we have been performing the pathname expansions without adjusting "nullglob", so the pattern "/usr/share/mysql/charsets/*.xml" has been directly strayed into the array "charsets" unless "nullgob" is set. Then the check has been always passed when "nullglob" is not set, which is the reason that we have been missing the problem until now.

* Instead of replacing "Index.xml" with an empty string after the pathname expansions, we now exclude "Index.xml" from the pathname pattern. * In case that there is an empty filename as "/usr/share/mysql/charsets/.xml", we explicitly exclude empty charset names by specifying the option, -X '', for compgen.

akinomyoga · 2022-04-17T21:06:12Z

I'm not sure if the hooks stuff discussed elsewhere will eventually lead into a generic ability to make the environment/options/settings what we want and reset them back at start and end of all our completions (e.g. _init_completion and trap trickery). If they do, this could become obsolete, but that stuff might take some time and might never actually realize, so I think this is fair enough at least in the meantime.

Yeah, I have also thought about it. Something we need to care about is whether the scope of these shell settings for the pathname expansions can be safely expanded to the entire completion. For example, if there are any completion functions that generate the completions depending on the user's current shell settings, expanding the scope of the adjusted shell settings can affect the results. But I'm not sure if there are any existing ones.

akinomyoga · 2022-04-17T21:24:28Z

Merging

akinomyoga changed the title ~~Fix failglob included ssh config files~~ feat(_comp_expand_glob): Add a utility to safely perform pathname expansions (Fix #705) Feb 25, 2022

akinomyoga force-pushed the fix-failglob-included_ssh_config_files branch 3 times, most recently from 2ad707c to 5d9d354 Compare February 26, 2022 01:29

scop requested changes Mar 17, 2022

View reviewed changes

akinomyoga force-pushed the fix-failglob-included_ssh_config_files branch from 3f86a09 to 925f29a Compare March 17, 2022 22:56

akinomyoga mentioned this pull request Apr 11, 2022

refactor(bash_completion): refactor { => _comp_}{dequote,quote} #736

Merged

scop reviewed Apr 15, 2022

View reviewed changes

akinomyoga added 2 commits April 18, 2022 05:54

test(bash_env_saved): add "save_{shopt,variable}" to allow any changes

4411787

test(bash_env_saved): add "{,save_}set" to save/restore "set" options

6bc91ed

akinomyoga force-pushed the fix-failglob-included_ssh_config_files branch from 62e2440 to 76cee0f Compare April 17, 2022 20:57

akinomyoga and others added 6 commits April 18, 2022 05:58

feat(_comp_expand_glob): add utility to safely expand pathnames

28c4ed9

Co-authored-by: Ville Skyttä <[email protected]>

fix(_included_ssh_config_files): fix for failglob

13464e4

fix: use "_comp_expand_glob" for failglob/nullglob/etc

5d49f24

refactor(ipmitool): filter -d optargs by compgen -X

ec907d9

akinomyoga force-pushed the fix-failglob-included_ssh_config_files branch from 76cee0f to ec907d9 Compare April 17, 2022 20:58

akinomyoga merged commit 3414cb6 into scop:master Apr 17, 2022

akinomyoga deleted the fix-failglob-included_ssh_config_files branch April 17, 2022 21:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(_comp_expand_glob): Add a utility to safely perform pathname expansions (Fix #705) #708

feat(_comp_expand_glob): Add a utility to safely perform pathname expansions (Fix #705) #708

akinomyoga commented Feb 25, 2022 •

edited

Loading

scop left a comment

akinomyoga commented Mar 17, 2022

scop left a comment

scop Apr 15, 2022

akinomyoga Apr 17, 2022 •

edited

Loading

scop Apr 15, 2022

akinomyoga Apr 17, 2022

scop Apr 15, 2022

akinomyoga Apr 17, 2022

akinomyoga commented Apr 17, 2022

akinomyoga commented Apr 17, 2022

		printf '%s\n' "${files[@]}" \|
		command sed -ne 's/^[^0-9]*\([0-9]\{1,\}\)/\1/p'

feat(_comp_expand_glob): Add a utility to safely perform pathname expansions (Fix #705) #708

feat(_comp_expand_glob): Add a utility to safely perform pathname expansions (Fix #705) #708

Conversation

akinomyoga commented Feb 25, 2022 • edited Loading

Description

Commits

Discussion

1. dotglob

2. nocaseglob

3. Naming of _comp_expand_glob

4. extglob

scop left a comment

Choose a reason for hiding this comment

akinomyoga commented Mar 17, 2022

scop left a comment

Choose a reason for hiding this comment

scop Apr 15, 2022

Choose a reason for hiding this comment

akinomyoga Apr 17, 2022 • edited Loading

Choose a reason for hiding this comment

scop Apr 15, 2022

Choose a reason for hiding this comment

akinomyoga Apr 17, 2022

Choose a reason for hiding this comment

scop Apr 15, 2022

Choose a reason for hiding this comment

akinomyoga Apr 17, 2022

Choose a reason for hiding this comment

akinomyoga commented Apr 17, 2022

akinomyoga commented Apr 17, 2022

akinomyoga commented Feb 25, 2022 •

edited

Loading

1. `dotglob`

2. `nocaseglob`

3. Naming of `_comp_expand_glob`

4. `extglob`

akinomyoga Apr 17, 2022 •

edited

Loading