bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

[Damien Cassou]

Hi,

the documentation of `project-remember-projects-under' is:

      "Index all projects below a directory DIR.  If RECURSIVE is
    non-nil, recurse into all subdirectories to find more projects.
    After finishing, a message is printed summarizing the progress.  The
    function returns the number of detected projects."

Regardless of the value of RECURSIVE, I understand from the above that
all child directories of the DIR argument will be investigated. The doc
doesn't say anything about investigating if DIR is itself a project or
not so I think it would make sense if the function wasn't.

But the code says otherwise (as far as I understand it):

(defun project-remember-projects-under (dir &optional recursive)
  (let ((queue (list dir)))
    ;; …
    (while queue
      (when-let ((subdir (pop queue))
                 ((file-directory-p subdir)))
        ;; maybe register `subdir' as a project
        ;; …
        (when (and recursive (file-directory-p subdir))
          (setq queue (nconc (directory-files subdir …) queue)))))))

The code above seems to investigate DIR first and, if RECURSIVE is
non-nil, look at the directories below it.

Also, the second check (file-directory-p subdir) seems unnecessary
because of the first one.

There is a part of the code I don't understand:

  (unless (eq recursive 'in-progress)

It seems nowhere in the code nor in the documentation do we say anything
about this 'in-progress special value. Is it a left over from a previous
(recursive) version of the algorithm?

Best

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

[Dmitry Gutov]

Hi Damien, thanks for the report.

On 20/10/2023 14:48, Damien Cassou wrote:
> the documentation of `project-remember-projects-under' is:
> 
>        "Index all projects below a directory DIR.  If RECURSIVE is
>      non-nil, recurse into all subdirectories to find more projects.
>      After finishing, a message is printed summarizing the progress.  The
>      function returns the number of detected projects."
> 
> Regardless of the value of RECURSIVE, I understand from the above that
> all child directories of the DIR argument will be investigated. The doc
> doesn't say anything about investigating if DIR is itself a project or
> not so I think it would make sense if the function wasn't.
> 
> But the code says otherwise (as far as I understand it):
> 
> (defun project-remember-projects-under (dir &optional recursive)
>    (let ((queue (list dir)))
>      ;; …
>      (while queue
>        (when-let ((subdir (pop queue))
>                   ((file-directory-p subdir)))
>          ;; maybe register `subdir' as a project
>          ;; …
>          (when (and recursive (file-directory-p subdir))
>            (setq queue (nconc (directory-files subdir …) queue)))))))
> 
> The code above seems to investigate DIR first and, if RECURSIVE is
> non-nil, look at the directories below it.
> 
> Also, the second check (file-directory-p subdir) seems unnecessary
> because of the first one.
> 
> There is a part of the code I don't understand:
> 
>    (unless (eq recursive 'in-progress)
> 
> It seems nowhere in the code nor in the documentation do we say anything
> about this 'in-progress special value. Is it a left over from a previous
> (recursive) version of the algorithm?

Philip, could you look into this?

bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

[Philip Kaludercic]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi Damien, thanks for the report.
>
> On 20/10/2023 14:48, Damien Cassou wrote:
>> the documentation of `project-remember-projects-under' is:
>>        "Index all projects below a directory DIR.  If RECURSIVE is
>>      non-nil, recurse into all subdirectories to find more projects.
>>      After finishing, a message is printed summarizing the progress.  The
>>      function returns the number of detected projects."
>> Regardless of the value of RECURSIVE, I understand from the above
>> that
>> all child directories of the DIR argument will be investigated. The doc
>> doesn't say anything about investigating if DIR is itself a project or
>> not so I think it would make sense if the function wasn't.
>> But the code says otherwise (as far as I understand it):
>> (defun project-remember-projects-under (dir &optional recursive)
>>    (let ((queue (list dir)))
>>      ;; …
>>      (while queue
>>        (when-let ((subdir (pop queue))
>>                   ((file-directory-p subdir)))
>>          ;; maybe register `subdir' as a project
>>          ;; …
>>          (when (and recursive (file-directory-p subdir))
>>            (setq queue (nconc (directory-files subdir …) queue)))))))
>> The code above seems to investigate DIR first and, if RECURSIVE is
>> non-nil, look at the directories below it.
>> Also, the second check (file-directory-p subdir) seems unnecessary
>> because of the first one.
>> There is a part of the code I don't understand:
>>    (unless (eq recursive 'in-progress)
>> It seems nowhere in the code nor in the documentation do we say
>> anything
>> about this 'in-progress special value. Is it a left over from a previous
>> (recursive) version of the algorithm?
>
> Philip, could you look into this?

One idea would be to just simplify the entire implementation by relying
on directory-files and directory-files-recursively.  Say something like
this:


[-- Attachment #2: Type: text/plain, Size: 2496 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index fda1081eb62..e0d5e706e82 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1821,35 +1821,28 @@ project-remember-projects-under
 projects."
   (interactive "DDirectory: \nP")
   (project--ensure-read-project-list)
-  (let ((queue (list dir))
-        (count 0)
-        (known (make-hash-table
-                :size (* 2 (length project--list))
-                :test #'equal )))
+  (let ((dirs (if recursive
+                  (directory-files-recursively dir "" t #'file-directory-p)
+                (cl-delete-if-not #'file-directory-p (directory-files dir t))))
+        (known (make-hash-table :size (* 2 (length project--list))
+                                :test #'equal))
+        (count 0))
     (dolist (project (mapcar #'car project--list))
       (puthash project t known))
-    (while queue
-      (when-let ((subdir (pop queue))
-                 ((file-directory-p subdir)))
-        (when-let ((project (project--find-in-directory subdir))
-                   (project-root (project-root project))
-                   ((not (gethash project-root known))))
-          (project-remember-project project t)
-          (puthash project-root t known)
-          (message "Found %s..." project-root)
-          (setq count (1+ count)))
-        (when (and recursive (file-directory-p subdir))
-          (setq queue
-                (nconc
-                 (directory-files
-                  subdir t directory-files-no-dot-files-regexp t)
-                 queue)))))
-    (unless (eq recursive 'in-progress)
-      (if (zerop count)
-          (message "No projects were found")
-        (project--write-project-list)
-        (message "%d project%s were found"
-                 count (if (= count 1) "" "s"))))
+    (dolist (subdir dirs)
+      (when-let (((file-directory-p subdir))
+                 (project (project--find-in-directory subdir))
+                 (project-root (project-root project))
+                 ((not (gethash project-root known))))
+        (project-remember-project project t)
+        (puthash project-root t known)
+        (message "Found %s..." project-root)
+        (setq count (1+ count))))
+    (if (zerop count)
+        (message "No projects were found")
+      (project--write-project-list)
+      (message "%d project%s were found"
+               count (if (= count 1) "" "s")))
     count))
 
 (defun project-forget-zombie-projects ()

bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

[Dmitry Gutov]

On 01/11/2023 15:12, Philip Kaludercic wrote:
> One idea would be to just simplify the entire implementation by relying
> on directory-files and directory-files-recursively.  Say something like
> this:

Looks, good, just once thing:

> +                  (directory-files-recursively dir "" t #'file-directory-p)

The argument PREDICATE is not in Emacs 26.1 (which should remain 
compatible). But it doesn't help anyway, because that predicate is only 
used to determine whether to recurse into a subdirectory, and not to 
filter out files. So the full list of all files is generated anyway.

If the performance still looks okay to you, I have no objections. Just 
remove that last argument, and it's good to install.

bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

[Philip Kaludercic]

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

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 01/11/2023 15:12, Philip Kaludercic wrote:
>> One idea would be to just simplify the entire implementation by relying
>> on directory-files and directory-files-recursively.  Say something like
>> this:
>
> Looks, good, just once thing:
>
>> +                  (directory-files-recursively dir "" t #'file-directory-p)
>
> The argument PREDICATE is not in Emacs 26.1 (which should remain
> compatible). But it doesn't help anyway, because that predicate is
> only used to determine whether to recurse into a subdirectory, and not
> to filter out files. So the full list of all files is generated
> anyway.

OK, here is the updated patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Simplify-project-remember-projects-under.patch --]
[-- Type: text/x-diff, Size: 2976 bytes --]

From cf1541ddba61e3c6106a133a9b6b662f0f34749d Mon Sep 17 00:00:00 2001
From: Philip Kaludercic <philipk@posteo.net>
Date: Wed, 1 Nov 2023 22:34:28 +0100
Subject: [PATCH] Simplify 'project-remember-projects-under'

* lisp/progmodes/project.el (project-remember-projects-under): Instead
of traversing the directories manually, re-use
`directory-files-recursively' to reduce complexity.  (Bug#66649)
---
 lisp/progmodes/project.el | 47 +++++++++++++++++----------------------
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index fda1081eb62..935f7a7b873 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1821,35 +1821,28 @@ project-remember-projects-under
 projects."
   (interactive "DDirectory: \nP")
   (project--ensure-read-project-list)
-  (let ((queue (list dir))
-        (count 0)
-        (known (make-hash-table
-                :size (* 2 (length project--list))
-                :test #'equal )))
+  (let ((dirs (if recursive
+                  (directory-files-recursively dir "" t)
+                (directory-files dir t)))
+        (known (make-hash-table :size (* 2 (length project--list))
+                                :test #'equal))
+        (count 0))
     (dolist (project (mapcar #'car project--list))
       (puthash project t known))
-    (while queue
-      (when-let ((subdir (pop queue))
-                 ((file-directory-p subdir)))
-        (when-let ((project (project--find-in-directory subdir))
-                   (project-root (project-root project))
-                   ((not (gethash project-root known))))
-          (project-remember-project project t)
-          (puthash project-root t known)
-          (message "Found %s..." project-root)
-          (setq count (1+ count)))
-        (when (and recursive (file-directory-p subdir))
-          (setq queue
-                (nconc
-                 (directory-files
-                  subdir t directory-files-no-dot-files-regexp t)
-                 queue)))))
-    (unless (eq recursive 'in-progress)
-      (if (zerop count)
-          (message "No projects were found")
-        (project--write-project-list)
-        (message "%d project%s were found"
-                 count (if (= count 1) "" "s"))))
+    (dolist (subdir dirs)
+      (when-let (((file-directory-p subdir))
+                 (project (project--find-in-directory subdir))
+                 (project-root (project-root project))
+                 ((not (gethash project-root known))))
+        (project-remember-project project t)
+        (puthash project-root t known)
+        (message "Found %s..." project-root)
+        (setq count (1+ count))))
+    (if (zerop count)
+        (message "No projects were found")
+      (project--write-project-list)
+      (message "%d project%s were found"
+               count (if (= count 1) "" "s")))
     count))
 
 (defun project-forget-zombie-projects ()
-- 
2.39.2


[-- Attachment #3: Type: text/plain, Size: 425 bytes --]



> If the performance still looks okay to you, I have no objections. Just
> remove that last argument, and it's good to install.

I'd say that Damien can try it out and report if it works well enough.
I worry that the files returned by `directory-files-recursively' might
slow down the function considerably.  Perhaps it might be better to
write a little function just for project.el to only generate a list of
directories.

bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

[Dmitry Gutov]

On 01/11/2023 23:36, Philip Kaludercic wrote:
>> If the performance still looks okay to you, I have no objections. Just
>> remove that last argument, and it's good to install.
> I'd say that Damien can try it out and report if it works well enough.

Let's wait, sure.

> I worry that the files returned by `directory-files-recursively' might
> slow down the function considerably.  Perhaps it might be better to
> write a little function just for project.el to only generate a list of
> directories.

It might, or it might not: the implementation of 'project-find-dir' is 
pretty unoptimized in this regard, and yet I'm still waiting for anyone 
to complain.

bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

[Damien Cassou]

Dmitry Gutov <dgutov@yandex.ru> writes:
> On 01/11/2023 23:36, Philip Kaludercic wrote:
>> I'd say that Damien can try it out and report if it works well enough.
>
> Let's wait, sure.

this is working great, thank you. The code is also simpler to understand
in my opinion. Good job.

If I may, the code of `project-remember-projects-under' seems to suffer
from a similar mismatch between the docstring and implementation when
RECURSIVE is nil: only the DIR directory is tested and not "known
projects below a directory DIR".

Best,

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

[Dmitry Gutov]

On 02/11/2023 21:58, Damien Cassou wrote:
> If I may, the code of `project-remember-projects-under' seems to suffer
> from a similar mismatch between the docstring and implementation when
> RECURSIVE is nil: only the DIR directory is tested and not "known
> projects below a directory DIR".

Have you seen this problem when testing it?

The last line in

+  (let ((dirs (if recursive
+                  (directory-files-recursively dir "" t)
+                (directory-files dir t)))

seems to do the job of including the direct subdirectories in the search.

bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

[Damien Cassou]

Dmitry Gutov <dgutov@yandex.ru> writes:
> On 02/11/2023 21:58, Damien Cassou wrote:
>> If I may, the code of `project-remember-projects-under' seems to suffer
>> from a similar mismatch between the docstring and implementation when
>> RECURSIVE is nil: only the DIR directory is tested and not "known
>> projects below a directory DIR".
>
> Have you seen this problem when testing it?

The patch is working perfectly find, thank you. I was talking about an
unrelated (but similar) problem in a different function:
`project-forget-projects-under'. My message incorrectly referred to the
function you already fixed. Sorry for the confusion.

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

[Philip Kaludercic]

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

Damien Cassou <damien@cassou.me> writes:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>> On 02/11/2023 21:58, Damien Cassou wrote:
>>> If I may, the code of `project-remember-projects-under' seems to suffer
>>> from a similar mismatch between the docstring and implementation when
>>> RECURSIVE is nil: only the DIR directory is tested and not "known
>>> projects below a directory DIR".
>>
>> Have you seen this problem when testing it?
>
> The patch is working perfectly find, thank you. I was talking about an
> unrelated (but similar) problem in a different function:
> `project-forget-projects-under'. My message incorrectly referred to the
> function you already fixed. Sorry for the confusion.

I don't think this is the same problem, in
`project-forget-projects-under' there is no manual recursive descent,
just some duplicated code.  We could also re-write it to look like this:


[-- Attachment #2: Type: text/plain, Size: 1064 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 95db9d0ef4c..5f1cce160b2 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -1905,15 +1905,12 @@ project-forget-projects-under
 forgotten projects."
   (interactive "DDirectory: \nP")
   (let ((count 0))
-    (if recursive
-        (dolist (proj (project-known-project-roots))
-          (when (file-in-directory-p proj dir)
-            (project-forget-project proj)
-            (setq count (1+ count))))
-      (dolist (proj (project-known-project-roots))
-        (when (file-equal-p (file-name-directory proj) dir)
-          (project-forget-project proj)
-          (setq count (1+ count)))))
+    (dolist (proj (project-known-project-roots))
+      (when (if recursive
+                (file-in-directory-p proj dir)
+              (file-equal-p (file-name-directory proj) dir))
+        (project-forget-project proj)
+        (setq count (1+ count))))
     (if (zerop count)
         (message "No projects were forgotten")
       (project--write-project-list)

[-- Attachment #3: Type: text/plain, Size: 168 bytes --]


But that would incur a branch in every iteration of `dolist'.

Either way, I'll push the first patch to master since there haven't been
any objections to that change.

bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

[Dmitry Gutov]

On 08/11/2023 10:13, Philip Kaludercic wrote:
> I don't think this is the same problem, in
> `project-forget-projects-under' there is no manual recursive descent,
> just some duplicated code.  We could also re-write it to look like this:
> 
> 
> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index 95db9d0ef4c..5f1cce160b2 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -1905,15 +1905,12 @@ project-forget-projects-under
>   forgotten projects."
>     (interactive "DDirectory: \nP")
>     (let ((count 0))
> -    (if recursive
> -        (dolist (proj (project-known-project-roots))
> -          (when (file-in-directory-p proj dir)
> -            (project-forget-project proj)
> -            (setq count (1+ count))))
> -      (dolist (proj (project-known-project-roots))
> -        (when (file-equal-p (file-name-directory proj) dir)
> -          (project-forget-project proj)
> -          (setq count (1+ count)))))
> +    (dolist (proj (project-known-project-roots))
> +      (when (if recursive
> +                (file-in-directory-p proj dir)
> +              (file-equal-p (file-name-directory proj) dir))
> +        (project-forget-project proj)
> +        (setq count (1+ count))))
>       (if (zerop count)
>           (message "No projects were forgotten")
>         (project--write-project-list)
> 
> 
> But that would incur a branch in every iteration of `dolist'.

LGTM too. The branch-per-iteration is unlikely to move a needle in any 
realistic scenario.

Up to you, whether to install this or keep the original version.

bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

[Philip Kaludercic]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 08/11/2023 10:13, Philip Kaludercic wrote:
>> I don't think this is the same problem, in
>> `project-forget-projects-under' there is no manual recursive descent,
>> just some duplicated code.  We could also re-write it to look like this:
>> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
>> index 95db9d0ef4c..5f1cce160b2 100644
>> --- a/lisp/progmodes/project.el
>> +++ b/lisp/progmodes/project.el
>> @@ -1905,15 +1905,12 @@ project-forget-projects-under
>>   forgotten projects."
>>     (interactive "DDirectory: \nP")
>>     (let ((count 0))
>> -    (if recursive
>> -        (dolist (proj (project-known-project-roots))
>> -          (when (file-in-directory-p proj dir)
>> -            (project-forget-project proj)
>> -            (setq count (1+ count))))
>> -      (dolist (proj (project-known-project-roots))
>> -        (when (file-equal-p (file-name-directory proj) dir)
>> -          (project-forget-project proj)
>> -          (setq count (1+ count)))))
>> +    (dolist (proj (project-known-project-roots))
>> +      (when (if recursive
>> +                (file-in-directory-p proj dir)
>> +              (file-equal-p (file-name-directory proj) dir))
>> +        (project-forget-project proj)
>> +        (setq count (1+ count))))
>>       (if (zerop count)
>>           (message "No projects were forgotten")
>>         (project--write-project-list)
>> But that would incur a branch in every iteration of `dolist'.
>
> LGTM too. The branch-per-iteration is unlikely to move a needle in any
> realistic scenario.
>
> Up to you, whether to install this or keep the original version.

I don't see a need, this is basically an aesthetic change.  Should we
close the bug report?

bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

[Dmitry Gutov]

On 08/11/2023 21:58, Philip Kaludercic wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
> 
>> On 08/11/2023 10:13, Philip Kaludercic wrote:
>>> I don't think this is the same problem, in
>>> `project-forget-projects-under' there is no manual recursive descent,
>>> just some duplicated code.  We could also re-write it to look like this:
>>> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
>>> index 95db9d0ef4c..5f1cce160b2 100644
>>> --- a/lisp/progmodes/project.el
>>> +++ b/lisp/progmodes/project.el
>>> @@ -1905,15 +1905,12 @@ project-forget-projects-under
>>>    forgotten projects."
>>>      (interactive "DDirectory: \nP")
>>>      (let ((count 0))
>>> -    (if recursive
>>> -        (dolist (proj (project-known-project-roots))
>>> -          (when (file-in-directory-p proj dir)
>>> -            (project-forget-project proj)
>>> -            (setq count (1+ count))))
>>> -      (dolist (proj (project-known-project-roots))
>>> -        (when (file-equal-p (file-name-directory proj) dir)
>>> -          (project-forget-project proj)
>>> -          (setq count (1+ count)))))
>>> +    (dolist (proj (project-known-project-roots))
>>> +      (when (if recursive
>>> +                (file-in-directory-p proj dir)
>>> +              (file-equal-p (file-name-directory proj) dir))
>>> +        (project-forget-project proj)
>>> +        (setq count (1+ count))))
>>>        (if (zerop count)
>>>            (message "No projects were forgotten")
>>>          (project--write-project-list)
>>> But that would incur a branch in every iteration of `dolist'.
>> LGTM too. The branch-per-iteration is unlikely to move a needle in any
>> realistic scenario.
>>
>> Up to you, whether to install this or keep the original version.
> I don't see a need, this is basically an aesthetic change.  Should we
> close the bug report?

Damien, is there anything else here to do?

Did you perhaps also (or instead) saw a problem with either of the 
docstrings? I'm not sure if I understood the last complaint correctly.

bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

[Damien Cassou]

Dmitry Gutov <dgutov@yandex.ru> writes:
> Damien, is there anything else here to do?
>
> Did you perhaps also (or instead) saw a problem with either of the 
> docstrings? I'm not sure if I understood the last complaint correctly.


The bug can be closed, no problem.

Thanks everyone for your great work.


-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

bug#66649: 29.1; `project-remember-projects-under' behavior doesn't match its doc

[Dmitry Gutov]

On 08/11/2023 23:13, Damien Cassou wrote:
> Dmitry Gutov<dgutov@yandex.ru>  writes:
>> Damien, is there anything else here to do?
>>
>> Did you perhaps also (or instead) saw a problem with either of the
>> docstrings? I'm not sure if I understood the last complaint correctly.
> 
> The bug can be closed, no problem.
> 
> Thanks everyone for your great work.

Thanks for reporting!