From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 100272 invoked by alias); 22 May 2015 02:47:10 -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 100179 invoked by uid 89); 22 May 2015 02:46:52 -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-ob0-f181.google.com Received: from mail-ob0-f181.google.com (HELO mail-ob0-f181.google.com) (209.85.214.181) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Fri, 22 May 2015 02:46:51 +0000 Received: by obfe9 with SMTP id e9so4186709obf.1 for ; Thu, 21 May 2015 19:46:49 -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=CsYQlLVEOjLHI6C2nUo9foA6x4E3oPI0eHwNDabB2vI=; b=GyuNQ53BXxrRCLubcucaPfw+y7fo3r+eGKqCKvKm7RBrBGT9kB9Mu9yff2SHY/nSy6 Sxq2xaUV7r2XFOcop42nDe0n6rldgAesITyR7XdkaBGrFF5lJYz1RZynrL032Vw7hgBy sPRGEeGpjfOEhFnkb1wDH5IanjaJHuzXA8+SGxdJcNG09QIzyR6O/BnfFq9fYZ3ASpOG /78QTptfcD0I1XrPb8kzY9B/QNk1iKeP/Pb9j5rYciKEoGy2cjqqOINMJgEl6mg7grq7 PBXpj4YWDdtLY7um5Xwgg6UNuvoJ2Z23h3bW8CDchq51EJD8img5kVEZMXNNvE5ePu9r CTMA== X-Gm-Message-State: ALoCoQl3b1D88WOSpLfLJ2pjzbC0u7WTXkp2Y+gp36FytlulfxgF4kgRg5HzZDK+L9zlYVeJO4Nb X-Received: by 10.182.102.129 with SMTP id fo1mr4937411obb.24.1432262809210; Thu, 21 May 2015 19:46:49 -0700 (PDT) MIME-Version: 1.0 Received: by 10.182.243.232 with HTTP; Thu, 21 May 2015 19:46:28 -0700 (PDT) In-Reply-To: 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 02:47: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/msg00596.txt.bz2 On Thu, May 21, 2015 at 8:56 PM, Patrick Palka wrote: > 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). 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 >>