Re: PATCH: Fix "35531: fallback on file completion"

[Oliver Kiddle <okiddle@xxxxxxxxxxx>]

Mikael Magnusson wrote:
> > Mikael Magnusson wrote:
> >> The above commit just changed a bunch of stuff without testing, the
> >> result was that the completer didn't work. It now works, to an extent.
> >
> > Specifically, in what way didn't it work?
>
> Every completion after "adb anything <tab>" only completed files.

I think the problem was a missing trailing colon when matching
$curcontext. The following patch is against the original before your
commit. Given that you squashed the backout and new changes together,
would you mind merging this. Again, my only testing first involves
hacking it to continue when it doesn't find adb working.

> >> The one remaining caveat is that if you enter a subcommand the completer
> >> doesn't recognize, it will go on to offer subcommands instead of falling
> >> back to _default. I'm not sure what it's trying to do with the

_default is nearly always the best option with unrecognised subcommands.
In the past, we've had various approaches such as _message which might
be helpful if a user has mistyped a sub-command but _default is
consistent with how we handle top-level commands for which there is no
completion and is the most graceful fallback when new subcommands have
been added since the function was written. It'd also be more graceful in
the case of adb on Solaris, though we should probably just stuff an if
condition of [[ $OSTYPE = solaris* ]] in the top.

> Yeah, there's some really questionable code in there, and lots of
> superfluous double quotes that end up in displayed descriptions. (like
> for i in $(ls -d /*)). Someone just complained earlier that the

Yuk. We could do better when it comes to code-reviewing new functions
while the original author is still around. Complaining about everything
before we get past the first dozen lines isn't really going to be
encouraging, however. In this case, the local variables should be moved
inside adb() otherwise they are only local the first time. setopt
nonomatch should not be required, though it is hard to know why it is
there. Then we have a variable naming scheme that leaves me wondering if
stuff like ADB_DEVICE_SPECIFICATION is an environmenent variable with a
meaning to adb. The -l option should not be there: autoload +X _adb is
close if you want to see the result of autoloading. And it goes on...

But as I've apparently demonstrated, I'm not able to clean it up without
also accidentally breaking it in the process.

Oliver

diff --git a/Completion/Unix/Command/_adb b/Completion/Unix/Command/_adb
index 88aca24..e6f7710 100644
--- a/Completion/Unix/Command/_adb
+++ b/Completion/Unix/Command/_adb
@@ -100,25 +100,25 @@ _adb_dispatch_command () {
   fi
 
   case ${curcontext} in
-    (*:adb-shell)
+    (*:adb-shell:)
       (( $+functions[_adb_dispatch_shell] )) && _adb_dispatch_shell
       ;;
-    (*:adb-connect|*:adb-disconnect)
+    (*:adb-connect:|*:adb-disconnect:)
       (( $+functions[_adb_dispatch_connection_handling] )) && _adb_dispatch_connection_handling
       ;;
-    (*:adb-logcat)
+    (*:adb-logcat:)
       (( $+functions[_adb_dispatch_logcat] )) && _adb_dispatch_logcat
       ;;
-    (*:adb-push)
+    (*:adb-push:)
       (( $+functions[_adb_dispatch_push] )) && _adb_dispatch_push
       ;;
-    (*:adb-pull)
+    (*:adb-pull:)
       (( $+functions[_adb_dispatch_pull] )) && _adb_dispatch_pull
       ;;
-    (*:adb-install)
+    (*:adb-install:)
       (( $+functions[_adb_dispatch_install] )) && _adb_dispatch_install
       ;;
-    (*:adb-uninstall)
+    (*:adb-uninstall:)
       (( $+functions[_adb_dispatch_uninstall] )) && _adb_dispatch_uninstall
       ;;
     (*:adb-*)