v2: Instead of a monolithic function, split the logic into two for loops. Still less code repetition than master.
Signed-off-by: Andres P <[email protected]> --- On Sun, May 23, 2010 at 06:43:21PM +0200, Xavier Chantry wrote: > On Sun, May 23, 2010 at 2:20 PM, Allan McRae <[email protected]> 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_fil...@]}; 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_fil...@]}; 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_fil...@]}; 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_fil...@]}; 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
