From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 88619 invoked by alias); 14 May 2015 10:16:24 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 88608 invoked by uid 89); 14 May 2015 10:16:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 14 May 2015 10:16:22 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t4EAGKnm012285 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 14 May 2015 06:16:20 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t4EAGHf2011911; Thu, 14 May 2015 06:16:19 -0400 Message-ID: <555475F1.2030504@redhat.com> Date: Thu, 14 May 2015 10:16:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Patrick Palka , gdb-patches@sourceware.org Subject: Re: [PATCH] Fix PR gdb/16999 References: <1431555450-15493-1-git-send-email-patrick@parcs.ath.cx> In-Reply-To: <1431555450-15493-1-git-send-email-patrick@parcs.ath.cx> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-05/txt/msg00360.txt.bz2 On 05/13/2015 11:17 PM, Patrick Palka wrote: > When GDB reads a nonsensical value for the HISTSIZE environment variable > variable, i.e. one that is non-numeric or negative, GDB then sets its > history size to 0. This behavior is contrary to that of bash, which > defaults the history size to unlimited in such cases. > > This patch makes the behavior of invalid HISTSIZE match that of bash. > When we encounter an invalid HISTSIZE we now set the history size to > unlimited instead of 0. > > 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 | 54 +++++++++++++++++++++++++++++ > gdb/top.c | 16 ++++----- > 2 files changed, 62 insertions(+), 8 deletions(-) > create mode 100644 gdb/testsuite/gdb.base/histsize-history.exp > > diff --git a/gdb/testsuite/gdb.base/histsize-history.exp b/gdb/testsuite/gdb.base/histsize-history.exp > new file mode 100644 > index 0000000..b7b13cf > --- /dev/null > +++ b/gdb/testsuite/gdb.base/histsize-history.exp > @@ -0,0 +1,54 @@ > +# 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 . > + > +# 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 > + > + if [info exists env(HISTSIZE)] { > + set old_histsize $env(HISTSIZE) > + } > + set env(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 [info exists old_histsize] { > + set env(HISTSIZE) $old_histsize > + } else { > + unset env(HISTSIZE) > + } > +} > + > +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..00fee8d 100644 > --- a/gdb/top.c > +++ b/gdb/top.c > @@ -1684,15 +1684,15 @@ 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); > + > + /* For the sake of consistency with bash, if HISTSIZE is > + non-numeric or if HISTSIZE is negative then set our history > + size to unlimited. */ > + if (*endptr != '\0' || var < 0) > + var = -1; Hmm, looking at the master branch (Bash-4.3 patch 33) in: http://git.savannah.gnu.org/cgit/bash.git This seems to be where bash implements this: #if defined (HISTORY) /* What to do after the HISTSIZE or HISTFILESIZE variables change. If there is a value for this HISTSIZE (and it is numeric), then stifle the history. Otherwise, if there is NO value for this variable, ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unstifle the history. If name is HISTFILESIZE, and its value is ^^^^^^^^^^^^^^^^^^^^ numeric, truncate the history file to hold no more than that many lines. */ void sv_histsize (name) char *name; { char *temp; intmax_t num; int hmax; temp = get_string_value (name); if (temp && *temp) { if (legal_number (temp, &num)) { hmax = num; if (hmax < 0 && name[4] == 'S') unstifle_history (); /* unstifle history if HISTSIZE < 0 */ else if (name[4] == 'S') { stifle_history (hmax); hmax = where_history (); if (history_lines_this_session > hmax) history_lines_this_session = hmax; } else if (hmax >= 0) /* truncate HISTFILE if HISTFILESIZE >= 0 */ { history_truncate_file (get_string_value ("HISTFILE"), hmax); if (hmax <= history_lines_in_file) history_lines_in_file = hmax; } } } else if (name[4] == 'S') unstifle_history (); } Note the comment I underlined above. If I'm reading right, stripping out all the HISTFILESIZE handling and then removing dead code, we get: void sv_histsize (char * name) { char *temp; intmax_t num; int hmax; temp = get_string_value (name); if (temp && *temp) { if (legal_number (temp, &num)) { hmax = num; if (hmax < 0) unstifle_history (); /* unstifle history if HISTSIZE < 0 */ else stifle_history (hmax); } } else unstifle_history (); } Note: #1 - if HISTSIZE is non-numeric nothing happens. I think that means bash ends up with the default history size. #2 - if HISTSIZE is set to the empty string, bash ends up with unlimited history size. It seems to me that your patch handles both of these differently. #2 appears consistent with what is suggested here: http://stackoverflow.com/questions/9457233/unlimited-bash-history "Set HISTSIZE and HISTFILESIZE to an empty string: HISTSIZE= HISTFILESIZE= In bash 4.3 and later you can also use HISTSIZE=-1 HISTFILESIZE=-1: " Thanks, Pedro Alves