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/
--
To unsubscribe from this list: send the line "unsubscribe util-linux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
