Bash completion functions for pmount and pumount

Question

I found a problem with Debian's Bash-completion for pmount and pumount, and intended to add a patch to the bug report - but I got carried away, and ended up completely rewriting the completion functions.

The original was in /etc/bash_completion.d/pmount, but the modern approach is to have two files in /usr/share/bash-completion/completions/ to support demand-loading of completion functions. We assume that the _init_completion is provided as part of this mechanism, to initialize shell variables cur and prev more usefully than the usual 2ドル and 3ドル (e.g. skipping redirection words).

I've gone to some lengths to find the actual available charsets and filesystem types that can be used, but the hairiest code is in finding the available names for devices (I like to be able to use the symlinks in /dev/disk/by-label/ to ensure I'm using the device/partition I'm expecting, for example).

---

/usr/share/bash-completion/completions/pmount

_pmount() {
 # shellcheck disable=SC2034
 local cur prev words cword
 _init_completion || return
 case "$prev" in
 -@(t|-type))
 COMPREPLY=($(grep "^[[:space:]]$cur" /proc/filesystems) $(find "/lib/modules/$(uname -r)/kernel/fs" -name "*.ko" -print0 | xargs -r -0 /sbin/modinfo | sed -ne 's/^alias: *fs-//p' | grep "^$cur"))
 return 0
 ;;
 -@(c|-charset))
 local encodings=(/sys/module/nls_* $(find "/lib/modules/$(uname -r)/kernel/fs/nls" -name '*.ko' -print0 | xargs -r -0 /sbin/modinfo | sed -ne 's/^\(name\|alias\): *//p'))
 COMPREPLY=($(compgen -W "${encodings[*]##*nls_}" -- "$cur"))
 return 0
 ;;
 -@(u|d|f|-umask|-dmask|-fmask))
 case "$cur" in
 '') COMPREPLY=( {0..7} ) ;;
 [0-7]|[0-7][0-7]) COMPREPLY=( $cur{0..7} ) ;;
 [0-7][0-7][0-7]) COMPREPLY=( $cur ) ;;
 *) return 1 ;;
 esac
 return 0
 ;;
 -@(p|-passphrase))
 _filedir
 return 0
 ;;
 esac
 if [[ "$cur" == -* ]]; then
 # transform "--help" output into completion list
 COMPREPLY=($(compgen -W "$(pmount --help | sed -e '/^..-/!d' -e 's/:.*//' -e 's/<[^>]*>//g' -e 'y/,/ /')" -- "$cur"))
 else
 local i allowed removable devices search
 local IFS=$'\n'
 allowed=($(grep -v '^[[:space:]]*#' /etc/pmount.allow))
 removable=($(for i in /sys/block/*
 do
 grep -Fxq 1 "$i/removable" || continue
 # Replace with its partitions, if it has any - N.B. final /. is crucial, as
 # subsystem entries are symlinks!
 find "$i"/*/subsystem/. -maxdepth 0 -samefile "$i"/subsystem/. -print0 \
 | awk -F/ -v RS='0円' -v i="${i##*/}" '{print"/dev/"$(NF-2)} END{if(!NR)print"/dev/"i}'
 # # alternative non-awk version:
 # j=$(find "$i"/*/subsystem/. -maxdepth 0 -samefile "$i"/subsystem/. -exec dirname '{}' \;)
 # sed -e 's,.*/,/dev/,' <<<"${j:-$i}"
 done))
 # Select only actual block devices that aren't already mounted
 # N.B. expansion of $allowed is unquoted, as wildcards are permitted
 devices=($(for i in ${allowed[*]} "${removable[@]}"; do test -b "$i" && echo "$i"; done | grep -vxF "$(cut -d' ' /proc/mounts -f1)"))
 test "${#devices[@]}" -gt 0 || return 0
 for i in "${devices[@]}"
 do search+=(-o -samefile "$i")
 done
 # alternative names - symlinks in /dev/disk/*/
 devices+=($(find -L /dev/disk -false "${search[@]}"))
 # all found names for mountable block devices, with and without initial /dev/
 COMPREPLY=($(compgen -W "$(printf '%q\n' "${devices[@]}" "${devices[@]#/dev/}")" -- "$cur"))
 fi
} &&
complete -F _pmount pmount

/usr/share/bash-completion/completions/pumount

_pumount() {
 # shellcheck disable=SC2034
 local cur prev words cword
 _init_completion || return
 if [[ "$cur" == -* ]]; then
 # transform "--help" output into completion list
 COMPREPLY=($(compgen -W "$(pumount --help | sed -e '/^..-/!d' -e 's/:.*//' -e 's/<[^>]*>//g' -e 'y/,/ /')" -- "$cur"))
 else
 local i mdir devices mounts search symlinks
 mdir=$(readlink -f /media)
 # shellcheck disable=SC2013
 for i in $(cut -d' ' -f1,2 /proc/mounts | grep -F " $mdir/")
 do
 # expand backslash escapes
 i=$(printf '%b' "$i")
 if test -b "$i"
 then
 search+=(-o -samefile "$i")
 devices+=("$i" "${i#/dev/}")
 elif test -d "$i"
 then
 mounts+=("$i" "${i#$mdir/}")
 fi
 done
 # alternative names - symlinks in /dev/disk/*/ and device or mountpoint basenames
 local IFS=$'\n'
 symlinks=($(find -L /dev/disk -false "${search[@]}"))
 COMPREPLY=($(compgen -W "$(printf '%q\n' "${mounts[@]}" "${devices[@]}" "${symlinks[@]}" "${symlinks[@]#/dev/}")" -- "$cur"))
 fi
} &&
complete -F _pumount pumount

---

Some specific questions:

• Should I be checking that commands such as modinfo actually exist? Which commands don't need checking? (I believe that grep and cut are in "Essential" packages, so can be assumed, but I'm less sure about readlink, for example).
• Is it guaranteed that /media is the only place to find the mountpoints?
• Can I make those long lines wrap nicely without hurting the clarity?

Answer 1

I don't really see room for groundbreaking improvements, so this review is going to be nitpicky, brace yourself!

Looping over pairs of values

This loops over multiple lines, with two values per line:

for i in $(cut -d' ' -f1,2 /proc/mounts | grep -F " $mdir/")

I think it would be much better to write this is a while read loop, reading into a pair of variables with descriptive names. That will make it easier to understand, and you will be able to get rid of the conditional processing in the loop body.

Which commands don't need checking

I don't know. I would use a very vanilla Linux installation as the benchmark. I would look around in docker run -it alpine. I just did, and I confirm that modinfo and readlink are both available, so I think it's safe to assume they are there.

Is it guaranteed that /media is the only place to find the mount points?

I don't think so. Why not consider all mount points of block devices?

Shortening long lines

As you yourself noticed, there are a few long lines that would be good to shorten somehow.

---

Here the path /lib/modules/$(uname -r)/kernel/fs is repeated, and the pipeline segment find ... -name "*.ko" -print0 | xargs -r -0 /sbin/modinfo is repeated:

COMPREPLY=($(grep "^[[:space:]]$cur" /proc/filesystems) $(find "/lib/modules/$(uname -r)/kernel/fs" -name "*.ko" -print0 | xargs -r -0 /sbin/modinfo | sed -ne 's/^alias: *fs-//p' | grep "^$cur"))
...
local encodings=(/sys/module/nls_* $(find "/lib/modules/$(uname -r)/kernel/fs/nls" -name '*.ko' -print0 | xargs -r -0 /sbin/modinfo | sed -ne 's/^\(name\|alias\): *//p'))

I would mitigate that with a variable and a helper function.

---

Here there are simply many parameters to pass that make the line long:

COMPREPLY=($(compgen -W "$(printf '%q\n' "${mounts[@]}" "${devices[@]}" "${symlinks[@]}" "${symlinks[@]#/dev/}")" -- "$cur"))

I would mitigate that by putting those arrays into another array with a descriptive name.

---

Here I don't really see why not write the loop on multiple lines:

devices=($(for i in ${allowed[*]} "${removable[@]}"; do test -b "$i" && echo "$i"; done | grep -vxF "$(cut -d' ' /proc/mounts -f1)"))

Variable naming

Most of the names are great, except i in all the loops. When iterating over elements instead of counting, I really prefer using a name that describes the current item.

Declaring local variables

In some places you declared variables long before they were actually used. I would delay the declaration as much as possible, and definitely combine with initialization when possible. That way I don't have to scan back in the code to see if the variable is local or not.

Indentation

The indentation is inconsistent, a mix of 3 or 4 spaces. I would use 4 consistently.

• Toby Speight: With regard to your first sentence - nitpicking is exactly what I was hoping for. Thanks for the review.
• Toby Speight: Re "Why not consider all mount points of block devices?", that's because we don't want to generate completions that will give pumount errors (because they weren't mounted using pmount). Perhaps this would work if constrained to ones mounted with user flag set?