GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bash-completion: add completion script by audeoudh · Pull Request #261 · google/fscrypt

Conversation

@audeoudh

@audeoudh audeoudh commented Oct 30, 2020

Copy link
Copy Markdown
Contributor

Confer #260.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
ebiggers
ebiggers reviewed Nov 7, 2020
View reviewed changes

@ebiggers ebiggers left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread util/fscrypt_completion Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread util/fscrypt_completion Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread util/fscrypt_completion Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread util/fscrypt_completion Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread util/fscrypt_completion Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread util/fscrypt_completion Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread util/fscrypt_completion Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread util/fscrypt_completion Resolved

Uh oh!

There was an error while loading. Please reload this page.

@ebiggers

ebiggers commented Nov 20, 2020

Copy link
Copy Markdown
Collaborator

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ebiggers
ebiggers reviewed Nov 20, 2020
View reviewed changes
Comment thread cmd/fscrypt/fscrypt_completion Resolved

Uh oh!

There was an error while loading. Please reload this page.

@ebiggers

ebiggers commented Nov 20, 2020

Copy link
Copy Markdown
Collaborator

Also, please squash your commits into one. Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@audeoudh

audeoudh commented Nov 27, 2020

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ebiggers

ebiggers commented Nov 27, 2020

Copy link
Copy Markdown
Collaborator

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@audeoudh
bash-completion: add completion script
19b7e84
@ebiggers
ebiggers merged commit b74cd30 into google:master Nov 30, 2020
@ebiggers

ebiggers commented Nov 30, 2020

Copy link
Copy Markdown
Collaborator

Thanks, merged now.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ebiggers ebiggers mentioned this pull request Nov 30, 2020
Bash completion for the fscrypt command #260
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@audeoudh @ebiggers
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.