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

On 23/05/10 20:58, Andres P wrote:

Signed-off-by: Andres P ---

-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