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 <[email protected]> > Signed-off-by: Matthew Sexton <[email protected]> > --- > 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 >
