Re: [PATCH] Fix bad test on GRUB_DISABLE_SUBMENU.

[Daniel Kiper]

On Thu, Sep 19, 2019 at 02:42:05PM +0200, Javier Martinez Canillas wrote:
> Hello Daniel,
>
> On 9/18/19 2:53 PM, Daniel Kiper wrote:
> > On Tue, Sep 17, 2019 at 05:52:10PM +0200, Javier Martinez Canillas wrote:
> >> From: Prarit Bhargava <address@hidden>
> >>
> >> The file /etc/grub.d/10_linux does
> >>
> >> if [ "x$is_top_level" = xtrue ] && [ "x${GRUB_DISABLE_SUBMENU}" != xy ]; 
> >> then
> >>
> >> when it should do
> >>
> >> if [ "x$is_top_level" = xtrue ] && [ "x${GRUB_DISABLE_SUBMENU}" != xtrue 
> >> ]; then
> >>
> >> which results in submenus in /boot/grub2/grub.cfg when
> >> GRUB_DISABLE_SUBMENU="yes".
> >>
> >> Resolves: rhbz#1063414
> >>
> >> Signed-off-by: Prarit Bhargava <address@hidden>
> >> Signed-off-by: Javier Martinez Canillas <address@hidden>
> >> ---
> >>
> >>  util/grub.d/10_linux.in | 6 +++++-
> >>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/util/grub.d/10_linux.in b/util/grub.d/10_linux.in
> >> index 4532266be68..58defdbd83f 100644
> >> --- a/util/grub.d/10_linux.in
> >> +++ b/util/grub.d/10_linux.in
> >> @@ -261,7 +261,11 @@ while [ "x$list" != "x" ] ; do
> >>      fi
> >>    fi
> >>
> >> -  if [ "x$is_top_level" = xtrue ] && [ "x${GRUB_DISABLE_SUBMENU}" != xy 
> >> ]; then
> >> +  if [ "x${GRUB_DISABLE_SUBMENU}" = "xyes" ] || [ 
> >> "x${GRUB_DISABLE_SUBMENU}" = "xy" ]; then
> >> +    GRUB_DISABLE_SUBMENU="true"
> >> +  fi
> >> +
> >> +  if [ "x$is_top_level" = xtrue ] && [ "x${GRUB_DISABLE_SUBMENU}" != 
> >> xtrue ]; then
> >>      linux_entry "${OS}" "${version}" simple \
> >>      "${GRUB_CMDLINE_LINUX} ${GRUB_CMDLINE_LINUX_DEFAULT}"
> >
> > Well, I understand the problem but I do not like the patch. Why do you
> > introduce "yes" and do not use "true"? Except GRUB_BUTTON_CMOS_ADDRESS,
> > which is wrong IMO, all variables accept only true/false. So, if you
> > want to add "yes" then do that for all. Otherwise you make only confusion.
>
> Right, the problem is that as you said GRUB_DISABLE_SUBMENU is inconsistent
> with all the other options.
>
> But since most variables use "true", someone who's reading the docs might
> wrongly assume that by "y" it really meant "yes" (since for all others a
> word is used and not a letter). That's what happened in the bug reported.
>
> So this patch makes the option to support all of "y", "yes" and "true" to
> make sure that even if a user gets confused by the documentation, will be
> able to set it.
>
> I don't think that will cause confusion if we fix the doc to mention that
> GRUB_DISABLE_SUBMENU has to be set to "true" like all the other options and
> add a comment in 10_linux.in explaining why it checks for the three values.

Works for me...

> At the very least we need to keep supporting "y" besides "true" to avoid
> regressions for users that set to what the documentation used to mention.

Yep.

> > And there are more files which has the same issue, e.g. 
> > util/grub.d/10_hurd.in
>
> You are right, I'll change this in util/grub.d/10_hurd.in as well.

Thanks!

Daniel