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

[Dave Reisner]

On Sat, Jul 09, 2011 at 07:13:33AM +1000, Allan McRae wrote:
> On 09/07/11 07:00, Dave Reisner wrote:
> >On Sat, Jul 09, 2011 at 06:47:34AM +1000, Allan McRae wrote:
> >>On 08/07/11 22:56, Dave Reisner wrote:
> >>>>@@ -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...
> >>>
> >>
> >>Send the patch.
> >>
> >>The key is just to have the value is assigned in this priority:
> >>1) --gpgdir value
> >>2) $CONFIF value
> >>3) default
> >>
> >>Allan
> >>
> >
> >I'll wait till this work is merged before sending, because I'm really
> >not sure what to base the patch on.
> >
> >Also noticing now that I have beef with get_from -- the comment says
> >that the equal sign _can_ be surrounded by random whitespace, but in
> >reality it _must_ be surrounded by whitespace or else the method will
> >fail to find anything.
> >
> 
> I'm not sure about the get_from function at all...  I really do not
> think it is very robust.  In fact, from memory I'm not sure an =
> sign is even actually checked for.
> 
> Allan
> 

It's not. I'm less concerned about the syntax checking because pacman
will do that for us. An entry such as 'GPGDir is /dev/null' will be
parsed as a valid entry by pacman-key, but pacman won't be happy about
it.

More concerning is the fact that 'key=val' isn't separated properly. We
should be setting IFS to '=' and doing whitespace trimming of the key
and value. Quickly...

get_from() {
  while IFS='=' read -r key value; do
    if [[ ${key%% *} = "$2" ]]; then
      echo "${value##* }"
      return 0
    fi
  done
  return 1
}

Amazingly, we have the 0/1 return value in place in the current function
(its magical). The break will imply a return of 0, and read will return
1 on EOF, forcing return of 1. Probably should make it explicit...

d