From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 99001 invoked by alias); 22 May 2015 00:26:49 -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 98992 invoked by uid 89); 22 May 2015 00:26:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.1 required=5.0 tests=AWL,BAYES_05,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 X-HELO: mail-oi0-f50.google.com Received: from mail-oi0-f50.google.com (HELO mail-oi0-f50.google.com) (209.85.218.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 22 May 2015 00:26:48 +0000 Received: by oihd6 with SMTP id d6so2679948oih.2 for ; Thu, 21 May 2015 17:26:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; bh=/ugDXFHOFjh98sRCFyuv3Hiv8/rlfnV1ohDaw2ZuQQw=; b=in2Hvt1ZrheD1JGp/7gkHpzfXKO2K5PFq+VayIirqRvv6HHpQkopJ1TcStBBw5dZee fTYoixipeziUz0U0KI3svnQrCi/1xq4/d9R+UPxqCgzj/+6QKLKE6VJ2vtXe25Wgf1dc jaKR6ZcRk67DPyVhgDDX9dD+muOIKdrtyzY2/S1EhPnIkhBbQ2s5GDQoUH5oUtILvq2q FCqGsAtIA/dKug1fcAqgSQBrLBW57nhbSOsI05GAc3+Wvx5NL557BalLKnpk2vktNuJI tyLrXYmOuIH4r5HH191dcm5YmI+1Nj4GvdD7QEikaM4/pbAcMSAp8/Zh1mSTJTLopbg5 C8Dw== X-Gm-Message-State: ALoCoQlRF6pAbAnXpSxWBdZ7vyxuywDXDjZJjkNC9r1GrA2nRz30m/tmyPBV8SUmPLOvDXIJFPrP X-Received: by 10.202.225.65 with SMTP id y62mr4194930oig.78.1432254406469; Thu, 21 May 2015 17:26:46 -0700 (PDT) MIME-Version: 1.0 Received: by 10.182.243.232 with HTTP; Thu, 21 May 2015 17:26:25 -0700 (PDT) In-Reply-To: <555E6B60.8040802@redhat.com> References: <1432248648-7402-1-git-send-email-patrick@parcs.ath.cx> <555E6B60.8040802@redhat.com> From: Patrick Palka Date: Fri, 22 May 2015 00:26:00 -0000 Message-ID: Subject: Re: [PATCH] Tweak the handling of $HISTSIZE edge cases [PR gdb/16999] To: Pedro Alves Cc: "gdb-patches@sourceware.org" Content-Type: text/plain; charset=UTF-8 X-SW-Source: 2015-05/txt/msg00589.txt.bz2 On Thu, May 21, 2015 at 7:33 PM, Pedro Alves 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 >