bug#976: 23.0.60; incorrect code for filesets-get-filelist

bug#976: 23.0.60; incorrect code for filesets-get-filelist

[Drew Adams]

The part that treats a :tree of the code defining
`filesets-get-filelist' is not correct and never could have been
correct. And it does not correspond to the (correct) code from the
filesets author.  One wonders if the GNU Emacs code was ever tested.
 
This is the `case' clause that treats :tree in the definition
of `filesets-get-filelist':
 
((:tree)
 (let ((dir  (nth 0 entry))
       (patt (nth 1 entry)))
   (filesets-directory-files dir patt ':files t)))
 
But `entry' here is a complete fileset, which is of the form
("my-fs" (:tree "/some/directory" "^.+\.suffix$"))
 
The above code thus tries to use "my-fs" as the directory, whereas it
should use "/some/directory".
 
This is the (correct) code in the latest version from the author
(http://members.a1.net/t.link/CompEmacsFilesets.html). (The comment is
from the author.)
 
((:tree)
 ;;well, the way trees are handled is a mess +++
 (let* ((dirpatt (if (consp (nth 1 entry))
                     (filesets-entry-get-tree entry)
                   entry))
        (dir     (nth 0 dirpatt))
        (patt    (nth 1 dirpatt)))
   (filesets-list-dir dir patt ':files t)))
 
However, I think the following would be sufficient:
 
((:tree)
 (let* ((dirpatt (filesets-entry-get-tree entry))
        (dir  (nth 0 dirpatt))
        (patt (nth 1 dirpatt)))
   (filesets-directory-files dir patt ':files t)))
 
I don't see why the author's more complex treatment would ever be
needed, since in order for the :tree clause of the `case' to be
reached (consp (nth 1 entry)) must be a cons, AFAICT.
 
At any rate, either the author's code or what I suggest immediately
above is needed. There is no way that the current GNU Emacs code can
work with a :tree fileset.


In GNU Emacs 23.0.60.1 (i386-mingw-nt5.1.2600)
 of 2008-09-03 on LENNART-69DE564
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (3.4) --no-opt --cflags -Ic:/g/include
-fno-crossjumping'
 

bug#976: 23.0.60; incorrect code for filesets-get-filelist

[Drew Adams]

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

The attached code might help you in fixing this - if not, ignore it. I haven't
tested all of the code that calls `filesets-get-filelist', but the version of
`filesets-get-filelist' attached seems to DTRT, AFAICT.

The problem with the original code, and with the suggestion I sent earlier, is
that `filesets-get-filelist' does not DTRT for type :tree. The original code is
completely broken - does nothing here. In my previously suggested code,
`filesets-get-filelist' does the same thing for type :tree as for type :pattern
- it returns only the files in the directory, not also the files in its
subdirectories.

See attached file for proposed new definition (with new function
filesets-files-under).

HTH


> From: Drew Adams Sent: Saturday, September 13, 2008 9:41 AM 
> The part that treats a :tree of the code defining
> `filesets-get-filelist' is not correct and never could have been
> correct. And it does not correspond to the (correct) code from the
> filesets author.  One wonders if the GNU Emacs code was ever tested.
>  
> This is the `case' clause that treats :tree in the definition
> of `filesets-get-filelist':
>  
> ((:tree)
>  (let ((dir  (nth 0 entry))
>        (patt (nth 1 entry)))
>    (filesets-directory-files dir patt ':files t)))
>  
> But `entry' here is a complete fileset, which is of the form
> ("my-fs" (:tree "/some/directory" "^.+\.suffix$"))
>  
> The above code thus tries to use "my-fs" as the directory, whereas it
> should use "/some/directory".
>  
> This is the (correct) code in the latest version from the author
> (http://members.a1.net/t.link/CompEmacsFilesets.html). (The comment is
> from the author.)
>  
> ((:tree)
>  ;;well, the way trees are handled is a mess +++
>  (let* ((dirpatt (if (consp (nth 1 entry))
>                      (filesets-entry-get-tree entry)
>                    entry))
>         (dir     (nth 0 dirpatt))
>         (patt    (nth 1 dirpatt)))
>    (filesets-list-dir dir patt ':files t)))
>  
> However, I think the following would be sufficient:
>  
> ((:tree)
>  (let* ((dirpatt (filesets-entry-get-tree entry))
>         (dir  (nth 0 dirpatt))
>         (patt (nth 1 dirpatt)))
>    (filesets-directory-files dir patt ':files t)))
>  
> I don't see why the author's more complex treatment would ever be
> needed, since in order for the :tree clause of the `case' to be
> reached (consp (nth 1 entry)) must be a cons, AFAICT.
>  
> At any rate, either the author's code or what I suggest immediately
> above is needed. There is no way that the current GNU Emacs code can
> work with a :tree fileset.
> 
> 
> In GNU Emacs 23.0.60.1 (i386-mingw-nt5.1.2600)
>  of 2008-09-03 on LENNART-69DE564
> Windowing system distributor `Microsoft Corp.', version 5.1.2600
> configured using `configure --with-gcc (3.4) --no-opt 
> --cflags -Ic:/g/include
> -fno-crossjumping'
>  
> 
> 
> 
> 
> 
> 

[-- Attachment #2: throw.el --]
[-- Type: application/octet-stream, Size: 2931 bytes --]

(defun filesets-get-filelist (entry &optional mode event)
  "Get all files for fileset ENTRY.
Assume MODE (see `filesets-entry-mode'), if provided."
  (let* ((mode (or mode (filesets-entry-mode entry)))
         (fl (case mode
               ((:files)   (filesets-entry-get-files entry))
               ((:file)    (list (filesets-entry-get-file entry)))
               ((:ingroup)
                (let ((entry (expand-file-name
                              (if (stringp entry)
                                  entry
                                (filesets-entry-get-master entry)))))
                  (cons entry (filesets-ingroup-cache-get entry))))
               ((:tree)
                (let* ((dirpatt  (filesets-entry-get-tree entry))
                       (dir      (nth 0 dirpatt))
                       (patt     (nth 1 dirpatt))
                       (depth    (or (filesets-entry-get-tree-max-level entry)
                                     filesets-tree-max-level)))
                  (filesets-files-under 0 depth entry dir patt)))
               ((:pattern)
                (let ((dirpatt (filesets-entry-get-pattern entry)))
                  (if dirpatt
                      (let ((dir (filesets-entry-get-pattern--dir dirpatt))
                            (patt (filesets-entry-get-pattern--pattern dirpatt)))
                        (filesets-directory-files dir patt ':files t))
                    (filesets-error 'error "Filesets: malformed entry: "
                                    entry)))))))
    (filesets-filter-list
     fl (lambda (file) (not (filesets-filetype-property file event))))))

(defun filesets-files-under (level depth entry dir patt &optional absolutep)
  "Files under DIR that match PATT.
LEVEL is the current level under DIR.
DEPTH is the maximal tree scanning depth for ENTRY.
ENTRY is a fileset.
DIR is a directory.
PATT is a regexp that included file names must match.
ABSOLUTEP non-nil means use absolute file names."
  (and (or (= depth 0) (< level depth))
       (let* ((dir         (file-name-as-directory dir))
              (files-here  (filesets-directory-files
                            dir patt nil absolutep
                            (filesets-entry-get-filter-dirs-flag entry)))
              (subdirs     (filesets-filter-dir-names files-here))
              (files
               (filesets-filter-dir-names
                (apply #'append
                       files-here
                       (mapcar
                        (lambda (subdir)
                          (let* ((subdir (file-name-as-directory subdir))
                                 (full-subdir  (concat dir subdir)))
                            (filesets-files-under (+ level 1) depth entry
                                         full-subdir patt)))
                        subdirs))
                t)))
         files)))

bug#976: 23.0.60; incorrect code for filesets-get-filelist

[Drew Adams]

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

Sorry for the added noise, but it's probably better for the default behavior to
use absolute filenames. The attached code does that.


> From: Drew Adams Sent: Tuesday, September 16, 2008 1:44 PM
> The attached code might help you in fixing this - if not, 
> ignore it. I haven't
> tested all of the code that calls `filesets-get-filelist', 
> but the version of
> `filesets-get-filelist' attached seems to DTRT, AFAICT.
> 
> The problem with the original code, and with the suggestion I 
> sent earlier, is
> that `filesets-get-filelist' does not DTRT for type :tree. 
> The original code is
> completely broken - does nothing here. In my previously 
> suggested code,
> `filesets-get-filelist' does the same thing for type :tree as 
> for type :pattern
> - it returns only the files in the directory, not also the 
> files in its
> subdirectories.
> 
> See attached file for proposed new definition (with new function
> filesets-files-under).
> 
> HTH
> 
> 
> > From: Drew Adams Sent: Saturday, September 13, 2008 9:41 AM 
> > The part that treats a :tree of the code defining
> > `filesets-get-filelist' is not correct and never could have been
> > correct. And it does not correspond to the (correct) code from the
> > filesets author.  One wonders if the GNU Emacs code was ever tested.
> >  
> > This is the `case' clause that treats :tree in the definition
> > of `filesets-get-filelist':
> >  
> > ((:tree)
> >  (let ((dir  (nth 0 entry))
> >        (patt (nth 1 entry)))
> >    (filesets-directory-files dir patt ':files t)))
> >  
> > But `entry' here is a complete fileset, which is of the form
> > ("my-fs" (:tree "/some/directory" "^.+\.suffix$"))
> >  
> > The above code thus tries to use "my-fs" as the directory, 
> whereas it
> > should use "/some/directory".
> >  
> > This is the (correct) code in the latest version from the author
> > (http://members.a1.net/t.link/CompEmacsFilesets.html). (The 
> comment is
> > from the author.)
> >  
> > ((:tree)
> >  ;;well, the way trees are handled is a mess +++
> >  (let* ((dirpatt (if (consp (nth 1 entry))
> >                      (filesets-entry-get-tree entry)
> >                    entry))
> >         (dir     (nth 0 dirpatt))
> >         (patt    (nth 1 dirpatt)))
> >    (filesets-list-dir dir patt ':files t)))
> >  
> > However, I think the following would be sufficient:
> >  
> > ((:tree)
> >  (let* ((dirpatt (filesets-entry-get-tree entry))
> >         (dir  (nth 0 dirpatt))
> >         (patt (nth 1 dirpatt)))
> >    (filesets-directory-files dir patt ':files t)))
> >  
> > I don't see why the author's more complex treatment would ever be
> > needed, since in order for the :tree clause of the `case' to be
> > reached (consp (nth 1 entry)) must be a cons, AFAICT.
> >  
> > At any rate, either the author's code or what I suggest immediately
> > above is needed. There is no way that the current GNU Emacs code can
> > work with a :tree fileset.
> > 
> > 
> > In GNU Emacs 23.0.60.1 (i386-mingw-nt5.1.2600)
> >  of 2008-09-03 on LENNART-69DE564
> > Windowing system distributor `Microsoft Corp.', version 5.1.2600
> > configured using `configure --with-gcc (3.4) --no-opt 
> > --cflags -Ic:/g/include
> > -fno-crossjumping'
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> 

[-- Attachment #2: throw.el --]
[-- Type: application/octet-stream, Size: 5631 bytes --]

;;; filesets+.el --- Extensions to `filesets.el'.
;;
;; Filename: filesets+.el
;; Description: Extensions to `filesets.el'.
;; Author: Drew Adams
;; Maintainer: Drew Adams
;; Copyright (C) 2008, Drew Adams, all rights reserved.
;; Created: Tue Sep 16 14:11:36 2008 (-0700)
;; Version: 22.0
;; Last-Updated: Tue Sep 16 14:27:01 2008 (-0700)
;;           By: dradams
;;     Update #: 13
;; URL: http://www.emacswiki.org/cgi-bin/wiki/filesets+.el
;; Keywords:
;; Compatibility: GNU Emacs 22.x
;;
;; Features that might be required by this library:
;;
;;   Required feature throw was not provided.
;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;
;;; Commentary:
;;
;;  This library provides some fixes to standard library
;;  `filesets.el'.  The reference version of that library is 1.8.4,
;;  but I believe the same fixes are appropriate for other versions
;;  (e.g. 2.2, named `filesets2.el', which is the latest version by
;;  the original author, at
;;  http://members.a1.net/t.link/CompEmacsFilesets.html).
;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;
;;; Change log:
;;
;;
;; 2008/09/16 dadams
;;     Created.
;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;
;; This program is free software; you can redistribute it and/or
;; modify it under the terms of the GNU General Public License as
;; published by the Free Software Foundation; either version 3, or
;; (at your option) any later version.
;;
;; This program is distributed in the hope that it will be useful,
;; but WITHOUT ANY WARRANTY; without even the implied warranty of
;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
;; General Public License for more details.
;;
;; You should have received a copy of the GNU General Public License
;; along with this program; see the file COPYING.  If not, write to
;; the Free Software Foundation, Inc., 51 Franklin Street, Fifth
;; Floor, Boston, MA 02110-1301, USA.;;; Code:
;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;
;;; Code:

(require 'filesets)

;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(defcustom filesets-menu-path '("file") ; Original was nil.
  "The menu under which the filesets menu should be inserted.
See `add-submenu' for documentation."
  :set (function filesets-set-default)
  :type 'sexp :group 'filesets)

(defcustom filesets-menu-before "Open File..." ; Original was "File".
  "The name of a menu before which this menu should be added.
See `add-submenu' for documentation."
  :set (function filesets-set-default)
  :type 'sexp :group 'filesets)

(defun filesets-get-filelist (entry &optional mode event)
  "Get all files for fileset ENTRY.
Assume MODE (see `filesets-entry-mode'), if provided."
  (let* ((mode (or mode (filesets-entry-mode entry)))
         (fl (case mode
               ((:files)   (filesets-entry-get-files entry))
               ((:file)    (list (filesets-entry-get-file entry)))
               ((:ingroup)
                (let ((entry (expand-file-name
                              (if (stringp entry)
                                  entry
                                (filesets-entry-get-master entry)))))
                  (cons entry (filesets-ingroup-cache-get entry))))
               ((:tree)
                (let* ((dirpatt  (filesets-entry-get-tree entry))
                       (dir      (nth 0 dirpatt))
                       (patt     (nth 1 dirpatt))
                       (depth    (or (filesets-entry-get-tree-max-level entry)
                                     filesets-tree-max-level)))
                  (filesets-files-under 0 depth entry dir patt)))
               ((:pattern)
                (let ((dirpatt (filesets-entry-get-pattern entry)))
                  (if dirpatt
                      (let ((dir (filesets-entry-get-pattern--dir dirpatt))
                            (patt (filesets-entry-get-pattern--pattern dirpatt)))
                        (filesets-directory-files dir patt ':files t))
                    (filesets-error 'error "Filesets: malformed entry: "
                                    entry)))))))
    (filesets-filter-list
     fl (lambda (file) (not (filesets-filetype-property file event))))))

(defun filesets-files-under (level depth entry dir patt &optional relativep)
  "Files under DIR that match PATT.
LEVEL is the current level under DIR.
DEPTH is the maximal tree scanning depth for ENTRY.
ENTRY is a fileset.
DIR is a directory.
PATT is a regexp that included file names must match.
RELATIVEP non-nil means use relative file names."
  (and (or (= depth 0) (< level depth))
       (let* ((dir         (file-name-as-directory dir))
              (files-here  (filesets-directory-files
                            dir patt nil (not relativep)
                            (filesets-entry-get-filter-dirs-flag entry)))
              (subdirs     (filesets-filter-dir-names files-here))
              (files
               (filesets-filter-dir-names
                (apply #'append
                       files-here
                       (mapcar
                        (lambda (subdir)
                          (let* ((subdir (file-name-as-directory subdir))
                                 (full-subdir  (concat dir subdir)))
                            (filesets-files-under (+ level 1) depth entry
                                         full-subdir patt)))
                        subdirs))
                t)))
         files)))

;;;;;;;;;;;;;;;;;;;;;;;;;;;;

(provide 'filesets+)

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;; filesets+.el ends here

bug#976: 23.0.60; incorrect code for filesets-get-filelist

[cyd]

Hi Thomas,

Could you take a look at the following bug report about filesets.el?

http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=976

Thanks.

bug#976: 23.0.60; incorrect code for filesets-get-filelist

[Thomas Link]

cyd@MIT.EDU wrote:
> Hi Thomas,
>
> Could you take a look at the following bug report about filesets.el?
>
> http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=
Since I don't have emacs accessible right now I'm not sure my comment 
are of much help. What I'm writing below is based on how I remember the 
code worked.

> ((:tree)
>  ;;well, the way trees are handled is a mess +++
>  (let* ((dirpatt (if (consp (nth 1 entry))
>                      (filesets-entry-get-tree entry)
>                    entry))
>         (dir     (nth 0 dirpatt))
>         (patt    (nth 1 dirpatt)))
>    (filesets-list-dir dir patt ':files t)))
>  
> However, I think the following would be sufficient:
>  
> ((:tree)
>  (let* ((dirpatt (filesets-entry-get-tree entry))
>         (dir  (nth 0 dirpatt))
>         (patt (nth 1 dirpatt)))
>    (filesets-directory-files dir patt ':files t)))
>   
It's possible this could be explained with my effort to maintain 
backward-compatibility with older versions of filesets with regard to 
values defined via the defcustom interface. Or maybe a problem with 
filesets-build-dir-submenu-now?

> The original code is completely broken
Well, it definitely worked for me when I last tried it -- but that was 4 
years ago. I rememb



> In my previously suggested code,
> `filesets-get-filelist' does the same thing for type :tree as for type :pattern
> - it returns only the files in the directory, not also the files in its
> subdirectories.
>   
When changing filesets-get-filelist, please also take a look at 
filesets-build-dir-submenu-now, which actually builds the tree in the menu.

:tree rebuilds a directory structure in the menu. It isn't supposed to 
collect all files in a directory recursively into a single fileset. So, 
every directory corresponds to one "virtual" fileset.


> The attached code might help you in fixing this
If I understand it right, this code tries to achieve something slightly 
different in collecting all files under a directory in one single 
fileset. Interesting idea. If emacs glob patterns understood something 
like "dir/**" (I assume it doesn't?), this could be done with :pattern, 
I suppose.


> In particular, I had some problems
> byte-compiling. It would be great if the version (1.8.4) in GNU Emacs could be 
> upgraded.
>   
For the latest version, everything emacs-specific is supposed to be 
located in filesets-emacs.el, which should be loaded instead of 
filesets(2).el. If igrep is xemacs-specific, it's in filesets2.el by 
accident. (It's possible that I didn't test the last version with emacs 
because of problems with cl.)


> There is no explanation of the Filesets menu items. There is an
> `About' item with a link to a non-GNU Web page
This should probably open the info page then.


>  What is the meaning of `#' and `+' in front of the
> submenus
These are keyboard shortcuts for easier navigation. If this makes sense, 
depends on whether the gui toolkit supports accelerator keys.


>  - Filesets Menu In Menu
>   
In case users don't want to clutter the main menu bar with an additional 
menu, users can select a parent menu.

>  - Filesets Menu Shortcuts Marker
>   
If the gui toolkit doesn't support accelerator keys (as they are 
commonly used on ms windows but also kde or gnome), this should be an 
empty string maybe.

>  - Filesets Menu Cache Contents
>  - Filesets Menu Cache Contents (needs to be explained better)
>   

>  - Filesets Cache Hostname Flag
>   
This may be useful if you sync emacs configuration files between hosts.

>  - Filesets Browse Dir Function (external command not clear)
>   

>  - Filesets Find File Delay (what for?)
>   
Xemacs-specific work-around.

>  - Filesets Commands (explanation unclear)
>  - Filesets External Viewers (Properties is especially unclear)
>   
This opens files with external viewers depending of file patterns. Are 
all properties unclear?

>  - Filesets Ingroup Patterns
>   
Recognize include-like statements. Only effective for :ingroup filesets. 
ingroup can be thought of as a logical tree of documents -- regardless 
of where the files are located physically. The root of an ingroup could, 
e.g., be an master tex file.

HTH a little.

bug#976: 23.0.60; incorrect code for filesets-get-filelist

[Andrew Hyatt]

"Drew Adams" <drew.adams@oracle.com> writes:

> The part that treats a :tree of the code defining
> `filesets-get-filelist' is not correct and never could have been
> correct. And it does not correspond to the (correct) code from the
> filesets author.  One wonders if the GNU Emacs code was ever tested.
>  
> This is the `case' clause that treats :tree in the definition
> of `filesets-get-filelist':
>  
> ((:tree)
>  (let ((dir  (nth 0 entry))
>        (patt (nth 1 entry)))
>    (filesets-directory-files dir patt ':files t)))
>  
> But `entry' here is a complete fileset, which is of the form
> ("my-fs" (:tree "/some/directory" "^.+\.suffix$"))
>  
> The above code thus tries to use "my-fs" as the directory, whereas it
> should use "/some/directory".

I'm looking into whether this still exists in Emacs 25.  The code still
is as you describe.  Can you give steps to reproduce a user-visible bug?

>  
> This is the (correct) code in the latest version from the author
> (http://members.a1.net/t.link/CompEmacsFilesets.html). (The comment is
> from the author.)
>  
> ((:tree)
>  ;;well, the way trees are handled is a mess +++
>  (let* ((dirpatt (if (consp (nth 1 entry))
>                      (filesets-entry-get-tree entry)
>                    entry))
>         (dir     (nth 0 dirpatt))
>         (patt    (nth 1 dirpatt)))
>    (filesets-list-dir dir patt ':files t)))
>  
> However, I think the following would be sufficient:
>  
> ((:tree)
>  (let* ((dirpatt (filesets-entry-get-tree entry))
>         (dir  (nth 0 dirpatt))
>         (patt (nth 1 dirpatt)))
>    (filesets-directory-files dir patt ':files t)))
>  
> I don't see why the author's more complex treatment would ever be
> needed, since in order for the :tree clause of the `case' to be
> reached (consp (nth 1 entry)) must be a cons, AFAICT.
>  
> At any rate, either the author's code or what I suggest immediately
> above is needed. There is no way that the current GNU Emacs code can
> work with a :tree fileset.
>
>
> In GNU Emacs 23.0.60.1 (i386-mingw-nt5.1.2600)
>  of 2008-09-03 on LENNART-69DE564
> Windowing system distributor `Microsoft Corp.', version 5.1.2600
> configured using `configure --with-gcc (3.4) --no-opt --cflags -Ic:/g/include
> -fno-crossjumping'
>  

bug#976: 23.0.60; incorrect code for filesets-get-filelist

[Drew Adams]

> I'm looking into whether this still exists in Emacs 25.  The code still
> is as you describe.  Can you give steps to reproduce a user-visible bug?

My guess is that no one has changed the filesets code, so yes,
I'm pretty sure the problem is still there as reported.

The user-visible bug is what I described.  See the original
report, which is quite detailed wrt the problem.

See also the version of `filesets-get-filelist' that I sent (in
attachment throw.el).  It should be a fix.  If it does not fix
everything 100%, it is at least an improvement over the current
code, which is completely broken for :tree.

You can also contact the original author of filesets.el.  As I
said in the bug report, the GNU Emacs version does not respect
what the author wrote, and it cannot possibly work (e.g., for
:tree).

I can't really help more than this now.  I'm sure someone on
the Emacs Dev team can fix it, but there is not much interest
in filesets, it seems.

Apparently there was only one person (Chong Yidong) who took a
look at the bug report, and he just asked someone else to look
into it.  And that person never followed up (didn't even reply
to Yidong's request, apparently).

bug#976: 23.0.60; incorrect code for filesets-get-filelist

[Andrew Hyatt]

Drew Adams <drew.adams@oracle.com> writes:

>> I'm looking into whether this still exists in Emacs 25.  The code still
>> is as you describe.  Can you give steps to reproduce a user-visible bug?
>
> My guess is that no one has changed the filesets code, so yes,
> I'm pretty sure the problem is still there as reported.
>
> The user-visible bug is what I described.  See the original
> report, which is quite detailed wrt the problem.
>
> See also the version of `filesets-get-filelist' that I sent (in
> attachment throw.el).  It should be a fix.  If it does not fix
> everything 100%, it is at least an improvement over the current
> code, which is completely broken for :tree.
>
> You can also contact the original author of filesets.el.  As I
> said in the bug report, the GNU Emacs version does not respect
> what the author wrote, and it cannot possibly work (e.g., for
> :tree).
>
> I can't really help more than this now.  I'm sure someone on
> the Emacs Dev team can fix it, but there is not much interest
> in filesets, it seems.
>
> Apparently there was only one person (Chong Yidong) who took a
> look at the bug report, and he just asked someone else to look
> into it.  And that person never followed up (didn't even reply
> to Yidong's request, apparently).

OK, thanks for the reply.  The original report seems lost (the bug
report at https://debbugs.gnu.org/cgi/bugreport.cgi?bug=976 starts with
your response, in fact).  But given what you've already provided, it
seems that someone familiar with filesets can reproduce and test your
fix.  We'll leave the bug open until someone gets to it.

bug#976: 23.0.60; incorrect code for filesets-get-filelist

[Drew Adams]

> >> I'm looking into whether this still exists in Emacs 25.  The code still
> >> is as you describe.  Can you give steps to reproduce a user-visible bug?
> >
> > My guess is that no one has changed the filesets code, so yes,
> > I'm pretty sure the problem is still there as reported.
> >
> > The user-visible bug is what I described.  See the original
> > report, which is quite detailed wrt the problem.
> >
> > See also the version of `filesets-get-filelist' that I sent (in
> > attachment throw.el).  It should be a fix.  If it does not fix
> > everything 100%, it is at least an improvement over the current
> > code, which is completely broken for :tree.
> >
> > You can also contact the original author of filesets.el.  As I
> > said in the bug report, the GNU Emacs version does not respect
> > what the author wrote, and it cannot possibly work (e.g., for
> > :tree).
> >
> > I can't really help more than this now.  I'm sure someone on
> > the Emacs Dev team can fix it, but there is not much interest
> > in filesets, it seems.
> >
> > Apparently there was only one person (Chong Yidong) who took a
> > look at the bug report, and he just asked someone else to look
> > into it.  And that person never followed up (didn't even reply
> > to Yidong's request, apparently).
> 
> OK, thanks for the reply.  The original report seems lost (the bug
> report at https://debbugs.gnu.org/cgi/bugreport.cgi?bug=976 starts with
> your response, in fact).  But given what you've already provided, it
> seems that someone familiar with filesets can reproduce and test your
> fix.  We'll leave the bug open until someone gets to it.

Hi Andrew,

No, AFAICT the thread at https://debbugs.gnu.org/cgi/bugreport.cgi?bug=976
is complete.  It starts with my bug report, which begins this way,
continuing from the bug title, which is "incorrect code for
filesets-get-filelist":

  The part that treats a :tree of the code defining
  `filesets-get-filelist' is not correct and never could have been
  correct. And it does not correspond to the (correct) code from the
  filesets author.  One wonders if the GNU Emacs code was ever tested.

Thx - Drew

bug#976: 23.0.60; incorrect code for filesets-get-filelist

[Mauro Aranda]

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

tags 976 patch
quit

"Drew Adams" <drew.adams@oracle.com> writes:

> Sorry for the added noise, but it's probably better for the default
behavior to
> use absolute filenames. The attached code does that.
>
>
>> From: Drew Adams Sent: Tuesday, September 16, 2008 1:44 PM
>> The attached code might help you in fixing this - if not,
>> ignore it. I haven't
>> tested all of the code that calls `filesets-get-filelist',
>> but the version of
>> `filesets-get-filelist' attached seems to DTRT, AFAICT.
>>
>> The problem with the original code, and with the suggestion I
>> sent earlier, is
>> that `filesets-get-filelist' does not DTRT for type :tree.
>> The original code is
>> completely broken - does nothing here. In my previously
>> suggested code,
>> `filesets-get-filelist' does the same thing for type :tree as
>> for type :pattern
>> - it returns only the files in the directory, not also the
>> files in its
>> subdirectories.
>>
>> See attached file for proposed new definition (with new function
>> filesets-files-under).
>>
>> HTH
>>
>>
>> > From: Drew Adams Sent: Saturday, September 13, 2008 9:41 AM
>> > The part that treats a :tree of the code defining
>> > `filesets-get-filelist' is not correct and never could have been
>> > correct. And it does not correspond to the (correct) code from the
>> > filesets author.  One wonders if the GNU Emacs code was ever tested.
>> >
>> > This is the `case' clause that treats :tree in the definition
>> > of `filesets-get-filelist':
>> >
>> > ((:tree)
>> >  (let ((dir  (nth 0 entry))
>> >        (patt (nth 1 entry)))
>> >    (filesets-directory-files dir patt ':files t)))
>> >
>> > But `entry' here is a complete fileset, which is of the form
>> > ("my-fs" (:tree "/some/directory" "^.+\.suffix$"))
>> >
>> > The above code thus tries to use "my-fs" as the directory,
>> whereas it
>> > should use "/some/directory".
>> >
>> > This is the (correct) code in the latest version from the author
>> > (http://members.a1.net/t.link/CompEmacsFilesets.html). (The
>> comment is
>> > from the author.)
>> >
>> > ((:tree)
>> >  ;;well, the way trees are handled is a mess +++
>> >  (let* ((dirpatt (if (consp (nth 1 entry))
>> >                      (filesets-entry-get-tree entry)
>> >                    entry))
>> >         (dir     (nth 0 dirpatt))
>> >         (patt    (nth 1 dirpatt)))
>> >    (filesets-list-dir dir patt ':files t)))
>> >
>> > However, I think the following would be sufficient:
>> >
>> > ((:tree)
>> >  (let* ((dirpatt (filesets-entry-get-tree entry))
>> >         (dir  (nth 0 dirpatt))
>> >         (patt (nth 1 dirpatt)))
>> >    (filesets-directory-files dir patt ':files t)))
>> >
>> > I don't see why the author's more complex treatment would ever be
>> > needed, since in order for the :tree clause of the `case' to be
>> > reached (consp (nth 1 entry)) must be a cons, AFAICT.
>> >
>> > At any rate, either the author's code or what I suggest immediately
>> > above is needed. There is no way that the current GNU Emacs code can
>> > work with a :tree fileset.
>> >
>> >
>> > In GNU Emacs 23.0.60.1 (i386-mingw-nt5.1.2600)
>> >  of 2008-09-03 on LENNART-69DE564
>> > Windowing system distributor `Microsoft Corp.', version 5.1.2600
>> > configured using `configure --with-gcc (3.4) --no-opt
>> > --cflags -Ic:/g/include
>> > -fno-crossjumping'
>> >

Hi Drew,

It's been a long time, but your fix looks right to me, and of course
you're right that the current code is broken.  I created a :tree fileset
for testing: without your fix I get wrong-type-argument errors, while
with your fix it works just fine.

I've recreated your fix below, added a commit message and formatted it
with `git format-patch'.  Could you please take a look and see if it
is OK to push it? Or maybe you would like to provide a different commit
message for your change?

Thanks.

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

[-- Attachment #2: 0001-Fix-finding-filelist-for-tree-fileset-Bug-976.patch --]
[-- Type: text/x-patch, Size: 2973 bytes --]

From a35ea838d4ad512bf9fa391b7009d72db423cc15 Mon Sep 17 00:00:00 2001
From: Drew Adams <drew.adams@oracle.com>
Date: Fri, 20 Nov 2020 11:41:41 -0300
Subject: [PATCH] Fix finding filelist for :tree fileset (Bug#976)

* lisp/filesets.el (filesets-files-under): New function, used to get
all files for a :tree fileset.
(filesets-get-filelist): Use it.  Look for the directory and the
pattern in the right place inside entry.
---
 lisp/filesets.el | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/lisp/filesets.el b/lisp/filesets.el
index c7ec3f77f4..84196561fc 100644
--- a/lisp/filesets.el
+++ b/lisp/filesets.el
@@ -1718,9 +1718,12 @@ filesets-get-filelist
 				(filesets-entry-get-master entry)))))
 		  (cons entry (filesets-ingroup-cache-get entry))))
 	       (:tree
-		(let ((dir  (nth 0 entry))
-		      (patt (nth 1 entry)))
-		  (filesets-directory-files dir patt ':files t)))
+                (let* ((dirpatt (filesets-entry-get-tree entry))
+                       (dir (nth 0 dirpatt))
+                       (patt (nth 1 dirpatt))
+                       (depth (or (filesets-entry-get-tree-max-level entry)
+                                  filesets-tree-max-level)))
+                  (filesets-files-under 0 depth entry dir patt)))
 	       (:pattern
 		(let ((dirpatt (filesets-entry-get-pattern entry)))
 		  (if dirpatt
@@ -1734,6 +1737,34 @@ filesets-get-filelist
 			  (lambda (file)
 			    (not (filesets-filetype-property file event))))))
 
+(defun filesets-files-under (level depth entry dir patt &optional relativep)
+  "Files under DIR that match PATT.
+LEVEL is the current level under DIR.
+DEPTH is the maximal tree scanning depth for ENTRY.
+ENTRY is a fileset.
+DIR is a directory.
+PATT is a regexp that included file names must match.
+RELATIVEP non-nil means use relative file names."
+  (and (or (= depth 0) (< level depth))
+       (let* ((dir         (file-name-as-directory dir))
+              (files-here  (filesets-directory-files
+                            dir patt nil (not relativep)
+                            (filesets-entry-get-filter-dirs-flag entry)))
+              (subdirs     (filesets-filter-dir-names files-here))
+              (files
+               (filesets-filter-dir-names
+                (apply #'append
+                       files-here
+                       (mapcar
+                        (lambda (subdir)
+                          (let* ((subdir (file-name-as-directory subdir))
+                                 (full-subdir  (concat dir subdir)))
+                            (filesets-files-under (+ level 1) depth entry
+                                                  full-subdir patt)))
+                        subdirs))
+                t)))
+         files)))
+
 (defun filesets-open (&optional mode name lookup-name)
   "Open the fileset called NAME.
 Use LOOKUP-NAME for searching additional data if provided."
-- 
2.29.2

bug#976: 23.0.60; incorrect code for filesets-get-filelist

[Drew Adams]

> tags 976 patch
> quit
>
> Hi Drew,
>
> It's been a long time, but your fix looks right to me, and of course
> you're right that the current code is broken.  I created a :tree fileset
> for testing: without your fix I get wrong-type-argument errors, while
> with your fix it works just fine.
>
> I've recreated your fix below, added a commit message and formatted it
> with `git format-patch'.  Could you please take a look and see if it
> is OK to push it? Or maybe you would like to provide a different commit
> message for your change?

Hi Mauro.  Thanks for working on this.

It's been so long since I filed this bug that I'm
not really any help on this.  And I really don't
have any time now to help with it or much else wrt
Emacs.

I have complete confidence that whatever you come
up with will be an improvement (and it will likely
be a complete fix).  Please do whatever you think
is needed.  Thx.

bug#976: 23.0.60; incorrect code for filesets-get-filelist

[Mauro Aranda]

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

tags 976 fixed
close 976 28.1
quit


Drew Adams <drew.adams@oracle.com> wrote:

> > tags 976 patch
> > quit
> >
> > Hi Drew,
> >
> > It's been a long time, but your fix looks right to me, and of course
> > you're right that the current code is broken.  I created a :tree fileset
> > for testing: without your fix I get wrong-type-argument errors, while
> > with your fix it works just fine.
> >
> > I've recreated your fix below, added a commit message and formatted it
> > with `git format-patch'.  Could you please take a look and see if it
> > is OK to push it? Or maybe you would like to provide a different commit
> > message for your change?
>
> Hi Mauro.  Thanks for working on this.
>
> It's been so long since I filed this bug that I'm
> not really any help on this.  And I really don't
> have any time now to help with it or much else wrt
> Emacs.

OK, no problem.  I pushed your fix.

> I have complete confidence that whatever you come
> up with will be an improvement (and it will likely
> be a complete fix).  Please do whatever you think
> is needed.  Thx.

Thanks to you.

[-- Attachment #2: Type: text/html, Size: 1421 bytes --]