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

[Dan McGee]

On Mon, Feb 16, 2009 at 3:32 AM, Sebastian Nowicki <sebnow at gmail.com> wrote:
>
> On 16/02/2009, at 5:24 PM, Xavier wrote:
>
>> On Mon, Feb 16, 2009 at 4:17 AM, Dan McGee <dpmcgee at gmail.com> wrote:
>>>
>>> On Mon, Feb 9, 2009 at 1:17 PM, Xavier <shiningxc at gmail.com> wrote:
>>>>
>>>> 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.
>>>
>>> It is actually way worse than 2x the number of forks- we now fork once
>>> per file *per line* in our .PKGINFO file, whereas before we only
>>> forked once per file.
>>>
>>> New patch incoming on the ML that should fork as many times as the old
>>> version, reducing that bottleneck a lot.
>>>
>>
>> Wow indeed, I always read the old version wrongly, I just realized the
>> old for was split on two lines :
>>>
>>> -       for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | \
>>> -               grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)|\1="\2"|'); do
>
> How about keeping that part, and just `read`ing the values in:
>
> for line in $(bsdtar -xOf "$pkgfile" .PKGINFO | \
>        grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)|\1 "\2"|'); do
>        echo $line | read var val
>        declare $var="$val"
>        # Rest is just as in the patch
> done
>
> I haven't actually tested it since the regex is somewhat faulty on Mac OS X
> for some reason, and I'm too lazy to boot Arch up :P.

When you do test it, I would put my money on it being broken the same
way the original one is. :)

> Note: The substitution now results in "pkgname foo" instead of
> "pkgname=foo".

This is more or less what I did in my revised patch, if you notice.
However, quote marks in the sed call itself are the whole crux of the
issue here, so we shouldn't have them there at all.

-Dan