[arch-releng] [PATCH 1/3] Rename interactive_grub_menulst to interactive_bootloader_menu

[Dieter Plaetinck]

On Tue,  8 Feb 2011 20:52:49 -0500
pyther at pyther.net wrote:

> In addition to the rename, the function was updated to be flexible with
> the use of multiple bootloaders.
> ---
>  src/core/libs/lib-ui-interactive.sh |   25 +++++++++++++++++--------
>  1 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/src/core/libs/lib-ui-interactive.sh b/src/core/libs/lib-ui-interactive.sh
> index 45d3282..a2d5759 100644
> --- a/src/core/libs/lib-ui-interactive.sh
> +++ b/src/core/libs/lib-ui-interactive.sh
> @@ -886,7 +886,7 @@ interactive_grub() {
>  			fi
>  		fi
>          # Create and edit the grub menu.lst
> -        interactive_grub_menulst
> +        interactive_bootloader_menu "GRUB" $grubmenu 
>  
>  	DEVS="$(findblockdevices '_ ')"
>          if [ "$DEVS" = " " ]; then
> @@ -1073,13 +1073,22 @@ initrd $subdir/kernel26-fallback.img
>  EOF
>  }
>  
> -interactive_grub_menulst () {
> -	generate_grub_menulst
> -	helptext=
> -	grep -q '^/dev/mapper' $TMP_FSTAB && helptext="  /dev/mapper/ users: Pay attention to the kernel line!"
> -	notify "Before installing GRUB, you must review the configuration file.  You will now be put into the editor.  After you save your changes and exit the editor, you can install GRUB.$helptext"
> -	seteditor || return 1
> -	$EDITOR $grubmenu
> +interactive_bootloader_menu() {
> +	# $1 - Bootloader Name
> +	# $2 - Bootloader Configuration Files
> +
> +	if [[ $1 = GRUB ]]; then
> +		generate_grub_menulst
> +	fi
> +

this all seems very premature. you're introducing all kinds of indirections which are unneeded (now).
maybe you could do this when introducing syslinux support, but even then, having a separate function per bootloader will probably be preferable.

> +	grep -q '^/dev/mapper' $TMP_FSTAB && local helptext="  /dev/mapper/ users: Pay attention to the kernel line!"
> +	notify "Before installing $1, you must review the configuration file.  You will now be put into the editor.  After you save your changes and exit the editor, you can install $1.$helptext"
> +	
> +	if [[ -z $EDITOR ]]; then
> +		seteditor || return 1
> +	fi

why?? the original seteditor call was fine, afaict.

> +	
> +	$EDITOR $2
>  }
>  
>  interactive_grub_install () {

bottomline is: I don't see the usefulness of this patch and would leave it away entirely.  when you introduce syslinux support, we can still see what's the best way of doing things.

Dieter