Re: [PATCH] ethtool: Add bash-completion script

[Kevin Locke <kevin@kevinlocke.name>]

On Thu, 2019-04-11 at 09:14 -0700, Jesse Brandeburg wrote:
> On Sun, 7 Apr 2019 20:19:37 -0600 Kevin Locke <kevin@kevinlocke.name> wrote:
>> To aid users constructing a valid ethtool invocation, create a
>> [bash-completion] script to provide [programmable completion] of ethtool
>> arguments.  It supports all current command options.
>> 
>> The script is placed in shell-completion/bash and installed to
>> completionsdir from pkg-config for bash-completion, similar to [kmod].
>> It requires pkg-config 0.18 or later to be installed on the build
>> system which runs aclocal (for the PKG_CHECK_MODULES m4 macro).
>> 
>> [...]
> 
> Thank you! I think this is super useful, and I agree FWIW that it
> should be part of ethtool.  I suspect that it needs to be installed by
> any package manager as part of ethtool install, into the right
> directory.  Could be a followup patch?

Great!  Glad to hear it.  Patched configure.ac and Makefile.am install
the script to `pkg-config --variable=completionsdir bash-completion`
(with fallback to $datadir/bash-completion/completions) by default.
It can be disabled by passing --without-bash-completion-dir or
overridden by --with-bash-completion-dir=$anypath.  I would expect
most distribution packages would install it by default, but if there
are followup patches (for ethtool or to the distributions), I'd be
happy to submit those as well.

> And the only (minor) complaint I have about your patch is that the
> commit message doesn't show how to install it (basically just copy
> ethtool file from this patch to
> f.e. /usr/share/bash-completion/completions/)

Good point.  I will document the install process and the configure
arguments mentioned above and send an updated patch shortly.

> I did a quick touch test of it and it seemed to be completing ethtool
> commands which made me really happy, especially the network interface
> names which are so long.
> 
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> 
> And now I'm going to go and tell everyone I work with about this
> patch. :-)

Wonderful!  I'm very glad to hear that you found it useful and I hope
your colleagues appreciate it as well.  Let me know if you or they
find any improvements that I can include in a future version of the
patch.

Thanks for taking the time to review it!

Best,
Kevin