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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Apr 22, 2023
+57 −6
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0eea357
fix(_comp_delimited): prepend prefix to all compreply args
Rogach Mar 30, 2023
e923e7f
fix(_comp_delimited): work around bash-4.2 quoting
akinomyoga Apr 8, 2023
e3840d0
docs(_comp_delimited): elaborate on prefix addition considerations
scop Apr 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion bash_completion
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,7 +895,11 @@ _comp_delimited()
_comp_compgen COMPREPLY "$@" -- "${cur##*"$delimiter"}"
fi

((${#COMPREPLY[@]} == 1)) && COMPREPLY=("$prefix$COMPREPLY")
local i
for i in "${!COMPREPLY[@]}"; do
COMPREPLY[i]="$prefix${COMPREPLY[i]}"
done
Comment on lines +905 to +908
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Collaborator

@akinomyoga akinomyoga Apr 22, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


[[ $delimiter != : ]] || __ltrim_colon_completions "$cur"
}

Expand Down
1 change: 0 additions & 1 deletion test/t/test_pgrep.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,3 @@ def test_nslist(self, completion):
)
def test_nslist_after_comma(self, completion):
assert completion
assert not any("," in x for x in completion)
2 changes: 0 additions & 2 deletions test/t/test_ssh_keygen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,11 @@ def test_filedir_pub_at_end_of_s(self, completion):
@pytest.mark.complete("ssh-keygen -s foo_key -n foo,")
def test_usernames_for_n(self, completion):
assert completion
assert not any("," in x for x in completion)
# TODO check that these are usernames

@pytest.mark.complete("ssh-keygen -s foo_key -h -n foo,")
def test_host_for_h_n(self, completion):
assert completion
assert not any("," in x for x in completion)
# TODO check that these are hostnames

@pytest.mark.complete("ssh-keygen -Y foo -n ")
Expand Down
2 changes: 1 addition & 1 deletion test/t/test_tox.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ def test_2(self, completion):

@pytest.mark.complete("tox -e foo,", cwd="tox")
def test_3(self, completion):
assert all(x in completion for x in "py37 ALL".split())
assert all("foo," + x in completion for x in "py37 ALL".split())

@pytest.mark.complete("tox -e foo -- ", cwd="tox")
def test_default_after_dashdash(self, completion):
Expand Down
2 changes: 1 addition & 1 deletion test/t/test_tshark.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def test_2(self, completion):
@pytest.mark.complete("tshark -O foo,htt", require_cmd=True)
def test_3(self, completion):
# p: one completion only; http: e.g. http and http2
assert completion == "p" or "http" in completion
assert completion == "p" or "foo,http" in completion

@pytest.mark.complete("tshark -o tcp", require_cmd=True)
def test_4(self, completion):
Expand Down
1 change: 1 addition & 0 deletions test/t/unit/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ EXTRA_DIST = \
test_unit_command_offset.py \
test_unit_compgen.py \
test_unit_count_args.py \
test_unit_delimited.py \
test_unit_deprecate_func.py \
test_unit_dequote.py \
test_unit_expand.py \
Expand Down
42 changes: 42 additions & 0 deletions test/t/unit/test_unit_delimited.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import pytest

from conftest import assert_bash_exec


@pytest.mark.bashcomp(cmd=None)
class TestUnitDelimited:
@pytest.fixture(scope="class")
def functions(self, request, bash):
assert_bash_exec(
bash,
"_comp_cmd_test_delim() {"
" local cur prev words cword comp_args;"
" _comp_get_words cur;"
" _comp_delimited , -W 'alpha beta bravo';"
"};"
"complete -F _comp_cmd_test_delim test_delim",
)

@pytest.mark.complete("test_delim --opt=a")
def test_1(self, functions, completion):
assert completion == ["lpha"]

@pytest.mark.complete("test_delim --opt=b")
def test_2(self, functions, completion):
assert completion == ["beta", "bravo"]

@pytest.mark.complete("test_delim --opt=alpha,b")
def test_3(self, functions, completion):
assert completion == ["alpha,beta", "alpha,bravo"]

@pytest.mark.complete("test_delim --opt=alpha,be")
def test_4(self, functions, completion):
assert completion == ["ta"]

@pytest.mark.complete("test_delim --opt=beta,a")
def test_5(self, functions, completion):
assert completion == ["lpha"]

@pytest.mark.complete("test_delim --opt=c")
def test_6(self, functions, completion):
assert not completion