Re: [PATCH 02/10] bash-completion: disk-utils

[Sami Kerola <kerolasa@iki.fi>]

On Wed, Mar 27, 2013 at 09:42:15PM -0400, Dave Reisner wrote:
> On Wed, Mar 27, 2013 at 10:07:44PM +0000, Sami Kerola wrote:
> > +			local DEVS
> > +			DEVS="$(lsblk -o NAME -n -r)"
> > +			OPTS="-h --help -V --version $DEVS"
> > +			COMPREPLY=( $(compgen -W "${OPTS[*]}" -- $cur) )
> 
> You've defined OPTS as a simple string variable and then you're
> expanding it as an array. Make sure to localize any variables you might
> define in the completion function as well, or else they leak to the
> user's environment (DEVS, OPTS). In general, I'd recommend *against*
> using all capitalized variable names, particularly for local variables.

I made attempt to find non-localized variables, and correct them.  It
could be that there are still some left.

> > +			;;
> > +		2)
> > +			COMPREPLY=( $(compgen -W "$((1 + $(ls $prev?* 2>/dev/null | wc -l)))" -- $cur) )
> 
> Please don't use ls to count items in a directory. It's slightly longer
> winded, but bash does this just fine without forking:
> 
>  filecount=0
>  files=("$prev"?*)
>  [[ -e ${files[0]} ]] && filecount=${#files[*]}
> 
> This advice and the above apply to a bunch of your patches.

All instances of 'ls' execution are gone.

> > +_delpart_module()
> > +{
> > +	local cur OPTS
> > +	COMPREPLY=()
> > +	cur="${COMP_WORDS[COMP_CWORD]}"
> > +	case $COMP_CWORD in
> > +		1)
> > +			local DEVS
> > +			DEVS="$(lsblk -o NAME,TYPE -n -r | awk '$2 ~ /disk/ {print "/dev/" $1}')"
> 
>   local dev typ
>   while read dev typ; do
>     [[ $dev = 'disk' ]] && devices+=("/dev/$dev")
>   done < <(lsblk -nro name,type)

Changed to version you proposed.

> > +			OPTS="-h --help -V --version $DEVS"
> > +			COMPREPLY=( $(compgen -W "${OPTS[*]}" -- $cur) )
> > +			;;
> > +		2)
> > +			COMPREPLY=( $(compgen -W "$(cat /sys/block/${prev##*/}/*/partition 2>/dev/null)" -- $cur) )
> 
> Bash has a builtin "cat" which uses mmap instead of direct reads, and is
> far more efficient:
> 
>   COMPREPLY=( $(compgen -W "$(</sys/block/"${prev##*/}"/*/partition 2>/dev/null)" -- $cur) )

I left that as-is.  Reading from a bunch of files would require a loop,
which and I don't think avoiding cat is worth of the mess.

Thank you for advice Dave.

-- 
   Sami Kerola
   http://www.iki.fi/kerolasa/