[PATCH] Completion batch #2: Misc. trivial fixes

[PATCH] Completion batch #2: Misc. trivial fixes

[dana]

Some trivial/miscellaneous changes to existing functions:

1. I fixed a bug in `tr` completion — it wasn't showing the option descriptions
   for non-GNU variants.

2. I updated `expand` and `unexpand` completion to account for numeric options
   as in `expand -4` (instead of `expand -t4`); most variants support this. I
   excluded BusyBox, even though the function doesn't currently make any effort
   to check for it, because i plan to gradually add support for BusyBox variants
   when it's not too irritating (will show up in later batches).

The changes after this one will be more complex.

dana


diff --git a/Completion/Unix/Command/_tr b/Completion/Unix/Command/_tr
index d244bf875..1cfe1200a 100644
--- a/Completion/Unix/Command/_tr
+++ b/Completion/Unix/Command/_tr
@@ -28,7 +28,7 @@ case $variant in
   ;|
   *)
     for k in c d s; do
-      args+=( -$k$descr[$k] )
+      args+=( -$k$descr[-$k] )
     done
   ;;
 esac
diff --git a/Completion/Unix/Command/_unexpand b/Completion/Unix/Command/_unexpand
index 13f6ce835..b548b3c3a 100644
--- a/Completion/Unix/Command/_unexpand
+++ b/Completion/Unix/Command/_unexpand
@@ -28,6 +28,10 @@ elif [[ $OSTYPE = (*bsd*|dragonfly*|darwin*) ]]; then
 fi
 [[ $service = *un* ]] && args+=(  "(--all --help --version)-a[$all]" )
 
+# Most (un)expand variants, excluding BusyBox, allow e.g. -4 instead of -t4
+[[ $_cmd_variant[$service] == *busybox* ]] ||
+args+=( '!(-0 -1 -2 -3 -4 -5 -6 -7 -8 -9 -t --tabs)-'{0..9} )
+
 _arguments -s -S "$args[@]" \
   "(--tabs --help)-t+${tabs}" \
   '*:file:_files'

Re: [PATCH] Completion batch #2: Misc. trivial fixes

[Oliver Kiddle]

dana wrote:
> 2. I updated `expand` and `unexpand` completion to account for numeric options
>    as in `expand -4` (instead of `expand -t4`); most variants support this. I

> +# Most (un)expand variants, excluding BusyBox, allow e.g. -4 instead of -t4
> +[[ $_cmd_variant[$service] == *busybox* ]] ||

Digging around in $_cmd_variant is essentially looking into the
internals of _pick_variant. The documented interface is to use the -r
option to _pick_variant. Also, it is not saving the full output of
expand --version, it will either have the value "gnu" or "unix". This
needs to do something like:
  local variant
  _pick_variant -r variant gnu="Free Soft" busybox=busybox unix --version

> +args+=( '!(-0 -1 -2 -3 -4 -5 -6 -7 -8 -9 -t --tabs)-'{0..9} )

Given that these are hidden options, excluding other numeric options is
pointless. It is also arguably wrong because the tab width can be more
than 9 characters wide: e.g. expand -20 is valid.

I've applied the patch in full because it is probably easier that way.

Re: [PATCH] Completion batch #2: Misc. trivial fixes

[dana]

On 3 Jan 2018, at 17:40, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
>Digging around in $_cmd_variant is essentially looking into the
>internals of _pick_variant. The documented interface is to use the -r
>option to _pick_variant. Also, it is not saving the full output of
>expand --version, it will either have the value "gnu" or "unix".

Well, maybe it's worth bringing this up now, then, before i submit any further
patches: The reason i'd added that check is that i wanted to be able to complete
commands like this:

  % busybox expand -<TAB>

This use case doesn't seem (necessarily) compatible with _pick_variant as-is,
because your unadorned `expand` may be a totally different variant from `busybox
expand`. The way i had handled this in the _busybox function is:

  _cmd_variant[${words[1]}]=busybox _normal

That way you can temporarily override what _pick_variant thinks the actual
variant is. This seems to work quite well, but i did feel some guilt about it,
since as you mention it's circumventing the interface.

Another issue i had with _pick_variant is dealing with risky commands. In most
cases i imagine it's probably fine to do something like `poweroff --help`... but
i certainly didn't want to take the chance, since a badly written poweroff might
just kill the machine if you're running as root. So i had again bypassed
_pick_variant and manipulated _cmd_variant myself.

Do you have a suggestion as to how i could accomplish things like this in a less
hacky way? Maybe _pick_variant could learn an option to set the variant
directly? Alternatively, maybe i am over-engineering everything again...?

On 3 Jan 2018, at 17:40, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
>Given that these are hidden options, excluding other numeric options is
>pointless. It is also arguably wrong because the tab width can be more
>than 9 characters wide: e.g. expand -20 is valid.

All i really wanted was to have it not offer -t if a numeric option's already
been given. The completion function doesn't have to know that -20 is different
from -2 -0 to do that, AFAIK. Didn't consider that excluding the other options
is pointless though, i suppose it is superfluous.

Thanks!

dana

Re: [PATCH] Completion batch #2: Misc. trivial fixes

[Oliver Kiddle]

dana's message doesn't seem to have come through on the mailing list so
I'll quote generously.

dana wrote:
> >Digging around in $_cmd_variant is essentially looking into the
> >internals of _pick_variant. The documented interface is to use the -r
> >option to _pick_variant. Also, it is not saving the full output of
> >expand --version, it will either have the value "gnu" or "unix".
>
> Well, maybe it's worth bringing this up now, then, before i submit any further
> patches: The reason i'd added that check is that i wanted to be able to complete
> commands like this:
>
>   % busybox expand -<TAB>
>
> This use case doesn't seem (necessarily) compatible with _pick_variant as-is,
> because your unadorned `expand` may be a totally different variant from `busybox
> expand`. The way i had handled this in the _busybox function is:
>
>   _cmd_variant[${words[1]}]=busybox _normal
>
> That way you can temporarily override what _pick_variant thinks the actual
> variant is. This seems to work quite well, but i did feel some guilt about it,
> since as you mention it's circumventing the interface.

Given that that works well, I'd go with that solution. It'd be possible
to add an option to _normal/_dispatch. Having them know about
_pick_variant internals is less ugly. It is, however, probably only worthwhile if
it is going to be used elsewhere. It might be wise to dig around for
other use cases. _builtin is the only thing that comes to mind.

> Another issue i had with _pick_variant is dealing with risky commands. In most
> cases i imagine it's probably fine to do something like `poweroff --help`... but
> i certainly didn't want to take the chance, since a badly written poweroff might
> just kill the machine if you're running as root. So i had again bypassed
> _pick_variant and manipulated _cmd_variant myself.

Yes, we don't want to ever call risky commands or commands that might
have side-effects. For poweroff, just don't use _pick_variant. Rely
on $OSTYPE and other tests like [[ -d /etc/systemd ]]. Caching the
result is unlikely to be necessary but if it is, don't use _cmd_variant.
poweroff likely doesn't get run many times in a single zsh session
anyway.

> Do you have a suggestion as to how i could accomplish things like this in a less
> hacky way? Maybe _pick_variant could learn an option to set the variant
> directly? Alternatively, maybe i am over-engineering everything again...?
>
> On 3 Jan 2018, at 17:40, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> >Given that these are hidden options, excluding other numeric options is
> >pointless. It is also arguably wrong because the tab width can be more
> >than 9 characters wide: e.g. expand -20 is valid.
>
> All i really wanted was to have it not offer -t if a numeric option's already
> been given. The completion function doesn't have to know that -20 is different
> from -2 -0 to do that, AFAIK. Didn't consider that excluding the other options
> is pointless though, i suppose it is superfluous.

Doesn't "superfluous" have much the same meaning as "pointless" in this
case. It seems to work as it is but I'd clean it up if making changes
for the busybox variant anyway. Doesn't busybox allow expand to be a
hard link to busybox to run expand?

I've applied the rename patch by the way. Bart makes a good point and I
also prefer _file_modes to _fmodes.

Oliver

Re: [PATCH] Completion batch #2: Misc. trivial fixes

[dana]

On 4 Jan 2018, at 05:47, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
>Given that that works well, I'd go with that solution. It'd be possible
>to add an option to _normal/_dispatch. Having them know about
>_pick_variant internals is less ugly. It is, however, probably only worthwhile if
>it is going to be used elsewhere. It might be wise to dig around for
>other use cases. _builtin is the only thing that comes to mind.

That's the only thing i can find that's similar too. As far as use cases we
might develop in the future: I've heard there is a GNU coreutils multi-call (so
you can do like `coreutils expand`), but i've never seen it in the wild. And
there is a BSD-licensed competitor to BusyBox called Toybox, but i've only ever
heard of it being used by Android.

On 4 Jan 2018, at 05:47, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
>Caching the
>result is unlikely to be necessary but if it is, don't use _cmd_variant.
>poweroff likely doesn't get run many times in a single zsh session
>anyway.

Caching is not necessary in terms of performance — checking for file paths like
you mentioned is a fast operation. But if i want to complete `busybox poweroff`
using the method discussed it seems like i'll either need to call _pick_variant
or deal with _cmd_variant myself, right? (Or i guess i could have _busybox pass
down some *other* variable, but that seems even messier.)

On 4 Jan 2018, at 05:47, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
>Doesn't "superfluous" have much the same meaning as "pointless" in this
>case.

It does indeed

>On 4 Jan 2018, at 05:47, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
It seems to work as it is but I'd clean it up if making changes
for the busybox variant anyway. Doesn't busybox allow expand to be a
hard link to busybox to run expand?

BusyBox does allow you to link its 'applets', yeah. And in that case of course
_pick_variant will correctly identify a command as BusyBox without any special
cases. But sometimes you have coreutils or whatever for your normal stuff
existing side-by-side with the busybox multi-call for other things. At least,
that was my own use case.

I will clean up my _expand change when i submit _busybox. Let me know if you
have any other thoughts in the mean time. Thanks!

dana

Re: [PATCH] Completion batch #2: Misc. trivial fixes

[dana]

On 3 Jan 2018, at 18:03, dana <dana@dana.is> wrote:
>The way i had handled this in the _busybox function is:
>
>  _cmd_variant[${words[1]}]=busybox _normal
>
>That way you can temporarily override what _pick_variant thinks the actual
>variant is. This seems to work quite well, but i did feel some guilt about it,
>since as you mention it's circumventing the interface.

I was playing with this some more to see if i could come up with a better way
and actually discovered a problem with the method i described: it is *not* a
temporary override. Apparently pre-command re-assignment of array members
doesn't work the same way it does with scalars — zsh doesn't put the value back
the way it was when it's done executing the command. I assume the same code is
involved here as with the issue i brought up in workers/42097:

http://www.zsh.org/mla/workers/2017/msg01761.html

Anyway, i suppose i could re-assign or unset it afterwards (according to
whatever the previous state was), but... it sure is starting to feel fragile.

Exporting something to the environment is the natural thing that comes to mind
when i need to communicate with a function that i'm two or three calls removed
from. I guess it could learn about a new scalar? _cmd_variant_override or
something? I don't know. There are lots of ways i could change this to work for
my use case, i just don't want to leave a mess. :/

dana