[libvirt] [PATCH 1/2] virsh-completer: Separate comma list construction into a function

[Michal Privoznik]

On 7/15/19 1:07 PM, Erik Skultety wrote:
> On Wed, Jun 19, 2019 at 01:50:14PM +0200, Michal Privoznik wrote:
>> On 6/19/19 12:59 PM, Erik Skultety wrote:
>>> On Tue, Jun 18, 2019 at 03:46:15PM +0200, Michal Privoznik wrote:
>>>> There are more arguments than 'shutdown --mode' that accept a
>>>> list of strings separated by commas. 'nodedev-list --cap' is one
>>>> of them. To avoid duplicating code, let's separate interesting
>>>> bits of virshDomainShutdownModeCompleter() into a function that
>>>> can then be reused.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>>> ---
>>>>    tools/virsh-completer.c | 120 ++++++++++++++++++++++++++--------------
>>>>    1 file changed, 77 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
>>>> index 7d5cf8cb90..ef2f39320e 100644
>>>> --- a/tools/virsh-completer.c
>>>> +++ b/tools/virsh-completer.c
>>>> @@ -69,6 +69,79 @@
>>>>     */
>>>>
>>>>
>>>> +/**
>>>> + * virshCommaStringListComplete:
>>>> + * @input: user input so far
>>>> + * @options: ALL options available for argument
>>>> + *
>>>> + * Some arguments to our commands accept the following form:
>>>> + *
>>>> + *   virsh command --arg str1,str2,str3
>>>> + *
>>>> + * This does not play nicely with our completer funtions, because
>>>> + * they have to return strings prepended with user's input. For
>>>> + * instance:
>>>> + *
>>>> + *   str1,str2,str3,strA
>>>> + *   str1,str2,str3,strB
>>>> + *   str1,str2,str3,strC
>>>
>>> ^This sounds rather sub-optimal. I wouldn't even insist on making the
>>> suggestions contextual like it is now, IOW not suggesting options which have
>>> already been specified and would rather return the same list of possible
>>> options than a string with the user input prepended.
>>
>> So IIUC, for 'shutdown --mode <TAB><TAB>' you want to see:
>>
>>    "acpi", "agent", "initctl", "signal", "paravirt"
>>
>> and for 'shutdown --mode acpi,agent,<TAB><TAB>' you want to see the same
>> list again (optionally with already specified strings removed)? Yep, that
>> would be great but I don't think that is how readline works. At least, I
>> don't know how to achieve that. Do you perhaps have an idea?
> 
> It very well may be the case that it doesn't work the way we'd like to and I
> don't understand how it actually works, but why does readline even matter here?
> Readline calls our completers which generate the output presented to the user,
> so we should be in charge what is returned, so why do we need to prepend the
> user input then? In fact, I found there's a function called vshCompleterFilter
> which removes the whole output list if the items are not prepended with the
> original user input, why is that? When I commented out the bit dropping items
> from the list and stopped pre-pending the user input, I achieved what I
> suggested in my original response to this series, a context-based list without
> unnecessary prefixes. 

This very likely did not work and only gave impression it is working. 
I've just tried what you suggest here and find it not working. The 
reason is that if we return only one option to complete it replaces the 
whole argument with that string. Or, if we return more strings then the 
argument is replaced with their longest shared prefix. For instance, if 
our completer would return only {"testA", "testB", NULL}, then the 
following input:

   virsh # start t<TAB><TAB>

would be overwritten to:

   virsh # start test
   testA testB

This is expected and in fact desired. But things get tricky when we 
start dealing with out argument lists:

   virsh # shutdown --mode <TAB><TAB>

gets you:

   virsh # shutdown --mode a
   acpi agent

So far so good. But then you introduce comma:

   virsh # shutdown --mode agent,a<TAB><TAB>

Now, there is only one possible completion = "acpi". So readline saves 
you some typing and turns that into:

   virsh # shutdown --mode acpi

Problem is that readline does not handle comma as a separator. Okay, we 
can fix that. It's easy to put comma at the end of @break_characters in 
vshReadlineInit(). But that breaks our option lookup because then @text 
== "a" in vshReadlineParse(). On one hand we want @text == "a" because 
that means that readline split user's input at the comma, on the other 
hand we can't now properly identify which --option is user trying to 
autocomplete because neither --option has "a" as its value (--mode has 
"agent,a").

> I also tried a few other random completions to see
> whether I didn't break something by stripping some code from
> vshCompleterFilter and it looks like it worked, so the question is, what was
> the reason for that function in the first place, since I haven't experienced
> the effects described by commit d4e63aff5d0 which introduced it?

The reason for existence of vshCompleterFilter() is to filter out 
non-relevant options. For instance, in aforementioned shutdown mode 
completer - we want, I want completers to be as simple as possible. 
Therefore, the shutdown mode completer returns all five strings, 
regardless of user's input. Then the filter function prunes out those 
strings (=options) which do not share prefix with user's input. For 
instance, if user's input is "a" then "initctl", "signal" and "paravirt" 
will be filtered out. If they weren't, and they would be returned back 
to readline, it would present them to the user for complete and it would 
look like this:

   virsh # shutdown --mode a<TAB><TAB>
   acpi      agent     initctl   paravirt  signal

And I believe we can both agree that this is bad behaviour.

Maybe solution is to not call rl_completion_matches() in 
vshReadlineCompletion() and utilize @start and @end arguments which 
point at the beginning and end of the word that user is trying to 
complete (although how would we use them to find the corresponding 
--option is something I do not know). But that's something I haven't tried.

Michal