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

v2: Instead of a monolithic function, split the logic into two for loops. Still less code repetition than master. Signed-off-by: Andres P --- On Sun, May 23, 2010 at 06:43:21PM +0200, Xavier Chantry wrote:

On Sun, May 23, 2010 at 2:20 PM, Allan McRae 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

On Sun, May 23, 2010 at 1:25 PM, Andres P wrote:

v2.1: Missed a space in a test clause

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

Signed-off-by: Andres P

See these three dashes right below here? Any comments you stick there won't end up in the git history, which is where 100% of the stuff above is not helpful one month from now. If you wouldn't mind resubmitting this with the iteration history *below* the dashes and something helpful *above*, that would be awesome. Thanks! -Dan P.S. The words "rep" and "bscript" are really confusing to a non-English speaker, let along me (who had to read the commit message a few times). Just make the title short and sweet (e.g. "makepkg: remove code duplication in buildscript parsing") and elaborate on the actual changes in the commit message.

---  scripts/makepkg.sh.in |   51 +++++++++++++-----------------------------------  1 files changed, 14 insertions(+), 37 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 1707245..4f1b852 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" +                               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