Hi Guillem,

you asked me to review your next/d-m-h-root branch. Thanks to all who've
worked on this! I've looked at the two commits as one diff
(12961967a563..6aa3bf8f98b8) without attributing individual hunks to the
respective authors. This is what I found:

 * Diagnostic messages tend to include DPKG_ROOT. Is that a useful thing
   to do? My intuition was that I'd not include DPKG_ROOT there.

   For example:

       echo "Obsolete conffile ${DPKG_ROOT}$CONFFILE has been modified by you."

   Counter examples:

       echo "Restoring backup of $SYMLINK ..."
       echo "Restoring backup of $PATHNAME ..."

   At the very least, consistency seems in order here.

 * In dir_to_symlink, in the case for postrm, in the second (long)
   condition, I think there is a line that misses a trailing backslash.

       [ \( ! -h "${DPKG_ROOT}$PATHNAME" -a

 * In readlink_canon, in the final realpath the $dst argument likely
   should be quoted.

 * In readlink_canon I was wondering whether passing DPKG_ROOT inside a
   sed expression is a good idea. How about reworking the final line:

   -realpath $dst | sed -e "s:^$DPKG_ROOT::"
   +dst=$(realpath $dst)
   +echo ${dst#"$DPKG_ROOT"}

   The quoting of the variable removes any pattern meaning from its
   contents.

 * I observe that readlink -f and readlink_canon significantly differ in
   their behaviour. While readlink -f canonicalizes symbolic links in
   every component of the path, readlink_canon only canonicalizes
   symbolic links in the final component. It then uses realpath saying
   that no symlinks are involved, which may be untrue.

   I do wonder whether implementing this feature here is a useful thing
   to do. It seems that we need a chroot-aware readlink and/or realpath
   in more occasions. Possibly extending coreutils is a better solution?

   In any case, I'm attaching a version of readlink_canon that deals
   with this.

 * I observe that the coding style is inconsistent about using test
   expressions using -a/-o and &&/||. There is a slight reason to prefer
   the latter, see:

   https://github.com/koalaman/shellcheck/wiki/SC2166

That's all. Looks like we can go ahead with this with minor changes.

Helmut

Attachment: readlink_f.sh
Description: Bourne shell script

Reply via email to