[pacman-dev] [PATCH 2/3] makepkg: use extglob in place of regex

[Dave Reisner]

On Tue, Sep 06, 2011 at 05:23:06PM +1000, Allan McRae wrote:
> On 05/09/11 14:57, Dave Reisner wrote:
> >We seem to enjoy using bash regex capabilities, but never referencing
> >the result with BASH_REMATCH. Replace almost all regexes with equivalent
> >extglobs which are faster and functionally equivalent in these cases.
> >
> >Signed-off-by: Dave Reisner<dreisner at archlinux.org>
> 
> I have no objections to this.  So ack with minor comments below.
> 
> >---
> >The only remaining regexes are in lib{depends,provides} logic and I'm not
> >really comfortable touching those. There's a lot of cleanup that should
> >be done separately for that code where removal of those regexes can be
> >handled accordingly...
> 
> One day I will get to finishing my adjustments there...  honest!
> 
> >
> >  scripts/makepkg.sh.in |   16 +++++++++-------
> >  1 files changed, 9 insertions(+), 7 deletions(-)
> >
> >diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> >index 2f06b9b..75d168b 100644
> >--- a/scripts/makepkg.sh.in
> >+++ b/scripts/makepkg.sh.in
> >@@ -81,6 +81,8 @@ FORCE_VER=""
> >
> >  PACMAN_OPTS=
> >
> >+shopt -s extglob
> >+
> >  ### SUBROUTINES ###
> >
> >  plain() {
> >@@ -341,7 +343,7 @@ in_array() {
> >  source_has_signatures(){
> >  	local file
> >  	for file in "${source[@]}"; do
> >-		if [[ $file =~ \.(sig|asc)$ ]]; then
> >+		if [[ $file = *.@(sig|asc) ]]; then
> >  			return 0
> >  		fi
> >  	done
> >@@ -420,7 +422,7 @@ download_file() {
> >  run_pacman() {
> >  	local cmd
> >  	printf -v cmd "%q " "$PACMAN" $PACMAN_OPTS "$@"
> >-	if (( ! ASROOT ))&&  [[ ! $1 =~ ^-(T|Qq)$ ]]; then
> >+	if (( ! ASROOT ))&&  [[ ! $1 = -@(T|Qq) ]]; then
> >  		if type -p sudo>/dev/null; then
> >  			cmd="sudo $cmd"
> >  		else
> >@@ -709,7 +711,7 @@ check_pgpsigs() {
> >
> >  	for file in "${source[@]}"; do
> >  		file="$(get_filename "$file")"
> >-		if [[ ! $file =~ \.(sig|asc)$ ]]; then
> >+		if [[ ! $file = *.@(sig|asc) ]]; then
> >  			continue
> >  		fi
> >
> >@@ -1451,7 +1453,7 @@ check_sanity() {
> >  	awk -F'=' '/^[[:space:]]*pkgver=/ { $1=""; print $0 }' "$BUILDFILE" |
> >  	while read -r i; do
> >  		eval i=\"$(sed 's/^\(['\''"]\)\(.*\)\1$/\2/'<<<  "$i")\"
> >-		if [[ $i =~ [[:space:]:-] ]]; then
> >+		if [[ $i = *+([[:space:]:-])* ]]; then
> >  			error "$(gettext "%s is not allowed to contain colons, hyphens or whitespace.")" "pkgver"
> >  			return 1
> >  		fi
> >@@ -1460,7 +1462,7 @@ check_sanity() {
> >  	awk -F'=' '/^[[:space:]]*pkgrel=/ { $1=""; print $0 }' "$BUILDFILE" |
> >  	while read -r i; do
> >  		eval i=\"$(sed 's/^\(['\''"]\)\(.*\)\1$/\2/'<<<  "$i")\"
> >-		if [[ $i =~ [[:space:]-] ]]; then
> >+		if [[ $i = *+([[:space:]-])* ]]; then
> >  			error "$(gettext "%s is not allowed to contain hyphens or whitespace.")" "pkgrel"
> >  			return 1
> >  		fi
> >@@ -1469,7 +1471,7 @@ check_sanity() {
> >  	awk -F'=' '/^[[:space:]]*epoch=/ { $1=""; print $0 }' "$BUILDFILE" |
> >  	while read -r i; do
> >  		eval i=\"$(sed 's/^\(['\''"]\)\(.*\)\1$/\2/'<<<  "$i")\"
> >-		if [[ ! $i =~ ^[0-9]*$ ]]; then
> >+		if [[ $i&&  $i != +([0-9]) ]]; then
> 
> Do we need the test for $i there given it should be non-null for us
> not to exit the while loop?  Also, should we use [:digit:] rather
> than [0-9]?
> 

You're correct that the -n test can go. [0-9] and [:digit:] are
actually equivalent -- locale isn't an issue here, but I'll change it in
favor of consistency/safety.

> >  			error "$(gettext "%s must be an integer.")" "epoch"
> >  			return 1
> >  		fi
> >@@ -1526,7 +1528,7 @@ check_sanity() {
> >  		sed -e "s/optdepends=/optdepends_list+=/" -e "s/#.*//" -e 's/\\$//')
> >  	for i in "${optdepends_list[@]}"; do
> >  		local pkg=${i%%:*}
> >-		if [[ ! $pkg =~ ^[[:alnum:]\>\<\=\.\+\_\-]+$ ]]; then
> >+		if [[ $pkg != +([[:alnum:]><=.+_-]) ]]; then
> 
> Any chance you want to fix this for versioned optdepends containing
> an epoch at the same time?  The optdepends work requires a colon
> followed by a space before the description.  So I guess:
> 
> local pkg=${i%%: *}
> if [[ $pkg != +([[:alnum:]><=.+_-:]) ]]; then
> 
> should do that?
> 

Seems reasonable. We probably have some packages that don't fit this
criteria of having a colon followed by a whitespace and description
(e.g. extra/weechat). Are we going to add that to our sanity checks?

d

> >  			error "$(gettext "Invalid syntax for %s : '%s'")" "optdepend" "$i"
> >  			ret=1
> >  		fi
> 
>