On Monday 26 July 2010 18:46:27 Dan McGee wrote:
+# gettext initialization +export TEXTDOMAIN='pacman' +export TEXTDOMAINDIR='@localedir@'
You don't end up actually using gettext anywhere. A script like this actually should.
Yes, totally forgot. Done.
+ +# Based on apt-key, from Debian +# Author: Denis A. Altoé Falqueto <denisfalqueto at gmail dot com>
These quickly become outdated- I'm fine with the copyright, but if you want this you also need to fix every single problem that comes up with the script. git does a perfectly fine job at telling you who the original author is.
I'm fine with handing off the copyright to Pacman Developers too, if you don't mind. i didn't do it at first because i'm not a pacman developer, so didn't to put me in a position I'm not. I'm not applying for it, this is just an occasional help :)
+PACMAN_SHARE_DIR="/usr/share/pacman"
This needs to be autoconf-ed. See doc/Makefile.am and pkgdatadir and use that like @sysconfdir@ in makepkg.sh.in.
Done. Corrected to @pkgdatadir@, which is in Makefile.am, in source root.
+ +# Default parameters for the command gpg. Some more will be added when needed +GPG="gpg" +GPG_NOKEYRING="${GPG} --ignore-time-conflict --no-options --no-default-keyring" +SIG_EXT=".sig"
Would we ever change this? I'd rather have .sig hardcoded just like we killed other things that will never really change because they are convention outside of just this package.
Yes, I was going to discuss in that other reply. In fact, the variables are there not because they can change. I just don't feel comfortable to use strings literals that mean something to the script. If I mistype them in one place and nobody sees it, it is a fool bug that slips off. That way, bash will make sure that the right values are on the right places.
+usage() { + echo "pacman-key - Pacman's keyring management utility" + echo "Usage: $(basename $0) [options] command [arguments]" + echo + echo "Manage pacman's list of trusted keys" + echo + echo "Options must be placed before commands. The abailable options are:" + echo " --config - set an alternative configuraton file to use. " + echo " Default is @sysconfdir@/pacman.conf" + echo " --gpgdir - set an alternativ home directory for gnupg. " + echo " Default is @sysconfdir@/pacman.d/gnupg" + echo + echo "The available commands are:" + echo " pacman-key add <file> ... - add the key contained in <file> ('-' for stdin)" + echo " pacman-key del <keyid> ... - remove the key <keyid>" + echo " pacman-key export <keyid> ... - output the key <keyid>" + echo " pacman-key exportall - output all trusted keys" + echo " pacman-key receive <keyserver> <keyid> ... - fetch the keyids from the specified keyserver URL" + echo " pacman-key trust <keyid> ... - set the truslevel of the given key" + echo " pacman-key updatedb - update the trustdb of pacman" + echo " pacman-key reload - reloads the keys from the keyring package" + echo " pacman-key list - list keys" + echo " pacman-key finger <keyid> ... - list fingerprints" + echo " pacman-key adv <params> - pass advanced options to gpg" + echo " pacman-key help - displays this message" + echo " pacman-key version - displays the current version" +}
I forgot we did this in makepkg; I don't really like it there. This stuff seems like it belongs in a manpage with a shorter usage() such as the one in repo-add. Anyone else have thoughts?
Either way, for a essential tool, we will need a manpage.
The man page will be done for sure, I just wanted to share my work soon :) About the usage message, I don't think it is too long. It just gives a quick explanation. But I can shorten it, stripping the details of each command.
+ +prepare_homedir() { + if [[ ! -d ${PACMAN_KEYRING_DIR} ]] ; then + mkdir -p "${PACMAN_KEYRING_DIR}" + [[ ! -f "${PACMAN_KEYRING_DIR}/secring.gpg" ]] && touch "${PACMAN_KEYRING_DIR}/secring.gpg" + [[ ! -f "${PACMAN_KEYRING_DIR}/pubring.gpg" ]] && touch "${PACMAN_KEYRING_DIR}/pubring.gpg"
If the dir didn't exist, why this -f check?
Yes, you're right. Corrected.
+ chmod 700 "${PACMAN_KEYRING_DIR}" + chmod 600 "${PACMAN_KEYRING_DIR}"/*
Why the glob? Can't we just explicitly list the two files previously touched? Minor nitpick but seems more sensible to me.
Done too.
+ fi +} + +add_key() { + ${GPG_PACMAN} --quiet --batch --import "$1" +} + +remove_key() { + ${GPG_PACMAN} --quiet --batch --delete-key --yes "$1" +} + +update_trustdb() { + ${GPG_PACMAN} --batch --check-trustdb +} + +list_sigs() { + ${GPG_PACMAN} --batch --list-sigs +} + +list_fingerprints() { + ${GPG_PACMAN} --batch --fingerprint $* +} + +export_key() { + ${GPG_PACMAN} --armor --export "$1" +} + +export_all() { + ${GPG_PACMAN} --armor --export +}
If these are all one-liners, it would seem to make more sense to inline them if they are only called from one place. Ignore me if they are not.
I did that way because of homogeneity. I moved them for the calling places. just update_trustdb can be called from two places, so it stayed.
+ +trust_key() { + # Verify if the key exists in pacman's keyring + if ${GPG_PACMAN} --list-key "$1" > /dev/null 2>&1 ; then + ${GPG_PACMAN} --edit-key "$1" + else + echo >&2 "The key identified by $1 doesn't exist"
I'd rather find a way to reuse msg/msg2/warning/error from repo-add then do this. We might want to move those into a sourced common utilities file.
Right. When the decision about the file is made, I'll change it.
+ if [[ -f ${REMOVED_KEYS} ]] && ! ${GPG_PACMAN} --quiet --verify ${REMOVED_KEYS}${SIG_EXT} ; then
You quoted paths elsewhere but then didn't quote here or many of the following places.
Yes, I corrected the quotes in the other places. If there's still some missing, please let me know.
+if [[ ! -r "$CONFIG" ]] ; then + echo >&2 "It is not possible to read $CONFIG." + exit 1 +fi +# Read GPGDIR from $CONFIG +GPGDIR=$(grep "^ *GPGDir *=" "$CONFIG" | cut -d= -f2) +if [[ "$GPGDIR" ]] ; then + PACMAN_KEYRING_DIR="$GPGDIR" +fi
Hmm, this is kinda ugly but not sure what to suggest just yet.
Agreed :) But pacman.conf is a kind of ini file, so the correct parsing would give too much trouble. Looking now, it doesn't work if there is a tab before GPGDir... I'll fix it tomorow.
+ --help) + usage + ;; + --version) + echo "pacman-key v${PACMAN_KEY_VERSION}" + echo " This program can be freely distributed under the GPL v2" + ;;
Please use a similar version() function to makepkg/repo-add.
You also have help/version above, and --help/--version here.
Done.
+ *) + usage + exit 1 + ;; +esac -- 1.7.1.1
General thoughts: 1. We don't use the non-dashed option type commands in any other utility. I'm not necessarily against it but thought it was worth pointing out.
Agreed. I already changed it to short and long options. I just got influenced by apt-key, because they always use non-dashed commands, as in apt-get install, etc.
2. A lot of times our style in shell scripts has tended towards omitting braces, e.g. ${foobar} would be just $foobar if possible.
I think I've seen some patches in pacman-dev changing variables that way (${} insttead of just $). But I can change it too, no big deal.
3. This was a static code analysis, I need to actually run this and give it a shot. 4. Don't get discouraged at my review here, I'm just trying to make sure when we introduce something new we don't have to spend the next few iterations making it a solid core piece of code. I'd rather get it right the first time.
Don't worry about that. We both want the same goal. I'll try to send the new patch in reply to this email, as soon as I learn how to do it with git send-email, but i'll leave that for tomorrow. 3 am here and I'm barely able to write this message :) -- A: Because it obfuscates the reading. Q: Why is top posting so bad? ------------------------------------------- Denis A. Altoe Falqueto -------------------------------------------