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

[Allan McRae]

On 23/05/10 20:58, Andres P wrote:
> Signed-off-by: Andres P<aepd87 at gmail.com>
> ---

-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.


>   scripts/makepkg.sh.in |   75 +++++++++++++++++--------------------------------
>   1 files changed, 26 insertions(+), 49 deletions(-)
>
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 1707245..00b96a9 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -1070,6 +1070,30 @@ create_package() {
>   	fi
>   }
>
> +parse_buildscript() {
> +	local i
> +	for i in 'changelog' 'install'; do
> +		local $i=$(sed -n "s,^\([[:space:]]*\)$i=,\1,p" "$BUILDSCRIPT")
> +		local file
> +		for file in ${!i}; do
> +			# evaluate any bash variables used
> +			eval file=${file}
> +
> +			[[ $1 ]] || file=${srclinks}/${pkgbase}/$file
> +			[[ -f $file ]]&&  return
> +
> +			if [[ $1 ]]; then
> +				error "$(gettext "${i^} file (%s) does not exist.")" "$file"
> +				return 1
> +			else
> +				msg2 "$(gettext "Adding $i file (%s)...")" "$file"
> +				ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/"
> +			fi
> +		done
> +	done
> +}
> +
> +
>   create_srcpackage() {
>   	cd "$startdir"
>
> @@ -1107,31 +1131,7 @@ 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 file
> -		for file in ${changelog_files[@]}; do
> -			# evaluate any bash variables used
> -			eval file=${file}
> -			if [[ ! -f "${srclinks}/${pkgbase}/$file" ]]; then
> -				msg2 "$(gettext "Adding package changelog (%s)...")" "$file"
> -				ln -s "${startdir}/$file" "${srclinks}/${pkgbase}/"
> -			fi
> -		done
> -	fi
> +	parse_buildscript
>
>   	local TAR_OPT
>   	case "$SRCEXT" in
> @@ -1250,30 +1250,7 @@ check_sanity() {
>   		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 $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
> -			# evaluate any bash variables used
> -			eval file=${file}
> -			if [[ ! -f $file ]]; then
> -				error "$(gettext "Changelog file (%s) does not exist.")" "$file"
> -				return 1
> -			fi
> -		done
> -	fi
> +	parse_buildscript check
>
>   	local valid_options=1
>   	local opt known kopt