[pacman-dev] [PATCH] makepkg: bad code rep in bscript parsing

[Andres P]

v2:
Instead of a monolithic function, split the logic into two for loops.
Still less code repetition than master.

Signed-off-by: Andres P <aepd87 at gmail.com>
---

On Sun, May 23, 2010 at 06:43:21PM +0200, Xavier Chantry wrote:
> On Sun, May 23, 2010 at 2:20 PM, Allan McRae <allan at archlinux.org> wrote:
> >
> > -1 from me. Â The new function does two things that are completely unrelated
> > apart from the regex they use. Â It makes no sense to combine them. Â Also the
> > function name does not reflect what it does at all.
> >
> > There is a limit to how much code refactoring is useful. Â It needs to be
> > balanced by readability.
> >
> 
> There is a double combination in that patch.
> IMO one of them is fine, the one that combines install and changelog
> in a for loop.
> And that combination could be done in the two places, but keeping them separate.
> 

I agree, it's now split

In case new fields get added, wether it be for splitdbg or a new zomg
feature, it's important to abstract this into a list than can be easily
amended.
...

 scripts/makepkg.sh.in |   53 +++++++++++++-----------------------------------
 1 files changed, 15 insertions(+), 38 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index 1707245..9920aba 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -1107,31 +1107,19 @@ create_srcpackage() {
 		fi
 	done
 
-	local install_files=( $(grep "^[[:space:]]*install=" "$BUILDSCRIPT" | sed "s/install=//") )
-	if [[ -n $install_files ]]; then
-		local file
-		for file in ${install_files[@]}; do
-		# evaluate any bash variables used
-		eval file=${file}
-			if [[ ! -f "${srclinks}/${pkgbase}/$file" ]]; then
-				msg2 "$(gettext "Adding install script (%s)...")" "$file"
-				ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/"
-			fi
-		done
-	fi
-
-	local changelog_files=( $(grep "^[[:space:]]*changelog=" "$BUILDSCRIPT" | sed "s/changelog=//") )
-	if [[ -n $changelog_files ]]; then
+	local i
+	for i in 'changelog' 'install'; do
+		local $i=$(sed -n "s,^\([[:space:]]*\)$i=,\1,p" "$BUILDSCRIPT")
 		local file
-		for file in ${changelog_files[@]}; do
+		for file in ${!i}; do
 			# evaluate any bash variables used
-			eval file=${file}
-			if [[ ! -f "${srclinks}/${pkgbase}/$file" ]]; then
-				msg2 "$(gettext "Adding package changelog (%s)...")" "$file"
+			eval file='${srclinks}/${pkgbase}/'$file
+			if [[ ! -f $file ]]; then
+				msg2 "$(gettext "Adding %s file (%s)...")" "$i" "$file"
 				ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/"
 			fi
 		done
-	fi
+	done
 
 	local TAR_OPT
 	case "$SRCEXT" in
@@ -1250,30 +1238,19 @@ check_sanity() {
 		fi
 	done
 
-	local install_files=( $(grep "^[[:space:]]*install=" "$BUILDSCRIPT" | sed "s/install=//") )
-	if [[ -n $install_files ]]; then
+	local i
+	for i in 'changelog' 'install'; do
+		local $i=$(sed -n "s,^\([[:space:]]*\)$i=,\1,p" "$BUILDSCRIPT")
 		local file
-		for file in ${install_files[@]}; do
-		# evaluate any bash variables used
-		eval file=${file}
-			if [[ ! -f $file ]]; then
-				error "$(gettext "Install scriptlet (%s) does not exist.")" "$file"
-				return 1
-			fi
-		done
-	fi
-
-	local changelog_files=( $(grep "^[[:space:]]*changelog=" "$BUILDSCRIPT" | sed "s/changelog=//") )
-	if [[ -n $changelog_files ]]; then
-		for file in ${changelog_files[@]}; do
+		for file in ${!i}; do
 			# evaluate any bash variables used
 			eval file=${file}
-			if [[ ! -f $file ]]; then
-				error "$(gettext "Changelog file (%s) does not exist.")" "$file"
+			if [[ !-f $file ]]; then
+				error "$(gettext "%s file (%s) does not exist.")" "${i^}" "$file"
 				return 1
 			fi
 		done
-	fi
+	done
 
 	local valid_options=1
 	local opt known kopt
-- 
1.7.1