Re: [PATCH] Tweak the handling of $HISTSIZE edge cases [PR gdb/16999]

[Patrick Palka <patrick at parcs dot ath dot cx>]

On Thu, May 21, 2015 at 7:33 PM, Pedro Alves <palves@redhat.com> wrote:
> On 05/21/2015 11:50 PM, Patrick Palka wrote:
>
> The test is OK.  Good use of with_test_prefix.
>
> Minor nit: I found no other use of with_test_prefix with
> spaces around the '='.
>
>> diff --git a/gdb/top.c b/gdb/top.c
>> index 74e1e07..38b4e5d 100644
>> --- a/gdb/top.c
>> +++ b/gdb/top.c
>> @@ -1684,17 +1684,21 @@ init_history (void)
>>    if (tmpenv)
>>      {
>>        int var;
>> +      char *endptr;
>>
>> -      var = atoi (tmpenv);
>> -      if (var < 0)
>> -     {
>> -       /* Prefer ending up with no history rather than overflowing
>> -          readline's history interface, which uses signed 'int'
>> -          everywhere.  */
>> -       var = 0;
>> -     }
>> +      var = strtol (tmpenv, &endptr, 10);
>>
>> -      history_size_setshow_var = var;
>> +      /* If HISTSIZE is the empty string, negative, or non-numeric then set the
>> +      history size to unlimited.  This behavior is mostly consistent with
>> +      that of bash.  Whereas bash ignores a non-numeric HISTSIZE, we set the
>> +      history to unlimited in that case to avoid potentially truncating the
>> +      user's history.  */
>> +      if (strlen (tmpenv) == 0
>> +       || var < 0
>> +       || *endptr != '\0')
>
> If I'm reading correctly, this treats HISTSIZE=" " as "disable history".
> Is that intended?

It's not really intended.  The motivation was to make sure that an
obvious typo in HISTSIZE (e.g. HISTSIZE="1000-") will not truncate the
history size, but HISTSIZE=" " is not really a typo.  But IMO adding a
more intelligent typo heuristic (one to replace *endptr != '\0') is
not worth it -- it's just a history file after all.

But that reminds me that the strings " 10" and "10 " should not be
considered non-numeric.  That could easily be achieved via a couple of
calls to isspace().

>
> Also, a nit: I find it a bit odd to see strlen to check empty string
> in one case, and != '\0' in another, instead of:
>
>       if (*tmpenv == '\0'
>           || var < 0
>           || *endptr != '\0')
>
>> +     history_size_setshow_var = -1;
>> +      else
>> +     history_size_setshow_var = var;
>>      }
>>    /* If the init file hasn't set a size yet, pick the default.  */
>>    else if (history_size_setshow_var == -2)

Well semantically endptr is less of a string and more of a pointer to
a char within a string -- at least that's how I view it.  But I will
change the first condition to check for '\0'.

On a related note, I wonder whether it is a good idea for GDB to look
at HISTSIZE at all.  As the buildbots and you have shown, some distros
export HISTSIZE by default and by doing so it renders useless GDB's
internal "history size" setting (as far as .gdbinit is concerned).  I
think people expect HISTSIZE to only affect shells, not e.g. readline
applications.  (Otherwise, I would expect the readline library to
already extract the default history size from HISTSIZE or from another
environment variable, something it currently has no support for.)  So
I wonder whether it would be better to stop reading HISTSIZE, to
instead read GDBHISTSIZE or something.

>
> Thanks,
> Pedro Alves
>