[libvirt] [PATCH v2 06/12] vshReadlineParse: Ignore vshReadlineOptionsGenerator for VSH_OT_ARGV options

[Michal Privoznik]

On 05/08/2018 04:20 PM, Lin Ma wrote:
> Currently the VSH_OT_ARGV options don't support complete, But some of
> VSH_OT_ARGV options are gonna support complete in upcoming patches.
> 
> Once applied the upcoming completion patches for VSH_OT_ARGV options, If
> we don't ignore VSH_OT_ARGV here, The vshReadlineOptionsGenerator will
> be called, Hence complete output will consist of the result by command
> completer + the result by option completer, It's confusing.
> e.g.
>     $ virsh domstats --domain <TAB><TAB>
>     --backing     --interface      --list-paused      --perf      --vcpu
>     --balloon     leap42.3         --list-persistent  --raw       win10
>     --block       --list-active    --list-running     sles12sp3
>     --cpu-total   --list-inactive  --list-shutoff     sles15
>     --enforce     --list-other     --list-transient   --state
> 
> After this patch and the upcoming completion patches:
>     $ virsh domstats --domain <TAB><TAB>
>     leap42.3    sles12sp3    sles15    win10
> 
> Signed-off-by: Lin Ma <lma at suse.com>
> ---
>  tools/vsh.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/vsh.c b/tools/vsh.c
> index 57f7589b53..e45bb0d825 100644
> --- a/tools/vsh.c
> +++ b/tools/vsh.c
> @@ -2824,7 +2824,9 @@ vshReadlineParse(const char *text, int state)
>          if (!cmd) {
>              list = vshReadlineCommandGenerator(text);
>          } else {
> -            if (!opt || (opt->type != VSH_OT_DATA && opt->type != VSH_OT_STRING))
> +            if (!opt || (opt->type != VSH_OT_DATA &&
> +                         opt->type != VSH_OT_STRING &&
> +                         opt->type != VSH_OT_ARGV))
>                  list = vshReadlineOptionsGenerator(text, cmd, partial);
>  
>              if (opt && opt->completer) {
> 

I don't think this is right. The VSH_OT_ARGV means that there can be
more than one argument. And these are usually specified at the end like
this:

  # virsh domstats dom1 dom2 dom3

This patch allows to specify just one domain. I guess we need a specific
completer for this command.

After going further with the review I had revisit this patch again. Now
it makes perfect sense, because after patch 09/12 you allow multiple
--domain arguments. Therefore I think a small reorder is in place. 09/12
should be placed before this patch. I'll fix that when pushing.

ACK

Michal