[pacman-dev] [PATCH 1/4] makepkg: extract parts of the write_pkginfo for use elsewhere

[Dave Reisner]

On Mon, Apr 17, 2017 at 10:03:00PM +1000, Allan McRae wrote:
> From: Levente Polyak <anthraxx at archlinux.org>
> 
> Signed-off-by: Allan McRae <allan at archlinux.org>
> ---

Sorry, a lot of these comments are irrelevant to the actual patch, but I
couldn't help pointing them out...

>  scripts/makepkg.sh.in | 42 ++++++++++++++++++++++++++----------------
>  1 file changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index 42a76004..d61c7fff 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -608,6 +608,15 @@ find_libprovides() {
>  	(( ${#libprovides[@]} )) && printf '%s\n' "${libprovides[@]}"
>  }
>  
> +get_packager() {
> +	if [[ -n $PACKAGER ]]; then
> +		local packager="$PACKAGER"
> +	else
> +		local packager="Unknown Packager"
> +	fi
> +	printf "%s\n" "$packager"

I was going to suggest that we simply make this:

  printf '%s\n' "${PACKAGER:-Unknown Packager}"

But then it occurred to me that if we just set this default value up
front, we don't need to treat this var as special...

Actually relevant to this patch, why not define this as
'write_kv_packager' to match other functions here, like
'write_kv_pkgname' and 'write_kv_pkgver'?

> +}
> +
>  write_kv_pair() {
>  	local key="$1"
>  	shift
> @@ -621,13 +630,22 @@ write_kv_pair() {
>  	done
>  }
>  
> -write_pkginfo() {
> -	if [[ -n $PACKAGER ]]; then
> -		local packager="$PACKAGER"
> -	else
> -		local packager="Unknown Packager"
> +write_kv_pkgname() {
> +	write_kv_pair "pkgname" "$pkgname"
> +	if (( SPLITPKG )) || [[ "$pkgbase" != "$pkgname" ]]; then
> +		write_kv_pair "pkgbase" "$pkgbase"
> +	fi

Wouldn't it be nice if we just *always* wrote the pkgbase?

> +}
> +
> +write_kv_pkgver() {
> +	local fullver=$(get_full_version)
> +	write_kv_pair "pkgver" "$fullver"
> +	if [[ "$fullver" != "$basever" ]]; then
> +		write_kv_pair "basever" "$basever"
>  	fi

Since 8a02abcf19, disallow pkgver overrides in package functions.
Therefore, I'm unclear on when we'd ever emit this basever attr.

> +}
>  
> +write_pkginfo() {
>  	local size="$(@DUPATH@ @DUFLAGS@)"
>  	size="$(( ${size%%[^0-9]*} * 1024 ))"
>  
> @@ -637,16 +655,8 @@ write_pkginfo() {
>  	printf "# Generated by makepkg %s\n" "$makepkg_version"
>  	printf "# using %s\n" "$(fakeroot -v)"
>  
> -	write_kv_pair "pkgname" "$pkgname"
> -	if (( SPLITPKG )) || [[ "$pkgbase" != "$pkgname" ]]; then
> -		write_kv_pair "pkgbase" "$pkgbase"
> -	fi
> -
> -	local fullver=$(get_full_version)
> -	write_kv_pair "pkgver" "$fullver"
> -	if [[ "$fullver" != "$basever" ]]; then
> -		write_kv_pair "basever" "$basever"
> -	fi
> +	write_kv_pkgname
> +	write_kv_pkgver
>  
>  	# TODO: all fields should have this treatment
>  	local spd="${pkgdesc//+([[:space:]])/ }"
> @@ -656,7 +666,7 @@ write_pkginfo() {
>  	write_kv_pair "pkgdesc" "$spd"
>  	write_kv_pair "url" "$url"
>  	write_kv_pair "builddate" "$SOURCE_DATE_EPOCH"
> -	write_kv_pair "packager" "$packager"
> +	write_kv_pair "packager" "$(get_packager)"
>  	write_kv_pair "size" "$size"
>  	write_kv_pair "arch" "$pkgarch"
>  
> -- 
> 2.12.0