Re: [libvirt] [PATCH v2 4/9] virsh: Add vshDomainCompleter

On 08/20/2013 02:02 PM, Tomas Meszaros wrote: Your commit message should give a brief summary of your change and why it is important ("Add vshDomainCompleter" is the what, but you didn't spell out the why - something along the lines of "future patches will use this to improve readline completion"). These three lines are a changelog from your v1 patch, and not the "why" you are making the change. In fact, since this patch does not mention "__my_conn", no one will understand why you mentioned it at all in the commit message. It is information that belongs better... ...here, after the --- separator, which is where 'git am' interprets the break between the "why" of the commit message that belongs in git history, vs. the "help" that is useful to list reviewers now, but has no bearing on a future person reading git history. Dead code. names is non-null, thanks to vshMalloc. Simpler to use virStringFreeList(names) here, instead of inlining the iteration. I'm not a fan of this. We worked hard to get some commands (like 'virsh echo') to NOT need a connection, and this would be undoing that work. I think we should NOT autoconnect UNTIL we reach the point that completion is being attempted on something where a connection is needed to get the completer list, rather than this code which autoconnects regardless of whether we need completion. Having 'virsh echo' avoid an autoconnect is useful - it can be used to prove whether virsh itself is working or has a problem such as a missing .so, without complicating the diagnosis with a question of whether it is a bad default connection to blame. Also, at some point in your series, you will want to introduce a new virsh command whose job is to perform the completion of its arguments. That way, we can have auto-completion both within the virsh interactive shell, and from bash, while only having to maintain it in one place. That is, if I type: $ virsh virsh> start a<TAB> I'm using the interactive virsh completion to list domains starting with a; meanwhile, if I type: $ virsh start a<TAB> then the bash completion handler would call: $ virsh complete start a under the hood, so that virsh outputs the list of names that can be fed back to the bash completion hooks. The bash completion routine is then dead simple - forward all completions to virsh itself. I don't want to have to teach bash completion routines which commands expect a domain, nor how to generate its own domain listing, when virsh already has that code. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org