[pacman-dev] [PATCH 1/2] Add support for verifying pgp signatures to makepkg

[Dan McGee]

On Fri, Jun 24, 2011 at 3:53 AM, Wieland Hoffmann
<themineo at googlemail.com> wrote:
> Hallo, Dan McGee:
>> On Thu, Jun 23, 2011 at 2:36 AM, Wieland Hoffmann
>> <themineo at googlemail.com> wrote:
>>
>> First, thanks for giving this a try.
>>
>> Commit descriptions are nice to have in permanent history, but all
>> that stuff you wrote in the cover letter won't show up. Can you
>> instead include some of that right here in the patch in commit
>> message-style writing?
>
> Will do (looks like I have to resubmit these anyway).
>
>> > +
>> > +       msg "$(gettext "Validating source files with gpg...")"
>> > +
>> > +       local file
>> > +       local errors=0
>> > +
>> > +       for file in "${pgpsigs[@]}"; do
>> > +               local valid
>> > +               local found=1
>> > +
>> > +               file="$(get_filename "$file")"
>> > +               echo -n "    ${file%.sig} ... " >&2
>> > +
>> > +               if ! file="$(get_filepath "$file")"; then
>> What are you doing here? Assignment or comparison? Do this in two
>> statements rather than trying to be cute, and use an actual [[ -z ]]
>> block please.
>
> That's just a copy of
> http://projects.archlinux.org/pacman.git/tree/scripts/makepkg.sh.in#n640
> get_filepath() already checks if the file exists.
Aha, OK. It's fine, it just is a bit hairy in what it is doing.

>
>> > +                       echo "$(gettext "NOT FOUND")" >&2
>> > +                       errors=1
>> > +                       found=0
>> > +               fi
>> > +
>> > +               if (( found )); then
>> > +                       if ! gpg --quiet --batch --verify "$file" 2> /dev/null; then
>> > +                               echo "$(gettext "Verification failed")" >&2
>> Any need to eat stderr? If things only show up in exceptional cases,
>> I'd rather it come through.
>
> Oh, that's supposed to be a 1.
I was suggesting no redirection at all.

>> This check should probably match how we do it elsewhere; e.g.,
>> check_signature() the first two lines.
>
> Hm, check_signature()? Either grep is lying to me or that doesn't exist
> anywhere.
>
>> I'd move it inside check_pgpsigs itself. Also declare it upfront where
>> we set the rest of these.

create_signature, whoops!