Re: [libvirt] [PATCH v2 03/12] virsh: Conditionally Ignore the first entry in list of completions

On 05/10/2018 05:17 PM, Michal Privoznik wrote: > On 05/08/2018 04:20 PM, Lin Ma wrote: >> The first entry in the returned array is the substitution for TEXT. It >> causes unnecessary output if other commands or options share the same >> prefix, e.g. >> >> $ virsh des<TAB><TAB> >> des      desc     destroy >> >> or >> >> $ virsh domblklist --d<TAB><TAB> >> --d        --details  --domain >> >> This patch fixes the above issue. >> >> Signed-off-by: Lin Ma <lma(a)suse.com&gt; >> --- >>   tools/vsh.c | 6 +++++- >>   1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/tools/vsh.c b/tools/vsh.c >> index 73ec007e56..57f7589b53 100644 >> --- a/tools/vsh.c >> +++ b/tools/vsh.c >> @@ -3458,6 +3458,7 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd) >>       const vshCmdOpt *opt = NULL; >>       char **matches = NULL, **iter; >>       virBuffer buf = VIR_BUFFER_INITIALIZER; >> +    int n; > This needs to be size_t. Even though it's not used to directly access > entries of @matches array, it kind of is. It's used to count them. And > int is not guaranteed to be able to address all of them. > >>         if (vshCommandOptStringQuiet(ctl, cmd, "string", &arg) <= 0) >>           goto cleanup; >> @@ -3493,8 +3494,11 @@ cmdComplete(vshControl *ctl, const vshCmd *cmd) >>       if (!(matches = vshReadlineCompletion(arg, 0, 0))) >>           goto cleanup; >>   -    for (iter = matches; *iter; iter++) >> +    for (n = 0, iter = matches; *iter; iter++, n++) { >> +        if (n == 0 && matches[1]) >> +            continue; > This can be rewritten so that we don't need @n at all: > >    if (iter == matches && matches[1]) >        continue; > > Or even better, just start iterating not from the first but second entry: > >      for (iter = &matches[1]; *iter; iter++) >          printf("%s\n", *iter); It seems it can't handle the case that no command or option share the same prefix. say 'event' command, when users type virsh ev<TAB><TAB>, there is no other command sharing 'ev' prefix, in this case, the matches[1] is NULL and we need to print the value in matches[0],I think we can't skip the first entry in this case. So the code block 'if (iter == matches && matches[1])  continue;' looks like a better choice.If you think so, I'd like to write a patch to do it, Do you agree? meanwhile, I want to remove those comment due to we can't skip first entry, Do you agree?