[pacman-dev] Fwd: [PATCH] repo-add: fix eval and quote issues

[Aaron Griffin]

On Mon, Feb 9, 2009 at 1:17 PM, Xavier <shiningxc at gmail.com> wrote:
> On Sun, Feb 8, 2009 at 7:31 PM, Dan McGee <dan at archlinux.org> wrote:
>> Yes, I think I sent this to myself on accident. Looks like it is time
>> for another cup of coffee.
>>
>> ---------- Forwarded message ----------
>> From: Dan McGee <dan at archlinux.org>
>> Date: Sun, Feb 8, 2009 at 12:30 PM
>> Subject: Re: [PATCH] repo-add: fix eval and quote issues
>> To: Dan McGee <dan at archlinux.org>
>>
>> On Sun, Feb 8, 2009 at 12:22 PM, Dan McGee <dan at archlinux.org> wrote:
>>> eval was ugly and dirty, and bit us here. Instead, use a safer form of
>>> variable declaration to ensure quotes don't foil us in pkgdesc or any other
>>> fields.
>>>
>>> This fixes FS#10837.
>>>
>>> Signed-off-by: Dan McGee <dan at archlinux.org>
>>> ---
>> We apparently had never handled quotes in descriptions correctly, so
>> this patch should fix these issues. I'm going to push this patch to
>> maint if there are no objections, although I expect our next release
>> will come from master.
>>
>>>  scripts/repo-add.sh.in |   25 +++++++++++++------------
>>>  1 files changed, 13 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in
>>> index be0859e..93fdd52 100644
>>> --- a/scripts/repo-add.sh.in
>>> +++ b/scripts/repo-add.sh.in
>>> @@ -152,18 +152,19 @@ db_write_entry()
>>>
>>>        # read info from the zipped package
>>>        local line
>> I did make one small fix here- added var and val to the local declaration.
>>> -       for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | \
>>> -               grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)|\1="\2"|'); do
>>> -               eval "$line"
>>> -               case "$line" in
>>> -                       group=*)    _groups="$_groups$group\n" ;;
>>> -                       depend=*)   _depends="$_depends$depend\n" ;;
>>> -                       backup=*)   _backups="$_backups$backup\n" ;;
>>> -                       license=*)  _licenses="$_licenses$license\n" ;;
>>> -                       replaces=*) _replaces="$_replaces$replaces\n" ;;
>>> -                       provides=*) _provides="$_provides$provides\n" ;;
>>> -                       conflict=*) _conflicts="$_conflicts$conflict\n" ;;
>>> -                       optdepend=*) _optdepends="$_optdepends$optdepend\n" ;;
>>> +       for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | grep -v '^#'); do
>>> +               var="$(echo $line | sed 's|\(\w*\)\s*=\s*\(.*\)|\1|')"
>>> +               val="$(echo $line | sed 's|\(\w*\)\s*=\s*\(.*\)|\2|')"
>>> +               declare $var="$val"
>>> +               case "$var" in
>>> +                       group)    _groups="$_groups$group\n" ;;
>>> +                       depend)   _depends="$_depends$depend\n" ;;
>>> +                       backup)   _backups="$_backups$backup\n" ;;
>>> +                       license)  _licenses="$_licenses$license\n" ;;
>>> +                       replaces) _replaces="$_replaces$replaces\n" ;;
>>> +                       provides) _provides="$_provides$provides\n" ;;
>>> +                       conflict) _conflicts="$_conflicts$conflict\n" ;;
>>> +                       optdepend) _optdepends="$_optdepends$optdepend\n" ;;
>>>                esac
>>>        done
>>>
>
> For testing, I added all the packages in my cache to a database with
> and without this patch, and the resulting databases are identical so
> we should be safe.
> However, we have twice as many forks as before now, so the time went
> from 1:36 to 3:14 (for adding 493 packages).
> I don't know if we should care or not, I guess we usually add a small
> numbers of packages. If we care, we should have a look at all the
> slowest operations in repo-add.

The reason I used eval originally was to catch the
pkgver=${_somever/-/_} type stuff. Does using declare still work that
way?

Additionally, regarding what Xavier said: why not keep the loop
expression as is and use "declare $line" ?