diff mbox

Prune duplicate command history entries

Message ID 1433301766-20101-1-git-send-email-patrick@parcs.ath.cx
State New, archived
Headers

Commit Message

Patrick Palka June 3, 2015, 3:22 a.m. UTC 
  This patch implements pruning of duplicate command-history entries using
a modest amount of lookbehind.  The motivation for this patch is to
reduce the prevalence of basic commands such as "up" and "down" in the
history file.  These common commands crowd out more unique commands in
the history file (when the history file has a fixed size), and they make
navigation of the history file via ^P, ^N and ^R more inconvenient.  By
pruning duplicate command-history entries we can significantly reduce
the occurrence of such entries, leaving the history file filled with
more interesting commands.

The maximum lookbehind is fixed to 50 (an arbitrary number) so that the
operation will be guaranteed to not take too long.

gdb/ChangeLog:

	* top.c (gdb_add_history): Prune duplicate command-history
	entries.
---
 gdb/top.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Andrew Burgess June 3, 2015, 8:20 a.m. UTC | #1 
* Patrick Palka <patrick@parcs.ath.cx> [2015-06-02 23:22:46 -0400]:

> This patch implements pruning of duplicate command-history entries using
> a modest amount of lookbehind.  The motivation for this patch is to
> reduce the prevalence of basic commands such as "up" and "down" in the
> history file.

Dropping commands such as up / down could be pretty annoying if you
wanted to figure out where you were in the past. Dropping things like
bt from the history would be less annoying.

I wonder if we should classify commands into navigation or
state-changing commands and diagnostic commands.
I'd be happier see repeated diagnostic commands disappear, and less so
for commands that change inferior state, or navigate me around the
stack.

> The maximum lookbehind is fixed to 50 (an arbitrary number) so that the
> operation will be guaranteed to not take too long.

I think at the very least you should make this threshold
configurable.  I'd then argue for off by default due to the loss of
state changing commands being too annoying (for me).

You should probably have some tests too, we already test C-p in
readline.exp, so it should be possible to test that this feature
works.

Thanks,
Andrew
Patrick Palka June 3, 2015, 2:15 p.m. UTC | #2 
On Wed, Jun 3, 2015 at 4:20 AM, Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> * Patrick Palka <patrick@parcs.ath.cx> [2015-06-02 23:22:46 -0400]:
>
>> This patch implements pruning of duplicate command-history entries using
>> a modest amount of lookbehind.  The motivation for this patch is to
>> reduce the prevalence of basic commands such as "up" and "down" in the
>> history file.
>
> Dropping commands such as up / down could be pretty annoying if you
> wanted to figure out where you were in the past. Dropping things like
> bt from the history would be less annoying.
>
> I wonder if we should classify commands into navigation or
> state-changing commands and diagnostic commands.
> I'd be happier see repeated diagnostic commands disappear, and less so
> for commands that change inferior state, or navigate me around the
> stack.

Hmm, but doesn't the empty-command shorthand already make it mostly
impossible to figure out where you were in the past?  If you run "up"
twice by typing "up\n\n" then only one history entry for "up" gets
added to the history file (with or without the patch).  So you won't
really be sure, by looking at your history, whether that
"up"/"down"/"continue"/"step" entry was invoked once or 5 times in a
row. It seems that this existing behavior throws any semblance of
state recovery out the window (unless you carefully avoid using the
shorthand :)).

>
>> The maximum lookbehind is fixed to 50 (an arbitrary number) so that the
>> operation will be guaranteed to not take too long.
>
> I think at the very least you should make this threshold
> configurable.  I'd then argue for off by default due to the loss of
> state changing commands being too annoying (for me).
>
> You should probably have some tests too, we already test C-p in
> readline.exp, so it should be possible to test that this feature
> works.

I did not think to make the threshold configurable (as opposed to
making some boolean variable configurable).  That makes sense.

>
> Thanks,
> Andrew
Joel Brobecker June 3, 2015, 5:09 p.m. UTC | #3 
> > This patch implements pruning of duplicate command-history entries using
> > a modest amount of lookbehind.  The motivation for this patch is to
> > reduce the prevalence of basic commands such as "up" and "down" in the
> > history file.
> 
> Dropping commands such as up / down could be pretty annoying if you
> wanted to figure out where you were in the past. Dropping things like
> bt from the history would be less annoying.

I guess it's a question of preference, then. I know that my shell
merges all duplicate commands into one when it comes to the history,
and I really appreciate that feature. A Lot.

> I wonder if we should classify commands into navigation or
> state-changing commands and diagnostic commands.
> I'd be happier see repeated diagnostic commands disappear, and less so
> for commands that change inferior state, or navigate me around the
> stack.

FWIW, I'd find that distinction a little too complex for the gains
we're trying to achieve. At most, I'd just suggest one toggle to
activate duplicate merging or not. Just my 2 cents.
Andrew Burgess June 3, 2015, 5:42 p.m. UTC | #4 
* Patrick Palka <patrick@parcs.ath.cx> [2015-06-03 10:15:47 -0400]:

> Hmm, but doesn't the empty-command shorthand already make it mostly
> impossible to figure out where you were in the past?  If you run "up"
> twice by typing "up\n\n" then only one history entry for "up" gets
> added to the history file (with or without the patch).

I confess I hadn't spotted that.  Looking back through the history to
figure out what I've been doing isn't a common operation (thankfully).

I guess so long as the feature was switchable then I have little
objection.

Thanks,
Andrew
diff mbox

Patch

diff --git a/gdb/top.c b/gdb/top.c
index 74e1e07..ec88fa3 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -897,6 +897,34 @@  static int command_count = 0;
 void
 gdb_add_history (const char *command)
 {
+  int lookbehind;
+  const int lookbehind_threshold = 50;
+
+  /* To help avoid cluttering the history file with entries for "up", "down",
+     "list", "bt", and so on, perform pruning of duplicate command-history
+     entries, but only among history entries added during this session.  Since
+     we update the history file by appending to it, history entries that are
+     already stored in the history file can't be pruned.  */
+  using_history ();
+  for (lookbehind = 0;
+       lookbehind < command_count && lookbehind < lookbehind_threshold;
+       lookbehind++)
+    {
+      HIST_ENTRY *temp = previous_history ();
+
+      if (temp == NULL)
+	break;
+
+      if (strcmp (temp->line, command) == 0)
+	{
+	  HIST_ENTRY *prev = remove_history (where_history ());
+	  command_count--;
+	  free_history_entry (prev);
+	  break;
+	}
+    }
+  using_history ();
+
   add_history (command);
   command_count++;
 }