Any intentions of fixing this in 4.0.1? I'd like to release that in about a week if possible.

I doubt I will get a chance to look at this before the upcoming weekend, so that would be pushing it for 4.0.1...

I put together a patch series that appears to result in pacman-key exiting with the correct return codes. I'm not really 100% familiar with the development process, but I've attached the patches here and maybe you will find them useful.

Dave/Allan, if one of you has a chance to review these at some point it would be much appreciated.

A quick look at these tells me this:
0001: this might be nice but is it necessary? Especially if we want to get this in a maint release it doesn't help one bit as it adds a lot of churn without benefit.
0002: seems pretty straightforward, although the "then/fi" coding style matches nothing else we do anywhere.
0003: if one function uses "check" in the name, don't use "chk" in your new function. Also, in add/delete, why don't we just omit the --quiet flag rather than coming up with all new messages of our own? s/signiture/signature/ was one typo I noticed.

0001:
- I'll leave this to Dan to decide on.

0002:
- I do not understand the need for all these type of changes:

-(( ! INIT )) && check_keyring
+if (( ! INIT )); then check_keyring; fi;

- The indentation is all over the place (spaces/tabs)
- rename sign_keys -> lsign_keys

0003:
- keep the --quiet calls as they do not hide error messages. We may want to remove some 2>/dev/null to keep other gpg errors shown.

- in import/import_trustdb, do not exit on first missing file, but rather report error and continue until end of loop. (such as is done in chk_key_exists function)

- in those two functions $importdir is used in the error message but will be the last file in the loop and not necessarily the one with the error...

@Dan, Allan

Thanks for taking the time to look at my patches. I've made some updates with your suggestions.

0001:
- This patch really isn't necessary. It probably isn't the best thing to do for a maintenance release so I've dropped it from this series.

0002:
- Any still present "then/fi" should match general coding style.
- The reason I changed everything to conditionals in the "program start" section was due to worry about the fact that arithmetic expansions can return non-zero statuses. I thought this would muck up the proper return codes in the cases where updatedb isn't run since that would be the last expansion tested. (( UPDATEDB )) would return non-zero in those cases, thereby making the whole program return status non-zero. But as long as that last expansion is a "if/then" this should not happen. I've dropped all of the other conditionals I added.
- Wild indentation should be corrected now.

0003:
- Changed chk_key_exists function name to check_key_exists.
- Kept --quiet as this apparently doesn't effect error reporting.
- I'll leave 2>/dev/null changes up to you, I kept that in all commands that used that prior to my changes.
- Embarrassing typos should now be fixed.
- import/import_trustdb continue with an error until the loop ends, I mirrored this change in edit_key, hopefully that isn't too presumptuous.
- Any errors referring to "$importdir" should now be the actual error and not the last one in the loop.

You get stream of consciousness as a review from me because I'm a bit short of time. Sorry about that ;)

06:08 falconindy » first one i definitely agree with
06:10 falconindy » second one is silly
06:10 falconindy » we don't care about the actual return code, just that it's non-zero
06:10 falconindy » if ! COMMANDS; then whine; exit; fi
06:11 falconindy » well, that's true of some of them
06:11 falconindy » the ones that loop need to continue to the end before exiting
06:12 falconindy » in lsign_keys, we should be using (( PIPESTATUS[0] ))
06:12 falconindy » er... that assumes he's looking for a SIGPIPE
06:13 falconindy » if he's looking for the exit of gpg, it's broken and should be (( PIPESTATUS[1] ))

@Dave

Oh! So if the specific return code doesn't matter and all you care about is if it is non-zero, you want the commands to use the logical negation (!) operator? That would make everything a lot cleaner.

I was testing (( PIPESTATUS )) in the lsign_keys function because that should return non-zero if any part of the pipe failed, no? Although if printf fails in the pipe, it isn't that important, the only thing that matters is whether the gpg command succeeded in (( PIPESTATUS[1] )). In the import_trustdb function, are you okay with (( PIPESTATUS )) since both parts of the pipe are important?

Thanks for taking the time to look. I've attached updated patches that are hopefully closer to what everyone wants.

Great, this looks much better. However:

check_key_exists => check_keyids_exist : more accurate description of what the function does.

in export_keys and finger_keys : we know the name of the key that failed finger/export, so we should point it out in the error message.

The --list-keys function will almost never throw that error, and when it does, it won't be quite accurate. gpg won't return non-zero unless _all_ keyids passes are invalid. This applies to --fingerprint as well, and --edit-key doesn't really care. Sigh. Part of the problem with this bug report is that gpg sucks, therefore pacman-key sucks.

> we know the name of the key that failed finger/export, so we should point it out in the error message.
I am missing how we know that? Unless we loop and do one gpg call per key...

Lets just ignore the stupidity of gpg return codes on partial failures. The check_keyids_exist function should catch those errors before calling --fingerprint etc. If that check is added to the list_keys and lsign_key, I will ack the patches.

And add an explicit "exit 0" at the end and then you do not have to worry about the "if" call for the (( UPDATEDB )).

I've changed the new function name to check_keyids_exist, added a call to it in list_keys and lsign_keys, and added an explicit exit 0.

I don't think you can get the problem key name in export_keys and finger_keys without a loop. Whether looping is worth it is probably a discussion for another time so I've left it as is.

These look good enough for me. Pulled to my working branch.

Thanks for all your revisions.