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