Name of buffers created by project-shell

Name of buffers created by project-shell

[Matthias Meulien]

Hi, 

When I C-x p v (project-vc-dir) from a buffer with project root 
called, say, "myproject" , I switch to a buffer called 
*vc-dir*<myproject>. 

When I C-x p s (project-shell) from a buffer with same project 
root, I switch to a buffer called *myproject-shell*

Later, I found it not very convenient when switching between 
buffers by name using C-x b (switch-to-buffer) to have one buffer 
with a prefix build from the project root and one with a suffix. 
Don't you think we should homogenize those namings?   Since when I 
M-x shell then C-u M-x shell, I switch to a *shell*<2> buffer, I'd 
suggest to rename the buffer created by project-shell to 
*shell*<myproject>...
-- 
Matthias

Re: Name of buffers created by project-shell

[Dmitry Gutov]

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

Hi Matthias,

On 02.03.2021 23:18, Matthias Meulien wrote:
> When I C-x p v (project-vc-dir) from a buffer with project root called, 
> say, "myproject" , I switch to a buffer called *vc-dir*<myproject>.
> When I C-x p s (project-shell) from a buffer with same project root, I 
> switch to a buffer called *myproject-shell*
> 
> Later, I found it not very convenient when switching between buffers by 
> name using C-x b (switch-to-buffer) to have one buffer with a prefix 
> build from the project root and one with a suffix. Don't you think we 
> should homogenize those namings?   Since when I M-x shell then C-u M-x 
> shell, I switch to a *shell*<2> buffer, I'd suggest to rename the buffer 
> created by project-shell to *shell*<myproject>...

What you're asking for makes sense, but there is a snag because of how 
these commands are implemented.

vc-dir doesn't choose the buffer name format itself. It ultimately calls 
create-file-buffer (which has an advice made by uniquify) which renames 
the buffer based on uniquify-buffer-name-style (if uniquify is loaded in 
the current session), of course. The problem with that function is that 
it always creates a new buffer. So, before calling it, 
vc-dir-prepare-status-buffer does a search for existing buffer and can 
find existing one if it matches by major mode and default directory.

Both shell and eshell buffers can change their default-directory, but we 
can track which project they belong to with a new variable. See the 
attached patch.

There's one problem, though: when called with C-u, the piece of behavior 
which reads as "create a new inferior shell buffer even if one already 
exists" now creates buffers uniquely named according to uniquify's 
rules, which seems to mean

   *shell*
   emacs-master/*shell*
   vc/emacs-master/*shell*

instead of what one might expect, like

   emacs-master/*shell*
   emacs-master/*shell*<2>
   emacs-master/*shell*<3>

Perhaps the solution is not to go through uniquify for this, but then we 
project-shell can't really be consistent with project-vc-dir.

[-- Attachment #2: uniquify-project-shell.diff --]
[-- Type: text/x-patch, Size: 2794 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index 944b886ce5..f61a3ebea6 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -917,6 +917,22 @@ project-vc-dir
   (interactive)
   (vc-dir (project-root (project-current t))))
 
+(defvar-local project--origin-project-root nil)
+
+(defun project--find-existing-buffer (mode root)
+  (seq-find
+   (lambda (buf)
+     (and
+      (buffer-live-p buf)
+      (provided-mode-derived-p
+       (buffer-local-value 'major-mode buf)
+       mode)
+      (equal (buffer-local-value
+              'project--origin-project-root
+              buf)
+             root)))
+   (buffer-list)))
+
 ;;;###autoload
 (defun project-shell ()
   "Start an inferior shell in the current project's root directory.
@@ -926,15 +942,12 @@ project-shell
 if one already exists."
   (interactive)
   (let* ((default-directory (project-root (project-current t)))
-         (default-project-shell-name
-           (concat "*" (file-name-nondirectory
-                        (directory-file-name
-                         (file-name-directory default-directory)))
-                   "-shell*"))
-         (shell-buffer (get-buffer default-project-shell-name)))
-    (if (and shell-buffer (not current-prefix-arg))
-        (pop-to-buffer shell-buffer)
-      (shell (generate-new-buffer-name default-project-shell-name)))))
+         (existing-buffer (project--find-existing-buffer
+                           'shell-mode default-directory)))
+    (if (and existing-buffer (not current-prefix-arg))
+        (pop-to-buffer existing-buffer)
+      (shell (create-file-buffer (expand-file-name "*shell*")))
+      (setq project--origin-project-root default-directory))))
 
 ;;;###autoload
 (defun project-eshell ()
@@ -946,15 +959,14 @@ project-eshell
   (interactive)
   (defvar eshell-buffer-name)
   (let* ((default-directory (project-root (project-current t)))
-         (eshell-buffer-name
-          (concat "*" (file-name-nondirectory
-                       (directory-file-name
-                        (file-name-directory default-directory)))
-                  "-eshell*"))
-         (eshell-buffer (get-buffer eshell-buffer-name)))
-    (if (and eshell-buffer (not current-prefix-arg))
-        (pop-to-buffer eshell-buffer)
-      (eshell t))))
+         (existing-buffer (project--find-existing-buffer
+                           'eshell-mode default-directory)))
+    (if (and existing-buffer (not current-prefix-arg))
+        (pop-to-buffer existing-buffer)
+      (let ((eshell-buffer-name
+             (buffer-name (create-file-buffer (expand-file-name "*eshell*")))))
+        (eshell)
+        (setq project--origin-project-root default-directory)))))
 
 ;;;###autoload
 (defun project-async-shell-command ()

Re: Name of buffers created by project-shell

[Stefan Monnier]

> which seems to mean
>
>   *shell*
>   emacs-master/*shell*
>   vc/emacs-master/*shell*
>
> instead of what one might expect, like
>
>   emacs-master/*shell*
>   emacs-master/*shell*<2>
>   emacs-master/*shell*<3>

FWIW, I think this is a bug in `uniquify.el` which you can also see with
real, honest to god, file buffers if you create several buffers visiting
the same file (as can be done via `C-x C-w`).


        Stefan

Re: Name of buffers created by project-shell

[Juri Linkov]

> There's one problem, though: when called with C-u, the piece of behavior
> which reads as "create a new inferior shell buffer even if one already
> exists" now creates buffers uniquely named according to uniquify's rules,
> which seems to mean
>
>   *shell*
>   emacs-master/*shell*
>   vc/emacs-master/*shell*
>
> instead of what one might expect, like
>
>   emacs-master/*shell*
>   emacs-master/*shell*<2>
>   emacs-master/*shell*<3>

For consistency with C-x p v (project-vc-dir) that creates buffers named

    *vc-dir*<myproject>

shouldn't C-x p s (project-shell) create buffers with the same style:

    *shell*<myproject><1>
    *shell*<myproject><2>
    *shell*<myproject><3>

> Perhaps the solution is not to go through uniquify for this, but then we
> project-shell can't really be consistent with project-vc-dir.

'project-shell' already constructs buffer names using own style,
so it could switch to another style compatible with
uniquify-buffer-name-style, then construct buffer names
like "*shell*<myproject>" and allow uniquify to append
a numeric suffix <1>, <2>, <3>.

Re: Name of buffers created by project-shell

[Dmitry Gutov]

On 04.03.2021 19:50, Juri Linkov wrote:
>> There's one problem, though: when called with C-u, the piece of behavior
>> which reads as "create a new inferior shell buffer even if one already
>> exists" now creates buffers uniquely named according to uniquify's rules,
>> which seems to mean
>>
>>    *shell*
>>    emacs-master/*shell*
>>    vc/emacs-master/*shell*
>>
>> instead of what one might expect, like
>>
>>    emacs-master/*shell*
>>    emacs-master/*shell*<2>
>>    emacs-master/*shell*<3>
> For consistency with C-x p v (project-vc-dir) that creates buffers named
> 
>      *vc-dir*<myproject>
> 
> shouldn't C-x p s (project-shell) create buffers with the same style:
> 
>      *shell*<myproject><1>
>      *shell*<myproject><2>
>      *shell*<myproject><3>

That's what I said, yes. You just have a different uniquify style 
enabled, so the current behavior looks slightly different.

>> Perhaps the solution is not to go through uniquify for this, but then we
>> project-shell can't really be consistent with project-vc-dir.
> 'project-shell' already constructs buffer names using own style,
> so it could switch to another style compatible with
> uniquify-buffer-name-style, then construct buffer names
> like "*shell*<myproject>" and allow uniquify to append
> a numeric suffix <1>, <2>, <3>.

The devil is in the details. To have uniquify do its job properly, I 
think we have to call create-file-buffer.

And apparently we need to fix uniquify--create-file-buffer-advice first.

Re: Name of buffers created by project-shell

[Matthias Meulien]

> What you're asking for makes sense, but there is a snag because of how
> these commands are implemented.
>
> vc-dir doesn't choose the buffer name format itself. It ultimately
> calls create-file-buffer (which has an advice made by uniquify) which
> renames the buffer based on uniquify-buffer-name-style (if uniquify is
> loaded in the current session), of course. The problem with that
> function is that it always creates a new buffer. So, before calling
> it, vc-dir-prepare-status-buffer does a search for existing buffer and
> can find existing one if it matches by major mode and default
> directory.

Thanks Dmitry, I had forgotten that users have different settings for
uniquify-buffer-name-style variable...

> Both shell and eshell buffers can change their default-directory, but
> we can track which project they belong to with a new variable. See the
> attached patch.

Thanks! I am testing it.

I am not an eshell user but I guess the patch should use the variable
`eshell-buffer-name' defined in eshell.el in place of the hardcoded
"*eshell*" string.

> There's one problem, though: when called with C-u, the piece of
> behavior which reads as "create a new inferior shell buffer even if
> one already exists" now creates buffers uniquely named according to
> uniquify's rules, which seems to mean
>
>   *shell*
>   emacs-master/*shell*
>   vc/emacs-master/*shell*
>
> instead of what one might expect, like
>
>   emacs-master/*shell*
>   emacs-master/*shell*<2>
>   emacs-master/*shell*<3>
>

I think there's a problem even without using C-u: 

`project-shell' from a buffer whose associated project root path is
/src/project1 creates a buffer named *shell*.

Then `project-shell' from a buffer whose associated project root path is
/src/project2 creates a buffer named *shell*<project2> (I am using the
default `post-forward-angle-brackets' for `uniquify-buffer-name-style').

But the uniquify rationalize mecanism doesn't rename the first buffer to
*shell*<project1> which happens to be the case for buffers visiting
files...

-- 
Matthias

Re: Name of buffers created by project-shell

[Matthias Meulien]

Dmitry Gutov <dgutov@yandex.ru> writes:

> There's one problem, though: when called with C-u, the piece of
> behavior which reads as "create a new inferior shell buffer even if
> one already exists" now creates buffers uniquely named according to
> uniquify's rules, which seems to mean
>
>   *shell*
>   emacs-master/*shell*
>   vc/emacs-master/*shell*
>
> instead of what one might expect, like
>
>   emacs-master/*shell*
>   emacs-master/*shell*<2>
>   emacs-master/*shell*<3>

Well, one might expect to be prompted for the name of the buffer like
he's used to be with C-u M-x shell, no?

But I agree that the default value for the buffer name would still not
be consistent with the one proposed by C-u M-x shell.
-- 
Matthias

Re: Name of buffers created by project-shell

[Dmitry Gutov]

On 14.03.2021 15:46, Matthias Meulien wrote:

>> Both shell and eshell buffers can change their default-directory, but
>> we can track which project they belong to with a new variable. See the
>> attached patch.
> 
> Thanks! I am testing it.
> 
> I am not an eshell user but I guess the patch should use the variable
> `eshell-buffer-name' defined in eshell.el in place of the hardcoded
> "*eshell*" string.

Makes sense.

> I think there's a problem even without using C-u:
> 
> `project-shell' from a buffer whose associated project root path is
> /src/project1 creates a buffer named *shell*.
> 
> Then `project-shell' from a buffer whose associated project root path is
> /src/project2 creates a buffer named *shell*<project2> (I am using the
> default `post-forward-angle-brackets' for `uniquify-buffer-name-style').
> 
> But the uniquify rationalize mecanism doesn't rename the first buffer to
> *shell*<project1> which happens to be the case for buffers visiting
> files...

Yup, that's another bug. It should also how up for vc-dir buffers too.

To reiterate the choices at this point, we either implement special 
buffer-naming scheme in project and give up on consistency with 
project-vc and uniquify-buffer-name-style, or we try to fix both 
problems we have found in uniquify so far.

In theory, the latter would be ideal, and uniquify has certainly been 
lacking in developer attention. But I have no idea how much work that 
entails. If you have time, help in that area would be very welcome. 
Otherwise I'll have to add it to my list.

Re: Name of buffers created by project-shell

[Matthias Meulien]

Dmitry Gutov <dgutov@yandex.ru> writes:

> To reiterate the choices at this point, we either implement special
> buffer-naming scheme in project and give up on consistency with
> project-vc and uniquify-buffer-name-style, or we try to fix both
> problems we have found in uniquify so far.
>
> In theory, the latter would be ideal, and uniquify has certainly been
> lacking in developer attention. But I have no idea how much work that
> entails. If you have time, help in that area would be very
> welcome.

I'll work on fixing uniquify.
-- 
Matthias

Re: Name of buffers created by project-shell

[Dmitry Gutov]

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

On 15.03.2021 02:41, Dmitry Gutov wrote:
>> I think there's a problem even without using C-u:
>>
>> `project-shell' from a buffer whose associated project root path is
>> /src/project1 creates a buffer named *shell*.
>>
>> Then `project-shell' from a buffer whose associated project root path is
>> /src/project2 creates a buffer named *shell*<project2> (I am using the
>> default `post-forward-angle-brackets' for `uniquify-buffer-name-style').
>>
>> But the uniquify rationalize mecanism doesn't rename the first buffer to
>> *shell*<project1> which happens to be the case for buffers visiting
>> files...
> 
> Yup, that's another bug. It should also how up for vc-dir buffers too.

Actually, it didn't occur for vc-dir buffers. So it's arguably a bug in 
my code.

Attaching the updated patch which fixes that particular problem, though 
I'm a little more suspicious of some of uniquify's choices now.

In particular, this code depends on list-buffers-directory being set to 
a value in particular format which is very non-obvious from this 
variable's docstring.

And to have uniquify work similarly with buffers created by M-x shell 
and M-x eshell as well, these commands need similar changes (we can't 
really depend on them in project.el because it's an ELPA Core package), 
as well as shell-mode and eshell-mode being listed in 
uniquify-list-buffers-directory-modes.

[-- Attachment #2: uniquify-project-shell.diff --]
[-- Type: text/x-patch, Size: 3579 bytes --]

diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el
index b6a886f731..5b1c92ec9d 100644
--- a/lisp/progmodes/project.el
+++ b/lisp/progmodes/project.el
@@ -895,6 +895,22 @@ project-vc-dir
   (interactive)
   (vc-dir (project-root (project-current t))))
 
+(defvar-local project--origin-project-root nil)
+
+(defun project--find-existing-buffer (mode root)
+  (seq-find
+   (lambda (buf)
+     (and
+      (buffer-live-p buf)
+      (provided-mode-derived-p
+       (buffer-local-value 'major-mode buf)
+       mode)
+      (equal (buffer-local-value
+              'project--origin-project-root
+              buf)
+             root)))
+   (buffer-list)))
+
 ;;;###autoload
 (defun project-shell ()
   "Start an inferior shell in the current project's root directory.
@@ -904,15 +920,16 @@ project-shell
 if one already exists."
   (interactive)
   (let* ((default-directory (project-root (project-current t)))
-         (default-project-shell-name
-           (concat "*" (file-name-nondirectory
-                        (directory-file-name
-                         (file-name-directory default-directory)))
-                   "-shell*"))
-         (shell-buffer (get-buffer default-project-shell-name)))
-    (if (and shell-buffer (not current-prefix-arg))
-        (pop-to-buffer-same-window shell-buffer)
-      (shell (generate-new-buffer-name default-project-shell-name)))))
+         (existing-buffer (unless current-prefix-arg
+                            (project--find-existing-buffer 'shell-mode
+                                                           default-directory))))
+    (if existing-buffer
+        (pop-to-buffer-same-window existing-buffer)
+      (let* ((uniquify-list-buffers-directory-modes
+              (cons 'shell-mode uniquify-list-buffers-directory-modes)))
+        (shell (create-file-buffer (expand-file-name "*shell*")))
+        (setq list-buffers-directory (expand-file-name "*shell*" default-directory))
+        (setq project--origin-project-root default-directory)))))
 
 ;;;###autoload
 (defun project-eshell ()
@@ -922,17 +939,22 @@ project-eshell
 With \\[universal-argument] prefix arg, create a new Eshell buffer even
 if one already exists."
   (interactive)
+  (require 'eshell)
   (defvar eshell-buffer-name)
   (let* ((default-directory (project-root (project-current t)))
-         (eshell-buffer-name
-          (concat "*" (file-name-nondirectory
-                       (directory-file-name
-                        (file-name-directory default-directory)))
-                  "-eshell*"))
-         (eshell-buffer (get-buffer eshell-buffer-name)))
-    (if (and eshell-buffer (not current-prefix-arg))
-        (pop-to-buffer-same-window eshell-buffer)
-      (eshell t))))
+         (existing-buffer (unless current-prefix-arg
+                            (project--find-existing-buffer 'eshell-mode
+                                                           default-directory))))
+    (if existing-buffer
+        (pop-to-buffer-same-window existing-buffer)
+      (let* ((uniquify-list-buffers-directory-modes
+              (cons 'eshell-mode uniquify-list-buffers-directory-modes))
+             (buf (create-file-buffer (expand-file-name eshell-buffer-name))))
+        (pop-to-buffer-same-window buf)
+        (unless (derived-mode-p 'eshell-mode)
+          (eshell-mode))
+        (setq list-buffers-directory (expand-file-name eshell-buffer-name default-directory))
+        (setq project--origin-project-root default-directory)))))
 
 ;;;###autoload
 (defun project-async-shell-command ()

Re: Name of buffers created by project-shell

[Matthias Meulien]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 15.03.2021 02:41, Dmitry Gutov wrote:
>>> I think there's a problem even without using C-u:
>>>
>>> `project-shell' from a buffer whose associated project root path is
>>> /src/project1 creates a buffer named *shell*.
>>>
>>> Then `project-shell' from a buffer whose associated project root path is
>>> /src/project2 creates a buffer named *shell*<project2> (I am using the
>>> default `post-forward-angle-brackets' for `uniquify-buffer-name-style').
>>>
>>> But the uniquify rationalize mecanism doesn't rename the first buffer to
>>> *shell*<project1> which happens to be the case for buffers visiting
>>> files...
>> Yup, that's another bug. It should also how up for vc-dir buffers
>> too.
>
> Actually, it didn't occur for vc-dir buffers. So it's arguably a bug
> in my code.
>
> Attaching the updated patch which fixes that particular problem,
> though I'm a little more suspicious of some of uniquify's choices now.

I confirm that with that patch names of buffers created using
`project-shell' (without using C-u) are correct.
-- 
Matthias

Re: Name of buffers created by project-shell

[Juri Linkov]

>> Attaching the updated patch which fixes that particular problem,
>> though I'm a little more suspicious of some of uniquify's choices now.
>
> I confirm that with that patch names of buffers created using
> `project-shell' (without using C-u) are correct.

Please define "correct".  Correct in the sense that it works only without C-u?
Because while currently buffer names created with C-u are nicely formatted:

project1:
- *project1-shell*
- *project1-shell*<2>
- *project1-shell*<3>

project2:
- *project2-shell*
- *project2-shell*<2>
- *project2-shell*<3>

But after the change buffers created by 'C-u project-shell' are quite messy:

project1:
- *shell*
- *shell*<2>
- *shell*<4>

project2:
- *shell*<project2>
- *shell*<3>
- *shell*<5>

Re: Name of buffers created by project-shell

[Dmitry Gutov]

On 24.03.2021 22:12, Juri Linkov wrote:
> Please define "correct".  Correct in the sense that it works only without C-u?

Yes.

> Because while currently buffer names created with C-u are nicely formatted:
> 
> project1:
> -*project1-shell*
> -*project1-shell*<2>
> -*project1-shell*<3>
> 
> project2:
> -*project2-shell*
> -*project2-shell*<2>
> -*project2-shell*<3>
> 
> But after the change buffers created by 'C-u project-shell' are quite messy:
> 
> project1:
> -*shell*
> -*shell*<2>
> -*shell*<4>
> 
> project2:
> -*shell*<project2>
> -*shell*<3>
> -*shell*<5>

The second bug remains, yes. Or we would be installing the patch already.

Re: Name of buffers created by project-shell

[Matthias Meulien]

Hi Dmitry,

Sorry for the late reply, I didn't found much time to work on this until
now.

> Actually, it didn't occur for vc-dir buffers. So it's arguably a bug
> in my code.
>
> Attaching the updated patch which fixes that particular problem,
> though I'm a little more suspicious of some of uniquify's choices now.

I agree that uniquify way of handling buffers not visiting files is
fragile...

> In particular, this code depends on list-buffers-directory being set
> to a value in particular format which is very non-obvious from this
> variable's docstring.

Yes, and shell-mode already sets list-buffers-directory to the default
directory which is not the

   (expand-file-name "*shell*" default-directory)

that uniquify would expect (and is overwritten by your patch).

> And to have uniquify work similarly with buffers created by M-x shell
> and M-x eshell as well, these commands need similar changes (we can't
> really depend on them in project.el because it's an ELPA Core
> package), as well as shell-mode and eshell-mode being listed in
> uniquify-list-buffers-directory-modes.

When shell minor mode dirtrack-mode is enabled (the default), the value
of list-buffers-directory is updated when cd, pushd, popd, etc. are
issued; It will thus interfere if uniquify has to rerationalize the
buffer name... 

I saw in commit 1c3a86e7fc2 that project-prefixed-buffer-name has been
introduced which I understand as "forgot of uniquify since it doesn't
handle buffer not visiting files properly". I have some time to work on
improving the uniquify situation but can you confirm there's still
interest for this?

My rough plan is to change uniquify-list-buffers-directory-modes to be a
mapping from major modes to functions that, given a buffer, returns the
buffer proposed name and a directory. This would remove the use of
list-buffers-directory in uniquify and gives the opportunity for each
major mode to provide its convenience function based on
current-directory or project-root.

The default won't be to add shell-mode and eshell-mode to this new
uniquify-list-buffers-directory-modes mapping (to avoid breaking current
behavior) but do it locally when shell or eshell are called from
project.el. Is this dependency acceptable?
-- 
Matthias

Re: Name of buffers created by project-shell

[Dmitry Gutov]

Hi Matthias,

On 08.05.2021 20:09, Matthias Meulien wrote:

> When shell minor mode dirtrack-mode is enabled (the default), the value
> of list-buffers-directory is updated when cd, pushd, popd, etc. are
> issued; It will thus interfere if uniquify has to rerationalize the
> buffer name...
> 
> I saw in commit 1c3a86e7fc2 that project-prefixed-buffer-name has been
> introduced which I understand as "forgot of uniquify since it doesn't
> handle buffer not visiting files properly".

Not really. Also see my comment here: https://debbugs.gnu.org/47975#8

> I have some time to work on
> improving the uniquify situation but can you confirm there's still
> interest for this?

Yes, of course. Even if we in the end decide that uniquify is not 
suitable for project.el's needs, some cleanup and better handling of 
edge cases would still be welcome.

But your plan below sounds reasonable, even if I don't have the time ATM 
to re-familiarize myself with the implementation details.

> My rough plan is to change uniquify-list-buffers-directory-modes to be a
> mapping from major modes to functions that, given a buffer, returns the
> buffer proposed name and a directory. This would remove the use of
> list-buffers-directory in uniquify and gives the opportunity for each
> major mode to provide its convenience function based on
> current-directory or project-root.
> 
> The default won't be to add shell-mode and eshell-mode to this new
> uniquify-list-buffers-directory-modes mapping (to avoid breaking current
> behavior) but do it locally when shell or eshell are called from
> project.el. Is this dependency acceptable?

:thumbsup: