In-snap bash tab completion #3150

Merged
merged 14 commits into from May 5, 2017

address review feedback

  • Loading branch information...
commit b02ba90d15d61ad5817eda8e241975669b3fff43 @chipaca chipaca committed Apr 25, 2017
View
@@ -89,14 +89,16 @@ func run() error {
return snapExecApp(snapApp, revision, opts.Command, extraArgs)
}
+const defaultShell = "/bin/bash"
+
func findCommand(app *snap.AppInfo, command string) (string, error) {
var cmd string
switch command {
case "shell":
- cmd = "/bin/bash"
+ cmd = defaultShell
case "complete":
if app.Completer != "" {
- cmd = "/bin/bash"
+ cmd = defaultShell
}
@jdstrand

jdstrand Apr 24, 2017

Contributor

Small nit-pick. It seems like perhaps setting defaultShell := /bin/bash up above and then setting "shell" and the fallback for "complete" to this might be better.

case "stop":
cmd = app.StopCommand
@@ -153,10 +155,10 @@ func snapExecApp(snapApp, revision, command string, args []string) error {
fullCmd := filepath.Join(app.Snap.MountDir(), cmd)
switch command {
case "shell":
- fullCmd = "/bin/bash"
+ fullCmd = defaultShell
cmdArgs = nil
case "complete":
- fullCmd = "/bin/bash"
+ fullCmd = defaultShell
cmdArgs = []string{
dirs.CompletionHelper,
filepath.Join(app.Snap.MountDir(), app.Completer),
@zyga

zyga Apr 13, 2017

Contributor

This is incorrect. The mount dir is different in the outside and on the inside of the snap execution environment. On the inside it is a /snap/... but on the outside it is variable.

@chipaca

chipaca Apr 18, 2017

Member

snap-exec always has app.Snap.MountDir() to /snap/..., which is correct because it's always "inside".

@zyga

zyga Apr 18, 2017

Contributor

Correct, sorry, I didn't notice this is inside the core snap already (snap-exec). Thank you for taking the effort to measure this experimentally.

@@ -6,7 +6,32 @@
_complete_from_snap() {
@zyga

zyga May 3, 2017

Contributor

Thank you for documenting this! This goes a long way towards explaining the concept in simple terms!

{
@jdstrand

jdstrand Apr 26, 2017

Contributor

Please add a comment here:

# De-serialize the output of 'snap run command=complete ...' into the format
# bash expects.
read -a opts
+ # opts is expected to be a series of compopt options
+ if [[ ${#opts[@]} -gt 0 ]]; then
+ if [[ "${opts[0]}" == "cannot" ]]; then
+ # older snap-execs sent errors over stdout :-(
+ return 1
+ fi
+
+ for i in "${opts[@]}"; do
+ if ! [[ "$i" =~ ^[a-z]+$ ]]; then
+ # non-alphanumeric option; something awry
@jdstrand

jdstrand Apr 26, 2017

Contributor

Please adjust this comment to # only lowercase alpha characters allowed

+ return 2
+ fi
+ done
+ fi
+
read bounced
+ case "$bounced" in
+ ""|"alias"|"export"|"job"|"variable")
@jdstrand

jdstrand Apr 26, 2017

Contributor

If I'm reading the tests correctly, we only have tests for "" and "export". Would it make sense to add tests for alias, job, variable and "unrecognised bounce"? Please correct me if there are tests for all of these.

@chipaca

chipaca Apr 27, 2017

Member

you are correct, more tests could be added

+ # OK
+ ;;
+ *)
+ # unrecognised bounce
+ return 2
+ ;;
+ esac
+
read sep
if [ "$sep" ]; then
@zyga

zyga Apr 18, 2017

Contributor

I didn't know that [ string ] is equivalent to [ -n string ].

@chipaca

chipaca Apr 18, 2017

Member

my personal problem with -n is that i always have to look it up as i never remember which is it and which is -z.

@jdstrand

jdstrand Apr 26, 2017

Contributor

Personal preference is to use if [ -n "$sep" ]; then but I won't block on it.

# non-blank separator? madness!
@@ -21,10 +46,6 @@ _complete_from_snap() {
fi
if [[ ${#opts[@]} -gt 0 ]]; then
- if [[ "${opts[0]}" == "cannot" ]]; then
- # older snap-execs sent errors over stdout :-(
- return 1
- fi
compopt $(printf " -o %s" "${opts[@]}")
@jdstrand

jdstrand Apr 26, 2017

Contributor

Note that shellcheck complained about this:

$ shellcheck -s bash ./complete.sh
In /usr/lib/snapd/complete.sh line 50:
            compopt $(printf " -o %s" "${opts[@]}")
                    ^-- SC2046: Quote this to prevent word splitting.
fi
if [ "$bounced" ]; then
@@ -48,7 +48,7 @@ fi
@jdstrand

jdstrand Apr 26, 2017

Contributor

Please add a comment:

# Source the bash-completion library functions and common completion setup
. /usr/share/bash-completion/bash_completion
@jdstrand

jdstrand Apr 26, 2017

Contributor

Please add a comment:

# Now source the snap's 'completer' script
-. $_compscript
+. "$_compscript"
# _compopts is an associative array, which keys are options.
@jdstrand

jdstrand Apr 26, 2017

Contributor

Can you update the comment to say what the options are for? When I read the comment, I asked myself "options for what?" :)

declare -A _compopts
@@ -201,8 +201,7 @@ var defaultTemplate = `
/usr/lib/snapd/snap-exec m,
# For in-snap tab completion
- /etc/bash_completion.d/ r,
- /etc/bash_completion.d/* r,
+ /etc/bash_completion.d/{,*} r,
/usr/lib/snapd/etelpmoc.sh ixr, # marshaller (see complete.sh for out-of-snap unmarshal)
/usr/share/bash-completion/bash_completion r, # user-provided completions (run in-snap) may use functions from here
@jdstrand

jdstrand Apr 24, 2017

Contributor

Are /etc/bash_completion.d/ and /usr/share/bash-completion/bash_completion what are used on all distros?

@chipaca

chipaca Apr 24, 2017

Member

AFAIK yes (but @morphis can probably confirm)