bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Protesilaos Stavrou]

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

Dear maintainers,

I noticed that M-x shell does not have a bookmark handler like M-x
eshell does.  What do you think about the attached patch?

All the best,
Protesilaos (or simply "Prot")

-- 
Protesilaos Stavrou
https://protesilaos.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-bookmark-handler-for-M-x-shell.patch --]
[-- Type: text/x-patch, Size: 2603 bytes --]

From 69744f953b95dc2f3c9c3039324e121a6d664570 Mon Sep 17 00:00:00 2001
Message-ID: <69744f953b95dc2f3c9c3039324e121a6d664570.1691073443.git.info@protesilaos.com>
From: Protesilaos Stavrou <info@protesilaos.com>
Date: Thu, 3 Aug 2023 17:35:10 +0300
Subject: [PATCH] Add bookmark handler for M-x shell

* etc/NEWS: Anounce the new feature.
* lisp/shell.el (shell-mode): Add buffer-local 'bookmark-make-record-function'.
(bookmark-prop-get, shell-bookmark-name, shell-bookmark-make-record)
(shell-bookmark-jump): Add bookmark handler.
---
 etc/NEWS      |  6 ++++++
 lisp/shell.el | 26 ++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index 7b521f3e6fe..da5d9b96002 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -286,6 +286,12 @@ When this user option is non-nil, 'shell-get-old-input' ('C-RET')
 includes multiple shell "\" continuation lines from command output.
 Default is nil.
 
+---
+*** Bookmark handler for 'shell' buffers
+Now the 'bookmark-set' command will record 'shell' buffers.  This
+means that 'bookmark-jump' will create the 'shell' buffer in the
+directory it was in.
+
 ** Prog Mode
 
 +++
diff --git a/lisp/shell.el b/lisp/shell.el
index 0a24b4ea4c2..bdf8eb17fbd 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -676,6 +676,7 @@ (define-derived-mode shell-mode comint-mode "Shell"
 
   (setq comint-prompt-regexp shell-prompt-pattern)
   (shell-completion-vars)
+  (setq-local bookmark-make-record-function #'shell-bookmark-make-record)
   (setq-local paragraph-separate "\\'")
   (setq-local paragraph-start comint-prompt-regexp)
   (setq-local font-lock-defaults '(shell-font-lock-keywords t))
@@ -1812,6 +1813,31 @@ (defun shell-highlight-undef-mode-restart ()
   (when shell-highlight-undef-mode
     (shell-highlight-undef-mode 1)))
 
+;;; Bookmark support
+
+;; Adapted from esh-mode.el
+(declare-function bookmark-prop-get "bookmark" (bookmark prop))
+
+(defun shell-bookmark-name ()
+  (format "shell-%s"
+          (file-name-nondirectory
+           (directory-file-name
+            (file-name-directory default-directory)))))
+
+(defun shell-bookmark-make-record ()
+  "Create a bookmark for the current Shell buffer."
+  `(,(shell-bookmark-name)
+    (location . ,default-directory)
+    (handler . shell-bookmark-jump)))
+
+;;;###autoload
+(defun shell-bookmark-jump (bookmark)
+  "Default bookmark handler for Shell buffers."
+  (let ((default-directory (bookmark-prop-get bookmark 'location)))
+    (shell)))
+
+(put 'shell-bookmark-jump 'bookmark-handler-type "Shell")
+
 (provide 'shell)
 
 ;;; shell.el ends here
-- 
2.41.0

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Eli Zaretskii]

> From: Protesilaos Stavrou <info@protesilaos.com>
> Date: Thu, 03 Aug 2023 17:41:27 +0300
> 
> I noticed that M-x shell does not have a bookmark handler like M-x
> eshell does.  What do you think about the attached patch?

I'll let users of bookmarks comment, but in any case, please also
check that the section "Bookmarks" in the Emacs user manual doesn't
need some update due to this feature.  (You marked the NEWS entry with
"---", which might mean you already checked that, but I'm not sure.)

Thanks.

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Visuwesh]

[வியாழன் ஆகஸ்ட் 03, 2023] Protesilaos Stavrou wrote:

> Dear maintainers,
>
> I noticed that M-x shell does not have a bookmark handler like M-x
> eshell does.  What do you think about the attached patch?

I think it would be nice to also store the "Remote shell path" for
remote TRAMP buffers.  I have no idea how to retrieve this value,
however.
[ When I visit a TRAMP ssh buffer and say M-x shell, it asks for the
  remote shell path.  ]

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Protesilaos Stavrou]

> From: Eli Zaretskii <eliz@gnu.org>
> Date: Thu,  3 Aug 2023 18:56:15 +0300
>
>> From: Protesilaos Stavrou <info@protesilaos.com>
>> Date: Thu, 03 Aug 2023 17:41:27 +0300
>> 
>> I noticed that M-x shell does not have a bookmark handler like M-x
>> eshell does.  What do you think about the attached patch?
>
> I'll let users of bookmarks comment, but in any case, please also
> check that the section "Bookmarks" in the Emacs user manual doesn't
> need some update due to this feature.  (You marked the NEWS entry with
> "---", which might mean you already checked that, but I'm not sure.)

I thought a change was not necessary.  Though I am happy to do it, if
needed.

-- 
Protesilaos Stavrou
https://protesilaos.com

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Protesilaos Stavrou]

> From: Visuwesh <visuweshm@gmail.com>
> Date: Thu,  3 Aug 2023 22:38:24 +0530
>
> [வியாழன் ஆகஸ்ட் 03, 2023] Protesilaos Stavrou wrote:
>
>> Dear maintainers,
>>
>> I noticed that M-x shell does not have a bookmark handler like M-x
>> eshell does.  What do you think about the attached patch?
>
> I think it would be nice to also store the "Remote shell path" for
> remote TRAMP buffers.  I have no idea how to retrieve this value,
> however.
> [ When I visit a TRAMP ssh buffer and say M-x shell, it asks for the
>   remote shell path.  ]

The code is adapted from Eshell, which has the capability you describe.
I do not have the means to test an SSH connection.  Though I tried the
'sudo' TRAMP method and the bookmarking correctly logs me in as root
when I do 'bookmark-jump'.  This works even if I kill the shell buffer
and all TRAMP buffers.

-- 
Protesilaos Stavrou
https://protesilaos.com

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Eli Zaretskii]

> From: Protesilaos Stavrou <info@protesilaos.com>
> Cc: 65039@debbugs.gnu.org
> Date: Fri, 04 Aug 2023 12:17:43 +0300
> 
> > I'll let users of bookmarks comment, but in any case, please also
> > check that the section "Bookmarks" in the Emacs user manual doesn't
> > need some update due to this feature.  (You marked the NEWS entry with
> > "---", which might mean you already checked that, but I'm not sure.)
> 
> I thought a change was not necessary.  Though I am happy to do it, if
> needed.

It sounds like the notion of "jumping" to a bookmark has evolved, and
nowadays jumping to a bookmark might do much more than just jump to a
buffer position.  Perhaps that node in the manual should say something
about that, and show a couple of examples?

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Visuwesh]

[வெள்ளி ஆகஸ்ட் 04, 2023] Protesilaos Stavrou wrote:

> The code is adapted from Eshell, which has the capability you describe.
> I do not have the means to test an SSH connection.  Though I tried the
> 'sudo' TRAMP method and the bookmarking correctly logs me in as root
> when I do 'bookmark-jump'.  This works even if I kill the shell buffer
> and all TRAMP buffers.

I see that `shell' sets the value of `explicit-shell-file-name' to the
filename of the remote shell chosen but unfortunately this gets set to
nil once `make-comint-in-buffer' function is called since `comint-mode'
kills all local variables.  :-(

I don't know how reliable of a solution

    (executable-find shell--start-prog)

is to get the absolute filename of the shell being used.

If that is an acceptable solution, then the following diff works fine
for both remote and local shells.

diff --git a/lisp/shell.el b/lisp/shell.el
index 5cf108bfa3..8396870a67 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -637,6 +637,7 @@ shell-mode
 
   (setq comint-prompt-regexp shell-prompt-pattern)
   (shell-completion-vars)
+  (setq-local bookmark-make-record-function #'shell-bookmark-make-record)
   (setq-local paragraph-separate "\\'")
   (setq-local paragraph-start comint-prompt-regexp)
   (setq-local font-lock-defaults '(shell-font-lock-keywords t))
@@ -1770,6 +1771,32 @@ shell-highlight-undef-mode-restart
   (when shell-highlight-undef-mode
     (shell-highlight-undef-mode 1)))
 
+;;; Bookmark support
+
+;; Adapted from esh-mode.el
+(declare-function bookmark-prop-get "bookmark" (bookmark prop))
+
+(defun shell-bookmark-name ()
+  (format "shell-%s"
+          (file-name-nondirectory
+           (directory-file-name
+            (file-name-directory default-directory)))))
+
+(defun shell-bookmark-make-record ()
+  "Create a bookmark for the current Shell buffer."
+  `(,(shell-bookmark-name)
+    (location . ,default-directory)
+    (shell-filename . ,(executable-find shell--start-prog))
+    (handler . shell-bookmark-jump)))
+
+;;;###autoload
+(defun shell-bookmark-jump (bookmark)
+  "Default bookmark handler for Shell buffers."
+  (let ((default-directory (bookmark-prop-get bookmark 'location)))
+    (shell nil (bookmark-prop-get bookmark 'shell-filename))))
+
+(put 'shell-bookmark-jump 'bookmark-handler-type "Shell")
+
 (provide 'shell)
 
 ;;; shell.el ends here

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Protesilaos Stavrou]

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

> From: Eli Zaretskii <eliz@gnu.org>
> Date: Fri,  4 Aug 2023 13:32:37 +0300
>
>> From: Protesilaos Stavrou <info@protesilaos.com>
>> Cc: 65039@debbugs.gnu.org
>> Date: Fri, 04 Aug 2023 12:17:43 +0300
>> 
>> > I'll let users of bookmarks comment, but in any case, please also
>> > check that the section "Bookmarks" in the Emacs user manual doesn't
>> > need some update due to this feature.  (You marked the NEWS entry with
>> > "---", which might mean you already checked that, but I'm not sure.)
>> 
>> I thought a change was not necessary.  Though I am happy to do it, if
>> needed.
>
> It sounds like the notion of "jumping" to a bookmark has evolved, and
> nowadays jumping to a bookmark might do much more than just jump to a
> buffer position.  Perhaps that node in the manual should say something
> about that, and show a couple of examples?

The revised patch includes a possible update to the manual.  Are those
examples sufficient?

-- 
Protesilaos Stavrou
https://protesilaos.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-bookmark-handler-for-M-x-shell.patch --]
[-- Type: text/x-patch, Size: 3519 bytes --]

From 4c903061803f7e41cc4f87f80b1b089c2174cc21 Mon Sep 17 00:00:00 2001
Message-ID: <4c903061803f7e41cc4f87f80b1b089c2174cc21.1691157813.git.info@protesilaos.com>
From: Protesilaos Stavrou <info@protesilaos.com>
Date: Fri, 4 Aug 2023 17:03:08 +0300
Subject: [PATCH] Add bookmark handler for M-x shell

* doc/emacs/regs.texi (Bookmarks): Explain that 'bookmark-jump'
establishes a remote connection.
* etc/NEWS: Announce the new feature.
* lisp/shell.el (shell-mode): Add buffer-local value for 'bookmark-make-record-function'.
(bookmark-prop-get, shell-bookmark-name, shell-bookmark-make-record)
(shell-bookmark-jump): Add section about the bookmark handler.
---
 doc/emacs/regs.texi |  6 ++++++
 etc/NEWS            |  6 ++++++
 lisp/shell.el       | 26 ++++++++++++++++++++++++++
 3 files changed, 38 insertions(+)

diff --git a/doc/emacs/regs.texi b/doc/emacs/regs.texi
index e52f68dd18e..2debf9988c2 100644
--- a/doc/emacs/regs.texi
+++ b/doc/emacs/regs.texi
@@ -391,6 +391,12 @@ many characters of context to record on each side of the bookmark's
 position.  (In buffers that are visiting encrypted files, no context
 is saved in the bookmarks file no matter the value of this variable.)
 
+  If the bookmark is stored in a remote location, @code{bookmark-jump}
+will establish the connection and then create the buffer.  This works
+with regular files, as well as the buffers of @kbd{M-x dired} and
+@kbd{M-x shell}.  @xref{Top, The Tramp Manual,, tramp, The Tramp
+Manual}.
+
   Here are some additional commands for working with bookmarks:
 
 @table @kbd
diff --git a/etc/NEWS b/etc/NEWS
index 7b521f3e6fe..6329165cda2 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -286,6 +286,12 @@ When this user option is non-nil, 'shell-get-old-input' ('C-RET')
 includes multiple shell "\" continuation lines from command output.
 Default is nil.
 
++++
+*** Bookmark handler for 'shell' buffers
+Now the 'bookmark-set' command will record 'shell' buffers.  This
+means that 'bookmark-jump' will create the 'shell' buffer in the
+directory it was in.
+
 ** Prog Mode
 
 +++
diff --git a/lisp/shell.el b/lisp/shell.el
index 0a24b4ea4c2..bdf8eb17fbd 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -676,6 +676,7 @@ (define-derived-mode shell-mode comint-mode "Shell"
 
   (setq comint-prompt-regexp shell-prompt-pattern)
   (shell-completion-vars)
+  (setq-local bookmark-make-record-function #'shell-bookmark-make-record)
   (setq-local paragraph-separate "\\'")
   (setq-local paragraph-start comint-prompt-regexp)
   (setq-local font-lock-defaults '(shell-font-lock-keywords t))
@@ -1812,6 +1813,31 @@ (defun shell-highlight-undef-mode-restart ()
   (when shell-highlight-undef-mode
     (shell-highlight-undef-mode 1)))
 
+;;; Bookmark support
+
+;; Adapted from esh-mode.el
+(declare-function bookmark-prop-get "bookmark" (bookmark prop))
+
+(defun shell-bookmark-name ()
+  (format "shell-%s"
+          (file-name-nondirectory
+           (directory-file-name
+            (file-name-directory default-directory)))))
+
+(defun shell-bookmark-make-record ()
+  "Create a bookmark for the current Shell buffer."
+  `(,(shell-bookmark-name)
+    (location . ,default-directory)
+    (handler . shell-bookmark-jump)))
+
+;;;###autoload
+(defun shell-bookmark-jump (bookmark)
+  "Default bookmark handler for Shell buffers."
+  (let ((default-directory (bookmark-prop-get bookmark 'location)))
+    (shell)))
+
+(put 'shell-bookmark-jump 'bookmark-handler-type "Shell")
+
 (provide 'shell)
 
 ;;; shell.el ends here
-- 
2.41.0

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Protesilaos Stavrou]

> From: Visuwesh <visuweshm@gmail.com>
> Date: Fri,  4 Aug 2023 18:31:16 +0530
>
> [வெள்ளி ஆகஸ்ட் 04, 2023] Protesilaos Stavrou wrote:
>
>> The code is adapted from Eshell, which has the capability you describe.
>> I do not have the means to test an SSH connection.  Though I tried the
>> 'sudo' TRAMP method and the bookmarking correctly logs me in as root
>> when I do 'bookmark-jump'.  This works even if I kill the shell buffer
>> and all TRAMP buffers.
>
> I see that `shell' sets the value of `explicit-shell-file-name' to the
> filename of the remote shell chosen but unfortunately this gets set to
> nil once `make-comint-in-buffer' function is called since `comint-mode'
> kills all local variables.  :-(
>
> I don't know how reliable of a solution
>
>     (executable-find shell--start-prog)
>
> is to get the absolute filename of the shell being used.

Thank you!  This seems reasonable.  Have you checked the variable
'shell-file-name'?

> If that is an acceptable solution, then the following diff works fine
> for both remote and local shells.

> [... 47 lines elided]

As noted before, I cannot test your suggested changes as I have no SSH
connection available.  Hopefully, someone can help try this.

-- 
Protesilaos Stavrou
https://protesilaos.com

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Visuwesh]

[வெள்ளி ஆகஸ்ட் 04, 2023] Protesilaos Stavrou wrote:

>>> The code is adapted from Eshell, which has the capability you describe.
>>> I do not have the means to test an SSH connection.  Though I tried the
>>> 'sudo' TRAMP method and the bookmarking correctly logs me in as root
>>> when I do 'bookmark-jump'.  This works even if I kill the shell buffer
>>> and all TRAMP buffers.
>>
>> I see that `shell' sets the value of `explicit-shell-file-name' to the
>> filename of the remote shell chosen but unfortunately this gets set to
>> nil once `make-comint-in-buffer' function is called since `comint-mode'
>> kills all local variables.  :-(
>>
>> I don't know how reliable of a solution
>>
>>     (executable-find shell--start-prog)
>>
>> is to get the absolute filename of the shell being used.
>
> Thank you!  This seems reasonable.  Have you checked the variable
> 'shell-file-name'?

Unfortunately, it is not always reliable.  I use mksh as my (local)
shell but I use bash in the remote system.  In these remote shells, I
don't see the correct value being set:

    (list major-mode (file-remote-p default-directory) shell-file-name shell--start-prog)
        ⇒ (shell-mode "/ssh:REDACTED@REDACTED:" "/bin/mksh" "bash")

`shell' also has this comment before the prompt for remote shell
filename:

    ;; On remote hosts, the local `shell-file-name' might be useless.

HTH.

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Drew Adams]

> It sounds like the notion of "jumping" to a bookmark has evolved, and
> nowadays jumping to a bookmark might do much more than just jump to a
> buffer position.  Perhaps that node in the manual should say something
> about that, and show a couple of examples?

Not weighing in on whether the manual
should be changed.  Just thought I'd
mention that the notion of "jumping"
to a bookmark has always included the
possibility of doing "much more" - as
well as much _less_.

It's _always_ been the case that
"jumping" to a bookmark can do anything
at all.  A bookmark can record nearly
any data, and a bookmark handler is
just a function - it can do anything
a function can do.

(But yes, it might help for the manual
to say this explicitly.  "Jumping" to
a bookmark is both evocative, for many
or most bookmarks, and misleading, for
some bookmarks.)

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Jim Porter]

On 8/4/2023 2:20 AM, Protesilaos Stavrou wrote:
> The code is adapted from Eshell, which has the capability you describe.
> I do not have the means to test an SSH connection.  Though I tried the
> 'sudo' TRAMP method and the bookmarking correctly logs me in as root
> when I do 'bookmark-jump'.  This works even if I kill the shell buffer
> and all TRAMP buffers.

For what it's worth, when I want to test Tramp support (especially in 
something like Eshell or Shell), I just connect to localhost via 
"/ssh:localhost:~" or similar. So long as your system is running sshd, 
that should work fine.

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Eli Zaretskii]

> From: Protesilaos Stavrou <info@protesilaos.com>
> Cc: 65039@debbugs.gnu.org
> Date: Fri, 04 Aug 2023 17:06:59 +0300
> 
> > From: Eli Zaretskii <eliz@gnu.org>
> > Date: Fri,  4 Aug 2023 13:32:37 +0300
> >
> >> From: Protesilaos Stavrou <info@protesilaos.com>
> >> Cc: 65039@debbugs.gnu.org
> >> Date: Fri, 04 Aug 2023 12:17:43 +0300
> >> 
> >> > I'll let users of bookmarks comment, but in any case, please also
> >> > check that the section "Bookmarks" in the Emacs user manual doesn't
> >> > need some update due to this feature.  (You marked the NEWS entry with
> >> > "---", which might mean you already checked that, but I'm not sure.)
> >> 
> >> I thought a change was not necessary.  Though I am happy to do it, if
> >> needed.
> >
> > It sounds like the notion of "jumping" to a bookmark has evolved, and
> > nowadays jumping to a bookmark might do much more than just jump to a
> > buffer position.  Perhaps that node in the manual should say something
> > about that, and show a couple of examples?
> 
> The revised patch includes a possible update to the manual.  Are those
> examples sufficient?

I guess so, thanks.

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Protesilaos Stavrou]

> From: Jim Porter <jporterbugs@gmail.com>
> Date: Fri,  4 Aug 2023 10:01:12 -0700
>
> On 8/4/2023 2:20 AM, Protesilaos Stavrou wrote:
>> The code is adapted from Eshell, which has the capability you describe.
>> I do not have the means to test an SSH connection.  Though I tried the
>> 'sudo' TRAMP method and the bookmarking correctly logs me in as root
>> when I do 'bookmark-jump'.  This works even if I kill the shell buffer
>> and all TRAMP buffers.
>
> For what it's worth, when I want to test Tramp support (especially in 
> something like Eshell or Shell), I just connect to localhost via 
> "/ssh:localhost:~" or similar. So long as your system is running sshd, 
> that should work fine.

Thank you!  This looks promising.  I am trying to make it work, but it
denies the connection.  Maybe you can share with me off-list the
relevant sshd settings?  I tried to disable public key checking and
enable passwords.  To no avail.

-- 
Protesilaos Stavrou
https://protesilaos.com

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Protesilaos Stavrou]

> From: Visuwesh <visuweshm@gmail.com>
> Date: Fri,  4 Aug 2023 20:05:03 +0530

> [... 20 lines elided]

>> Thank you!  This seems reasonable.  Have you checked the variable
>> 'shell-file-name'?
>
> Unfortunately, it is not always reliable.  I use mksh as my (local)
> shell but I use bash in the remote system.  In these remote shells, I
> don't see the correct value being set:
>
>     (list major-mode (file-remote-p default-directory) shell-file-name shell--start-prog)
>         ⇒ (shell-mode "/ssh:REDACTED@REDACTED:" "/bin/mksh" "bash")
>
> `shell' also has this comment before the prompt for remote shell
> filename:
>
>     ;; On remote hosts, the local `shell-file-name' might be useless.
>
> HTH.

I see.  Thanks for the explanation!  I shall revisit this patch as soon
as I have a way to test ssh myself.  Not sure when...

-- 
Protesilaos Stavrou
https://protesilaos.com

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Protesilaos Stavrou <info@protesilaos.com>
>> Cc: 65039@debbugs.gnu.org
>> Date: Fri, 04 Aug 2023 17:06:59 +0300
>>
>> > From: Eli Zaretskii <eliz@gnu.org>
>> > Date: Fri,  4 Aug 2023 13:32:37 +0300
>> >
>> >> From: Protesilaos Stavrou <info@protesilaos.com>
>> >> Cc: 65039@debbugs.gnu.org
>> >> Date: Fri, 04 Aug 2023 12:17:43 +0300
>> >>
>> >> > I'll let users of bookmarks comment, but in any case, please also
>> >> > check that the section "Bookmarks" in the Emacs user manual doesn't
>> >> > need some update due to this feature.  (You marked the NEWS entry with
>> >> > "---", which might mean you already checked that, but I'm not sure.)
>> >>
>> >> I thought a change was not necessary.  Though I am happy to do it, if
>> >> needed.
>> >
>> > It sounds like the notion of "jumping" to a bookmark has evolved, and
>> > nowadays jumping to a bookmark might do much more than just jump to a
>> > buffer position.  Perhaps that node in the manual should say something
>> > about that, and show a couple of examples?
>>
>> The revised patch includes a possible update to the manual.  Are those
>> examples sufficient?
>
> I guess so, thanks.

Just to let you know, I had an issue with applying the patch, and had to
manually edit it:

  1 git … am --3way -- ~/wip/emacs/0001-Add-bookmark-handler-for-M-x-shell.patch
Line longer than 78 characters in commit message
Commit aborted; please see the file CONTRIBUTE

Also, when the bug number is known, it is good if you can include it
somewhere in the commit message.

I was going to review and install this patch, but I noticed that there
was some further discussion in a subthread regarding some Tramp stuff?
Should that be resolved first, or is this ready as-is?

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sun, 3 Sep 2023 04:15:57 -0700
> Cc: Protesilaos Stavrou <info@protesilaos.com>, 65039@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> The revised patch includes a possible update to the manual.  Are those
> >> examples sufficient?
> >
> > I guess so, thanks.
> 
> Just to let you know, I had an issue with applying the patch, and had to
> manually edit it:
> 
>   1 git … am --3way -- ~/wip/emacs/0001-Add-bookmark-handler-for-M-x-shell.patch
> Line longer than 78 characters in commit message
> Commit aborted; please see the file CONTRIBUTE
> 
> Also, when the bug number is known, it is good if you can include it
> somewhere in the commit message.
> 
> I was going to review and install this patch, but I noticed that there
> was some further discussion in a subthread regarding some Tramp stuff?
> Should that be resolved first, or is this ready as-is?

Yes, I think those issues need to be resolved first.  I only reviewed
part of the patch.

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Protesilaos Stavrou]

Good morning Stefan,

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sun,  3 Sep 2023 04:15:57 -0700

> [... 32 lines elided]

> Just to let you know, I had an issue with applying the patch, and had to
> manually edit it:
>
>   1 git … am --3way -- ~/wip/emacs/0001-Add-bookmark-handler-for-M-x-shell.patch
> Line longer than 78 characters in commit message
> Commit aborted; please see the file CONTRIBUTE
>
> Also, when the bug number is known, it is good if you can include it
> somewhere in the commit message.

Good to know.  Thanks!

> I was going to review and install this patch, but I noticed that there
> was some further discussion in a subthread regarding some Tramp stuff?
> Should that be resolved first, or is this ready as-is?

I am using the patch locally and it works for me, including for the
Tramp 'sudo' method.  The problem is with a Tramp 'ssh' connection.  I
don't have a machine with ssh access to test this.  Another person
suggested a setup to establish an ssh connection to localhost, but I
cannot get it to work.

All the best,
Protesilaos (or simply "Prot")

-- 
Protesilaos Stavrou
https://protesilaos.com

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hello Prot,

> The problem is with a Tramp 'ssh' connection.  I don't have a machine
> with ssh access to test this.  Another person suggested a setup to
> establish an ssh connection to localhost, but I cannot get it to work.

If it helps, you can send me your SSH public key (possibly off-list),
and I'll give you SSH access to one of my machines for testing.


Best,

Eshel

bug#65039: 30.0.50; [PATCH] Add bookmark handler for M-x shell

[Protesilaos Stavrou]

> From: Eshel Yaron <me@eshelyaron.com>
> Date: Tue,  5 Sep 2023 08:17:50 +0200
>
> Hello Prot,

Hello Eshel,

Sorry for being slow to respond.  I did not have electricity at home.
Now I do and am back in action.

>> The problem is with a Tramp 'ssh' connection.  I don't have a machine
>> with ssh access to test this.  Another person suggested a setup to
>> establish an ssh connection to localhost, but I cannot get it to work.
>
> If it helps, you can send me your SSH public key (possibly off-list),
> and I'll give you SSH access to one of my machines for testing.

Thank you!  I will send it now off-list.

All the best,
Prot

-- 
Protesilaos Stavrou
https://protesilaos.com