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

feat(_comp_compgen_filedir,_comp_compgen_filedir_xspec): don’t suggest . and .. #1230

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Maelan
Copy link
Contributor

@Maelan Maelan commented Jul 2, 2024

This is a resurrection of #364, adapting to the current codebase, and addressing (I think) objections made there.

With this patch, file-and-directory completion provided by _comp_compgen_filedir and _comp_compgen_filedir_xspec do not suggest . nor .. nor */. nor */.. unless the user input is .. or */.. .

The condition can easily be changed from “unless the user has typed ..” to “unless there is only one suggestion” (just replace if [[ "${cur}" != @(..|*/..) ]] by if (( ${#toks[@]} > 1 )) ).

The motivation is to be able to tab-complete hidden files with more comfort: as hidden files are relatively rare, it is not uncommon to have no more than one hidden file in a given directory (or, in the case of a git repo, hidden files with a common prefix: .git, .gitignore…). It should then be enough to type just the dot and get the hidden file completed. Especially considering that typing a letter after a dot is uncomfortable, depending on your keyboard.

But . and .. are always there and get in the way of completion, despite being uninteresting suggestions: . alone can never get a slash appended, because it is always a prefix of the suggested .. ; and .. is strictly easier to type by pressing the dot key twice than by pressing the dot key, then whatever key is used to trigger completion.

There is one use case for suggesting .. though: in the previous PR, it was mentioned that some users may want to rely on completion to append a slash after they have typed .. ; so we must still suggest .. in some cases, which is what the “unless” condition addresses.

Another use case mentioned in the previous PR is that some users might(?) want to type . and expect bash-completion to complete it to .. ; this use case does not work with this patch, but it does not work without, either, because . itself is a valid suggestion too… So we are not degrading user experience in any way. We might support this use case with a more complex logic (always prune . ; prune .. unless [condition]).

If, barring all this, the feature is judged too disruptive a change, perhaps the new behavior could be controlled by a user option?

Implementation note: I’d have preferred to use the builtin filtering feature of compgen/complete (-X '@(.|..|*/.|*/..)') but it does not combine with filtering a custom xspec (we cannot use -X twice; there is a comment about that in _comp_delimited).

Regards,

Copy link
Collaborator

@akinomyoga akinomyoga left a comment

Choose a reason for hiding this comment

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

Implementation note: I’d have preferred to use the builtin filtering feature of compgen/complete (-X '@(.|..|*/.|*/..)') but it does not combine with filtering a custom xspec (we cannot use -X twice; there is a comment about that in _comp_delimited).

Technically one could do that by -X '!(!(pat1)|!(pat2))', but it is practically extremely slow due to Bash's implementation of extglob. (edit: Hmm, in this case, I think you want to remove candidates matching either pat1 or pat2, so you can simply do -X '@(pat1|pat2)'.) You can instead call compgen again for additional filtering by -X.

Comment on lines +1155 to +1159
local i
for i in "${!toks[@]}" ; do
[[ "${toks[$i]}" == @(.|..|*/.|*/..) ]] && \
unset -v "toks[$i]"
done
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
local i
for i in "${!toks[@]}" ; do
[[ "${toks[$i]}" == @(.|..|*/.|*/..) ]] && \
unset -v "toks[$i]"
done
if ((${#toks[@]})); then
_comp_compgen -Rv toks -- -X '@(.|..|*/.|*/..)' -W '"${toks[@]}"'
fi

In addition, I think we can return from the function when ((${#toks[@]} == 0)) before the above compopt -o filenames 2>/dev/null (line 1148). Then, the check for ((${#toks[@]})) can be skipped.

Copy link
Collaborator

@akinomyoga akinomyoga Jul 2, 2024

Choose a reason for hiding this comment

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

This filtering should probably be done before setting compopt -o filenames since the number of completions can be reduced to 0 by this filtering.

Comment on lines +3060 to +3064
local i
for i in "${!toks[@]}" ; do
[[ "${toks[$i]}" == @(.|..|*/.|*/..) ]] && \
unset -v "toks[$i]"
done
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
local i
for i in "${!toks[@]}" ; do
[[ "${toks[$i]}" == @(.|..|*/.|*/..) ]] && \
unset -v "toks[$i]"
done
_comp_compgen -Rv toks -- -X '@(.|..|*/.|*/..)' -W '"${toks[@]}"'

@akinomyoga
Copy link
Collaborator

There is a test failure in CI.

Also, can you add tests for the new behavior (in test/t/unit/test_unit_compgen_filedir.py)?

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.

None yet

2 participants