diff mbox

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

Message ID 1432248648-7402-1-git-send-email-patrick@parcs.ath.cx
State New
Headers show

Commit Message

Patrick Palka May 21, 2015, 10:50 p.m. UTC
When GDB reads a nonsensical value for the HISTSIZE environment
variable, i.e. one that is non-numeric or negative, GDB then sets its
history size to 0.  This behavior is annoying and also inconsistent
with the behavior of bash.

This patch makes the behavior of invalid HISTSIZE mostly match that of bash.
When we encounter an invalid or null HISTSIZE we now set the history
size to unlimited instead of 0.  Whereas bash ignores a non-numeric
HISTSIZE, we set the history to unlimited in that case so that an
accidental typo will not potentially truncate the user's history.

gdb/ChangeLog:

	PR gdb/16999
	* top.c (init_history): For invalid HISTSIZE, set history size
	to unlimited.

gdb/testsuite/ChangeLog:

	PR gdb/16999
	* gdb.base/histsize-history.exp: New test.
---
 gdb/testsuite/gdb.base/histsize-history.exp | 60 +++++++++++++++++++++++++++++
 gdb/top.c                                   | 22 ++++++-----
 2 files changed, 73 insertions(+), 9 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/histsize-history.exp

Comments

Pedro Alves May 21, 2015, 11:33 p.m. UTC | #1
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?

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)

Thanks,
Pedro Alves
Patrick Palka May 22, 2015, 12:26 a.m. UTC | #2
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
>
Pedro Alves May 22, 2015, 12:41 a.m. UTC | #3
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
Patrick Palka May 22, 2015, 12:56 a.m. UTC | #4
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
>
Patrick Palka May 22, 2015, 2:46 a.m. UTC | #5
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
>>
Pedro Alves May 22, 2015, 10 a.m. UTC | #6
Changing title to call for attention.  Maybe we should ask
on gdb@.  Background here:

 https://sourceware.org/ml/gdb-patches/2015-05/msg00349.html
 https://sourceware.org/ml/gdb-patches/2015-05/msg00563.html

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

What about GDBHISTFILE?  I think that the rationale for the existence
of one should apply to both.  (with the HISTSIZE vs GDBHISTSIZE distinction
being a separate matter.)

I'm really not sure.  Trying to play devil's advocate:

#1 - An env var can be set once, for all users.  But that can be
   done with --with-system-gdbinit=FILE as well.

#2 - Along with GDBHISTFILE, it survives -nx.  Does it really matter?
   I don't know.

#3 - Seems friendly to allow at least GDBHISTFILE be an env var so it
   can easily be toggled per host.  Though that can be done through
   Python inside .gdbinit nowadays.  Though^2, Python isn't always
   available.

OTOH, I'm getting more convinced that we should at least
rename HISTSIZE -> GDBHISTSIZE.  The cost of keeping that
doesn't seem to be much.

Thanks,
Pedro Alves
Patrick Palka May 22, 2015, 11:57 a.m. UTC | #7
On Fri, May 22, 2015 at 6:00 AM, Pedro Alves <palves@redhat.com> wrote:
> Changing title to call for attention.  Maybe we should ask
> on gdb@.  Background here:
>
>  https://sourceware.org/ml/gdb-patches/2015-05/msg00349.html
>  https://sourceware.org/ml/gdb-patches/2015-05/msg00563.html
>
>> 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.
>
> What about GDBHISTFILE?  I think that the rationale for the existence
> of one should apply to both.  (with the HISTSIZE vs GDBHISTSIZE distinction
> being a separate matter.)

GDBHISTFILE is less useless than HISTSIZE I think.  I can imagine
unique use cases for GDBHISTFILE (e.g. to have separate per-project
history files) whereas for HISTSIZE, not so much.  So I don't think
their usefulness can be conflated.

>
> I'm really not sure.  Trying to play devil's advocate:
>
> #1 - An env var can be set once, for all users.  But that can be
>    done with --with-system-gdbinit=FILE as well.
>
> #2 - Along with GDBHISTFILE, it survives -nx.  Does it really matter?
>    I don't know.
>
> #3 - Seems friendly to allow at least GDBHISTFILE be an env var so it
>    can easily be toggled per host.  Though that can be done through
>    Python inside .gdbinit nowadays.  Though^2, Python isn't always
>    available.
>
> OTOH, I'm getting more convinced that we should at least
> rename HISTSIZE -> GDBHISTSIZE.  The cost of keeping that
> doesn't seem to be much.

I just don't see any utility in this environment variable.  I imagine
most users stumble upon this feature by realizing that their global
HISTSIZE variable is being read by GDB.  Once we rename it to
GDBHISTSIZE we will no longer have this coincidence and the variable
will be forever ignored.

>
> Thanks,
> Pedro Alves
>
Patrick Palka May 22, 2015, 12:09 p.m. UTC | #8
On Fri, May 22, 2015 at 7:57 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> On Fri, May 22, 2015 at 6:00 AM, Pedro Alves <palves@redhat.com> wrote:
>> Changing title to call for attention.  Maybe we should ask
>> on gdb@.  Background here:
>>
>>  https://sourceware.org/ml/gdb-patches/2015-05/msg00349.html
>>  https://sourceware.org/ml/gdb-patches/2015-05/msg00563.html
>>
>>> 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.
>>
>> What about GDBHISTFILE?  I think that the rationale for the existence
>> of one should apply to both.  (with the HISTSIZE vs GDBHISTSIZE distinction
>> being a separate matter.)
>
> GDBHISTFILE is less useless than HISTSIZE I think.  I can imagine
> unique use cases for GDBHISTFILE (e.g. to have separate per-project
> history files) whereas for HISTSIZE, not so much.  So I don't think
> their usefulness can be conflated.
>
>>
>> I'm really not sure.  Trying to play devil's advocate:
>>
>> #1 - An env var can be set once, for all users.  But that can be
>>    done with --with-system-gdbinit=FILE as well.
>>
>> #2 - Along with GDBHISTFILE, it survives -nx.  Does it really matter?
>>    I don't know.
>>
>> #3 - Seems friendly to allow at least GDBHISTFILE be an env var so it
>>    can easily be toggled per host.  Though that can be done through
>>    Python inside .gdbinit nowadays.  Though^2, Python isn't always
>>    available.
>>
>> OTOH, I'm getting more convinced that we should at least
>> rename HISTSIZE -> GDBHISTSIZE.  The cost of keeping that
>> doesn't seem to be much.
>
> I just don't see any utility in this environment variable.  I imagine
> most users stumble upon this feature by realizing that their global
> HISTSIZE variable is being read by GDB.  Once we rename it to
> GDBHISTSIZE we will no longer have this coincidence and the variable
> will be forever ignored.

... but since the work to rename it to GDBHISTSIZE has already been
done, we might as well keep it :)

>
>>
>> Thanks,
>> Pedro Alves
>>
diff mbox

Patch

diff --git a/gdb/testsuite/gdb.base/histsize-history.exp b/gdb/testsuite/gdb.base/histsize-history.exp
new file mode 100644
index 0000000..10fc453
--- /dev/null
+++ b/gdb/testsuite/gdb.base/histsize-history.exp
@@ -0,0 +1,60 @@ 
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite.
+
+# Test the setting of "history size" via the HISTSIZE environment variable
+
+
+# Check that the history size is properly set to SIZE when env(HISTSIZE) is set
+# to HISTSIZE.
+
+proc test_histsize_history_setting { histsize size } {
+    global env
+
+    set have_old_histsize 0
+    if [info exists env(HISTSIZE)] {
+        set have_old_histsize 1
+        set old_histsize $env(HISTSIZE)
+    }
+    set env(HISTSIZE) $histsize
+
+    with_test_prefix "histsize = $histsize" {
+	gdb_exit
+	gdb_start
+
+	gdb_test "show history size" "The size of the command history is $size."
+
+	if { $size == "0" } {
+	    gdb_test_no_output "show commands"
+	} elseif { $size != "1" } {
+	    gdb_test "show commands" \
+		     "    .  show history size\r\n    .  show commands"
+	}
+
+	if { $have_old_histsize } {
+	    set env(HISTSIZE) $old_histsize
+	} else {
+	    unset env(HISTSIZE)
+	}
+    }
+}
+
+test_histsize_history_setting "" "unlimited"
+test_histsize_history_setting "0" "0"
+test_histsize_history_setting "20" "20"
+test_histsize_history_setting "-5" "unlimited"
+test_histsize_history_setting "not_an_integer" "unlimited"
+test_histsize_history_setting "10zab" "unlimited"
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')
+	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)