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.