Skip to content

fix: load from more new style completion dirs #1569

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

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

fix: load from more new style completion dirs #1569

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions bash_completion
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3478,15 +3478,15 @@ _comp_load()
if [[ ${BASH_COMPLETION_USER_DIR-} ]]; then
_comp_split -F : dirs "$BASH_COMPLETION_USER_DIR"
Copy link
Collaborator

@akinomyoga akinomyoga Feb 5, 2026

Choose a reason for hiding this comment

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

BASH_COMPLETION_USER_DIR is supposed to contain a path like ~/.local/share/bash-completion, so we need to suffix /completion to them too. I think we can use _comp_compgen. For example, something like this: _comp_compgen -Rv dirs -F : -- -W '$BASH_COMPLETION_USER_DIR' -S /completions, though I haven't tested this carefully.

Copy link
Owner Author

@scop scop Feb 9, 2026

Choose a reason for hiding this comment

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

I haven't thought of this too deeply, but I think we do not need to do that. This dir is a single directory to load completions from, solely under the user's control, I don't think we need to add any more structure or segments to it. Adding would also be a breaking change.

Copy link
Owner Author

@scop scop Feb 9, 2026

Choose a reason for hiding this comment

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

Hm README.md seems to disagree with me, I misremembered. $BASH_COMPLETION_USER_DIR is by the way not documented in doc/configuration.md, only its $XDG_CONFIG_HOME/bash_completion fallback is. We should fix that.

else
dirs=("${XDG_DATA_HOME:-$HOME/.local/share}/bash-completion")
dirs=("${XDG_DATA_HOME:-$HOME/.local/share}/bash-completion/completions")
fi

# 2) From the location of bash_completion: Completions relative to the main
# script. This is primarily for run-in-place-from-git-clone setups, where
# we want to prefer in-tree completions over ones possibly coming with a
# system installed bash-completion. (Due to usual install layouts, this
# often hits the correct completions in system installations, too.)
dirs+=("$_comp__base_directory")
dirs+=("$_comp__base_directory"/completions{,-core,-fallback})

# 3) From bin directories extracted from the specified path to the command,
# the real path to the command, and $PATH
Expand All @@ -3496,28 +3496,24 @@ _comp_load()
_comp_split -aF : paths "$PATH"
for dir in "${paths[@]%/}"; do
[[ $dir == ?*/@(bin|sbin) ]] &&
dirs+=("${dir%/*}/share/bash-completion")
dirs+=("${dir%/*}/share/bash-completion"/completions{,-core,-fallback})
Copy link
Collaborator

@akinomyoga akinomyoga Feb 5, 2026

Choose a reason for hiding this comment

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

For the same reason, I think we don't want to load completions-{core,fallback} in the bin directories.

Also, as I mentioned above, I would like to suggest exchanging the ordering of 2) and 3).

done

# 4) From XDG_DATA_DIRS or system dirs (e.g. /usr/share, /usr/local/share):
# Completions in the system data dirs.
_comp_split -F : paths "${XDG_DATA_DIRS:-/usr/local/share:/usr/share}" &&
dirs+=("${paths[@]/%//bash-completion}")
dirs+=("${paths[@]/%//bash-completion}"/completions{,-core,-fallback})
Copy link
Collaborator

@akinomyoga akinomyoga Feb 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Should we search the system completions-{core,fallback} after none were found in the in-tree completions-{core,fallback} (i.e., $_comp__base_directory/completions-{core,fallback} in the tree of the current bash_completion)? I think all default/fallback completions should already be found in the in-tree completions-{core,fallback}. Are there cases where the completion file is not found in the in-tree completions-{core,fallback} but found in the system completions-{core,fallback}?

I also think loading external completions-{core,fallback} would cause the compatibility problem with the internal API. We've been assuming that we can use the latest internal APIs defined in bash_completion inside the in-tree completions of the same version. When the in-tree completions-{core,fallback} doesn't contain the completion for a command while the system completions-{core,fallback} contains one, I think that implies the system bash-completion is newer than the current bash-completion. In this case, we should avoid loading the system completions-{core,fallback} because it is likely to contain uses of new internal APIs.

In short, I think the completions in completions-{core,fallback} are tightly bound, in the sense of internal API, to the exact version of bash_completion in the same tree. It's not safe to load completions-{core,fallback} outside the tree.

Copy link
Owner Author

@scop scop Feb 9, 2026

Choose a reason for hiding this comment

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

Yep, agreed.


# Look up and source
shift
local -a source_args=("$@")

local i
for i in "${!dirs[@]}"; do
dir=${dirs[i]}/completions
for dir in "${dirs[@]}"; do
Copy link
Owner Author

@scop scop Feb 5, 2026

Choose a reason for hiding this comment

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

The result of this is that we end up loading completions without the .bash suffix from more dirs we did before this change. We may want to tighten that up some.

Could check if the dir ends with -core or -fallback and load only .bash for them. That isn't 100% correct in the case if someone has for whatever reason configured their BASH_COMPLETION_USER_DIR to have one of those suffixes (we'd want to load both with and without .bash from there for backwards compatibility).

Copy link
Collaborator

@akinomyoga akinomyoga Feb 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I agree to check the suffix -{core,fallback} (probably inside _comp_load__visit_file).

[[ -d $dir ]] || continue
for compfile in "$cmdname.bash" "$cmdname"; do
_comp_load__visit_file "$dir/$compfile" && return 0
done
done
_comp_load__visit_file "$_comp__base_directory/completions-core/$cmdname.bash" && return 0
_comp_load__visit_file "$_comp__base_directory/completions-fallback/$cmdname.bash" && return 0

# Look up simple "xspec" completions
[[ -v _comp_xspecs[$cmdname] || -v _xspecs[$cmdname] ]] &&
Expand Down
Loading