bug#58839: [Patch] Re: bug#58839: 29.0.50; project-kill-buffer fails when Eglot is running

[Philip Kaludercic <philipk@posteo.net>]

João Távora <joaotavora@gmail.com> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>>> bytes, but it shouldn't rely on their values and certainly can't free()
>>> what it didn't malloc().
>> But to extend this metaphor, if I kill a programm that allocated malloc,
>> I would expect that memory to be cleaned up.
>
> Yes, but to kill a process you have to own it.  project.el is not the
> owner (not even a co-owner) of Eglot LSP internal buffers, and so it
> can't kill them.

What would be a co-owner?

My view is that project.el is a manager of buffers, but that isn't
relevant anymore.

>>> I think the command is pretty useful.  But perhaps, I'm just guessing
>>> here, Philip is focusing too much it as if it were the only sanctioned
>>> way for Emacs users to stop working on a given set of files in a
>>> programming project and clean up.
>> Of course it isn't, it is just my prefered way and Eglot breaks it.
>
> I don't think we should play the who-broke-what game.  It doesn't help,
> and if we decided to look up the dates of introduction of your
> project-kill-buffers way and eglot's process management, we might reach
> a different conclusion.

I really just meant "break" as in works until Eglot is enabled, and
nothing more than that.

>>> Neither do I.  But when I M-x cd to other places, project.el thinks that
>>> scratch belongs to that project.  It doesn't, I keep stuff of various
>>> projects in it.
>>
>> I agree, this is bad, but that can easily be solved by adding
>> `lisp-interaction-mode' to the list of major modes that are not
>> killed.
>
> Also *ielm*, the global Elisp repl created by M-x ielm. has the same
> problem.  And *Completions*.  I'm quite sure that *sly-scratch* in the
> SLY CL IDE would also be targeted.  And the CIDER Clojure IDE, as
> someone has already reported.  And probably many more.  This blanket
> default-directory heuristic is practical in some common cases but flawed
> in many others.

Project.el uses the same condition format like `buffer-match-p', which
is quite flexible.  Maybe all earmuffed buffers should be spared
("\\`\\*.+\\*\\'"), but in my experience that can be too lenient.

>>> Just commit the original tested patch I gave you that exempts hidden
>>> buffers without buffer-file-name from project-buffers.  Philip's command
>>> will keep working perfectly and we can close this bug.
>>
>> So if I understand correctly, with `eglot-autoshutdown' enabled as soon
>> as all the buffers have been killed, the server will also shut down,
>> right?
>
> Yes! This is exactly what the docstring says:
>
>    eglot-autoshutdown is a variable defined in `eglot.el'.
>
>    If non-nil, shut down server after killing last managed buffer.

Ok, great.

>> Regarding the patch itself, I think it would be better to use
>> `project-kill-buffer-conditions', so that if a project.el backend
>> defines a new implementation for `project-buffers', the issue doesn't
>> pop up again.
>
> Your concern is quite valid.  Fortunately, CLOS generic functions have
> us covered.  This is even simpler than the first patch:
>
> diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
> index ac278edd40..f8190eb1fc 100644
> --- a/lisp/progmodes/project.el
> +++ b/lisp/progmodes/project.el
> @@ -362,6 +362,13 @@ project-buffers
>          (push buf bufs)))
>      (nreverse bufs)))
>  
> +(cl-defmethod project-buffers :around (_project)
> +  "Ensure hidden/private buffers do not belong to PROJECT."
> +  (cl-remove-if-not (lambda (b)
> +                      (and (string-prefix-p " " (buffer-name b))
> +                           (not (buffer-file-name b))))
> +                    (cl-call-next-method)))
> +
>  (defgroup project-vc nil
>    "Project implementation based on the VC package."
>    :version "25.1"

I have to still admit that I am uncertain if the general ignoring of all
hidden buffers is justified.  I have certainly used hidden buffers
frequently enough but never assumed that they were outside of anyone's
control.  They are just regular buffers with a special kind of name
after all.

We ought to be able to define a cleaner way of clarifying what buffers
can and cannot belong to projects.  Personally I think a buffer-local
variable/predicate would be a better approach.

> Note this still leaves the *scratch* and *ielm* and other things
> uncovered.  It addresses this specific bug and most importantly doesn't
> blow up in the users.