fix(_comp_delimited): prepend prefix to all compreply args #913

Rogach · 2023-03-30T10:20:11Z

Fixes #552.

Previously prefix was only prepended if COMPREPLY only contained one argument - this commit fixes it so prefix is prepended to all arguments.

The problem existed since 4138714, in the very first version of _comp_delimited function. I'm not sure why the original only prepended the prefix back when there was only one completion, so I'm worried I'm missing something.

akinomyoga · 2023-03-30T11:30:02Z

Previously prefix was only prepended if COMPREPLY only contained one argument - this commit fixes it so prefix is prepended to all arguments.

Thank you for tracking down the problem.

I'm not sure why the original only prepended the prefix back when there was only one completion, so I'm worried I'm missing something.

I guess the original intent is to compactify the candidate list shown when TAB is pressed twice.

$ complete -F _echo echo
$ _echo() { local cur prev words cword split comp_args; _comp_initialize -s -- "$@" && _comp_delimited , -W 'hello{1..3}'; }
$ echo x,y,z,he[TAB][TAB]
hello1  hello2  hello3
$ echo x,y,z,he

With this PR, it becomes longer as follows. This gets even longer when we already input many names separated by commas.

x,y,z,hello1  x,y,z,hello2  x,y,z,hello3
$ echo x,y,z,hello

Now I have tested the behavior with Bash. Bash somehow doesn't do anything when bind 'set show-all-if-ambiguous on' is set and the length of the current word is longer than the longest common prefix of the ambiguous candidates. For example,

$ bind 'set show-all-if-unmodified on'
$ echo x,y,he[TAB]
hello1  hello2  hello3
$ echo x,y,he         # The original word is preserved
$ echo x,y,h[TAB]
hello1  hello2  hello3
$ echo hello          # The original word is lost

So _comp_delimited was relying on this strange behavior of Bash, and it actually wasn't ensured to work even when bind 'set show-all-if-ambiguous on' is set.

In the past, there was also a problem (reported twice in #544 and #858) that the make completion tries to be "clever" in showing a compact candidate list but unintendedly removes the existing word under some conditions. We finally gave up the fancy treatment in #546.

I conclude that it's useless to try to adjust the candidate list by a kind of hack. As far as Bash doesn't provide an official way to control the candidate list, it is hard to achieve it in a robust way. So I would vote for the change in this PR.

bash_completion

Rogach · 2023-03-30T11:47:40Z

Updated with suggested changes.

Regarding "&" - not sure if it's interesting, but for me tab-completion just stops working if I add "&" as part of previous delimited argument:

$ ssh-keyscan -t dsa,e<TAB> # nothing happens

akinomyoga

Can you add tests in the subdirectory test/t/unit with the filename test_unit_delimited.py? You can reference other files such as test_unit_ip_addresses.py for how to make tests. For the implementation of a mock completion function, you may look at _echo in #913 (comment). To run the test, you can use the following command:

$ pytest test/t/unit/test_unit_delimited.py

The command name pytest might be pytest-3 depending on the installation.

edit: If you want a detailed output, you can use

$ pytest -v test/t/unit/test_unit_delimited.py

akinomyoga · 2023-03-30T12:03:45Z

Regarding "&" - not sure if it's interesting, but for me tab-completion just stops working if I add "&" as part of previous delimited argument:

Where did you add &? & may be specified in the command line, such as

$ complete -F _echo echo
$ _echo() { local cur prev words cword split comp_args; _comp_initialize -s -- "$@" && _comp_delimited , -W 'hello{1..3}'; }
$ echo "&",h[TAB]
$ echo "",hello    <-- when $prefix is not quoted
$ echo "&",hello   <-- when $prefix is quoted as "$prefix"

$ ssh-keyscan -t dsa,e<TAB> # nothing happens

Isn't this because the longest common prefix of ecdsa and ed25519 is just e?

Rogach · 2023-03-30T12:41:02Z

Added tests.

Where did you add &? & may be specified in the command line, such as

It's late here, I'm getting tired :)

I forgot to quote the & - I had a setup like this:

$ ssh-keyscan ds&,e<TAB><TAB>

and obviously nothing was happening, because I was tring to complete ,e, everything before was cut off.

However if I quote the "&" then I see no difference between quoting the prefix and not quoting it:

$ ssh-keyscan -t "ds&",e<TAB><TAB>
"ds&",ecdsa    "ds&",ed25519

There is the same result, even with COMPREPLY=("${COMPREPLY[@]/#/$prefix}").

akinomyoga

Thank you! Looks perfect!

Rogach · 2023-03-30T12:44:30Z

I finally understood what was my problem with "&" - you were talking about bash 5.2 and pathsub_replacement, while I have older bash (5.1) without that option.

akinomyoga · 2023-03-30T12:50:10Z

It's late here, I'm getting tired :)

Thank you for your hard work!

$ ssh-keyscan -t "ds&",e<TAB><TAB>
"ds&",ecdsa    "ds&",ed25519

As far as I test, the above behavior seems to be different depending on the quoting of $prefix.

I finally understood what was my problem with "&" - you were talking about bash 5.2 and pathsub_replacement, while I have older bash (5.1) without that option.

Ah, I see. Sorry for confusing you.

akinomyoga · 2023-03-30T12:55:07Z

CI pre-commit has an error. black suggests the following change:

diff --git a/test/t/unit/test_unit_delimited.py b/test/t/unit/test_unit_delimited.py
index 5eb74bdb..249dc1b6 100644
--- a/test/t/unit/test_unit_delimited.py
+++ b/test/t/unit/test_unit_delimited.py
@@ -2,6 +2,7 @@ import pytest

 from conftest import assert_bash_exec

+
 @pytest.mark.bashcomp(cmd=None)
 class TestUnitDelimited:
     @pytest.fixture(scope="class")

Rogach · 2023-03-30T13:26:41Z

Tests are failing, I'll look into it tomorrow.

Rogach · 2023-03-30T13:34:45Z

Failing tests weren't too difficult, these are all resulting from changes in completion output. However for ssh-keygen and pgrep tests I'm not sure how to actually fix the tests - currently I just removed the outdated asserions, leaving only assert completion.

Rogach · 2023-04-01T10:54:26Z

Two tests are still failing, but I don't understand why. They are failing only in one distribution (ubuntu14), one timeout (in seemingly innocious test), and one environment modification (where there shouldn't be any).

akinomyoga · 2023-04-01T19:02:50Z

The second error of the environment modification is probably caused by the first error. There are also many errors in centos7 before it is canceled.

../../../test/t/test_fio.py::TestFio::test_3 FAILED                      [ 19%]
../../../test/t/test_fio.py::TestFio::test_cmdhelp_all PASSED            [ 19%]
../../../test/t/test_fio.py::TestFio::test_enghelp PASSED                [ 19%]
../../../test/t/test_fio.py::TestFio::test_cmdhelp_boolean PASSED        [ 19%]
../../../test/t/test_fio.py::TestFio::test_cmdhelp_valid_values PASSED   [ 19%]
../../../test/t/test_fio.py::TestFio::test_cmdhelp_nonexistent PASSED    [ 19%]
../../../test/t/test_fio.py::TestFio::test_crctest PASSED                [ 19%]
../../../test/t/test_fio.py::TestFio::test_debug FAILED                  [ 19%]

...

../../../test/t/test_man.py::TestMan::test_delimited_first FAILED        [ 39%]
../../../test/t/test_man.py::TestMan::test_delimited_after_delimiter PASSED [ 39%]
../../../test/t/test_man.py::TestMan::test_delimited_later FAILED        [ 39%]
../../../test/t/test_man.py::TestMan::test_delimited_deduplication ERROR [ 39%]
../../../test/t/test_man.py::TestMan::test_zstd_arbitrary_sectsuffix PASSED [ 39%]
../../../test/t/test_man.py::TestMan::test_zstd_arbitrary_sectsuffix ERROR [ 39%]

...

../../../test/t/test_ps.py::TestPs::test_7 FAILED                        [ 52%]

...

../../../test/t/test_ss.py::TestSs::test_3 FAILED                        [ 63%]

...

../../../test/t/test_ssh_keygen.py::TestSshKeygen::test_usernames_for_n FAILED [ 64%]
../../../test/t/test_ssh_keygen.py::TestSshKeygen::test_host_for_h_n FAILED [ 64%]

...

../../../test/t/test_tox.py::TestTox::test_2 FAILED                      [ 69%]
../../../test/t/test_tox.py::TestTox::test_3 FAILED                      [ 69%]

...

../../../test/t/test_tshark.py::TestTshark::test_3 FAILED                [ 70%]

I reran the CI test twice, but the same error happened in both reruns. I also ran the tests locally, and the error was reproduced with bash 4.2. The following is the error message:

FAILED test/t/test_man.py::TestMan::test_delimited_first - assert not <CompletionResult ['\x08""1']>
FAILED test/t/test_man.py::TestMan::test_delimited_later - assert not <CompletionResult ['\x08"1:"2']>
ERROR test/t/test_man.py::TestMan::test_delimited_deduplication - AssertionError: Unexpected output: [bash: COMPREPLY[@]: unbound variable
ERROR test/t/test_man.py::TestMan::test_zstd_arbitrary_sectsuffix - AssertionError: Environment should not be modified

The third line must be the cause of the error. Ah, OK. In Bash 4.2, ${arr[@]} fails when set -u is set and there are no elements in the array even if the array exists. You need to check if it has any elements, such as

((${#COMPREPLY[@]})) && COMPREPLY=("${COMPREPLY[@]/#/"$prefix"}")

bash_completion

akinomyoga · 2023-04-02T13:11:02Z

Can you add test_unit_delimited.py in test/t/unit/Makefile.am?

Rogach · 2023-04-03T09:33:54Z

Done.

Rogach · 2023-04-03T11:21:15Z

Something broke on centos7, but I don't know how to check it :(

akinomyoga · 2023-04-08T14:23:46Z

I realized that the double quotations "$prefix" inside "${var/#/...}" is not processed in Bash 4.2 (while those in unquoted ${var/#/...} are processed). I added a fix d84f1ec. We finally go back to something similar to your first version (using the for loop to process each element).

Rogach · 2023-04-09T11:49:02Z

CI seems to pass fine now. Thank you!

Fixes scop#552. Previously prefix was only prepended if COMPREPLY only contained one argument - this commit fixes it so prefix is prepended to all arguments.

scop · 2023-04-21T20:54:14Z

bash_completion

+    local i
+    for i in "${!COMPREPLY[@]}"; do
+        COMPREPLY[i]="$prefix${COMPREPLY[i]}"
+    done


While this certainly seems to work, it's also somewhat unfortunate, because displaying the completions with the prefix in place makes them harder to read, and with show-all-if-ambiguous on it appears we wouldn't have to do it (I'm biased, I have it on). So I'm contemplating something like:

Suggested change

local i

for i in "${!COMPREPLY[@]}"; do

COMPREPLY[i]="$prefix${COMPREPLY[i]}"

done

# If `show-all-if-ambiguous` is off, existing completions preceding the

# comma and the comma itself are lost if there are multiple completions

# offered for the thing after the prefix. This does not seem to happen

# with it on, so avoid repeating the prefix in that case for readability.

# https://github.com/scop/bash-completion/pull/532#issuecomment-873675676

if _comp_readline_variable_on show-all-if-ambiguous; then

((${#COMPREPLY[@]} == 1)) && COMPREPLY=("$prefix$COMPREPLY")

else

local i

for i in "${!COMPREPLY[@]}"; do

COMPREPLY[i]="$prefix${COMPREPLY[i]}"

done

fi

I suppose this would require some addressing/complexity in tests, too.

Thoughts?

As we have discussed in the past in #546, the current test framework totally relies on set show-all-if-ambiguous on, so I'm afraid there is no way to test the behavior for the case set show-all-if-ambiguous off.

@akinomyoga showed that ((${#COMPREPLY[@]} == 1)) && COMPREPLY=("$prefix$COMPREPLY") isn't guaranteed to work even if show-all-if-unmodified is on (#913 (comment)). Copying the snippet here:

$ bind 'set show-all-if-unmodified on' $ echo x,y,he[TAB] hello1 hello2 hello3 $ echo x,y,he # The original word is preserved $ echo x,y,h[TAB] hello1 hello2 hello3 $ echo hello # The original word is lost

I'm not an expert here, but it feels very brittle to be relying on some obscure undocumented interaction between options, that might break easily down the road (as showcased above).

Ah, yes. I was also about to write it. I tried the change suggested by @scop with set show-all-if-ambiguous and confirmed that it still fails. Also, I was experimenting to see if it is possible to detect the insertion by looking at COMP_TYPE, but there doesn't seem to be a reliable way.

(Experimental code)

Suggested change

local i

for i in "${!COMPREPLY[@]}"; do

COMPREPLY[i]="$prefix${COMPREPLY[i]}"

done

if _comp_has_insertion; then

local i

for i in "${!COMPREPLY[@]}"; do

COMPREPLY[i]="$prefix${COMPREPLY[i]}"

done

fi

with

# usage: _comp__get_common_prefix words... # @var[out] ret _comp__get_common_prefix() { ret=${1-} shift local v l u m for v; do l=0 u=${#ret}+1 while ((l + 1 < u)); do ((m = (l + u) / 2)) if [[ $v == "${ret::m}"* ]]; then l=$m else u=$m fi done ret=${ret::l} done } # Predict whether any string will be possibly inserted in the current command # line supposed the current COMPREPLY is passed to Bash as is. # @var[in] COMPREPLY # @exit 0 if any string is expected to be inserted, or otherwise 1 _comp_has_insertion() { local ret if ((COMP_TYPE == 9)); then # TAB: When TAB is pressed (without show-all-if-ambiguous), an # insertion happens when there is a non-empty common prefix. _comp__get_common_prefix "${COMPREPLY[@]}" [[ $ret ]] elif ((COMP_TYPE == 33)); then # TAB w/ show-all-if-ambiguous: When TAB is pressed (with # show-all-if-ambiguous), an insertion happens when there is a # non-empty unique completion or the common prefix is at least as long # as the current word. _comp__get_common_prefix "${COMPREPLY[@]}" ((${#COMPREPLY[@]} == 1)) && [[ $ret ]] || ((${#ret} >= ${#COMP_WORDS[COMP_CWORD]})) elif ((COMP_TYPE == 37 || COMP_TYPE == 42)); then # menu-complete or menu-complete-backward: An insertion always happens # as far as there is at least one completion. ((${#COMPREPLY[@]} > 0)) else false fi }

Ok. Then let's just go with the implementation here that is known to work in all cases, you guys have spent more than enough time here already. Thanks!

Added a note in e3840d0 as a reminder to take stuff into account if someone is tempted to revisit this later.

akinomyoga reviewed Mar 30, 2023

View reviewed changes

bash_completion Outdated Show resolved Hide resolved

Rogach force-pushed the pr/fix-comp-delimited-on-ambiguous-args branch from de9cd55 to e2b58f6 Compare March 30, 2023 11:46

akinomyoga requested changes Mar 30, 2023

View reviewed changes

Rogach force-pushed the pr/fix-comp-delimited-on-ambiguous-args branch from e2b58f6 to 7d799ed Compare March 30, 2023 12:34

akinomyoga approved these changes Mar 30, 2023

View reviewed changes

Rogach force-pushed the pr/fix-comp-delimited-on-ambiguous-args branch from 7d799ed to 9314a42 Compare March 30, 2023 12:59

akinomyoga approved these changes Mar 30, 2023

View reviewed changes

Rogach force-pushed the pr/fix-comp-delimited-on-ambiguous-args branch from 9314a42 to 75e8913 Compare March 30, 2023 13:33

akinomyoga requested changes Apr 1, 2023

View reviewed changes

bash_completion Outdated Show resolved Hide resolved

Rogach force-pushed the pr/fix-comp-delimited-on-ambiguous-args branch 3 times, most recently from 81cfc71 to 6c8eb52 Compare April 3, 2023 09:33

akinomyoga mentioned this pull request Apr 8, 2023

CI debugging for scop#913 akinomyoga/bash-completion#12

Closed

fix(_comp_delimited): prepend prefix to all compreply args

0eea357

Fixes scop#552. Previously prefix was only prepended if COMPREPLY only contained one argument - this commit fixes it so prefix is prepended to all arguments.

fix(_comp_delimited): work around bash-4.2 quoting

e923e7f

scop force-pushed the pr/fix-comp-delimited-on-ambiguous-args branch from d84f1ec to e923e7f Compare April 21, 2023 20:34

scop reviewed Apr 21, 2023

View reviewed changes

docs(_comp_delimited): elaborate on prefix addition considerations

e3840d0

scop approved these changes Apr 22, 2023

View reviewed changes

scop merged commit 9b1cb54 into scop:master Apr 22, 2023

Rogach deleted the pr/fix-comp-delimited-on-ambiguous-args branch May 10, 2023 09:06

akinomyoga mentioned this pull request Aug 25, 2023

Support for setfacl #639

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(_comp_delimited): prepend prefix to all compreply args #913

fix(_comp_delimited): prepend prefix to all compreply args #913

Rogach commented Mar 30, 2023

akinomyoga commented Mar 30, 2023

Rogach commented Mar 30, 2023

akinomyoga left a comment •

edited

Loading

akinomyoga commented Mar 30, 2023

Rogach commented Mar 30, 2023 •

edited

Loading

akinomyoga left a comment

Rogach commented Mar 30, 2023

akinomyoga commented Mar 30, 2023

akinomyoga commented Mar 30, 2023

Rogach commented Mar 30, 2023

Rogach commented Mar 30, 2023

Rogach commented Apr 1, 2023 •

edited

Loading

akinomyoga commented Apr 1, 2023 •

edited

Loading

akinomyoga commented Apr 2, 2023

Rogach commented Apr 3, 2023

Rogach commented Apr 3, 2023

akinomyoga commented Apr 8, 2023

Rogach commented Apr 9, 2023

scop Apr 21, 2023

akinomyoga Apr 22, 2023

Rogach Apr 22, 2023

akinomyoga Apr 22, 2023 •

edited

Loading

scop Apr 22, 2023

scop Apr 22, 2023

-    local i
-    for i in "${!COMPREPLY[@]}"; do
-        COMPREPLY[i]="$prefix${COMPREPLY[i]}"
-    done
+    # If `show-all-if-ambiguous` is off, existing completions preceding the
+    # comma and the comma itself are lost if there are multiple completions
+    # offered for the thing after the prefix. This does not seem to happen
+    # with it on, so avoid repeating the prefix in that case for readability.
+    # https://github.com/scop/bash-completion/pull/532#issuecomment-873675676
+    if _comp_readline_variable_on show-all-if-ambiguous; then
+        ((${#COMPREPLY[@]} == 1)) && COMPREPLY=("$prefix$COMPREPLY")
+    else
+        local i
+        for i in "${!COMPREPLY[@]}"; do
+            COMPREPLY[i]="$prefix${COMPREPLY[i]}"
+        done
+    fi

fix(_comp_delimited): prepend prefix to all compreply args #913

fix(_comp_delimited): prepend prefix to all compreply args #913

Conversation

Rogach commented Mar 30, 2023

akinomyoga commented Mar 30, 2023

Rogach commented Mar 30, 2023

akinomyoga left a comment • edited Loading

Choose a reason for hiding this comment

akinomyoga commented Mar 30, 2023

Rogach commented Mar 30, 2023 • edited Loading

akinomyoga left a comment

Choose a reason for hiding this comment

Rogach commented Mar 30, 2023

akinomyoga commented Mar 30, 2023

akinomyoga commented Mar 30, 2023

Rogach commented Mar 30, 2023

Rogach commented Mar 30, 2023

Rogach commented Apr 1, 2023 • edited Loading

akinomyoga commented Apr 1, 2023 • edited Loading

akinomyoga commented Apr 2, 2023

Rogach commented Apr 3, 2023

Rogach commented Apr 3, 2023

akinomyoga commented Apr 8, 2023

Rogach commented Apr 9, 2023

scop Apr 21, 2023

Choose a reason for hiding this comment

akinomyoga Apr 22, 2023

Choose a reason for hiding this comment

Rogach Apr 22, 2023

Choose a reason for hiding this comment

akinomyoga Apr 22, 2023 • edited Loading

Choose a reason for hiding this comment

scop Apr 22, 2023

Choose a reason for hiding this comment

scop Apr 22, 2023

Choose a reason for hiding this comment

akinomyoga left a comment •

edited

Loading

Rogach commented Mar 30, 2023 •

edited

Loading

Rogach commented Apr 1, 2023 •

edited

Loading

akinomyoga commented Apr 1, 2023 •

edited

Loading

akinomyoga Apr 22, 2023 •

edited

Loading