[pacman-dev] Unusable pacman-key help by default

[Dan McGee]

On Mon, Mar 28, 2011 at 6:05 PM, Ivan c00kiemon5ter Kanak
<ivan.kanak at gmail.com> wrote:
> On 28 March 2011 20:27, Dan McGee <dpmcgee at gmail.com> wrote:
>
>> This is not cool at all, patches welcome from anyone. :)
>>
>> dmcgee at clifden ~/projects/pacman (master)
>> $ ./scripts/pacman-key --help
>> mkdir: cannot create directory `/etc/pacman.d/gnupg': Permission denied
>>
>>
> Hi all, I'm pretty new here, so I'm not sure how I should send this. I've
> read the submitting-patches page but there are lots of things to experiment
> with. So I thought I'd just send it as it is now and look more into how to
> submit patches tomorrow and maybe ask for some help on irc. This is actually
> the output of 'git show'.
git format-patch and git send-email are preferred (required?)-  this
should be in that document.

> also two comments:
> a. On line 227 we define a default location for the PACMAN_KEYRING_DIR . On
> lines 228-234 we read the command line options and set the
> PACMAN_KEYRING_DIR to the one specified by the appropriate switch. On lines
> 244-246 we read the configuration file and look for GPGDir variable which
> sets again the PACMAN_KEYRING_DIR.
> I find this behaviour a bit strange. I would expect the command line options
> to overide the configuration file settings. Is that a bug or a wanted
> behavior ?
Command line should always override config file, so this is a bug.
default < config file < command line.

This check also seems totally bogus:
    if [[ ! -r "${CONFIG}" ]]; then
Why should we care? We have a default anyway if the config file isn't there.

> b. I like that we use bash, and not restrict ourselves to sh. Is that valid
> for devtools too ?
Devtools isn't managed on this list, but as most of those scripts have
#!/bin/bash, draw your own conclusions there.
> Are we restricted to some bash version ? I've seen some
> of the devtools scripts and I think bash would make some things easier and
> cleaner.
Here we try to stay 3.2+ compatible; devtools are Arch-specific so
probably no such restriction.

> ---
>
> commit 4004b8e927705cab0e0d4fefcf7d04cf7630a2e7
> Author: Ivan Kanakarakis <ivan.kanak at gmail.com>
> Date:   Tue Mar 29 01:47:07 2011 +0300
>
>    This fixes the issue raised by Dan McGee, the wrong processing of -h,
>         --help switches. It ignores any other switch given if any of those
>         two are present. It also skips the root check. Why would one need
>         to be root to see the help message ?
>
>    Signed-off-by: Ivan Kanakarakis <ivan.kanak at gmail.com>
>
> diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in
> index 89e52fc..490dc39 100644
> --- a/scripts/pacman-key.sh.in
> +++ b/scripts/pacman-key.sh.in
> @@ -211,6 +211,14 @@ if ! type gettext &>/dev/null; then
>  }
>  fi
>
> +opts=($@)
> +for opt in ${opts[@]}; do
> +    if [[ $opt == "--help" || $opt == "-h" ]]; then
> +        usage
> +        exit 0
> +    fi
> +done

I hate this special casing. You also omitted -V/--version. And now
repo-add and this script do it in two completely different ways.

One case statement to parse all opts in both of these scripts should
be used if possible; delaying any necessary actions until the command
has been determined. If each of the add/update/etc. ops called a
"setup_gpg" method first or something that did the config file parsing
and ensuring the GPG directory exists, that would be best.

> +
>  if [[ $1 != "--version" && $1 != "-V" && $1 != "--help" && $1 != "-h" && $1
> != "" ]]; then
>  if type -p gpg >/dev/null 2>&1 = 1; then
>  error "$(gettext "gnupg does not seem to be installed.")"
> @@ -316,8 +324,6 @@ case "${command}" in
>  ${GPG_PACMAN} "$@" || ret=$?
>  exit $ret
>  ;;
> - -h|--help)
> - usage; exit 0 ;;
>  -V|--version)
>  version; exit 0 ;;
>  *)
>
>