tools: Actually enable bash-completion

[libvirt] [PATCH 1/3] virshDomainNameCompleter: Prune accepted flags

Posted by Michal Privoznik 7 years, 3 months ago

Only a small subset of VIR_CONNECT_LIST_DOMAINS_* flags are
actually used in virsh code. Remove the unused ones.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 tools/virsh-completer.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
index e216d9076..e3b8234b4 100644
--- a/tools/virsh-completer.c
+++ b/tools/virsh-completer.c
@@ -45,18 +45,11 @@ virshDomainNameCompleter(vshControl *ctl,
 
     virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE |
                   VIR_CONNECT_LIST_DOMAINS_INACTIVE |
+                  VIR_CONNECT_LIST_DOMAINS_OTHER |
+                  VIR_CONNECT_LIST_DOMAINS_PAUSED |
                   VIR_CONNECT_LIST_DOMAINS_PERSISTENT |
-                  VIR_CONNECT_LIST_DOMAINS_TRANSIENT |
                   VIR_CONNECT_LIST_DOMAINS_RUNNING |
-                  VIR_CONNECT_LIST_DOMAINS_PAUSED |
-                  VIR_CONNECT_LIST_DOMAINS_SHUTOFF |
-                  VIR_CONNECT_LIST_DOMAINS_OTHER |
-                  VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE |
-                  VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE |
-                  VIR_CONNECT_LIST_DOMAINS_AUTOSTART |
-                  VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART |
-                  VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT |
-                  VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT,
+                  VIR_CONNECT_LIST_DOMAINS_SHUTOFF,
                   NULL);
 
     if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/3] virshDomainNameCompleter: Prune accepted flags

Posted by Martin Kletzander 7 years, 3 months ago

On Thu, Jan 25, 2018 at 03:22:18PM +0100, Michal Privoznik wrote:
>Only a small subset of VIR_CONNECT_LIST_DOMAINS_* flags are
>actually used in virsh code. Remove the unused ones.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> tools/virsh-completer.c | 13 +++----------
> 1 file changed, 3 insertions(+), 10 deletions(-)
>

ACK

>diff --git a/tools/virsh-completer.c b/tools/virsh-completer.c
>index e216d9076..e3b8234b4 100644
>--- a/tools/virsh-completer.c
>+++ b/tools/virsh-completer.c
>@@ -45,18 +45,11 @@ virshDomainNameCompleter(vshControl *ctl,
>
>     virCheckFlags(VIR_CONNECT_LIST_DOMAINS_ACTIVE |
>                   VIR_CONNECT_LIST_DOMAINS_INACTIVE |
>+                  VIR_CONNECT_LIST_DOMAINS_OTHER |
>+                  VIR_CONNECT_LIST_DOMAINS_PAUSED |
>                   VIR_CONNECT_LIST_DOMAINS_PERSISTENT |
>-                  VIR_CONNECT_LIST_DOMAINS_TRANSIENT |
>                   VIR_CONNECT_LIST_DOMAINS_RUNNING |
>-                  VIR_CONNECT_LIST_DOMAINS_PAUSED |
>-                  VIR_CONNECT_LIST_DOMAINS_SHUTOFF |
>-                  VIR_CONNECT_LIST_DOMAINS_OTHER |
>-                  VIR_CONNECT_LIST_DOMAINS_MANAGEDSAVE |
>-                  VIR_CONNECT_LIST_DOMAINS_NO_MANAGEDSAVE |
>-                  VIR_CONNECT_LIST_DOMAINS_AUTOSTART |
>-                  VIR_CONNECT_LIST_DOMAINS_NO_AUTOSTART |
>-                  VIR_CONNECT_LIST_DOMAINS_HAS_SNAPSHOT |
>-                  VIR_CONNECT_LIST_DOMAINS_NO_SNAPSHOT,
>+                  VIR_CONNECT_LIST_DOMAINS_SHUTOFF,
>                   NULL);
>
>     if (!priv->conn || virConnectIsAlive(priv->conn) <= 0)
>-- 
>2.13.6
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/3] virshDomainNameCompleter: Prune accepted flags

Posted by Peter Krempa 7 years, 3 months ago

On Thu, Jan 25, 2018 at 15:22:18 +0100, Michal Privoznik wrote:
> Only a small subset of VIR_CONNECT_LIST_DOMAINS_* flags are
> actually used in virsh code. Remove the unused ones.

I don't think this is well justified. All of the flags are implemented
in virsh code in general.

I see almost all of them implemented in the cmdList command.

You either need to rewrite the commit message or the patch below is
flawed.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/3] virshDomainNameCompleter: Prune accepted flags

Posted by Michal Privoznik 7 years, 3 months ago

On 01/31/2018 01:09 PM, Peter Krempa wrote:
> On Thu, Jan 25, 2018 at 15:22:18 +0100, Michal Privoznik wrote:
>> Only a small subset of VIR_CONNECT_LIST_DOMAINS_* flags are
>> actually used in virsh code. Remove the unused ones.
> 
> I don't think this is well justified. All of the flags are implemented
> in virsh code in general.
> 
> I see almost all of them implemented in the cmdList command.

We don't have to care about cmdList since it does not use
virshDomainNameCompleter. What this patch addresses is use of
VIR_CONNECT_LIST_DOMAINS_* for this specific completer callback. And our
commands accept domains only from a small subset of those. It's safe to
say that 99% of commands take active/inactive domains, and the rest 1%
is more specific. Anyway, lets try to print out all used flags for this
completer:

libvirt.git $ git grep VIRSH_COMMON_OPT_DOMAIN tools/ | cut -d'(' -f 2 |
cut -d')' -f 1 | sort -u

VIR_CONNECT_LIST_DOMAINS_ACTIVE
VIR_CONNECT_LIST_DOMAINS_INACTIVE
VIR_CONNECT_LIST_DOMAINS_OTHER
VIR_CONNECT_LIST_DOMAINS_PAUSED
VIR_CONNECT_LIST_DOMAINS_PERSISTENT
VIR_CONNECT_LIST_DOMAINS_RUNNING

> 
> You either need to rewrite the commit message or the patch below is
> flawed.

I guess you just misunderstood this patch.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/3] virshDomainNameCompleter: Prune accepted flags

Posted by Peter Krempa 7 years, 3 months ago

On Wed, Jan 31, 2018 at 14:34:24 +0100, Michal Privoznik wrote:
> On 01/31/2018 01:09 PM, Peter Krempa wrote:
> > On Thu, Jan 25, 2018 at 15:22:18 +0100, Michal Privoznik wrote:
> >> Only a small subset of VIR_CONNECT_LIST_DOMAINS_* flags are
> >> actually used in virsh code. Remove the unused ones.
> > 
> > I don't think this is well justified. All of the flags are implemented
> > in virsh code in general.
> > 
> > I see almost all of them implemented in the cmdList command.
> 
> We don't have to care about cmdList since it does not use
> virshDomainNameCompleter. What this patch addresses is use of
> VIR_CONNECT_LIST_DOMAINS_* for this specific completer callback. And our
> commands accept domains only from a small subset of those. It's safe to
> say that 99% of commands take active/inactive domains, and the rest 1%
> is more specific. Anyway, lets try to print out all used flags for this
> completer:

All this information is not present in the commit message. You
generalize that the "flags are (not) actually used in virsh code", which
is simply false. The commit message does in no way mention that you are
specifically talking about usage of the flags in the completion code,
which leads to false assumptions.

> 
> libvirt.git $ git grep VIRSH_COMMON_OPT_DOMAIN tools/ | cut -d'(' -f 2 |
> cut -d')' -f 1 | sort -u
> 
> VIR_CONNECT_LIST_DOMAINS_ACTIVE
> VIR_CONNECT_LIST_DOMAINS_INACTIVE
> VIR_CONNECT_LIST_DOMAINS_OTHER
> VIR_CONNECT_LIST_DOMAINS_PAUSED
> VIR_CONNECT_LIST_DOMAINS_PERSISTENT
> VIR_CONNECT_LIST_DOMAINS_RUNNING
> 
> > 
> > You either need to rewrite the commit message or the patch below is
> > flawed.
> 
> I guess you just misunderstood this patch.

You did not explain it well enough in the commit message.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/3] virshDomainNameCompleter: Prune accepted flags

Posted by Michal Privoznik 7 years, 3 months ago

On 01/31/2018 02:39 PM, Peter Krempa wrote:
> On Wed, Jan 31, 2018 at 14:34:24 +0100, Michal Privoznik wrote:
>> On 01/31/2018 01:09 PM, Peter Krempa wrote:
>>> On Thu, Jan 25, 2018 at 15:22:18 +0100, Michal Privoznik wrote:
>>>> Only a small subset of VIR_CONNECT_LIST_DOMAINS_* flags are
>>>> actually used in virsh code. Remove the unused ones.
>>>
>>> I don't think this is well justified. All of the flags are implemented
>>> in virsh code in general.
>>>
>>> I see almost all of them implemented in the cmdList command.
>>
>> We don't have to care about cmdList since it does not use
>> virshDomainNameCompleter. What this patch addresses is use of
>> VIR_CONNECT_LIST_DOMAINS_* for this specific completer callback. And our
>> commands accept domains only from a small subset of those. It's safe to
>> say that 99% of commands take active/inactive domains, and the rest 1%
>> is more specific. Anyway, lets try to print out all used flags for this
>> completer:
> 
> All this information is not present in the commit message. You
> generalize that the "flags are (not) actually used in virsh code", which
> is simply false. The commit message does in no way mention that you are
> specifically talking about usage of the flags in the completion code,
> which leads to false assumptions.

Ah sorry, I though that "virsh*Completer: " prefix was clear enough. It
isn't. Okay, I'll update the commit message before pushing.

> 
>>
>> libvirt.git $ git grep VIRSH_COMMON_OPT_DOMAIN tools/ | cut -d'(' -f 2 |
>> cut -d')' -f 1 | sort -u
>>
>> VIR_CONNECT_LIST_DOMAINS_ACTIVE
>> VIR_CONNECT_LIST_DOMAINS_INACTIVE
>> VIR_CONNECT_LIST_DOMAINS_OTHER
>> VIR_CONNECT_LIST_DOMAINS_PAUSED
>> VIR_CONNECT_LIST_DOMAINS_PERSISTENT
>> VIR_CONNECT_LIST_DOMAINS_RUNNING
>>
>>>
>>> You either need to rewrite the commit message or the patch below is
>>> flawed.
>>
>> I guess you just misunderstood this patch.
> 
> You did not explain it well enough in the commit message.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list