Improve array handling in alias completion #2293

convergedtarkus · 2025-03-11T21:02:07Z

Description

I was seeing issues where certain aliases do not complete as expected.

For example, my alias gbD='git branch -D' alias should provide completions to local branches only, and it does when run without an alias. However when running completion on the alias, I get all the branches which appears to be the git default completion response.

I tracked this down to an issue where an array was being incorrectly handled. The old handling caused this result, COMP_WORDS=[‘git’, ‘branch -D’, 'ma'] (length 3) where branch and -D were combined into a single entry. This produces the incorrect behavior. By changing the array to string handling to better maintain individual arguments the new result is COMP_WORDS=[‘git’, ‘branch’ ,‘-D’, 'ma'] (length 4) which correctly keeps branch and -D apart.

Motivation and Context

This improves alias completions to act more correctly.

How Has This Been Tested?

I have been unable to create a unit test that is able to call the alias completion function. I have tested this manually on my machine and it works as expected.

An easy way to manually test this is to add the alias biea='bash-it enable alias' and load bash-it with the bash-it completion enabled. Without my fix the complete for this alias are the base level bash-it commands such as doctor, version etc. With my change the output is the aliases that can be enabled as would be expected.

Screenshots (if appropriate):

N/A

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
If my change requires a change to the documentation, I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
I have added tests to cover my changes, and all the new and existing tests pass.

The old approach would cause the alias_arg_words array to be incorrectly converted to a string leading to a command like git branch -D being converted into ('git' 'branch -D') which was incorrect. The new approach keeps each argument separate leading to ('git' 'branch' '-D') which produces correct completion.

seefood

is it necessary to go through this extra printf? I would think just switching [*] to [@] might have worked too? please try to do this a bit cleaner maybe, or at least make like 84 use a local var and calculate it down by the other line. the distance between the two doesn't make sense to me, unless I missed something.

completion/available/aliases.completion.bash

convergedtarkus · 2025-03-12T16:28:30Z

@seefood The printf produces different output than using @ or * which is important to ensure the echo on line 85 produces the correct output string.

For example:

myArray=(one two three)
echo "* version: '${myArray[*]}'"
echo "@ version: '${myArray[@]}'"
printf "printf version: '"
printf "'%s' " "${myArray[@]}"
printf "'\n";

That outputs

* version: 'one two three'
@ version: 'one two three'
printf version:''one' 'two' 'three' '

Notice that the printf version put each array value in quotes. That is the important part, even more so if one of the array values has a space in it. In addition, the printf command needs to use "${myArray[@]}" not "${myArray[*]}" as the * version will also not quote individual elements of the array.

I can definitely look into cleaning up how this is done though!

convergedtarkus · 2025-03-12T16:58:06Z

The CI failure does not look like it is related to my changes. I'm seeing commits in master that have the same issues where some tests appears to not be running.
I'm not seeing an option to re-run the failing actions, maybe that's not supported here.

akinomyoga · 2025-03-12T20:42:30Z

completion/available/aliases.completion.bash

@@ -89,7 +93,7 @@ function _bash-it-component-completion-callback-on-init-aliases() {
                            prec_word=\${prec_word#* }
                        fi
                        (( COMP_CWORD += ${#alias_arg_words[@]} ))
-                        COMP_WORDS=(\"$alias_cmd\" \"${alias_arg_words[*]}\" \"\${COMP_WORDS[@]:1}\")
+                        COMP_WORDS=(\"$alias_cmd\" $(printf "'%s' " "${alias_arg_words[@]}") \"\${COMP_WORDS[@]:1}\")


This still fails when alias_arg_words[@] contains '. You can use %q

Did this in 15ec29d

Good call. Using %q does correctly escape things better.

I also had to switch to wrapping the %q output in double quotes rather than single quotes, which should be fine since everything is escaped now. If an alias contained single quotes, like the bash-it reload_completion alias does, this would produce invalid output. That is due to the complexity of escaping single quotes in single quotes and due to how that works, I believe it is impossible to make a single array entry that contains single quotes and is wrapped in single quotes.

I also had to switch to wrapping the %q output in double quotes rather than single quotes, which should be fine since everything is escaped now.

No, it doesn't work. %q already quotes everything needed. If you further quote it within '...' or "...", the special characters for quoting is quoted and then remain in the final results.

$ a=('a b c') $ eval "b=($(printf "\"%q\" " "${a[@]}"))" $ declare -p a b declare -a a=([0]="a b c") declare -a b=([0]="a\\ b\\ c")

You should use %q (but not "%q" ).

You are correct. My concern had been about a case like

myArray=("one two") printf "%q " "${myArray[@]}"

That the print would produce one two when it needed to be "one two". What %q actually does in a case like that is it escapes the space, which I didn't even think was possible, to produce one\ two which works just fine.

I removed the quotes in 3e9a033, thanks for teaching me something new!

This correctly quotes and escapes characters to be shell safe. This also requires wrapping the result in rather than '' to avoid problems with escaping single quotes.

seefood · 2025-03-13T06:56:02Z

completion/available/aliases.completion.bash

@@ -87,9 +91,9 @@ function _bash-it-component-completion-callback-on-init-aliases() {
                        if [[ \$COMP_LINE == \"\$prec_word \$compl_word\" ]]; then
                            prec_word='$alias_cmd $alias_args'
                            prec_word=\${prec_word#* }
-                        fi
+                        fi:


sorry, but this seems like a typo. I don't recognise this syntax nor could I make it work in a local test...

Yep, definitely a typo. Removed in ec9d229

This causes incorrect escaping of characters. %q handles all cases that need to be handled including mulitple words in one array entry

convergedtarkus changed the title ~~FUNCTIONAL: Improve array handling in alias completion~~ Improve array handling in alias completion Mar 11, 2025

seefood requested changes Mar 12, 2025

View reviewed changes

completion/available/aliases.completion.bash Outdated Show resolved Hide resolved

seefood self-assigned this Mar 12, 2025

CLEANUP: Inline the printf to avoid variable

6765427

convergedtarkus requested a review from seefood March 12, 2025 16:54

akinomyoga reviewed Mar 12, 2025

View reviewed changes

FUNCTIONAL: Use %q for printing array

15ec29d

This correctly quotes and escapes characters to be shell safe. This also requires wrapping the result in rather than '' to avoid problems with escaping single quotes.

seefood reviewed Mar 13, 2025

View reviewed changes

convergedtarkus added 2 commits March 13, 2025 09:04

TYPO: Remove typo character

ec9d229

FUNCTIONAL: Do not quote %q

3e9a033

This causes incorrect escaping of characters. %q handles all cases that need to be handled including mulitple words in one array entry

convergedtarkus requested a review from seefood March 13, 2025 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve array handling in alias completion #2293

Improve array handling in alias completion #2293

convergedtarkus commented Mar 11, 2025

seefood left a comment

convergedtarkus commented Mar 12, 2025 •

edited

Loading

convergedtarkus commented Mar 12, 2025

akinomyoga Mar 12, 2025

convergedtarkus Mar 12, 2025

akinomyoga Mar 13, 2025

convergedtarkus Mar 13, 2025

seefood Mar 13, 2025

convergedtarkus Mar 13, 2025

Improve array handling in alias completion #2293

Are you sure you want to change the base?

Improve array handling in alias completion #2293

Conversation

convergedtarkus commented Mar 11, 2025

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

seefood left a comment

Choose a reason for hiding this comment

convergedtarkus commented Mar 12, 2025 • edited Loading

convergedtarkus commented Mar 12, 2025

akinomyoga Mar 12, 2025

Choose a reason for hiding this comment

convergedtarkus Mar 12, 2025

Choose a reason for hiding this comment

akinomyoga Mar 13, 2025

Choose a reason for hiding this comment

convergedtarkus Mar 13, 2025

Choose a reason for hiding this comment

seefood Mar 13, 2025

Choose a reason for hiding this comment

convergedtarkus Mar 13, 2025

Choose a reason for hiding this comment

convergedtarkus commented Mar 12, 2025 •

edited

Loading