On 5/11/19 5:36 am, Matthew Sexton wrote:

Added two new functions, lsigned_already() and revoked_already() that check whether a key has been locally signed or revoked respectively during --populate. If the key is already signed or revoked, it is quietly ignored.

It looks as though the implementation is fine at first glance. I also see this was discussed on the IRC channel with Dave and Eli, so I'm focusing on stylistic elements here. The function naming is more about what you are going to do with its output, rather than what the function does. I suggest: lsigned_already() -> key_is_lsigned() revoked_already() -> key_is_revoked() Also, you added the functions in alphabetically. But that is the collection of functions directly related to pacman-key options. Please move them up to where all the helper functions are at the top. Under check_keyids_exist() seems a logical place. Finally, the code uses tabs, not spaces for indents. You seem to have some mixture of the two.

Suggested-by: Eli Schwartz Signed-off-by: Matthew Sexton --- v2. (ACTUAL v2.) Previous email was erroneous, I attached the wrong patch. Corrected some inconsistencies. Gave proper attribution to Eli,

scripts/pacman-key.sh.in | 43 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in index 3627a805..25bcb3de 100644 --- a/scripts/pacman-key.sh.in +++ b/scripts/pacman-key.sh.in @@ -247,7 +247,7 @@ check_keyring() { fi fi

- if (( LSIGNKEY )); then + if (( LSIGNKEY || POPULATE )); then if [[ $(secret_keys_available) -lt 1 ]]; then error "$(gettext "There is no secret key available to sign with.")" msg "$(gettext "Use '%s' to generate a default secret key.")" "pacman-key --init" @@ -337,13 +337,18 @@ populate_keyring() { local key_count=0 msg "$(gettext "Disabling revoked keys in keyring...")" for key_id in "${!revoked_ids[@]}"; do + if revoked_already "$key_id" ; then + continue + fi if (( VERBOSE )); then msg2 "$(gettext "Disabling key %s...")" "${key_id}" fi printf 'disable\nquit\n' | LANG=C "${GPG_PACMAN[@]}" --command-fd 0 --quiet --batch --edit-key "${key_id}" 2>/dev/null key_count=$((key_count+1)) done - msg2 "$(gettext "Disabled %s keys.")" "${key_count}" + if (( key_count )); then + msg2 "$(gettext "Disabled %s keys.")" "${key_count}" + fi fi }

@@ -447,6 +452,22 @@ list_sigs() { exit 1 fi } +lsigned_already() { + # Determines whether a key has already been signed locally by getting the + # local pacman secret key and comparing it against signatures on the key + # returns 0 if key is signed, 1 if it is unsigned + secret_key=$("${GPG_PACMAN[@]}" --with-colons --list-secret-key | head -n1 | awk -F : '{print $5}') + while IFS=: read -r _ valid _ _ signkey _; do + if [[ "$valid" != "!" ]]; then

We don't quote the left hand side.

+ continue + fi + if [[ "$signkey" = "$secret_key" ]]; then + return 0 + fi + done < <("${GPG_PACMAN[@]}" --with-colons --check-signatures "$1") + return 1 + +}

lsign_keys() { check_keyids_exist @@ -454,6 +475,7 @@ lsign_keys() { local ret=0 local key_count=0 for key_id in "$@"; do + if lsigned_already "$key_id" ; then continue; fi

Put this over multiple lines.

if (( VERBOSE )); then msg2 "$(gettext "Locally signing key %s...")" "${key_id}" fi @@ -469,7 +491,9 @@ lsign_keys() { if (( ret )); then exit 1 fi - msg2 "$(gettext "Locally signed %s keys.")" "${key_count}" + if (( key_count )); then + msg2 "$(gettext "Locally signed %s keys.")" "${key_count}" + fi }

receive_keys() { @@ -511,6 +535,19 @@ refresh_keys() { fi }

+revoked_already() { + + while IFS=: read -r type _ _ _ _ _ _ _ _ _ _ flags _; do + if [[ "$type" != "pub" ]]; then + continue + fi + if [[ "$flags" = *"D"* ]]; then

That quoting on the RHS looked weird to me, but I think is fine...

+ return 0 + fi + done < <("${GPG_PACMAN[@]}" --with-colons --list-key "$1") + return 1 +} + verify_sig() { local ret=0 sig=$1 file=$2 if [[ -z $file && -f ${sig%.*} ]]; then

On Mon, 4 Nov 2019 at 19:36, Matthew Sexton wrote:

Added two new functions, lsigned_already() and revoked_already() that check whether a key has been locally signed or revoked respectively during --populate. If the key is already signed or revoked, it is quietly ignored.

Suggested-by: Eli Schwartz Signed-off-by: Matthew Sexton --- v2. (ACTUAL v2.) Previous email was erroneous, I attached the wrong patch. Corrected some inconsistencies. Gave proper attribution to Eli,

scripts/pacman-key.sh.in | 43 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in index 3627a805..25bcb3de 100644 --- a/scripts/pacman-key.sh.in +++ b/scripts/pacman-key.sh.in @@ -247,7 +247,7 @@ check_keyring() { fi fi

- if (( LSIGNKEY )); then + if (( LSIGNKEY || POPULATE )); then if [[ $(secret_keys_available) -lt 1 ]]; then error "$(gettext "There is no secret key available to sign with.")" msg "$(gettext "Use '%s' to generate a default secret key.")" "pacman-key --init" @@ -337,13 +337,18 @@ populate_keyring() { local key_count=0 msg "$(gettext "Disabling revoked keys in keyring...")" for key_id in "${!revoked_ids[@]}"; do + if revoked_already "$key_id" ; then + continue + fi if (( VERBOSE )); then msg2 "$(gettext "Disabling key %s...")" "${key_id}" fi printf 'disable\nquit\n' | LANG=C "${GPG_PACMAN[@]}" --command-fd 0 --quiet --batch --edit-key "${key_id}" 2>/dev/null key_count=$((key_count+1)) done - msg2 "$(gettext "Disabled %s keys.")" "${key_count}" + if (( key_count )); then + msg2 "$(gettext "Disabled %s keys.")" "${key_count}" + fi fi }

@@ -447,6 +452,22 @@ list_sigs() { exit 1 fi } +lsigned_already() { + # Determines whether a key has already been signed locally by getting the + # local pacman secret key and comparing it against signatures on the key + # returns 0 if key is signed, 1 if it is unsigned + secret_key=$("${GPG_PACMAN[@]}" --with-colons --list-secret-key | head -n1 | awk -F : '{print $5}') + while IFS=: read -r _ valid _ _ signkey _; do + if [[ "$valid" != "!" ]]; then + continue + fi + if [[ "$signkey" = "$secret_key" ]]; then + return 0 + fi + done < <("${GPG_PACMAN[@]}" --with-colons --check-signatures "$1") + return 1 + +}

lsign_keys() { check_keyids_exist @@ -454,6 +475,7 @@ lsign_keys() { local ret=0 local key_count=0 for key_id in "$@"; do + if lsigned_already "$key_id" ; then continue; fi if (( VERBOSE )); then msg2 "$(gettext "Locally signing key %s...")" "${key_id}" fi @@ -469,7 +491,9 @@ lsign_keys() { if (( ret )); then exit 1 fi - msg2 "$(gettext "Locally signed %s keys.")" "${key_count}" + if (( key_count )); then + msg2 "$(gettext "Locally signed %s keys.")" "${key_count}" + fi }

receive_keys() { @@ -511,6 +535,19 @@ refresh_keys() { fi }

+revoked_already() { + + while IFS=: read -r type _ _ _ _ _ _ _ _ _ _ flags _; do + if [[ "$type" != "pub" ]]; then + continue + fi + if [[ "$flags" = *"D"* ]]; then + return 0 + fi + done < <("${GPG_PACMAN[@]}" --with-colons --list-key "$1") + return 1 +} + verify_sig() { local ret=0 sig=$1 file=$2 if [[ -z $file && -f ${sig%.*} ]]; then -- 2.23.0

This may just be my personal preference. But "pacman-key: ignores keys already lsigned/deleted" kind of sounds past tense, almost like pacman already does this and you're reporting a bug. I much prefer just saying "pacman-key: ignore already signed/deleted keys".