[PATCH 1/1] bash completion without relying on /etc/bash-completion.d

[Jason A. Donenfeld]

Thanks for this. Sorry for the delay. Some nits before it's mergeable:


On Sun, Oct 7, 2018 at 10:34 PM Lars Flitter
<password-store at larsflitter.de> wrote:
>  install-common:
>         @install -v -d "$(DESTDIR)$(MANDIR)/man1" && install -m 0644 -v man/pass.1 "$(DESTDIR)$(MANDIR)/man1/pass.1"
> -       @[ "$(WITH_BASHCOMP)" = "yes" ] || exit 0; install -v -d "$(DESTDIR)$(BASHCOMPDIR)" && install -m 0644 -v src/completion/pass.bash-completion "$(DESTDIR)$(BASHCOMPDIR)/pass"
> +       @[ "$(WITH_BASHCOMP)" = "yes" ] || exit 0;  \
> +               install -v -d "$(DESTDIR)$(BASHCOMPDIR)" && \
> +               trap 'rm -f src/completion/.bash' EXIT; \
> +               sed 's:^SYSTEM_EXTENSION_DIR=.*:SYSTEM_EXTENSION_DIR="$(LIBDIR)/password-store/extensions":' src/completion/pass.bash-completion > src/completion/.bash && \
> +               install -m 0644 -v src/completion/.bash "$(DESTDIR)$(BASHCOMPDIR)/pass"

Either clean up the other usage of this pattern to be multi-line in
preceding commit, or make this single line like the other.

> +_pass_extension_commands() {
> +       if [[ -n $SYSTEM_EXTENSION_DIR ]]; then
> +               find $SYSTEM_EXTENSION_DIR -name \*.bash  -printf "%f " | sed 's/.bash//g'
> +       fi
> +
> +       if [[ $PASSWORD_STORE_ENABLE_EXTENSIONS == true ]]; then
> +               find $EXTENSIONS -name \*.bash  -printf "%f " | sed 's/.bash//g'
> +       fi
> +}

These should use pure bash instead of find+sed. If you're concerned
about nested directories, you can enable extglob for the duration of
the function and the ** operator. Suffixes can be removed using the
normal bash % operator on the results. Also -d should be used instead
of -n, above.

> +
> +_pass_extension_completion_file() {
> +       local extension_command=$1
> +       if [[ $PASSWORD_STORE_ENABLE_EXTENSIONS == true ]] && [ -f $EXTENSIONS/$extension_command.bash.completion ]; then

Please always use [[ and not [. You can combine these two clauses into
a single one.

Also - elsewhere we use bash-completion, not bash.completion.

> +               echo $EXTENSIONS/$extension_command.bash.completion

Missing quotation marks.

> +               return 0
> +       fi
> +
> +       if [[ -n $SYSTEM_EXTENSION_DIR ]] && [ -f $SYSTEM_EXTENSION_DIR/$extension_command.bash.completion ]; then
> +               echo $SYSTEM_EXTENSION_DIR/$extension_command.bash.completion

Same feedback as above. However, you probably should just be copying
the logic in pass' cmd_extension instead, where you just build the
full path, test which one exists, and source things opportunistically.
Copy and paste it, even.

> -       local commands="init ls find grep show insert generate edit rm mv cp git help version ${PASSWORD_STORE_EXTENSION_COMMANDS[*]}"
> +       local commands="init ls find grep show insert generate edit rm mv cp git help version $(_pass_extension_commands)"

Not sure this is such a good idea. I get that sometimes you want to
list the commands, it seems there are actually three cases:

- User hits tab in the state when listing all commands --> call
_pass_extension_commands to see what should be appended
- User hits tab in the state of a built-in command --> nothing.
- User hits tab in the state of an unknown command --> see if there
exists a bash completion for the extension that should be sourced for
that command.

Right now you're doing the first all the time, which I'm not sure is
so smart performance-wise.

> +               # To add completion for an extension command create <COMMAND>.bash.completion in the extension folder that contains the completion code like this:

Same nit about filename template.

> +               #
> +               # COMPREPLY+=($(compgen -W "-o --option" -- ${cur}))
> +               # _pass_complete_entries 1
>                 #
> -               # and add the command to the $PASSWORD_STORE_EXTENSION_COMMANDS array
> -               if [[ " ${PASSWORD_STORE_EXTENSION_COMMANDS[*]} " == *" ${COMP_WORDS[1]} "* ]] && type "__password_store_extension_complete_${COMP_WORDS[1]}" &> /dev/null; then
> -                       "__password_store_extension_complete_${COMP_WORDS[1]}"
> +               if [[ " $(_pass_extension_commands)" == *" ${COMP_WORDS[1]} "* ]] && [ -n "$(_pass_extension_completion_file ${COMP_WORDS[1]})" ] ; then

Same nit as above.

> +                       source $(_pass_extension_completion_file ${COMP_WORDS[1]})
>                 fi
>         else
>                 COMPREPLY+=($(compgen -W "${commands}" -- ${cur}))
> --
> 2.19.1
>
> _______________________________________________
> Password-Store mailing list
> Password-Store at lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/password-store