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

[Patrick Palka <patrick@parcs.ath.cx>]

On Thu, May 21, 2015 at 8:56 PM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Thu, May 21, 2015 at 8:41 PM, Pedro Alves <palves@redhat.com> wrote:
>> On 05/22/2015 01:26 AM, Patrick Palka wrote:
>>> On Thu, May 21, 2015 at 7:33 PM, Pedro Alves <palves@redhat.com> wrote:
>>
>>>> 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.
>>
>> I haven't gone back to recheck what bash does, but, I can see
>> that happening in scripts, like:
>>
>>   if whatever; then
>>      mysize=1000
>>   fi
>>   HISTSIZE="$mysize " HISTFILESIZE="$mysize"
>>
>> and then mysize ends up unset.
>
> bash would treat HISTSIZE=" " as non-numeric and thus do nothing.
> This patch on the other hand treats non-numeric values as if they are
> typos and thus sets the history size to unlimited to avoid truncation.
> But now I'm starting to question whether this is a good idea...
> sigh...
>
>>
>>>
>>> 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().
>>
>> Exactly, I was thinking of those too, but I didn't want to call
>> out what the behavior should be.  :-)
>>
>>>
>>>>
>>>> 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'.
>>
>> Ah.  Good point.  I'm fine either way then.
>>
>>>
>>> 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.
>>
>> Yeah, I'm inclined to agree.
>
> I will make a small patch series that does this then (which will
> include this patch).

What do you think about removing HISTSIZE/GDBHISTSIZE support
altogether?  It is awfully redundant (we can already automatically set
the history size via .gdbinit or via -ex "set history size foo") and
thus not really useful.  Even if we go along with replacing HISTSIZE
with GDBHISTSIZE I just can't see much use for it.

>
>>
>> Thanks,
>> Pedro Alves
>>