On Mon, Feb 9, 2009 at 1:17 PM, Xavier <shiningxc@gmail.com> wrote:
On Sun, Feb 8, 2009 at 7:31 PM, Dan McGee <dan@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@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@archlinux.org>
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@archlinux.org> --- We apparently had never handled quotes in descriptions correctly, so
On Sun, Feb 8, 2009 at 12:22 PM, Dan McGee <dan@archlinux.org> wrote: 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" ?