bug#7899: Unsatisfactory interaction between shell-mode-hook and comint-read-input-ring

[Pete Beardmore <pete.beardmore@msn.com>]

[-- Attachment #1: Type: text/plain, Size: 2498 bytes --]

Quoting Pete Beardmore <pete.beardmore@msn.com>:

> [patches against bzr master attached (hopefully!)]
>
> hi,
>
> i believe this is a bug in comint-read-input-ring. shell-mode sets a  
> buffer-local version of comint-read-input-size which is effectively  
> ignored due to comint-read-input-ring's use of '(with-temp-buffer  
> ...' . i've lost ~40000 line bash history on more than one occasion  
> over the last several years and am elated to have finally pinned the  
> problem on something
>
> (very loosely) related to this issue is the question of why the  
> default of 'comint-input-history-ignore' is set to anything at all?  
> it's currently "^#", and therefore without having pro-actively made  
> any changes to their emacs setup, a user's shell history (for  
> instance) doesn't emerge unscathed from a trip through comint if it  
> contains comments. if modifying this default touches too many other  
> comint uses, perhaps an override in shell-mode.el?
>
> cheers,
> Pete

[patches attached superseed previous patches]

hello,

patch 1:
i've extended the original fix for ignoring buffer-local variables to  
incorporate 'comint-input-ring-separator',  
'comint-input-history-ignore' and 'comint-input-ignoredups' vars which  
suffered from the same issue

patch 2:
as before, but note that this request to change the default  
'comint-input-history-ignore' from '^#' to '' exposed another bug in  
the 'comint-read-input-ring' code. see patch 3

patch 3:
if 'comint-input-history-ignore' is set to "" (not 'nil' as we're  
using string-match), string-match will always return 0 ..and as this  
isn't nil, all potential items are dropped as they match the ignore  
string. i'll leave 'patch 2' as a request, but the fix for this bug is  
a necessity i think as there's nothing stopping users setting ignore  
to "" as it stands, and that causes issues

patch 4, the ignore-dupes functionality didn't work at all*. the  
comparison of the current item (to be placed into the ring) was being  
made against (ring-ref ring 0) ..which is static, and not the last  
item we added as is needed here. the docs on  
'ring-insert-at-beginning'/'ring-insert'/'ring-ref' would confuse  
anyone on first glance (in defense of whoever slipped here initially)

*it does 'work' if the only dupes in the file are all adjacent and  
equal to the last item

cheers,
Pete

ps. there's still a nasty mix of tabs/space formatting in  
'comint-read-input-ring'. i harmonised only the block i touched

[-- Attachment #2: 0001.comint_.ensure.buffer.local.comint-input-ring-_.variables.are.visible.through.input-read-ring.logic.diff --]
[-- Type: text/x-patch, Size: 3199 bytes --]

#------------------------------------------------------------
#revno: 112736
#committer: Pete Beardmore <pete.beardmore@msn.com>
#branch nick: bzr
#timestamp: Thu 2013-05-30 13:17:39 +0100
#message:
#  comint: ensure buffer local comint-input-ring-* variables are visible through input-read-ring logic
=== modified file 'lisp/comint.el'
--- lisp/comint.el	2013-05-25 02:40:33 +0000
+++ lisp/comint.el	2013-05-30 12:17:39 +0000
@@ -938,33 +938,33 @@
 		;; to huge numbers.  Don't allocate a huge ring right
 		;; away; there might not be that much history.
 		(ring-size (min 1500 comint-input-ring-size))
-		(ring (make-ring ring-size)))
-	   (with-temp-buffer
-             (insert-file-contents file)
-             ;; Save restriction in case file is already visited...
-             ;; Watch for those date stamps in history files!
-             (goto-char (point-max))
-             (let (start end history)
-               (while (and (< count comint-input-ring-size)
-                           (re-search-backward comint-input-ring-separator
-                                               nil t)
-                           (setq end (match-beginning 0)))
-                 (setq start
-                       (if (re-search-backward comint-input-ring-separator
-                                               nil t)
-                           (match-end 0)
-                         (point-min)))
-                 (setq history (buffer-substring start end))
-                 (goto-char start)
-                 (when (and (not (string-match comint-input-history-ignore
-					       history))
-			    (or (null comint-input-ignoredups)
-				(ring-empty-p ring)
-				(not (string-equal (ring-ref ring 0)
-						   history))))
+		(ring-size-max (max 1500 comint-input-ring-size))
+		(ring (make-ring ring-size))
+    (ring-separator comint-input-ring-separator)
+    (ignore comint-input-history-ignore)
+    (ignoredups comint-input-ignoredups))
+    (with-temp-buffer
+       (insert-file-contents file)
+       ;; Save restriction in case file is already visited...
+       ;; Watch for those date stamps in history files!
+       (goto-char (point-max))
+       (let (start end history)
+         (while (and (< count ring-size-max)
+                     (re-search-backward ring-separator nil t)
+                     (setq end (match-beginning 0)))
+           (setq start
+                 (if (re-search-backward ring-separator nil t)
+                     (match-end 0)
+                   (point-min)))
+           (setq history (buffer-substring start end))
+           (goto-char start)
+           (when (and (not (string-match ignore history))
+                      (or (null ignoredups)
+                          (ring-empty-p ring)
+                          (not (string-equal (ring-ref ring 0) 
+                                             history))))
 		   (when (= count ring-size)
-		     (ring-extend ring (min (- comint-input-ring-size ring-size)
-					    ring-size))
+		     (ring-extend ring (min (- ring-size-max ring-size) ring-size))
 		     (setq ring-size (ring-size ring)))
 		   (ring-insert-at-beginning ring history)
 		   (setq count (1+ count))))))


[-- Attachment #3: 0002.comint_.don't.strip.anything.by.default.on.comint-input-ring-read.diff --]
[-- Type: text/x-patch, Size: 715 bytes --]

#------------------------------------------------------------
#revno: 112737
#committer: Pete Beardmore <pete.beardmore@msn.com>
#branch nick: bzr
#timestamp: Thu 2013-05-30 13:20:40 +0100
#message:
#  comint: don't strip anything by default on comint-input-ring-read
=== modified file 'lisp/comint.el'
--- lisp/comint.el	2013-05-30 12:17:39 +0000
+++ lisp/comint.el	2013-05-30 12:20:40 +0000
@@ -318,7 +318,7 @@
 (defvar comint-input-ring-separator "\n"
   "Separator between commands in the history file.")
 
-(defvar comint-input-history-ignore "^#"
+(defvar comint-input-history-ignore ""
   "Regexp for history entries that should be ignored when Comint initializes.")
 
 (defcustom comint-process-echoes nil


[-- Attachment #4: 0003.comint_.don't.match.an.empty.ignore.string.diff --]
[-- Type: text/x-patch, Size: 840 bytes --]

#------------------------------------------------------------
#revno: 112738
#committer: Pete Beardmore <pete.beardmore@msn.com>
#branch nick: bzr
#timestamp: Thu 2013-05-30 13:29:26 +0100
#message:
#  comint: don't match an empty ignore string
=== modified file 'lisp/comint.el'
--- lisp/comint.el	2013-05-30 12:20:40 +0000
+++ lisp/comint.el	2013-05-30 12:29:26 +0000
@@ -958,7 +958,8 @@
                    (point-min)))
            (setq history (buffer-substring start end))
            (goto-char start)
-           (when (and (not (string-match ignore history))
+           (when (and (or (= (length ignore) 0)
+                          (not (string-match ignore history)))
                       (or (null ignoredups)
                           (ring-empty-p ring)
                           (not (string-equal (ring-ref ring 0) 


[-- Attachment #5: 0004.comint_.fix.ignore-dupe.functionality.diff --]
[-- Type: text/x-patch, Size: 841 bytes --]

#------------------------------------------------------------
#revno: 112739
#committer: Pete Beardmore <pete.beardmore@msn.com>
#branch nick: bzr
#timestamp: Thu 2013-05-30 13:31:52 +0100
#message:
#  comint: fix ignore-dupe functionality
=== modified file 'lisp/comint.el'
--- lisp/comint.el	2013-05-30 12:29:26 +0000
+++ lisp/comint.el	2013-05-30 12:31:52 +0000
@@ -962,7 +962,7 @@
                           (not (string-match ignore history)))
                       (or (null ignoredups)
                           (ring-empty-p ring)
-                          (not (string-equal (ring-ref ring 0) 
+                          (not (string-equal (ring-ref ring (1- count))
                                              history))))
 		   (when (= count ring-size)
 		     (ring-extend ring (min (- ring-size-max ring-size) ring-size))