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

[Dan McGee]

On Sun, May 23, 2010 at 1:25 PM, Andres P <aepd87 at gmail.com> 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 <aepd87 at gmail.com>

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