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

[Pedro Alves <palves@redhat.com>]

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.

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

Thanks,
Pedro Alves