Re: [PATCH] Add option to remove duplicate command history entries

[Patrick Palka <patrick at parcs dot ath dot cx>]

On Fri, Jun 26, 2015 at 10:18 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/20/2015 12:33 AM, Patrick Palka wrote:
>
>> index c9a532a..c5885eb 100644
>> --- a/gdb/doc/gdb.texinfo
>> +++ b/gdb/doc/gdb.texinfo
>> @@ -22644,6 +22644,21 @@ to 256 if this variable is not set.  Non-numeric values of @env{GDBHISTSIZE}
>>  are ignored.  If @var{size} is @code{unlimited} or if @env{GDBHISTSIZE} is
>>  either a negative number or the empty string, then the number of commands
>>  @value{GDBN} keeps in the history list is unlimited.
>> +
>> +@cindex remove duplicate history
>> +@kindex set history remove-duplicates
>> +@item set history remove-duplicates @var{size}
>> +@itemx set history remove-duplicates unlimited
>> +Control the removal of duplicate history entries in the command history list.
>> +If @var{size} is non-zero, @value{GDBN} will look back at the last @var{size}
>
> Somehow, "size" here sounds a bit confusing to me.  This not about
> that size of the duplicates.  :-)  And it's not about when the history
> gets to a certain size either.  I'd suggest s/size/count/, or s/size/lookbehind/
> or some such.

Ah yeah, I had already changed that variable name locally to "count".

>
>> +history entries and remove the first entry that is a duplicate of the current
>> +entry being added to the command history list.  If @var{size} is
>> +@code{unlimited} then this lookbehind is unbounded.  If @var{size} is 0, then
>> +removal of duplicate history entries is disabled.
>> +
>> +Only history entries added during the current session are considered for
>> +removal.  This option is set to 0 by default.
>> +
>
>
>
>> +proc check_prev_history_entry { entry { test_suffix "" } } {
>> +    set test "history entry is $entry"
>> +    if { $test_suffix != "" } {
>> +        append test " $test_suffix"
>> +    }
>> +
>> +    # Send ^P followed by ^L.
>> +    send_gdb "\x10\x0c"
>
> I have a feeling this may cause problems, but we'll see.

Hopefully not because I copied this "technique" of traversing through
history from readline.exp.  I considered using looking at the output
of "show commands" but doing ^P seems easier and more directly tests
the code paths we want to test.

>
>> +
>> +    gdb_expect {
>> +     -re $entry {
>> +         pass $test
>> +     }
>> +     timeout {
>> +         fail $test
>> +     }
>> +    }
>> +}
>> +
>> +# By default the option is set to 0.
>> +gdb_exit
>> +gdb_start
>> +gdb_test "show history remove-duplicates" "is 0\\."
>> +
>> +# Test the "unlimited" setting.
>> +with_test_prefix "remove-duplicates=unlimited" {
>> +    gdb_exit
>> +    gdb_start
>> +    gdb_test "set history remove-duplicates unlimited"
>> +
>> +    gdb_test "print 0"
>> +    gdb_test "print 1"
>> +    gdb_test "print 2"
>> +    gdb_test "print 1"
>> +    gdb_test "print 1"
>> +    gdb_test "print 2"
>> +    gdb_test "print 3"
>> +    gdb_test "print 3"
>> +    gdb_test "print 4"
>> +    gdb_test "print 1"
>> +    gdb_test "print 2"
>> +    gdb_test "print 3"
>> +    gdb_test "print 4"
>
> Duplicate test names here.  This should catch all:

I could use some kind of loop over a list here I think, prefixing each
test name with the index of the list.

>
>  $ make check RUNTESTFLAGS="history-duplicates.exp"
>  $ grep "PASS" testsuite/gdb.sum | sort | uniq -c | sort -n
>
> Otherwise this looks good to me.

Thanks a lot for reviewing!

>
> Thanks,
> Pedro Alves
>