[v2] Fix gdb.base/gdbinit-history.exp when HISTSIZE is set in the environment (Re: [PATCH] [COMMITTED] Fix PR gdb/17820)

  On 05/15/2015 06:03 PM, Patrick Palka wrote:
> On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
>> +    if {$have_old_histsize} {
>> +       set env(HISTSIZE) $old_histsize
>> +    }
> 
> Why not change this predicate to
> 
>      if [info exists old_histsize]
> 
> to obviate the need for $have_old_histsize altogether?

Yeah, "info exists" is ok since this is local scope; I was worrying
that the code ends up copied elsewhere to global context, and
then the "info exists" would be the wrong thing to use, considering
e.g., [1] and [2].  Maybe I'm worrying too much.  But how about instead
simply saving/restoring the whole env array, like in the updated
patch below, which sidesteps that issue?

[1] https://sourceware.org/ml/gdb-patches/2015-04/msg00261.html
[2] https://sourceware.org/ml/gdb-patches/2015-04/msg00537.html

--------
From b5c09f8bdd4f780d176d110ce61f4eb153ecc9ef Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 15 May 2015 19:27:07 +0100
Subject: [PATCH] Fix gdb.base/gdbinit-history.exp when HISTSIZE is set in the
 environment

Some buildslaves are showing that this test is failing.  E.g.,:

 https://sourceware.org/ml/gdb-testers/2015-q2/msg04164.html

The issue is that HISTSIZE is set to 1000 in the environment that runs
the tests (that's the default in Fedora, set in /etc/profile).

We can trivially reproduce it with:

 $ HISTSIZE=1000 make check RUNTESTFLAGS="gdbinit-history.exp"
 (...)
 Running /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.base/gdbinit-history.exp ...
 FAIL: gdb.base/gdbinit-history.exp: show history size
 FAIL: gdb.base/gdbinit-history.exp: show history size
 FAIL: gdb.base/gdbinit-history.exp: show commands

gdb.log shows:
 ...
 (gdb) set height 0
 (gdb) set width 0
 (gdb) show history size
 The size of the command history is 1000.
 (gdb) FAIL: gdb.base/gdbinit-history.exp: show history size

gdb/testsuite/ChangeLog:
2015-05-15  Pedro Alves  <palves@redhat.com>

	* gdb.base/gdbinit-history.exp (test_gdbinit_history_setting):
	Save the whole env array instead of just HOME.  Unset HISTSIZE in
	the environment while testing.  Restore whole environment
	afterwards.
---
 gdb/testsuite/gdb.base/gdbinit-history.exp | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
  

On Fri, May 15, 2015 at 2:33 PM, Pedro Alves <palves@redhat.com> wrote:
> On 05/15/2015 06:03 PM, Patrick Palka wrote:
>> On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
>>> +    if {$have_old_histsize} {
>>> +       set env(HISTSIZE) $old_histsize
>>> +    }
>>
>> Why not change this predicate to
>>
>>      if [info exists old_histsize]
>>
>> to obviate the need for $have_old_histsize altogether?
>
> Yeah, "info exists" is ok since this is local scope; I was worrying
> that the code ends up copied elsewhere to global context, and
> then the "info exists" would be the wrong thing to use, considering
> e.g., [1] and [2].  Maybe I'm worrying too much.  But how about instead
> simply saving/restoring the whole env array, like in the updated
> patch below, which sidesteps that issue?

What a neat language (TCL).

Both the original approach (with $have_old_histsize) and this one look
fine by me.
  

On 05/15/2015 10:42 PM, Patrick Palka wrote:
> On Fri, May 15, 2015 at 2:33 PM, Pedro Alves <palves@redhat.com> wrote:
>> On 05/15/2015 06:03 PM, Patrick Palka wrote:
>>> On Fri, May 15, 2015 at 12:05 PM, Pedro Alves <palves@redhat.com> wrote:
>>>> +    if {$have_old_histsize} {
>>>> +       set env(HISTSIZE) $old_histsize
>>>> +    }
>>>
>>> Why not change this predicate to
>>>
>>>      if [info exists old_histsize]
>>>
>>> to obviate the need for $have_old_histsize altogether?
>>
>> Yeah, "info exists" is ok since this is local scope; I was worrying
>> that the code ends up copied elsewhere to global context, and
>> then the "info exists" would be the wrong thing to use, considering
>> e.g., [1] and [2].  Maybe I'm worrying too much.  But how about instead
>> simply saving/restoring the whole env array, like in the updated
>> patch below, which sidesteps that issue?
> 
> What a neat language (TCL).

:-)

(It's more an issue with DejaGnu sourcing all test files into
the same context, than language.)

> 
> Both the original approach (with $have_old_histsize) and this one look
> fine by me.

I've pushed this one in now.

Thanks,
Pedro Alves