Re: [PATCH] completion: add _uids, _gids

[Oliver Kiddle <opk@xxxxxxx>]

On 10 Jul, dana wrote:
> how about this
>
> - _users and _groups can complete either or both of names + ids
> - _uids and _gids are thin wrappers for _users/_groups -u

That sounds good. The thin wrappers and shared implementation makes
sense.

> - as we discussed on irc, the listing was confusing when both types
>   were completed because they looked the same but inserted different
>   things. to help with this, i made it use the id as the match
>   description with -u

That's fine. I think we can do better for -Nu where both forms are
matched (such as with empty $PREFIX) - sorting them together but adding
both as matches. While -Nu offers both, where both are valid, it'd
mostly be better if UIDs were only a fallback.

You are missing && ret=0 from _describe so it doesn't stop at the
_complete completer.

> - however it's annoying to have each of these entries take up a whole
>   line. _numbers builds the display strings itself to avoid this issue.
>   rather than copy+paste that method twice here, i added an option -c
>   (as in print -c) to _describe to make it not use compadd -l. i'll add
>   tests and update _numbers to use this if you approve

I'm not keen on this added _describe -c option. The main purpose
of _describe is its ability to combine matches that share a common
description. Even with short matches/descriptions, that's easily too
long for columns to work. It seems that with -c it is switching back to
lines given any shared description. Multiple usernames sharing a UID is
not uncommon and has uses such as the toor user on BSD. There's some
complexity associated with that functionality to add hidden matches. So
the internals of _describe are complex and I fear that while with very
small changes it all appears to work, we'll be chasing obscure and hard
to fix bugs.

Next _describe does not do a good job of accepting more unusual compadd
options like -A, -D. It even conflicts for -O. Username completion
is useful to have working with matching across a composite word
where the other part is supplied separately. That also needs hidden
prefixes/suffixes. I'm not sure whether we rely on this working with
_users in any currently included function but I definitely rely on it
myself. One example of that is for Kerberos principals which I should
cleanup and post.

There are many cases where we've manually constructed compadd options
that include descriptions to avoid unwanted overkill and limitations
of _describe. Given the need to check the separator style and the fact
that we often go to the effort of aligning separators, I wouldn't argue
against factoring much of this out into a simpler helper. But I think
we can improve on the interface provided by _describe and support the
full gamut of compadd options. Perhaps a helper like _description that
populates an array with options, doesn't do a tags/labels loop and
leaves you to call compadd.

> - i made it filter the ids to those matching the names in the
>   users/groups styles. i realise these names may not actually exist but
>   that felt better than doing nothing, and idk if there's a use case for
>   fake ids

Most commands that want raw UIDs are fairly low-level system stuff so
I'm not sure how useful that is. Often the style is used where _users
otherwise doesn't successfully retrieve the users. On Linux, getent
often won't get all users (Redhat recommend against enumerate = true in
sssd.conf). So you just get the local system users which may well be
what you want when using things like ionice and nsenter. This affects me
on some FreeIPA clients but I take the approach of installing a
replacement _users. We could perhaps support ldapsearch with styles for
the options such as exists for _email_addresses.

> - i added nis support to the no-getent case in _users, to match _groups.
>   but i'd be inclined to remove it from both of them tbh. i imagine it's
>   not used much any more

I can't remember whether getent works on NIS systems for enumerating all
users. It probably still gets some use because ldap is not as simple
as it might be.
I have vague memories that it may need to be ypcat passwd.byname

> - i removed the cache from _groups. i assume that was for nis and
>   probably isn't that necessary now

We'll see if anyone complains.

Thanks

Oliver