bug#25049: ibuffer bug when saving existing filter, with patches

[Christopher Genovese <genovese@cmu.edu>]

[-- Attachment #1.1: Type: text/plain, Size: 782 bytes --]

I've attached a modified patch file that includes
all your suggested changes.  I did some squashing
and editing, so this patch has the same three parts
as before, with properly formatted Change Logs in
each.

Let me know if you think anything else is needed.

Thanks, Chris


On Tue, Nov 29, 2016 at 6:49 PM, Tino Calancha <tino.calancha@gmail.com>
wrote:

> Christopher Genovese <genovese@cmu.edu> writes:
>
> > Do you want me to submit a new patch with these changes?
> It would be useful.  Someone other than me may try this patch
> without reading all the bug report, and repeat some of my comments.
>
> Once we update the patch we can wait like 1 week; if we don't
> get further comments after that week, then it's OK for me to push
> the fix to master branch.
>
> Thank you
>

[-- Attachment #1.2: Type: text/html, Size: 1305 bytes --]

[-- Attachment #2: ibuffer-saved-filters-bug-clean.patch --]
[-- Type: text/x-diff, Size: 18130 bytes --]

From 53efb62b6bddb40b5bf460c2cc6bdc8b294b27e9 Mon Sep 17 00:00:00 2001
From: "Christopher R. Genovese" <genovese@cmu.edu>
Date: Sun, 27 Nov 2016 23:34:50 -0500
Subject: [PATCH 1/3] ibuffer: one possible fix for bug when saving existing
 filter

The function 'ibuffer-save-filters' handles saving existing
filters and new filters inconsistently. Specifically,
at the following point in the original function:

  (ibuffer-aif (assoc name ibuffer-saved-filters)
      (setcdr it filters)
    (push (list name filters) ibuffer-saved-filters))

the setcdr (existing filters) and the push (new filters)
save different formats for the variable, with the latter
having an extra layer of parentheses.

As a specific example of failure, using the current default
value of 'ibuffer-saved-filters'

        (("gnus"
          ((or
            (mode . message-mode)
            (mode . mail-mode)
            (mode . gnus-group-mode)
            (mode . gnus-summary-mode)
            (mode . gnus-article-mode))))
         ("programming"
          ((or
            (mode . emacs-lisp-mode)
            (mode . cperl-mode)
            (mode . c-mode)
            (mode . java-mode)
            (mode . idl-mode)
            (mode . lisp-mode)))))

and doing

   (ibuffer-save-filters "foo" '((name . "foo") (derived-mode . text-mode)))
   (ibuffer-save-filters "gnus" '((filename . ".")
                                  (or (derived-mode . prog-mode)
                                      (mode . "compilation-mode"))))

gives the following incorrect value for `ibuffer-saved-filters'

        (("foo"
          ((name . "foo")
           (derived-mode . text-mode)))
         ("gnus"
          (filename . ".")
          (or
           (derived-mode . prog-mode)
           (mode . "compilation-mode")))
         ("programming"
          ((or
            (mode . emacs-lisp-mode)
            (mode . cperl-mode)
            (mode . c-mode)
            (mode . java-mode)
            (mode . idl-mode)
            (mode . lisp-mode)))))

because the "foo" and "gnus" entries have different formats,
the latter not consistent with later code to access it
(e.g., in 'ibuffer-included-in-filter-p-1' and 'ibuffer-decompose-filter').

There are two immediate paths for fixing this:

  1. Change the setcdr to add the extra level of nesting.
  2. Change the format of 'ibuffer-saved-filters' to remove
     the level of testing, change the push (list->cons) and
     the later accesses (cadr->cdr).

The first is very simple, but the extra level of nesting
is unsightly, inconsistent with the structure of filter groups,
and mildly annoying when writing filters by hand.  The
second is fairly simple, requiring only a few more small changes,
but introduces the complication of changing an existing
variable's format. For what it's worth, I prefer the second.

This commit takes the first choice.

Change Log:

2016-11-27 Christopher R. Genovese  <genovese@cmu.edu>

* lisp/ibuf-ext.el (ibuffer-save-filters): Add extra
level of nesting when saving filter to an existing name.
---
 lisp/ibuf-ext.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/ibuf-ext.el b/lisp/ibuf-ext.el
index 5ef0746..103232c 100644
--- a/lisp/ibuf-ext.el
+++ b/lisp/ibuf-ext.el
@@ -935,7 +935,7 @@ Interactively, prompt for NAME, and use the current filters."
       (read-from-minibuffer "Save current filters as: ")
       ibuffer-filtering-qualifiers)))
   (ibuffer-aif (assoc name ibuffer-saved-filters)
-      (setcdr it filters)
+      (setcdr it (list filters))
     (push (list name filters) ibuffer-saved-filters))
   (ibuffer-maybe-save-stuff))
 
-- 
2.10.0


From dd57dffcec0030861c4fb95340fbc081d233e5c2 Mon Sep 17 00:00:00 2001
From: "Christopher R. Genovese" <genovese@cmu.edu>
Date: Mon, 28 Nov 2016 00:33:36 -0500
Subject: [PATCH 2/3] ibuffer: another fix for bug when saving an existing
 filter

As described in the previous commit, the function 'ibuffer-save-filters'
handles saving existing filters and new filters inconsistently,
and there are two paths to fixing the problem.

The previous commit fixed the problem while maintaining the format of
the variable 'ibuffer-saved-filters' with its extra level of nesting.

This commit instead takes the second path: removing the extra level of
nesting from the saved filter format.

Change Log:

2016-11-27 Christopher R. Genovese  <genovese@cmu.edu>

* lisp/ibuf-ext.el (ibuffer-saved-filters): Remove extra
nesting level in the alist values and updated docstring.
(ibuffer-save-filters): Remove extra level of nesting
in saved filter alist values when saving new filters.
(ibuffer-included-in-filter-p): Change access of saved
filter data (cadr->cdr) to account for reduced nesting.
(ibuffer-decompose-filter): Change access of saved
filter data (cadr->cdr) to account for reduced nesting.
---
 lisp/ibuf-ext.el | 67 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 33 insertions(+), 34 deletions(-)

diff --git a/lisp/ibuf-ext.el b/lisp/ibuf-ext.el
index 103232c..e4a7dfa 100644
--- a/lisp/ibuf-ext.el
+++ b/lisp/ibuf-ext.el
@@ -120,28 +120,32 @@ Buffers whose major mode is in this list, are not searched."
 (defvar ibuffer-auto-buffers-changed nil)
 
 (defcustom ibuffer-saved-filters '(("gnus"
-				    ((or (mode . message-mode)
-					 (mode . mail-mode)
-					 (mode . gnus-group-mode)
-					 (mode . gnus-summary-mode)
-					 (mode . gnus-article-mode))))
-				   ("programming"
-				    ((or (mode . emacs-lisp-mode)
-					 (mode . cperl-mode)
-					 (mode . c-mode)
-					 (mode . java-mode)
-					 (mode . idl-mode)
-					 (mode . lisp-mode)))))
-
-  "An alist of filter qualifiers to switch between.
-
-This variable should look like ((\"STRING\" QUALIFIERS)
-                                (\"STRING\" QUALIFIERS) ...), where
-QUALIFIERS is a list of the same form as
-`ibuffer-filtering-qualifiers'.
-See also the variables `ibuffer-filtering-qualifiers',
-`ibuffer-filtering-alist', and the functions
-`ibuffer-switch-to-saved-filters', `ibuffer-save-filters'."
+                                    (or (mode . message-mode)
+                                        (mode . mail-mode)
+                                        (mode . gnus-group-mode)
+                                        (mode . gnus-summary-mode)
+                                        (mode . gnus-article-mode)))
+                                   ("programming"
+                                    (or (mode . emacs-lisp-mode)
+                                        (mode . cperl-mode)
+                                        (mode . c-mode)
+                                        (mode . java-mode)
+                                        (mode . idl-mode)
+                                        (mode . lisp-mode))))
+
+  "An alist mapping saved filter names to filter specifications.
+
+Each element should look like (\"NAME\" . FILTER-LIST), where
+FILTER-LIST has the same structure as the variable
+`ibuffer-filtering-qualifiers', which see. The filters defined
+here are joined with an implicit logical `and' and associated
+with NAME. The combined specification can be used by name in
+other filter specifications via the `saved' qualifier (again, see
+`ibuffer-filtering-qualifiers'). They can also be switched to by
+name (see the functions `ibuffer-switch-to-saved-filters' and
+`ibuffer-save-filters'). The variable `ibuffer-save-with-custom'
+affects how this information is saved for future sessions. This
+variable can be set directly from lisp code."
   :type '(repeat sexp)
   :group 'ibuffer)
 
@@ -535,13 +539,11 @@ To evaluate a form without viewing the buffer, see `ibuffer-do-eval'."
 			   (ibuffer-included-in-filter-p buf x))
 		       (cdr filter))))
       (`saved
-       (let ((data
-	      (assoc (cdr filter)
-		     ibuffer-saved-filters)))
+       (let ((data (assoc (cdr filter) ibuffer-saved-filters)))
 	 (unless data
 	   (ibuffer-filter-disable t)
 	   (error "Unknown saved filter %s" (cdr filter)))
-	 (ibuffer-included-in-filters-p buf (cadr data))))
+	 (ibuffer-included-in-filters-p buf (cdr data))))
       (_
        (pcase-let ((`(,_type ,_desc ,func)
                     (assq (car filter) ibuffer-filtering-alist)))
@@ -849,15 +851,12 @@ turned into two separate filters [name: foo] and [mode: bar-mode]."
 					  (cdr lim)
 					  ibuffer-filtering-qualifiers)))
       (`saved
-       (let ((data
-	      (assoc (cdr lim)
-		     ibuffer-saved-filters)))
+       (let ((data (assoc (cdr lim) ibuffer-saved-filters)))
 	 (unless data
 	   (ibuffer-filter-disable)
 	   (error "Unknown saved filter %s" (cdr lim)))
-	 (setq ibuffer-filtering-qualifiers (append
-					    (cadr data)
-					    ibuffer-filtering-qualifiers))))
+	 (setq ibuffer-filtering-qualifiers
+               (append (cdr data) ibuffer-filtering-qualifiers))))
       (`not
        (push (cdr lim)
 	     ibuffer-filtering-qualifiers))
@@ -935,8 +934,8 @@ Interactively, prompt for NAME, and use the current filters."
       (read-from-minibuffer "Save current filters as: ")
       ibuffer-filtering-qualifiers)))
   (ibuffer-aif (assoc name ibuffer-saved-filters)
-      (setcdr it (list filters))
-    (push (list name filters) ibuffer-saved-filters))
+      (setcdr it filters)
+    (push (cons name filters) ibuffer-saved-filters))
   (ibuffer-maybe-save-stuff))
 
 ;;;###autoload
-- 
2.10.0


From 2d8b0044deaf0570edbb97fd63519857696fde07 Mon Sep 17 00:00:00 2001
From: "Christopher R. Genovese" <genovese@cmu.edu>
Date: Mon, 28 Nov 2016 01:29:04 -0500
Subject: [PATCH 3/3] ibuffer: add support for saved filter format change and
 test

As described in the previous commit, one fix to the
inconsistency in 'ibuffer-save-filters' involves simplifying
the format of 'ibuffer-saved-filters' to reduce the extra
nesting level. This adds some support for this transition,
including a customize setter to transparently update old
format values and a command to check and repair the saved
values if desired.

This also adds a test of 'ibuffer-save-filter'.

Change Log:

2016-11-27 Christopher R. Genovese  <genovese@cmu.edu>

* lisp/ibuf-ext.el (ibuffer-saved-filters): Add more accurate
customization type and transparent setter to adjust old-format
values.
(ibuffer-update-saved-filters-format): Update old-format
for saved buffer data to new format with reduced nesting level.
(ibuffer-repair-saved-filters): Add new command to check and
repair saved filters format.
(ibuffer-old-saved-filters-warning): Add new variable with
clickable message with repair options to be displayed
as a warning if 'ibuffer-repair-saved-filters' detects
a format mismatch.
* test/lisp/ibuffer-tests.el (ibuffer-save-filters): Add
a test that filters are saved in the proper format.
---
 lisp/ibuf-ext.el           | 95 +++++++++++++++++++++++++++++++++++++++++++++-
 test/lisp/ibuffer-tests.el | 29 ++++++++++++++
 2 files changed, 122 insertions(+), 2 deletions(-)

diff --git a/lisp/ibuf-ext.el b/lisp/ibuf-ext.el
index e4a7dfa..3fa47c0 100644
--- a/lisp/ibuf-ext.el
+++ b/lisp/ibuf-ext.el
@@ -35,7 +35,8 @@
 
 (eval-when-compile
   (require 'ibuf-macs)
-  (require 'cl-lib))
+  (require 'cl-lib)
+  (require 'subr-x))
 
 ;;; Utility functions
 (defun ibuffer-delete-alist (key alist)
@@ -119,6 +120,26 @@ Buffers whose major mode is in this list, are not searched."
 
 (defvar ibuffer-auto-buffers-changed nil)
 
+(defun ibuffer-update-saved-filters-format (filters)
+  "Transforms alist from old to new `ibuffer-saved-filters' format.
+
+Specifically, converts old-format alist with values of the
+form (STRING (FILTER-SPECS...)) to alist with values of the
+form (STRING FILTER-SPECS...), where each filter spec should be a
+cons cell with a symbol in the car. Any elements in the latter
+form are kept as is.
+
+Returns (OLD-FORMAT-DETECTED . UPDATED-SAVED-FILTERS-LIST)."
+  (when filters
+    (let* ((old-format-detected nil)
+           (fix-filter (lambda (filter-spec)
+                         (if (symbolp (car (cadr filter-spec)))
+                             filter-spec
+                           (setq old-format-detected t) ; side-effect
+                           (cons (car filter-spec) (cadr filter-spec)))))
+           (fixed (mapcar fix-filter filters)))
+      (cons old-format-detected fixed))))
+
 (defcustom ibuffer-saved-filters '(("gnus"
                                     (or (mode . message-mode)
                                         (mode . mail-mode)
@@ -146,9 +167,79 @@ name (see the functions `ibuffer-switch-to-saved-filters' and
 `ibuffer-save-filters'). The variable `ibuffer-save-with-custom'
 affects how this information is saved for future sessions. This
 variable can be set directly from lisp code."
-  :type '(repeat sexp)
+  :version "26.1"
+  :type '(alist :key-type (string :tag "Filter name")
+                :value-type (repeat :tag "Filter specification" sexp))
+  :set (lambda (symbol value)
+         ;; Just set-default but update legacy old-style format
+         (set-default symbol (cdr (ibuffer-update-saved-filters-format value))))
   :group 'ibuffer)
 
+(defvar ibuffer-old-saved-filters-warning
+  (concat "Deprecated format detected for variable `ibuffer-saved-filters'.
+
+The format has been repaired and the variable modified accordingly.
+You can save the current value through the customize system by
+either clicking or hitting return "
+          (make-text-button
+           "here" nil
+           'face '(:weight bold :inherit button)
+           'mouse-face '(:weight normal :background "gray50" :inherit button)
+           'follow-link t
+           'help-echo "Click or RET: save new value in customize"
+           'action (lambda (b)
+                     (if (not (fboundp 'customize-save-variable))
+                         (message "Customize not available; value not saved")
+                       (customize-save-variable 'ibuffer-saved-filters
+                                                ibuffer-saved-filters)
+                       (message "Saved updated ibuffer-saved-filters."))))
+          ". See below for
+an explanation and alternative ways to save the repaired value.
+
+Explanation: For the list variable `ibuffer-saved-filters',
+elements of the form (STRING (FILTER-SPECS...)) are deprecated
+and should instead have the form (STRING FILTER-SPECS...), where
+each filter spec is a cons cell with a symbol in the car. See
+`ibuffer-saved-filters' for details. The repaired value fixes
+this format without changing the meaning of the saved filters.
+
+Alternative ways to save the repaired value:
+
+  1. Do M-x customize-variable and entering `ibuffer-saved-filters'
+     when prompted.
+
+  2. Set the updated value manually by copying the
+     following emacs-lisp form to your emacs init file.
+
+%s
+"))
+
+(defun ibuffer-repair-saved-filters ()
+  "Updates `ibuffer-saved-filters' to its new-style format, if needed.
+
+If this list has any elements of the old-style format, a
+deprecation warning is raised, with a button allowing persistent
+update. Any updated filters retain their meaning in the new
+format. See `ibuffer-update-saved-filters-format' and
+`ibuffer-saved-filters' for details of the old and new formats."
+  (interactive)
+  (when (and (boundp 'ibuffer-saved-filters) ibuffer-saved-filters)
+    (let ((fixed (ibuffer-update-saved-filters-format ibuffer-saved-filters)))
+      (prog1
+          (setq ibuffer-saved-filters (cdr fixed))
+        (when-let ((old-format-detected (car fixed))
+                   (warning-buffer-name "*Warnings*"))
+          (with-current-buffer (get-buffer-create warning-buffer-name)
+            (help-mode))
+          (let ((warning-series t)
+                (updated-form
+                 (with-output-to-string
+                   (pp `(setq ibuffer-saved-filters ',ibuffer-saved-filters)))))
+            (display-warning
+             'ibuffer
+             (format ibuffer-old-saved-filters-warning updated-form)
+             nil warning-buffer-name)))))))
+
 (defvar ibuffer-filtering-qualifiers nil
   "A list like (SYMBOL . QUALIFIER) which filters the current buffer list.
 See also `ibuffer-filtering-alist'.")
diff --git a/test/lisp/ibuffer-tests.el b/test/lisp/ibuffer-tests.el
index 3a4def3..6d5187a 100644
--- a/test/lisp/ibuffer-tests.el
+++ b/test/lisp/ibuffer-tests.el
@@ -66,5 +66,34 @@
       (mapc (lambda (buf) (when (buffer-live-p buf)
                             (kill-buffer buf))) (list buf1 buf2)))))
 
+(ert-deftest ibuffer-save-filters ()
+  "Tests that `ibuffer-save-filters' saves in the proper format."
+  (skip-unless (featurep 'ibuf-ext))
+  (let ((ibuffer-save-with-custom nil)
+        (ibuffer-saved-filters nil)
+        (test1 '((mode . org-mode)
+                 (or (size-gt . 10000)
+                     (and (not (starred-name))
+                          (directory . "\<org\>")))))
+        (test2 '((or (mode . emacs-lisp-mode) (file-extension . "elc?")
+                     (and (starred-name) (name . "elisp"))
+                     (mode . lisp-interaction-mode))))
+        (test3 '((size-lt . 100) (derived-mode . prog-mode)
+                 (or (filename . "scratch")
+                     (filename . "bonz")
+                     (filename . "temp")))))
+    (ibuffer-save-filters "test1" test1)
+    (should (equal (car ibuffer-saved-filters) (cons "test1" test1)))
+    (ibuffer-save-filters "test2" test2)
+    (should (equal (car ibuffer-saved-filters) (cons "test2" test2)))
+    (should (equal (cadr ibuffer-saved-filters) (cons "test1" test1)))
+    (ibuffer-save-filters "test3" test3)
+    (should (equal (car ibuffer-saved-filters) (cons "test3" test3)))
+    (should (equal (cadr ibuffer-saved-filters) (cons "test2" test2)))
+    (should (equal (car (cddr ibuffer-saved-filters)) (cons "test1" test1)))
+    (should (equal (cdr (assoc "test1" ibuffer-saved-filters)) test1))
+    (should (equal (cdr (assoc "test2" ibuffer-saved-filters)) test2))
+    (should (equal (cdr (assoc "test3" ibuffer-saved-filters)) test3))))
+
 (provide 'ibuffer-tests)
 ;; ibuffer-tests.el ends here
-- 
2.10.0