bug#8280: 24.0.50; ibuffer: "filters to filter group" broken

bug#8280: 24.0.50; ibuffer: "filters to filter group" broken

[Rafaël Fourquet]

This bug is introduced by the fix of bug #7969, please tell me if I
should have re-opened this one.

The infinite loop indicated in #7969 was fixed in file ibuf-ext.el
(commit 81911782 in git, Feb 2) by clearing all the filter groups
(ibuffer-filter-groups) in addition to the active filters
(ibuffer-filtering-qualifiers), in the function ibuffer-filter-disable.
I am not sure that this is what this function was intended to do. At
least it introduces an unwanted behavior: the function
ibuffer-filters-to-filter-group is supposed to transform active filters
into a filtering group, so it calls ibuffer-filter-disable once those
filters have been set as groups. But now, this last function clears
everything.

This can be solved by adding an optional parameter in
ibuffer-filter-disable, or by replacing at the call site in
ibuffer-included-in-filter-p-1 the call to ibuffer-filter-disable by:

 (setq ibuffer-filtering-qualifiers nil)
 (setq ibuffer-filter-groups nil)
 (ibuffer-update nil t)

But maybe removing all the cleaning at all, but keeping the error or
returning nil (so an invalid filter becomes an always false filter), or
asking the user, is better? So a user who has set complex filters
interactively won't loose everything if she tries to load an invalid
filter.

More generally, I think that the parser should be more tolerant and
accept the example given in the original message of #7969. The idea
is that we have operator 'not, 'or, 'saved, but also a last kind of
operator, 'and, which is implicit, but currently only allowed at the top
level, it simply consists of a list of other filters. It is easy enough
to create explicitly an 'and qualifier, for example:

 (define-ibuffer-filter and
    "and filter"
  (:description "and")
  (ibuffer-included-in-filters-p buf qualifier))

But why not extend the built-in behavior? Here is a patch, not well
tested, but I'm just new to ibuffer and elisp in general, so I really
don't know if it breaks something else. I tried to change as little as
possible, but maybe the functions ibuffer-included-in-filters-p,
ibuffer-included-in-filter-p, and ibuffer-included-in-filter-p-1 could
be merged into one function or two (I may do this). It
also updates ibuffer-format-qualifier and ibuffer-decompose-filter
accordingly. Let me know if you see something else to update. The last
part of the patch corrects a minor bug in a related function (a sanity
check was done on the wrong variable).

Thanks to the developers for this great piece of code.



--- emacs/lisp/ibuf-ext.el.orig	2011-03-18 11:58:11.000000000 +0100
+++ emacs/lisp/ibuf-ext.el	2011-03-18 13:35:40.000000000 +0100
@@ -488,9 +488,12 @@
 	  filters))))
 
 (defun ibuffer-included-in-filter-p (buf filter)
-  (if (eq (car filter) 'not)
-      (not (ibuffer-included-in-filter-p-1 buf (cdr filter)))
-    (ibuffer-included-in-filter-p-1 buf filter)))
+  (if (consp (car filter))
+      ;; an implicit "and": multiple filters
+      (ibuffer-included-in-filters-p buf filter)
+    (if (eq (car filter) 'not)
+        (not (ibuffer-included-in-filter-p buf (cdr filter)))
+      (ibuffer-included-in-filter-p-1 buf filter))))
 
 (defun ibuffer-included-in-filter-p-1 (buf filter)
   (not
@@ -505,7 +508,6 @@
 	      (assoc (cdr filter)
 		     ibuffer-saved-filters)))
 	 (unless data
-	   (ibuffer-filter-disable)
 	   (error "Unknown saved filter %s" (cdr filter)))
 	 (ibuffer-included-in-filters-p buf (cadr data))))
       (t
@@ -514,7 +516,6 @@
 	 ;; filterdat should be like (TYPE DESCRIPTION FUNC)
 	 ;; just a sanity check
 	(unless filterdat
-	  (ibuffer-filter-disable)
 	  (error "Undefined filter %s" (car filter)))
 	(not
 	 (not
@@ -771,8 +772,7 @@
 (defun ibuffer-filter-disable ()
   "Disable all filters currently in effect in this buffer."
   (interactive)
-  (setq ibuffer-filtering-qualifiers nil
-	ibuffer-filter-groups nil)
+  (setq ibuffer-filtering-qualifiers nil)
   (let ((buf (ibuffer-current-buffer)))
     (ibuffer-update nil t)
     (when buf
@@ -824,7 +824,10 @@
        (push (cdr lim)
 	     ibuffer-filtering-qualifiers))
       (t
-       (error "Filter type %s is not compound" (car lim)))))
+       (if (consp (car lim))
+           (setq ibuffer-filtering-qualifiers
+                 (append lim ibuffer-filtering-qualifiers))
+         (error "Filter type %s is not compound" (car lim))))))
   (ibuffer-update nil t))
 
 ;;;###autoload
@@ -950,9 +953,11 @@
 				 " "))))
 
 (defun ibuffer-format-qualifier (qualifier)
-  (if (eq (car-safe qualifier) 'not)
-      (concat " [NOT" (ibuffer-format-qualifier-1 (cdr qualifier)) "]")
-    (ibuffer-format-qualifier-1 qualifier)))
+  (if (consp (car-safe qualifier))
+      (concat " [" (mapconcat #'ibuffer-format-qualifier qualifier " ") "]")
+    (if (eq (car-safe qualifier) 'not)
+        (concat " [NOT" (ibuffer-format-qualifier (cdr qualifier)) "]")
+      (ibuffer-format-qualifier-1 qualifier))))
 
 (defun ibuffer-format-qualifier-1 (qualifier)
   (case (car qualifier)
@@ -963,7 +968,7 @@
 			       (cdr qualifier) "") "]"))
     (t
      (let ((type (assq (car qualifier) ibuffer-filtering-alist)))
-       (unless qualifier
+       (unless type
 	 (error "Ibuffer: bad qualifier %s" qualifier))
        (concat " [" (cadr type) ": " (format "%s]" (cdr qualifier)))))))

bug#8280: 24.0.50; ibuffer: "filters to filter group" broken

[Chong Yidong]

fourquet.d@gmail.com (Rafaël Fourquet) writes:

> This bug is introduced by the fix of bug #7969, please tell me if I
> should have re-opened this one.
>
> The infinite loop indicated in #7969 was fixed in file ibuf-ext.el
> (commit 81911782 in git, Feb 2) by clearing all the filter groups
> (ibuffer-filter-groups) in addition to the active filters
> (ibuffer-filtering-qualifiers), in the function ibuffer-filter-disable.
> I am not sure that this is what this function was intended to do. At
> least it introduces an unwanted behavior: the function
> ibuffer-filters-to-filter-group is supposed to transform active filters
> into a filtering group, so it calls ibuffer-filter-disable once those
> filters have been set as groups. But now, this last function clears
> everything.

Thanks for the analysis.

I've committed the more conservative fix you suggested, i.e. adding an
optional arg to ibuffer-filter-disable and using it in
ibuffer-included-in-filter-p-1.