[pacman-dev] [PATCH 5/7] pacman-key: fix quotation on several variable assignments

[Dave Reisner]

On Fri, Jul 08, 2011 at 09:59:30PM +1000, Allan McRae wrote:
> From: Ivan Kanakarakis <ivan.kanak at gmail.com>
> 
> This commit adds quotes to several variable assignments. Unquoted values
> can cause problems on several occasions if the value is empty. It is
> safer to have every assignment quoted.

Perhaps this is just academic, but is there a test case for this? I'm
not aware of any situation where this it's necessary to quote to
assignment of a command sub or simple variable to a var to avoid any
oddities.

  $ foo= bar="" baz=${notavar}
  $ echo ${#foo} ${#bar} ${#baz}
  0 0 0

> Signed-off-by: Ivan Kanakarakis <ivan.kanak at gmail.com>
> ---
>  scripts/pacman-key.sh.in |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in
> index d75c111..b15a3a5 100644
> --- a/scripts/pacman-key.sh.in
> +++ b/scripts/pacman-key.sh.in
> @@ -137,12 +137,12 @@ reload_keyring() {
>  	if [[ -r "${REMOVED_KEYS}" ]]; then
>  		while read key; do
>  			local key_values name
> -			key_values=$(${GPG_PACMAN} --quiet --with-colons --list-key "${key}" | grep ^pub | cut -d: -f5,10 --output-delimiter=' ')
> +			key_values="$(${GPG_PACMAN} --quiet --with-colons --list-key "${key}" | grep ^pub | cut -d: -f5,10 --output-delimiter=' ')"
>  			if [[ -n $key_values ]]; then
>  				# The first word is the key_id
> -				key_id=${key_values%% *}
> +				key_id="${key_values%% *}"
>  				# the rest if the name of the owner
> -				name=${key_values#* }
> +				name="${key_values#* }"
>  				if [[ -n ${key_id} ]]; then
>  					# Mark this key to be deleted
>  					removed_ids[$key_id]="$name"
> @@ -152,12 +152,12 @@ reload_keyring() {
>  	fi
>  
>  	# List of keys that must be kept installed, even if in the list of keys to be removed
> -	local HOLD_KEYS=$(get_from "$CONFIG" "HoldKeys")
> +	local HOLD_KEYS="$(get_from "$CONFIG" "HoldKeys")"
>  
>  	# Remove the keys that must be kept from the set of keys that should be removed
>  	if [[ -n ${HOLD_KEYS} ]]; then
>  		for key in ${HOLD_KEYS}; do
> -			key_id=$(${GPG_PACMAN} --quiet --with-colons --list-key "${key}" | grep ^pub | cut -d: -f5)
> +			key_id="$(${GPG_PACMAN} --quiet --with-colons --list-key "${key}" | grep ^pub | cut -d: -f5)"
>  			if [[ -n "${removed_ids[$key_id]}" ]]; then
>  				unset removed_ids[$key_id]
>  			fi
> @@ -168,7 +168,7 @@ reload_keyring() {
>  	# be updated automatically.
>  	if [[ -r "${ADDED_KEYS}" ]]; then
>  		msg "$(gettext "Appending official keys...")"
> -		local add_keys=$(${GPG_NOKEYRING} --keyring "${ADDED_KEYS}" --with-colons --list-keys | grep ^pub | cut -d: -f5)
> +		local add_keys="$(${GPG_NOKEYRING} --keyring "${ADDED_KEYS}" --with-colons --list-keys | grep ^pub | cut -d: -f5)"
>  		for key_id in ${add_keys}; do
>  			# There is no point in adding a key that will be deleted right after
>  			if [[ -z "${removed_ids[$key_id]}" ]]; then
> @@ -179,7 +179,7 @@ reload_keyring() {
>  
>  	if [[ -r "${DEPRECATED_KEYS}" ]]; then
>  		msg "$(gettext "Appending deprecated keys...")"
> -		local add_keys=$(${GPG_NOKEYRING} --keyring "${DEPRECATED_KEYS}" --with-colons --list-keys | grep ^pub | cut -d: -f5)
> +		local add_keys="$(${GPG_NOKEYRING} --keyring "${DEPRECATED_KEYS}" --with-colons --list-keys | grep ^pub | cut -d: -f5)"
>  		for key_id in ${add_keys}; do
>  			# There is no point in adding a key that will be deleted right after
>  			if [[ -z "${removed_ids[$key_id]}" ]]; then
> @@ -264,7 +264,7 @@ if [[ ! -r "${CONFIG}" ]]; then
>  fi
>  
>  # Get GPGDIR from pacman.conf iff not specified on command line
> -if [[ -z PACMAN_KEYRING_DIR && GPGDIR=$(get_from "$CONFIG" "GPGDir") == 0 ]]; then
> +if [[ -z PACMAN_KEYRING_DIR && GPGDIR="$(get_from "$CONFIG" "GPGDir")" == 0 ]]; then

This doesn't pertain to this patch, but I don't understand this logic.
get_from should be writing the value of GPGDir as it's read from
$CONFIG. It looks like the goal here was to make sure that get_from was
successful, which would be written as:

  if [[ -z PACMAN_KEYRING_DIR ]] && GPGDIR=$(get_from "$CONFIG" "GPGDir"); then
    PACMAN_KEYRING_DIR=$GPGDIR
  fi
  PACMAN_KEYRING_DIR=${PACMAN_KEYRING_DIR:- at sysconfdir@/pacman.d/gnupg}

Or through a single default assignment to tidy the whole thing up:

  # if PACMAN_KEYRING_DIR isn't assigned, try to get it from the config
  # file, falling back on a hard default.
  : ${PACMAN_KEYRING_DIR:=$(get_from "$CONFIG" "GPGDir" || echo "@sysconfdir@/pacman.d/gnupg")}

Will happily write up a patch if this is what was actually intended...

d

>  	PACMAN_KEYRING_DIR="${GPGDIR}"
>  fi
>  PACMAN_KEYRING_DIR=${PACMAN_KEYRING_DIR:- at sysconfdir@/pacman.d/gnupg}
> -- 
> 1.7.6
> 
>