bug#65704: 29.1; Most code of `project-ignores' seems to be dead code

bug#65704: 29.1; Most code of `project-ignores' seems to be dead code

[Damien Cassou]

Hi,

The function `project-ignores' starts like this:

  (cl-defmethod project-ignores ((project (head vc)) dir)
    (let* ((root (nth 2 project))
           backend)
      (append
       (when (and backend
                  (file-equal-p dir root))

It seems to me that `backend' is always going to be nil at this point
and thus the 25-line long `when' block will always return nil without
doing anything.

Am I missing something?

This code seems to have been introduced in commit
785fa801596ad7bb9f838cac865f00de29e253d1 "New user option:
project-vc-extra-root-markers".

Best

-- 
Damien Cassou

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

bug#65704: [PATCH] project.el: Fix bug in project-ignores

[Damien Cassou]

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

In case what I said in the issue's description is correct, please find
attach a possible patch.

-- 
Damien Cassou

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-project.el-Fix-bug-in-project-ignores.patch --]
[-- Type: text/patch, Size: 1162 bytes --]

From f04731cc2a31510163dc2ed484f47467ea88f036 Mon Sep 17 00:00:00 2001
From: Damien Cassou <damien@cassou.me>
Date: Sat, 2 Sep 2023 18:08:49 +0200
Subject: [PATCH] project.el: Fix bug in project-ignores

The variable `backend' was always nil preventing the 25-line long
`when' block to do anything. This bug seems to have been introduced in commit
785fa801596ad7bb9f838cac865f00de29e253d1 "New user option:
project-vc-extra-root-markers". (Bug#65704)

* lisp/progmodes/project.el: (project-ignores) Use backend-specific
code if possible.
---
 lisp/progmodes/project.el | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 37db677943a..29a81c7e151 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -733,11 +733,10 @@ project--git-submodules
 
 (cl-defmethod project-ignores ((project (head vc)) dir)
   (let* ((root (nth 2 project))
-         backend)
+         (backend (cadr project)))
     (append
      (when (and backend
                 (file-equal-p dir root))
-       (setq backend (cadr project))
        (delq
         nil
         (mapcar
-- 
2.41.0

bug#65704: 29.1; Most code of `project-ignores' seems to be dead code

[Dmitry Gutov]

Hi Damien,

Thanks for the patch, pushed to master (with a few amendments in summary).

I'd like to backport it to emacs-29 as well, if Eli/Stefan don't mind.

On 02/09/2023 19:03, Damien Cassou wrote:
> Hi,
> 
> The function `project-ignores' starts like this:
> 
>    (cl-defmethod project-ignores ((project (head vc)) dir)
>      (let* ((root (nth 2 project))
>             backend)
>        (append
>         (when (and backend
>                    (file-equal-p dir root))
> 
> It seems to me that `backend' is always going to be nil at this point
> and thus the 25-line long `when' block will always return nil without
> doing anything.
> 
> Am I missing something?

Not much. Maybe just that that code path isn't exercised by any in-tree 
function if the backend is Git (with recent enough git installed) or Hg 
(the project-files method uses faster, specialized shell invocations), 
so that might be the reason why this was missed in testing.

Any chance you could tell us what made you notice the bug?

bug#65704: 29.1; Most code of `project-ignores' seems to be dead code

[Damien Cassou]

Dmitry Gutov <dgutov@yandex.ru> writes:
> much. Maybe just that that code path isn't exercised by any in-tree
> function if the backend is Git (with recent enough git installed)

My Git repository has several .gitignore files in several directories,
something that `project-ignores' (for VC) seems to not support:

    ;; we only support .gitignore at the root.

My project looks like this:

.
├── client
│   ├── .gitignore
│   ├── node_modules
│   └── .project
├── .git
└── .gitignore

Because I spend most of my time in the client/ directory, I want the
`project' library to consider client/ to be a project on its own. I use
the code below and the client/.project file to do this:

    (defconst my/project-root-marker ".project"
      "File indicating the root of a project.")

    (defun my/project-find-root (path)
      "Search up the PATH for `my/project-root-marker'."
      (when-let* ((root (locate-dominating-file path my/project-root-marker)))
        (cons 'transient root)))

    (add-to-list 'project-find-functions #'my/project-find-root)

The client/.gitignore file contains a line "node_modules/" which is a
very large directory. The command `project-find-file' doesn't see my
client/.gitignore and thus suggests 83k files and make the completion UI
very slow. So I'm not using it but this instead:

  (project-find-file-in (thing-at-point 'filename)
                        (list "/my/project/client")
                        (project-current nil "/my/project"))

The function `project-find-file-in' uses `project-files' which in turn
uses `project--dir-ignores'. The later uses `project-ignores' which has
the bug mentioned in this issue.

Am I doing something reasonable? Is there a better way? It looks like a
lot of setup to get Emacs to ignore directories listed .gitignore files.

-- 
Damien Cassou

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

bug#65704: 29.1; Most code of `project-ignores' seems to be dead code

[Dmitry Gutov]

On 04/09/2023 19:03, Damien Cassou wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
>> much. Maybe just that that code path isn't exercised by any in-tree
>> function if the backend is Git (with recent enough git installed)
> 
> My Git repository has several .gitignore files in several directories,
> something that `project-ignores' (for VC) seems to not support:
> 
>      ;; we only support .gitignore at the root.

Yes. That's not really the fault of project-vc's implementation, but the 
way vc-default-ignore-completion-table and vc-git-find-ignore-file are 
implemented. But again, it shouldn't be a problem for most users/uses.

> My project looks like this:
> 
> .
> ├── client
> │   ├── .gitignore
> │   ├── node_modules
> │   └── .project
> ├── .git
> └── .gitignore
> 
> Because I spend most of my time in the client/ directory, I want the
> `project' library to consider client/ to be a project on its own. I use
> the code below and the client/.project file to do this:

Also an aside: you could add ".project" or ".gitignore" to 
project-vc-extra-root-markers if your project.el is recent enough (e.g. 
from Emacs 29).

>      (defconst my/project-root-marker ".project"
>        "File indicating the root of a project.")
> 
>      (defun my/project-find-root (path)
>        "Search up the PATH for `my/project-root-marker'."
>        (when-let* ((root (locate-dominating-file path my/project-root-marker)))
>          (cons 'transient root)))
> 
>      (add-to-list 'project-find-functions #'my/project-find-root)
> 
> The client/.gitignore file contains a line "node_modules/" which is a
> very large directory. The command `project-find-file' doesn't see my
> client/.gitignore and thus suggests 83k files and make the completion UI
> very slow.

The above customization going against the general recommendation in 
project.el's Commentary and ends up with Emacs detecting an instance of 
a different backend: 'transient'. Which doesn't have a dedicaded 
definition for project-ignores and falls back to the default one (lines 
292:307). Naturally, it doesn't scan for ignore files in any VC backend.

That's the reason I've added project-vc-extra-root-markers: so that 
honoring files like .gitignore still makes sense, but the user can split 
the project into pieces using these markers while keeping the same 
backend in use.

> So I'm not using it but this instead:
> 
>    (project-find-file-in (thing-at-point 'filename)
>                          (list "/my/project/client")
>                          (project-current nil "/my/project"))
> 
> The function `project-find-file-in' uses `project-files' which in turn
> uses `project--dir-ignores'.

Just keep in mind that 'project-files' also has two definition in core 
(and however many outside of it): one default that always calls 
project--dir-ignores, and one for 'vc' (lines 609:629) that only calls 
project--dir-ignores in fallback situations (backend is not Git or Hg, 
or when Git is older than 1.9, or when DIR is not the project root). The 
last case might still be reconsidered, but it works for 'C-u M-x 
project-find-regexp' when you want to search inside an ignored dir 
(fully or partially).

> Am I doing something reasonable? Is there a better way? It looks like a
> lot of setup to get Emacs to ignore directories listed .gitignore files.

To reiterate: use 'project-vc-extra-root-markers'.

And the nested .gitignore should also use used automatically without 
that too by 'C-x p f' if you remove #'my/project-find-root from 
project-find-functions.

Anyway, thank you for the explanation, it's valuable feedback.

If anything is unclear, further question welcome.

bug#65704: 29.1; Most code of `project-ignores' seems to be dead code

[Dmitry Gutov]

Eli? Stefan?

On 03/09/2023 03:26, Dmitry Gutov wrote:
> I'd like to backport it to emacs-29 as well, if Eli/Stefan don't mind.

Any objections to the backport?

bug#65704: 29.1; Most code of `project-ignores' seems to be dead code

[Eli Zaretskii]

> Date: Mon, 4 Sep 2023 21:31:36 +0300
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> Eli? Stefan?
> 
> On 03/09/2023 03:26, Dmitry Gutov wrote:
> > I'd like to backport it to emacs-29 as well, if Eli/Stefan don't mind.
> 
> Any objections to the backport?

Fine by me.

bug#65704: 29.1; Most code of `project-ignores' seems to be dead code

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Mon, 4 Sep 2023 21:31:36 +0300
>> From: Dmitry Gutov <dgutov@yandex.ru>
>>
>> Eli? Stefan?
>>
>> On 03/09/2023 03:26, Dmitry Gutov wrote:
>> > I'd like to backport it to emacs-29 as well, if Eli/Stefan don't mind.
>>
>> Any objections to the backport?
>
> Fine by me.

No objections from me either.

bug#65704: 29.1; Most code of `project-ignores' seems to be dead code

[Damien Cassou]

Dmitry Gutov <dgutov@yandex.ru> writes:
> That's the reason I've added project-vc-extra-root-markers: so that 
> honoring files like .gitignore still makes sense, but the user can split 
> the project into pieces using these markers while keeping the same 
> backend in use.

thank you very much for your explanation. I simplified my setup as you
suggested.

That being said, `project-files' for 'vc' seems not to work for
me. Inside my client/ folder, (project-current) returns

  (vc nil "…/client/")

The `project-files' method has this line:

    (if (and (file-equal-p dir (cdr project))

Because `dir' is a string and (cdr project) is the list (nil
"…/client/), `file-equal-p' always returns nil. If I change `cdr' to be
`caddr', things seem to work perfectly.

Am I doing something wrong again or is it a bug and I should submit a
patch?

Best

-- 
Damien Cassou

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

bug#65704: 29.1; Most code of `project-ignores' seems to be dead code

[Dmitry Gutov]

On 04/09/2023 23:11, Damien Cassou wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
>> That's the reason I've added project-vc-extra-root-markers: so that
>> honoring files like .gitignore still makes sense, but the user can split
>> the project into pieces using these markers while keeping the same
>> backend in use.
> 
> thank you very much for your explanation. I simplified my setup as you
> suggested.

Very good.

> That being said, `project-files' for 'vc' seems not to work for
> me. Inside my client/ folder, (project-current) returns
> 
>    (vc nil "…/client/")
> 
> The `project-files' method has this line:
> 
>      (if (and (file-equal-p dir (cdr project))

Its current definition (in Emacs 29 and later) looks like this:

        (if (and (file-equal-p dir (nth 2 project))

Perhaps you have an older version installed somewhere?

Though that wouldn't explain why (project-current) would return an 
incompatible value.

> Because `dir' is a string and (cdr project) is the list (nil
> "…/client/), `file-equal-p' always returns nil. If I change `cdr' to be
> `caddr', things seem to work perfectly.
> 
> Am I doing something wrong again or is it a bug and I should submit a
> patch?

Some investigation could help.

bug#65704: 29.1; Most code of `project-ignores' seems to be dead code

[Damien Cassou]

Hi Dmitry,

Dmitry Gutov <dmitry@gutov.dev> writes:
> Perhaps you have an older version installed somewhere?

I think I experimented a bit too much :-), thanks for noticing. I
restarted Emacs and am now using project 0.9.8 from elpa. The expression
(project-current) now returns (vc nil "…/client/") in my client/
directory. Notice the nil in the second position. This means that
`project-files' for 'vc will set nil as backend. As a result, the
optimized `project--vc-list-files' isn't used. This means the files
listed in client/.gitignore are not ignored.

I'm certainly doing something wrong again.

-- 
Damien Cassou

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

bug#65704: 29.1; Most code of `project-ignores' seems to be dead code

[Dmitry Gutov]

Hi Damien,

On 05/09/2023 10:58, Damien Cassou wrote:
> Hi Dmitry,
> 
> Dmitry Gutov<dmitry@gutov.dev>  writes:
>> Perhaps you have an older version installed somewhere?
> I think I experimented a bit too much 😄, thanks for noticing. I
> restarted Emacs and am now using project 0.9.8 from elpa. The expression
> (project-current) now returns (vc nil "…/client/") in my client/
> directory.

I'm still not sure about the relative path in the 3rd element, but if it 
doesn't cause problems, let's set it aside for now.

> Notice the nil in the second position. This means that
> `project-files' for 'vc will set nil as backend. As a result, the
> optimized `project--vc-list-files' isn't used. This means the files
> listed in client/.gitignore are not ignored.
> 
> I'm certainly doing something wrong again.

And that... is a geniune bug. ;-(

Or an omission compared to the design I had in my head (mentioned 
upthread), but at least not a regression compared to the previous 
behavior or to what people did with adding to project-find-functions.

So I'm not 100% sure about changing that in Emacs 29, but here's a 
patch, please try it out:

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 29a81c7e151..2eea0ef72e0 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -566,6 +566,12 @@ project-try-vc
                 (project--submodule-p root))
            (let* ((parent (file-name-directory (directory-file-name 
root))))
              (setq root (vc-call-backend 'Git 'root parent))))
+        (when (not backend)
+          (let* ((project-vc-extra-root-markers nil)
+                 ;; Avoid submodules scan.
+                 (enable-dir-local-variables nil)
+                 (parent (project-try-vc root)))
+            (and parent (setq backend (nth 1 parent)))))
          (when root
            (setq project (list 'vc backend root))
            ;; FIXME: Cache for a shorter time.

bug#65704: 29.1; Most code of `project-ignores' seems to be dead code

[Damien Cassou]

Dmitry Gutov <dmitry@gutov.dev> writes:
> On 05/09/2023 10:58, Damien Cassou wrote:
>> The expression (project-current) now returns (vc nil "…/client/") in
>> my client/ directory.
>
> I'm still not sure about the relative path in the 3rd element, but if it 
> doesn't cause problems, let's set it aside for now.


The "..." in the result of (project-current) above is not a sign of a
relative path but a sign of me removing the noise in my message. I'm
sorry it was more confusing than helpful. The real string starts with
"~/Documents/".


> So I'm not 100% sure about changing that in Emacs 29, but here's a 
> patch, please try it out:


This is working great for me as far as I can tell. Thank you. Updating
Emacs 29 doesn't seem necessary: this is why we have Elpa after all.

-- 
Damien Cassou

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

bug#65704: 29.1; Most code of `project-ignores' seems to be dead code

[Damien Cassou]

Dmitry Gutov <dmitry@gutov.dev> writes:
> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index 29a81c7e151..2eea0ef72e0 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -566,6 +566,12 @@ project-try-vc
>                  (project--submodule-p root))
>             (let* ((parent (file-name-directory (directory-file-name 
> root))))
>               (setq root (vc-call-backend 'Git 'root parent))))
> +        (when (not backend)

it seems that `root' is sometimes nil at this point so I would change
this line to

  (when (and (not backend) root)

I've used this for the day and it seems to work fine.

-- 
Damien Cassou

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

bug#65704: 29.1; Most code of `project-ignores' seems to be dead code

[Dmitry Gutov]

On 06/09/2023 22:12, Damien Cassou wrote:
> Dmitry Gutov<dmitry@gutov.dev>  writes:
>> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
>> index 29a81c7e151..2eea0ef72e0 100644
>> --- a/lisp/progmodes/project.el
>> +++ b/lisp/progmodes/project.el
>> @@ -566,6 +566,12 @@ project-try-vc
>>                   (project--submodule-p root))
>>              (let* ((parent (file-name-directory (directory-file-name
>> root))))
>>                (setq root (vc-call-backend 'Git 'root parent))))
>> +        (when (not backend)
> it seems that `root' is sometimes nil at this point so I would change
> this line to
> 
>    (when (and (not backend) root)
> 
> I've used this for the day and it seems to work fine.

Thanks for the extra testing.

I just moved the new code a little deeper down inside (when root ...) 
block, should do the same. Pushed as 62229fb2d11 and bumped project.el 
version to 0.10.0.

Should be out in ELPA shortly along with a few other new features added 
recently.

This seems to resolve the last of the concerns here, so closing the bug 
as well. Please clarify if something remains.

Addendum regarding the chosen solution: I guess there is a downside or 
two: when working over Tramp, the latency will increase. And if the 
subproject's dir is in the parent's .gitignore, the behavior will get worse.

Let's wait for feedback to see if those are real problems, but worst 
case, the solution which you had used previously should take care at 
least of the second usage scenario (ignoring the ignores).