This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Tweak the handling of $HISTSIZE edge cases [PR gdb/16999]
- From: Patrick Palka <patrick at parcs dot ath dot cx>
- To: Pedro Alves <palves at redhat dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Thu, 21 May 2015 20:56:27 -0400
- Subject: Re: [PATCH] Tweak the handling of $HISTSIZE edge cases [PR gdb/16999]
- Authentication-results: sourceware.org; auth=none
- References: <1432248648-7402-1-git-send-email-patrick at parcs dot ath dot cx> <555E6B60 dot 8040802 at redhat dot com> <CA+C-WL-89Y2_CaOpKe_tYP1S2BvKvwgFjW-NQ90tEN6MWW7VUg at mail dot gmail dot com> <555E7B52 dot 6050100 at redhat dot com>
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).
>
> Thanks,
> Pedro Alves
>