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

[Mikael Magnusson <mikachu@xxxxxxxxx>]

On Sat, Mar 19, 2016 at 12:20 AM, Oliver Kiddle <okiddle@xxxxxxxxxxx> 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.

> Testing will have been limited because I don't have any device adb could
> connect to (still using an ancient Nokia) and I don't typically use or
> install it. There's a different adb command on Solaris which is perhaps
> why I was annoyed at it not falling back to default file completion.
>
>> 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
>> "sanitize context" stuff, or "shift words" etc which just makes the rest
>> of the code not know where it is, but I don't care enough to rewrite it.
>
> The "sanitize context" stuff was because it was putting the sub-command
> into the wrong component of $curcontext. Completion contexts are
> supposed to be:
>   :completion:<function>:<completer>:<command>:<arg>:<tag>
>
> We typically put subcommands into <command> by changing it to
> <command>-<subcommand>. If you use a colon instead of a dash, the
> subcommand will be in <arg> and thereafter, everything gets messed up.
>
>> -(( $+functions[_adb_check_log_redirect] )) ||
>> +(( $+functions[adb_check_log_redirect] )) ||
>>  _adb_check_log_redirect () {
>
>> -  LOG_REDIRECT=${$(adb ${=ADB_DEVICE_SPECIFICATION} shell getprop log.redirect-stdio 2>/dev/null)//
>> +  LOG_REDIRECT=${$(adb ${=ADB_DEVICE_SPECIFICATION} shell getprop log.redirect-stdio)//
>>  /}
>
> This should really be using _call_program but I'm sure I redirected
> stderr for a reason. It was probably spewing out errors to the terminal
> because it couldn't find an attached device.
>
> Are you sure you want to back that out?

No, I removed those hunks from the commit before pushing, included in
the sent diff by mistake.

> I'm not actually fond of these guards on function definitions myself
> because they prevent the function from being reloaded when the
> underlying file in $fpath is modified and reloaded. I only use them in
> cases where it seems likely that someone would want to override the
> function.
>
> I also don't really like _adb being chatty with lots of _message -r
> calls. I tend to prefer completion functions to just do the best they
> can of generating matches and otherwise keep quiet.

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
completer broke, and since reverting this commit fixed it, I spent
some time to get file completion after unhandled subcommands and
working completion after handled subcommands. I didn't really spend
any time looking into why the changed code didn't work, sorry. If you
have any theories about it, I'd be happy to try them out though.

-- 
Mikael Magnusson