Re: [PATCH] Fix bad test on GRUB_DISABLE_SUBMENU.

[Javier Martinez Canillas]

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.

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.

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

> Daniel
>

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat