diff mbox

Don't truncate the history file when history size is unlimited

Message ID 1433878062-23560-1-git-send-email-patrick@parcs.ath.cx
State New
Headers show

Commit Message

Patrick Palka June 9, 2015, 7:27 p.m. UTC
We still do not handle "set history size unlimited" correctly.  In
particular, after writing to the history file, we truncate the history
even if it is unlimited.

This patch makes sure that we do not call history_truncate_file() if the
history is not stifled (i.e. if it's unlimited).  This bug causes the
history file to be truncated to zero on exit when one has "set history
size unlimited" in their gdbinit file.  Although this code exists in GDB
7.8 it is masked by a pre-existing bug that's been only fixed in GDB 7.9
(PR gdb/17820).

I tried to make a test to check that the history does not get truncated
on exit when the history size is unlimited, but I could not get the test
to work properly.  Also I could not figure out a good way to create a
temporary file (to act as the history file) in tcl versions earlier than
8.6.  I am not sure if it's worth the effort to add a test.

(Apparently I did not test my changes to history file handling
extensively enough.  Sorry..)

gdb/ChangeLog:

	* top.c (gdb_safe_append_history): Do not call
	history_truncate_file if the history is not stifled.
---
 gdb/top.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Pedro Alves June 15, 2015, 3:25 p.m. UTC | #1
On 06/09/2015 08:27 PM, Patrick Palka wrote:
> We still do not handle "set history size unlimited" correctly.  In
> particular, after writing to the history file, we truncate the history
> even if it is unlimited.

Whoops.

> 
> This patch makes sure that we do not call history_truncate_file() if the
> history is not stifled (i.e. if it's unlimited).  This bug causes the
> history file to be truncated to zero on exit when one has "set history
> size unlimited" in their gdbinit file.  Although this code exists in GDB
> 7.8 it is masked by a pre-existing bug that's been only fixed in GDB 7.9
> (PR gdb/17820).
> 
> I tried to make a test to check that the history does not get truncated
> on exit when the history size is unlimited, but I could not get the test
> to work properly.

How so?

> Also I could not figure out a good way to create a
> temporary file (to act as the history file) in tcl versions earlier than
> 8.6.  I am not sure if it's worth the effort to add a test.

Not sure I follow.  Why do you need a temporary file?
You can leave the file in the build/test dir.  In fact, that's
encouraged, to make it easier to debug the test on failures.
You can instead delete a stale file from a previous run at the
start of the (new) run.

> (Apparently I did not test my changes to history file handling
> extensively enough.  Sorry..)

Don't stress about it.  But, it does show out that having a test
would be useful.  So I think it's worth the effort.

The code changes look OK to me.

Thanks,
Pedro Alves
Patrick Palka June 15, 2015, 4 p.m. UTC | #2
On Mon, Jun 15, 2015 at 11:25 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/09/2015 08:27 PM, Patrick Palka wrote:
>> We still do not handle "set history size unlimited" correctly.  In
>> particular, after writing to the history file, we truncate the history
>> even if it is unlimited.
>
> Whoops.
>
>>
>> This patch makes sure that we do not call history_truncate_file() if the
>> history is not stifled (i.e. if it's unlimited).  This bug causes the
>> history file to be truncated to zero on exit when one has "set history
>> size unlimited" in their gdbinit file.  Although this code exists in GDB
>> 7.8 it is masked by a pre-existing bug that's been only fixed in GDB 7.9
>> (PR gdb/17820).
>>
>> I tried to make a test to check that the history does not get truncated
>> on exit when the history size is unlimited, but I could not get the test
>> to work properly.
>
> How so?

I could not observe that the history file gets truncated with this
patch reverted.  Maybe I simply have forgotten to revert the patch or
something stupid like that.  I'll take another stab at it.

>
>> Also I could not figure out a good way to create a
>> temporary file (to act as the history file) in tcl versions earlier than
>> 8.6.  I am not sure if it's worth the effort to add a test.
>
> Not sure I follow.  Why do you need a temporary file?
> You can leave the file in the build/test dir.  In fact, that's
> encouraged, to make it easier to debug the test on failures.
> You can instead delete a stale file from a previous run at the
> start of the (new) run.

Good point... What's the right way to refer to the
$buildroot/gdb/testsuite/gdb.base directory?

>
>> (Apparently I did not test my changes to history file handling
>> extensively enough.  Sorry..)
>
> Don't stress about it.  But, it does show out that having a test
> would be useful.  So I think it's worth the effort.
>
> The code changes look OK to me.
>
> Thanks,
> Pedro Alves
>
Pedro Alves June 15, 2015, 4:18 p.m. UTC | #3
On 06/15/2015 05:00 PM, Patrick Palka wrote:
> On Mon, Jun 15, 2015 at 11:25 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 06/09/2015 08:27 PM, Patrick Palka wrote:
>>> We still do not handle "set history size unlimited" correctly.  In
>>> particular, after writing to the history file, we truncate the history
>>> even if it is unlimited.
>>
>> Whoops.
>>
>>>
>>> This patch makes sure that we do not call history_truncate_file() if the
>>> history is not stifled (i.e. if it's unlimited).  This bug causes the
>>> history file to be truncated to zero on exit when one has "set history
>>> size unlimited" in their gdbinit file.  Although this code exists in GDB
>>> 7.8 it is masked by a pre-existing bug that's been only fixed in GDB 7.9
>>> (PR gdb/17820).
>>>
>>> I tried to make a test to check that the history does not get truncated
>>> on exit when the history size is unlimited, but I could not get the test
>>> to work properly.
>>
>> How so?
> 
> I could not observe that the history file gets truncated with this
> patch reverted.  Maybe I simply have forgotten to revert the patch or
> something stupid like that.  I'll take another stab at it.
> 
>>
>>> Also I could not figure out a good way to create a
>>> temporary file (to act as the history file) in tcl versions earlier than
>>> 8.6.  I am not sure if it's worth the effort to add a test.
>>
>> Not sure I follow.  Why do you need a temporary file?
>> You can leave the file in the build/test dir.  In fact, that's
>> encouraged, to make it easier to debug the test on failures.
>> You can instead delete a stale file from a previous run at the
>> start of the (new) run.
> 
> Good point... What's the right way to refer to the
> $buildroot/gdb/testsuite/gdb.base directory?

Use [standard_output_file $some_file].

E.g., take a look at gdb.trace/tfile.exp, gdb.xml/tdesc-arch.exp,
gdb.xml/tdesc-regs.exp and others.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/top.c b/gdb/top.c
index 837bf16..f5a0819 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -981,7 +981,8 @@  gdb_safe_append_history (void)
       else
 	{
 	  append_history (command_count, local_history_filename);
-	  history_truncate_file (local_history_filename, history_max_entries);
+	  if (history_is_stifled ())
+	    history_truncate_file (local_history_filename, history_max_entries);
 	}
 
       ret = rename (local_history_filename, history_filename);