Skip to content

bash-completion: add completion script#261

Merged
ebiggers merged 1 commit intogoogle:masterfrom
audeoudh:master
Nov 30, 2020
Merged

bash-completion: add completion script#261
ebiggers merged 1 commit intogoogle:masterfrom
audeoudh:master

Conversation

@audeoudh
Copy link
Contributor

@audeoudh audeoudh commented Oct 30, 2020

Confer #260.

ebiggers
ebiggers reviewed Nov 7, 2020
View reviewed changes
Copy link
Collaborator

@ebiggers ebiggers left a comment

Choose a reason for hiding this comment

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

Thanks, a few comments. It's hard to review this without experience writing bash completion scripts, though.

@ebiggers
Copy link
Collaborator

ebiggers commented Nov 20, 2020

I think I'm finally understanding most of this, but it's difficult to read because of the code duplication with the bash-completion boilerplate. Could you add some helper functions like:

_fscrypt_complete_mountpoint()                                                   
{                                                                                
    COMPREPLY=($(compgen -W "$(_fscrypt_mountpoints)" -- "$cur"))                
}

_fscrypt_complete_word()                                                         
{                                                                                
    local wordlist=$1                                                            
                                                                                 
    COMPREPLY=($(compgen -W "$wordlist" -- "$cur"))                              
}                                                                                
                                                                                 
_fscrypt_complete_option()                                                       
{                                                                                
    local additional_opts=${1-}                                                  
    local GLOBAL_OPTS="--verbose --quiet --help"                                 
                                                                                 
    _fscrypt_complete_word "$additional_opts $GLOBAL_OPTS"                       
}

... and use them everywhere where they can. Also, please use if instead of case when there are only two branches.

That should make things a lot easier to understand.

E.g. the following:

        case $cur in
            -*)
                COMPREPLY=($(compgen -W "--time= --force ${GLOBAL_OPTS}" \
                           -- "${cur}"))
                ;;
            *)
                COMPREPLY=($(compgen -W "$(_fscrypt_mountpoints)" -- "${cur}"))
                ;;
        esac

... would become:

        if [[ $cur == -* ]]; then
            _fscrypt_complete_option "--time= --force"
        else
            _fscrypt_complete_mountpoint
        fi

ebiggers
ebiggers reviewed Nov 20, 2020
View reviewed changes
@ebiggers
Copy link
Collaborator

ebiggers commented Nov 20, 2020

Also, please squash your commits into one. Thanks!

@audeoudh
Copy link
Contributor Author

audeoudh commented Nov 27, 2020

I mainly:

  • changed “too simple” case structures to if/else blocks,
  • added functions for conceptualize bash-completion typical code,
  • added more comments in the script,
  • added the script in make lint and make install.
  • squashed commits

Let me know if there is still something to do.

@ebiggers
Copy link
Collaborator

ebiggers commented Nov 27, 2020

It looks much better now, thanks. Can you address the two comments I just left (remove --drop-caches, and update make uninstall)? Afterwards I think this will be ready to merge, unless @josephlr has comments.

@ebiggers ebiggers merged commit b74cd30 into google:master Nov 30, 2020
@ebiggers
Copy link
Collaborator

ebiggers commented Nov 30, 2020

Thanks, merged now.

@ebiggers ebiggers mentioned this pull request Nov 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ebiggers ebiggers ebiggers left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@audeoudh @ebiggers