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

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

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'
 





Reply | Threaded
Open this post in threaded view
|

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

Drew Adams
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'
>  
>
>
>
>
>
>

throw.el (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Drew Adams
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'
> >  
> >
> >
> >
> >
> >
> >
>

throw.el (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Andrew Hyatt-2
In reply to this post by Drew Adams
"Drew Adams" <[hidden email]> 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'
>  



Reply | Threaded
Open this post in threaded view
|

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).



Reply | Threaded
Open this post in threaded view
|

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

Andrew Hyatt-2
Drew Adams <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|

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



Reply | Threaded
Open this post in threaded view
|

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

Mauro Aranda
In reply to this post by Drew Adams
tags 976 patch
quit

"Drew Adams" <[hidden email]> 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.

0001-Fix-finding-filelist-for-tree-fileset-Bug-976.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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.



Reply | Threaded
Open this post in threaded view
|

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

Mauro Aranda
In reply to this post by Drew Adams
tags 976 fixed
close 976 28.1
quit


Drew Adams <[hidden email]> 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.