From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 95768 invoked by alias); 22 May 2015 00:56:51 -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 95259 invoked by uid 89); 22 May 2015 00:56:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 X-HELO: mail-oi0-f46.google.com Received: from mail-oi0-f46.google.com (HELO mail-oi0-f46.google.com) (209.85.218.46) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 22 May 2015 00:56:49 +0000 Received: by oihd6 with SMTP id d6so2985837oih.2 for ; Thu, 21 May 2015 17:56:47 -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=OmFlazhw9k/ieM9WsYnzF+Swqy3djaN9vOYbyricHCU=; b=Z82BX8yd4aNLNjdHO+CyGW+gcVP3DMEBH8hpoggdWsiZyIFVvbrI+qpUmbQQz1hlhv 8VHP5Lo4fg/jqTnndPVzvZBvC0MU0BRiQymrH+ZyDyxx2PVSwBTTnl/Sb+L8m725L7Ll 8+b5i+zWrXHd9SdSlk6LQmnym+cnGvEeu4+pVrqRdgtX2zJfhn5fPuKUpRN0XMCKLOiy mGfOQJvfHnl9gQ34qzyNJB8qO53C9Qoudy7HBx8dkyBZfuMbaoRe7p51W5IXOEREoZ+P VNpcwRXmRVI31e9XOye98fUC5bV9HFvWnwA7CA3rJW7mX7/IAMFafCKjFg9A66HPQqrc 5F3A== X-Gm-Message-State: ALoCoQl7Ng7HZWUdr26CT57KDWxv4tUVg80qThGvGEyuyCyGWJ5vyzZHxvC13F3yUXd863ttzR7I X-Received: by 10.60.101.195 with SMTP id fi3mr2488876oeb.65.1432256207630; Thu, 21 May 2015 17:56:47 -0700 (PDT) MIME-Version: 1.0 Received: by 10.182.243.232 with HTTP; Thu, 21 May 2015 17:56:27 -0700 (PDT) In-Reply-To: <555E7B52.6050100@redhat.com> References: <1432248648-7402-1-git-send-email-patrick@parcs.ath.cx> <555E6B60.8040802@redhat.com> <555E7B52.6050100@redhat.com> From: Patrick Palka Date: Fri, 22 May 2015 00:56: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/msg00594.txt.bz2 On Thu, May 21, 2015 at 8:41 PM, Pedro Alves wrote: > On 05/22/2015 01:26 AM, Patrick Palka wrote: >> On Thu, May 21, 2015 at 7:33 PM, Pedro Alves 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 >