INSIDE_EMACS and Tramp (was: master f28166d: Copy INSIDE_EMACS env variable to subprocesses in Eshell (Bug#25496))

INSIDE_EMACS and Tramp (was: master f28166d: Copy INSIDE_EMACS env variable to subprocesses in Eshell (Bug#25496))

[Michael Albinus]

npostavs@gmail.com (Noam Postavsky) writes:

> +** Eshell
> +
> +---
> +*** Environment variable INSIDE_EMACS is now copied to subprocesses.
> +Its value equals the result of evaluating '(format "%s,eshell" emacs-version)'.

This reminds me of a problem lying around for a while. Tramp, like other
packages, sets the environment variable INSIDE_EMACS. So if you eval

--8<---------------cut here---------------start------------->8---
(let ((default-directory "/ssh::"))
  (shell-command-to-string "echo $INSIDE_EMACS"))
--8<---------------cut here---------------end--------------->8---

you get "28.0.50,tramp:2.5.0-pre". But if you run in an eshell buffer

--8<---------------cut here---------------start------------->8---
~/src/emacs $ cd /ssh::
/ssh:detlef:/home/albinus $ *echo $INSIDE_EMACS
--8<---------------cut here---------------end--------------->8---

you get "28.0.50,eshell". Tramp's setting is overwritten.

Shouldn't the specifics be merged, so that we get
"28.0.50,eshell,tramp:2.5.0-pre"?

Same for the other packages setting INSIDE_EMACS, like compile, comint,
term and epg.

Best regards, Michael.

Re: INSIDE_EMACS and Tramp (was: master f28166d: Copy INSIDE_EMACS env variable to subprocesses in Eshell (Bug#25496))

[Federico Tedin]

Hey Michael, some questions:

> --8<---------------cut here---------------start------------->8---
> (let ((default-directory "/ssh::"))
>   (shell-command-to-string "echo $INSIDE_EMACS"))
> --8<---------------cut here---------------end--------------->8---
>
> you get "28.0.50,tramp:2.5.0-pre". But if you run in an eshell buffer

Would it make sense to change this to "28.0.50,,tramp:2.5.0-pre"
(notice the extra comma), or "28.0.50,shell,tramp:2.5.0-pre"?
This way, the second element in the list will always be the shell
type, and the optional third will be the Tramp version. Though I'm
not sure if "shell" would correspond to "shell-command-to-string".

> --8<---------------cut here---------------start------------->8---
> ~/src/emacs $ cd /ssh::
> /ssh:detlef:/home/albinus $ *echo $INSIDE_EMACS
> --8<---------------cut here---------------end--------------->8---
>
> you get "28.0.50,eshell". Tramp's setting is overwritten.
>
> Shouldn't the specifics be merged, so that we get
> "28.0.50,eshell,tramp:2.5.0-pre"?
>
> Same for the other packages setting INSIDE_EMACS, like compile, comint,
> term and epg.

I could maybe start working on adding this for Eshell (now that I more
or less know how it internally handles variables) if you think that
makes sense.

- Fede

Re: INSIDE_EMACS and Tramp

[Michael Albinus]

Federico Tedin <federicotedin@gmail.com> writes:

> Hey Michael, some questions:

Hi Federico,

>> --8<---------------cut here---------------start------------->8---
>> (let ((default-directory "/ssh::"))
>>   (shell-command-to-string "echo $INSIDE_EMACS"))
>> --8<---------------cut here---------------end--------------->8---
>>
>> you get "28.0.50,tramp:2.5.0-pre". But if you run in an eshell buffer
>
> Would it make sense to change this to "28.0.50,,tramp:2.5.0-pre"
> (notice the extra comma), or "28.0.50,shell,tramp:2.5.0-pre"?
> This way, the second element in the list will always be the shell
> type, and the optional third will be the Tramp version. Though I'm
> not sure if "shell" would correspond to "shell-command-to-string".

Tramp is the obvious victim, but I don't know whether it is the only
package which will be mentioned in combination with another package in
INSIDE_EMACS.

Furthermore, in eshell or any other package you don't see what Tramp
sets in the remote shell. I believe we need a mechanism where different
packages add their settings (it's not only "shell" related, epg or
compile say something different). And when this variable is set for a
process, there must be a way to use these cumulated settings inside of
just the value a single package want set.

In the local case, this cumulative setting might be just the package
name like "eshell".

>> --8<---------------cut here---------------start------------->8---
>> ~/src/emacs $ cd /ssh::
>> /ssh:detlef:/home/albinus $ *echo $INSIDE_EMACS
>> --8<---------------cut here---------------end--------------->8---
>>
>> you get "28.0.50,eshell". Tramp's setting is overwritten.
>>
>> Shouldn't the specifics be merged, so that we get
>> "28.0.50,eshell,tramp:2.5.0-pre"?
>>
>> Same for the other packages setting INSIDE_EMACS, like compile, comint,
>> term and epg.
>
> I could maybe start working on adding this for Eshell (now that I more
> or less know how it internally handles variables) if you think that
> makes sense.

First we shall agree a common mechanism. Something which works exactly
for eshel and Tramp isn't sufficient.

> - Fede

Best regards, Michael.

Re: INSIDE_EMACS and Tramp

[Federico Tedin]

> Tramp is the obvious victim, but I don't know whether it is the only
> package which will be mentioned in combination with another package in
> INSIDE_EMACS.
>
> Furthermore, in eshell or any other package you don't see what Tramp
> sets in the remote shell. I believe we need a mechanism where different
> packages add their settings (it's not only "shell" related, epg or
> compile say something different). And when this variable is set for a
> process, there must be a way to use these cumulated settings inside of
> just the value a single package want set.
>
> In the local case, this cumulative setting might be just the package
> name like "eshell".
>
> First we shall agree a common mechanism. Something which works exactly
> for eshel and Tramp isn't sufficient.

Aah OK, so then this mechanism for generating the value for INSIDE_EMACS
needs to be more general. How about something like:

(defvar inside-emacs-extras nil)

(defun inside-emacs (&optional context)
  (mapconcat #'identity `(,emacs-version
			  ,@(when context (list context))
			  ,@inside-emacs-extras)
	     ","))

So in Eshell we would call (inside-emacs "eshell"), and Tramp could
optionally add "tramp:2.5.0-pre" to inside-emacs-extras sometime before
the function is called. Other parts of Emacs could add extras as well
(and at some point remove them, I suppose). Do you think something like
this would make sense given how Tramp works internally?

Re: INSIDE_EMACS and Tramp

[Michael Albinus]

Federico Tedin <federicotedin@gmail.com> writes:

Hi Federico,

> Aah OK, so then this mechanism for generating the value for INSIDE_EMACS
> needs to be more general. How about something like:
>
> (defvar inside-emacs-extras nil)
>
> (defun inside-emacs (&optional context)
>   (mapconcat #'identity `(,emacs-version
> 			  ,@(when context (list context))
> 			  ,@inside-emacs-extras)
> 	     ","))
>
> So in Eshell we would call (inside-emacs "eshell"), and Tramp could
> optionally add "tramp:2.5.0-pre" to inside-emacs-extras sometime before
> the function is called. Other parts of Emacs could add extras as well
> (and at some point remove them, I suppose). Do you think something like
> this would make sense given how Tramp works internally?

I don't believe a global variable would fly. In Eshell, for example,
this variable must be changed whenever the default-directory is changed
(being a local or a remote file name). Tramp is not involved, when you
change the default-directory.

Instead, there might be a hook which is called whenever your
inside-emacs function is applied. Something like this:

--8<---------------cut here---------------start------------->8---
(defvar inside-emacs-hook nil)

(defun inside-emacs (&optional context)
  (mapconcat
   #'identity
   (delq nil `(,emacs-version
	       ,@(when (stringp context) (list context))
	       ,(run-hook 'inside-emacs-hook)))
   ","))

(add-hook
 'inside-emacs-hook
 (lambda ()
   (when (file-remote-p default-directory) '("tramp:2.5.0-pre"))))
--8<---------------cut here---------------end--------------->8---

The add-hook call would be implemented in Tramp itself.

Best regards, Michael.

Re: INSIDE_EMACS and Tramp

[Federico Tedin]

Hi Michael,

> I don't believe a global variable would fly. In Eshell, for example,
> this variable must be changed whenever the default-directory is changed
> (being a local or a remote file name). Tramp is not involved, when you
> change the default-directory.
>
> Instead, there might be a hook which is called whenever your
> inside-emacs function is applied. Something like this:
>
> (defvar inside-emacs-hook nil)
>
> (defun inside-emacs (&optional context)
>   (mapconcat
>    #'identity
>    (delq nil `(,emacs-version
> 	       ,@(when (stringp context) (list context))
> 	       ,(run-hook 'inside-emacs-hook)))
>    ","))
>
> (add-hook
>  'inside-emacs-hook
>  (lambda ()
>    (when (file-remote-p default-directory) '("tramp:2.5.0-pre"))))
>
> The add-hook call would be implemented in Tramp itself.
>
> Best regards, Michael.

OK, I see that it is more useful in this case to have a list of
functions that are called rather than a fixed list of strings. However I
didn't understand your use of `run-hooks' - wouldn't this function call
return nil in all cases? What would be an easy way of getting the
results of calling all the hooks?

- Fede

Re: INSIDE_EMACS and Tramp

[Michael Albinus]

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

Federico Tedin <federicotedin@gmail.com> writes:

> Hi Michael,

Hi Federico,

> OK, I see that it is more useful in this case to have a list of
> functions that are called rather than a fixed list of strings. However I
> didn't understand your use of `run-hooks' - wouldn't this function call
> return nil in all cases? What would be an easy way of getting the
> results of calling all the hooks?

You're right, `run-hooks' was a stupid idea. I have adapted my proposal
and prepared a patch which works for me as expected. See appended.

Eli, would it be OK to push something like this to master?

> - Fede

Best regards, Michael.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 5566 bytes --]

diff --git a/lisp/comint.el b/lisp/comint.el
index ea06f8af87..1045d02641 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -831,7 +831,7 @@ comint-exec-1
   (let ((process-environment
 	 (nconc
           (comint-term-environment)
-	  (list (format "INSIDE_EMACS=%s,comint" emacs-version))
+	  (list (inside-emacs-envvar "comint"))
 	  process-environment))
 	(default-directory
 	  (if (file-accessible-directory-p default-directory)
diff --git a/lisp/epg.el b/lisp/epg.el
index 222fd913e1..a5c06c9ae8 100644
--- a/lisp/epg.el
+++ b/lisp/epg.el
@@ -606,8 +606,7 @@ epg--start
 	    (cons (concat "GPG_TTY=" terminal-name)
 		  (cons "TERM=xterm" process-environment))))
     (setq process-environment
-	  (cons (format "INSIDE_EMACS=%s,epg" emacs-version)
-		process-environment))
+	  (cons (inside-emacs-envvar "epg") process-environment))
     ;; Record modified time of gpg-agent socket to restore the Emacs
     ;; frame on text terminal in `epg-wait-for-completion'.
     ;; See
diff --git a/lisp/eshell/em-dirs.el b/lisp/eshell/em-dirs.el
index 51df6fa1d5..7218279e08 100644
--- a/lisp/eshell/em-dirs.el
+++ b/lisp/eshell/em-dirs.el
@@ -168,9 +168,6 @@ eshell-dirstack
 (defvar eshell-last-dir-ring nil
   "The last directory that Eshell was in.")

-(defconst eshell-inside-emacs (format "%s,eshell" emacs-version)
-  "Value for the `INSIDE_EMACS' environment variable.")
-
 ;;; Functions:

 (defun eshell-dirs-initialize ()    ;Called from `eshell-mode' via intern-soft!
@@ -195,7 +192,8 @@ eshell-dirs-initialize
 			  (expand-file-name
 			   (ring-ref eshell-last-dir-ring 0))))
             t)
-           ("INSIDE_EMACS" eshell-inside-emacs
+           ("INSIDE_EMACS" ,(lambda (_indices)
+                              (inside-emacs "eshell"))
             t))))

   (when eshell-cd-on-directory
diff --git a/lisp/net/tramp-sh.el b/lisp/net/tramp-sh.el
index a39d503e22..414fca10cc 100644
--- a/lisp/net/tramp-sh.el
+++ b/lisp/net/tramp-sh.el
@@ -4166,10 +4166,10 @@ tramp-open-shell
        vec (format
 	    (eval-when-compile
 	      (concat
-	       "exec env TERM='%s' INSIDE_EMACS='%s,tramp:%s' "
+	       "exec env TERM='%s' INSIDE_EMACS='%s' "
 	       "ENV=%s %s PROMPT_COMMAND='' PS1=%s PS2='' PS3='' %s %s"))
             tramp-terminal-type
-            emacs-version tramp-version  ; INSIDE_EMACS
+            (inside-emacs)
             (or (getenv-internal "ENV" tramp-remote-process-environment) "")
 	    (if (stringp tramp-histfile-override)
 		(format "HISTFILE=%s"
diff --git a/lisp/net/trampver.el b/lisp/net/trampver.el
index 8d21133b3b..2351867129 100644
--- a/lisp/net/trampver.el
+++ b/lisp/net/trampver.el
@@ -76,6 +76,13 @@ tramp-repository-version
             (replace-regexp-in-string "\n" "" (emacs-version))))))
   (unless (string-equal "ok" x) (error "%s" x)))

+;; `inside-emacs-functions' has been introduces with Emacs 28.1.
+(when (symbolp inside-emacs-functions)
+  (add-hook
+   'inside-emacs-functions
+   (lambda ()
+     (when (file-remote-p default-directory) "tramp:2.5.0-pre"))))
+
 ;; Tramp versions integrated into Emacs.  If a user option declares a
 ;; `:package-version' which doesn't belong to an integrated Tramp
 ;; version, it must be added here as well (see `tramp-syntax', for
diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
index a76a3c44a3..65c5257d93 100644
--- a/lisp/progmodes/compile.el
+++ b/lisp/progmodes/compile.el
@@ -1823,7 +1823,7 @@ compilation-start
 	     (append
 	      compilation-environment
               (comint-term-environment)
-	      (list (format "INSIDE_EMACS=%s,compile" emacs-version))
+	      (list (inside-emacs-envvar "compile"))
 	      (copy-sequence process-environment))))
 	(set (make-local-variable 'compilation-arguments)
 	     (list command mode name-function highlight-regexp))
diff --git a/lisp/simple.el b/lisp/simple.el
index b5ba05426f..70621225ec 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -4295,6 +4295,29 @@ list-processes
     (tabulated-list-print))
   (display-buffer buffer)
   nil)
+
+(defvar inside-emacs-functions nil
+  "List of functions to compose the environment variable INSIDE_EMACS.
+Every package which needs to set a string in that envirenmont
+variable shall add a function without arguments, which returns
+the respective string, or nil.")
+
+(defun inside-emacs (&optional context)
+  "Return the string to be set in environment variable INSIDE_EMACS.
+CONTEXT could be a string which is added."
+  (mapconcat
+   #'identity
+   (delq nil (append `(,emacs-version
+ 	               ,(and (stringp context) context))
+ 	             (mapcar #'funcall inside-emacs-functions)))
+   ","))
+
+(defun inside-emacs-envvar (&optional context)
+  "Return the \"INSIDE_EMACS=...\" string.
+This can be used to modify the `process-environment'.  See
+`inside-emacs' for the CONTEXT parameter."
+  (concat "INSIDE_EMACS=" (inside-emacs context)))
+
 \f
 ;;;; Prefix commands

diff --git a/lisp/term.el b/lisp/term.el
index b990c83cfc..d6a1bdfd1f 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -1535,7 +1535,7 @@ term-exec-1
 	   (format term-termcap-format "TERMCAP="
 		   term-term-name term-height term-width)

-	   (format "INSIDE_EMACS=%s,term:%s" emacs-version term-protocol-version)
+	   (inside-emacs-envvar (format "term:%s" term-protocol-version))
 	   (format "LINES=%d" term-height)
 	   (format "COLUMNS=%d" term-width))
 	  process-environment))

Re: INSIDE_EMACS and Tramp

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Sat, 02 May 2020 16:54:57 +0200
> Cc: emacs-devel@gnu.org
> 
> You're right, `run-hooks' was a stupid idea. I have adapted my proposal
> and prepared a patch which works for me as expected. See appended.
> 
> Eli, would it be OK to push something like this to master?

You don't need to ask for permission to push to master.  Unless
there's something very controversial or problematic about this change,
in which case please tell which part.

Thanks.

Re: INSIDE_EMACS and Tramp

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> Eli, would it be OK to push something like this to master?
>
> You don't need to ask for permission to push to master.  Unless
> there's something very controversial or problematic about this change,
> in which case please tell which part.

There's nothing controversial. Maybe the function names could be
better. And documentation is missing; I'll produce some few phrases for
the manual.

If there's no objection, I'll push in a few days.

> Thanks.

Best regards, Michael.

Re: INSIDE_EMACS and Tramp

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: federicotedin@gmail.com,  emacs-devel@gnu.org
> Date: Sat, 02 May 2020 18:04:01 +0200
> 
> There's nothing controversial. Maybe the function names could be
> better. And documentation is missing; I'll produce some few phrases for
> the manual.

I sent some comments just now.

Re: INSIDE_EMACS and Tramp

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Sat, 02 May 2020 16:54:57 +0200
> Cc: emacs-devel@gnu.org
> 
> +(defvar inside-emacs-functions nil
> +  "List of functions to compose the environment variable INSIDE_EMACS.
> +Every package which needs to set a string in that envirenmont
> +variable shall add a function without arguments, which returns
> +the respective string, or nil.")

This should tell what happens with the values when there are more than
one function in the list.

> +(defun inside-emacs (&optional context)
> +  "Return the string to be set in environment variable INSIDE_EMACS.

"to be set in environment variable" sounds confusing.  How do you set
a string in a variable?

> +CONTEXT could be a string which is added."

Added to what?

This also needs a NEWS entry, I think.

Thanks.

Re: INSIDE_EMACS and Tramp

[Stefan Monnier]

> +(defvar inside-emacs-functions nil
[...]
> +(defun inside-emacs (&optional context)
> +  "Return the string to be set in environment variable INSIDE_EMACS.
> +CONTEXT could be a string which is added."
> +  (mapconcat
> +   #'identity
> +   (delq nil (append `(,emacs-version
> + 	               ,(and (stringp context) context))
> + 	             (mapcar #'funcall inside-emacs-functions)))
> +   ","))

Any chance we could live without `inside-emacs-functions` and instead do:

    (defun inside-emacs (&optional context)
      "Return the string to be set in environment variable INSIDE_EMACS.
    CONTEXT could be a string which is added."
      (let ((base (or (getenv "INSIDE_EMACS") emacs-version)))
        (if context (concat case "," context) base)))


-- Stefan

Re: INSIDE_EMACS and Tramp

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> +(defvar inside-emacs-functions nil
> [...]
>> +(defun inside-emacs (&optional context)
>> +  "Return the string to be set in environment variable INSIDE_EMACS.
>> +CONTEXT could be a string which is added."
>> +  (mapconcat
>> +   #'identity
>> +   (delq nil (append `(,emacs-version
>> + 	               ,(and (stringp context) context))
>> + 	             (mapcar #'funcall inside-emacs-functions)))
>> +   ","))
>
> Any chance we could live without `inside-emacs-functions` and instead do:
>
>     (defun inside-emacs (&optional context)
>       "Return the string to be set in environment variable INSIDE_EMACS.
>     CONTEXT could be a string which is added."
>       (let ((base (or (getenv "INSIDE_EMACS") emacs-version)))
>         (if context (concat case "," context) base)))

No. The point is that it is dynamic, whether Tramp needs to add a
substring to INSIDE_EMACS, or not. Compare

--8<---------------cut here---------------start------------->8---
(let ((default-directory "/"))
  (inside-emacs "foo"))

=> "28.0.50,foo"
--8<---------------cut here---------------end--------------->8---

--8<---------------cut here---------------start------------->8---
(let ((default-directory "/ssh::/"))
  (inside-emacs "foo"))

=> "28.0.50,foo,tramp:2.5.0-pre"
--8<---------------cut here---------------end--------------->8---

> -- Stefan

Best regards, Michael.

Re: INSIDE_EMACS and Tramp

[Stefan Monnier]

> No. The point is that it is dynamic, whether Tramp needs to add a
> substring to INSIDE_EMACS, or not. Compare
>
> --8<---------------cut here---------------start------------->8---
> (let ((default-directory "/"))
>   (inside-emacs "foo"))
>
> => "28.0.50,foo"
> --8<---------------cut here---------------end--------------->8---
>
> --8<---------------cut here---------------start------------->8---
> (let ((default-directory "/ssh::/"))
>   (inside-emacs "foo"))
>
> => "28.0.50,foo,tramp:2.5.0-pre"
> --8<---------------cut here---------------end--------------->8---

I don't understand this example: why should `inside-emacs` return
something related to Tramp just because default-directory happens to
currently point to a Tramp directory?

`inside-emacs` is only of interest when creating a subprocess, and if
that subprocess is created on a remote machine, Tramp will get a change
to add ",tramp" to it when it gets called by the
file-name-handler mechanism.

In the original situation, IIUC what happens that Eshell sets up
INSIDE_EMACS with ",eshell" then creates the process, which gets
delegated to Tramp, at which point Tramp would add ",tramp" to
INSIDE_EMACS so we'd get what we need.


        Stefan

Re: INSIDE_EMACS and Tramp

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

Hi Stefan,

> I don't understand this example: why should `inside-emacs` return
> something related to Tramp just because default-directory happens to
> currently point to a Tramp directory?
>
> `inside-emacs` is only of interest when creating a subprocess, and if
> that subprocess is created on a remote machine, Tramp will get a change
> to add ",tramp" to it when it gets called by the
> file-name-handler mechanism.

This is a shortened example for discussion; in reality we're speaking
about processes of course.

> In the original situation, IIUC what happens that Eshell sets up
> INSIDE_EMACS with ",eshell" then creates the process, which gets
> delegated to Tramp, at which point Tramp would add ",tramp" to
> INSIDE_EMACS so we'd get what we need.

This is the scenario indeed. But what happens now, w/o the patch:

- start-file-process (for example) is called.

- Tramp is invoked. It starts a remote shell, passing "
  "INSIDE=EMACS=28.0.50,tramp:2.5.0-pre".

- The command given by start-file-process is executed, including
  "INSIDE=EMACS=28.0.50,foo" (for example).

Whatever Tramp does, it is overwritten. This is the reason I have
proposed this mechanism. Calling '(inside-emacs "foo")' returns in the
remote case "28.0.50,foo,tramp:2.5.0-pre", as requested.

>         Stefan

Best regards, Michael.

Re: INSIDE_EMACS and Tramp

[Stefan Monnier]

>> In the original situation, IIUC what happens that Eshell sets up
>> INSIDE_EMACS with ",eshell" then creates the process, which gets
>> delegated to Tramp, at which point Tramp would add ",tramp" to
>> INSIDE_EMACS so we'd get what we need.
> This is the scenario indeed. But what happens now, w/o the patch:

w/o which patch?

> - start-file-process (for example) is called.
>
> - Tramp is invoked. It starts a remote shell, passing "
>   "INSIDE=EMACS=28.0.50,tramp:2.5.0-pre".
>
> - The command given by start-file-process is executed, including
>   "INSIDE_EMACS=28.0.50,foo" (for example).

I'm not sure I understand.  Do you mean to say that the code which calls
`start-file-process` runs a command of the form

    /usr/bin/env "INSIDE_EMACS=28.0.50,foo" CMD

[ or something morally equivalent ] ?

If so indeed my approach won't work, but is there a good reason
why the caller would want to do that instead of binding
process-environment around the call to `start-file-process`
(IOW, could we simply say that if the caller does that, they get what
they deserve?).

The problem with your approach is that it means calling `start-process`
from within a Tramp directory could launch a process with INSIDE_EMACS
that contains ",tramp" even tho the process is running locally.


        Stefan

Re: INSIDE_EMACS and Tramp

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

Hi Stefan,

>> This is the scenario indeed. But what happens now, w/o the patch:
>
> w/o which patch?

The patch I have shown two days ago and you have commented (Message-ID
<87o8r6wfni.fsf@gmx.de>).

>> - start-file-process (for example) is called.
>>
>> - Tramp is invoked. It starts a remote shell, passing "
>>   "INSIDE=EMACS=28.0.50,tramp:2.5.0-pre".
>>
>> - The command given by start-file-process is executed, including
>>   "INSIDE_EMACS=28.0.50,foo" (for example).
>
> I'm not sure I understand.  Do you mean to say that the code which calls
> `start-file-process` runs a command of the form
>
>     /usr/bin/env "INSIDE_EMACS=28.0.50,foo" CMD
>
> [ or something morally equivalent ] ?
>
> If so indeed my approach won't work, but is there a good reason
> why the caller would want to do that instead of binding
> process-environment around the call to `start-file-process`
> (IOW, could we simply say that if the caller does that, they get what
> they deserve?).

I'm mistaken. The packages use indeed process-environment for this setting.

> The problem with your approach is that it means calling `start-process`
> from within a Tramp directory could launch a process with INSIDE_EMACS
> that contains ",tramp" even tho the process is running locally.

No, Tramp adds the following code snippet to inside-emacs-functions:

  (when (file-remote-p default-directory) "tramp:2.5.0-pre"))

The substring about Tramp appears only when it is needed.

>         Stefan

Best regards, Michael.

Re: INSIDE_EMACS and Tramp

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

Hi Stefan,

>> If so indeed my approach won't work, but is there a good reason
>> why the caller would want to do that instead of binding
>> process-environment around the call to `start-file-process`
>> (IOW, could we simply say that if the caller does that, they get what
>> they deserve?).
>
> I'm mistaken. The packages use indeed process-environment for this setting.

This gives me another idea: Tramp could adapt INSIDE_EMACS as provided
by the packages via process-environment. Hmm, so simple - I should have
thought about earlier.

Pushed to master.

>>         Stefan

Best regards, Michael.

Re: INSIDE_EMACS and Tramp

[Stefan Monnier]

>> The problem with your approach is that it means calling `start-process`
>> from within a Tramp directory could launch a process with INSIDE_EMACS
>> that contains ",tramp" even tho the process is running locally.
>
> No, Tramp adds the following code snippet to inside-emacs-functions:
>
>   (when (file-remote-p default-directory) "tramp:2.5.0-pre"))
>
> The substring about Tramp appears only when it is needed.

No: `start-process` doesn't look at `default-directory` so the above
code snippet will add ",tramp" when the default-directory is remote, but
the process will still be launched locally.

> This gives me another idea: Tramp could adapt INSIDE_EMACS as provided
> by the packages via process-environment. Hmm, so simple - I should have
> thought about earlier.

Isn't that what my suggestion does?


        Stefan

Re: INSIDE_EMACS and Tramp

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> The problem with your approach is that it means calling `start-process`
>>> from within a Tramp directory could launch a process with INSIDE_EMACS
>>> that contains ",tramp" even tho the process is running locally.
>>
>> No, Tramp adds the following code snippet to inside-emacs-functions:
>>
>>   (when (file-remote-p default-directory) "tramp:2.5.0-pre"))
>>
>> The substring about Tramp appears only when it is needed.
>
> No: `start-process` doesn't look at `default-directory` so the above
> code snippet will add ",tramp" when the default-directory is remote, but
> the process will still be launched locally.

You're right.

>> This gives me another idea: Tramp could adapt INSIDE_EMACS as provided
>> by the packages via process-environment. Hmm, so simple - I should have
>> thought about earlier.
>
> Isn't that what my suggestion does?

Likely. Meanwhile, I've pushed a respective change to master.

>         Stefan

Best regards, Michael.

Re: INSIDE_EMACS and Tramp

[Federico Tedin]

Hey Michael,

> You're right, `run-hooks' was a stupid idea. I have adapted my proposal
> and prepared a patch which works for me as expected. See appended.

Thanks for creating this patch! I have a couple of
comments/observations. Note that this is my first time reviewing a patch
by mail so formatting may not be very tidy.

> diff --git a/lisp/comint.el b/lisp/comint.el
> index ea06f8af87..1045d02641 100644
> --- a/lisp/comint.el
> +++ b/lisp/comint.el
> @@ -831,7 +831,7 @@ comint-exec-1
>    (let ((process-environment
>  	 (nconc
>            (comint-term-environment)
> -	  (list (format "INSIDE_EMACS=%s,comint" emacs-version))
> +	  (list (inside-emacs-envvar "comint"))
>  	  process-environment))
>  	(default-directory
>  	  (if (file-accessible-directory-p default-directory)
> diff --git a/lisp/epg.el b/lisp/epg.el
> index 222fd913e1..a5c06c9ae8 100644
> --- a/lisp/epg.el
> +++ b/lisp/epg.el
> @@ -606,8 +606,7 @@ epg--start
>  	    (cons (concat "GPG_TTY=" terminal-name)
>  		  (cons "TERM=xterm" process-environment))))
>      (setq process-environment
> -	  (cons (format "INSIDE_EMACS=%s,epg" emacs-version)
> -		process-environment))
> +	  (cons (inside-emacs-envvar "epg") process-environment))
>      ;; Record modified time of gpg-agent socket to restore the Emacs
>      ;; frame on text terminal in `epg-wait-for-completion'.
>      ;; See
> diff --git a/lisp/eshell/em-dirs.el b/lisp/eshell/em-dirs.el
> index 51df6fa1d5..7218279e08 100644
> --- a/lisp/eshell/em-dirs.el
> +++ b/lisp/eshell/em-dirs.el
> @@ -168,9 +168,6 @@ eshell-dirstack
>  (defvar eshell-last-dir-ring nil
>    "The last directory that Eshell was in.")
>
> -(defconst eshell-inside-emacs (format "%s,eshell" emacs-version)
> -  "Value for the `INSIDE_EMACS' environment variable.")
> -
>  ;;; Functions:
>
>  (defun eshell-dirs-initialize ()    ;Called from `eshell-mode' via intern-soft!
> @@ -195,7 +192,8 @@ eshell-dirs-initialize
>  			  (expand-file-name
>  			   (ring-ref eshell-last-dir-ring 0))))
>              t)
> -           ("INSIDE_EMACS" eshell-inside-emacs
> +           ("INSIDE_EMACS" ,(lambda (_indices)
> +                              (inside-emacs "eshell"))
>              t))))
>
>    (when eshell-cd-on-directory
> diff --git a/lisp/net/tramp-sh.el b/lisp/net/tramp-sh.el
> index a39d503e22..414fca10cc 100644
> --- a/lisp/net/tramp-sh.el
> +++ b/lisp/net/tramp-sh.el
> @@ -4166,10 +4166,10 @@ tramp-open-shell
>         vec (format
>  	    (eval-when-compile
>  	      (concat
> -	       "exec env TERM='%s' INSIDE_EMACS='%s,tramp:%s' "
> +	       "exec env TERM='%s' INSIDE_EMACS='%s' "
>  	       "ENV=%s %s PROMPT_COMMAND='' PS1=%s PS2='' PS3='' %s %s"))
>              tramp-terminal-type
> -            emacs-version tramp-version  ; INSIDE_EMACS
> +            (inside-emacs)
>              (or (getenv-internal "ENV" tramp-remote-process-environment) "")
>  	    (if (stringp tramp-histfile-override)
>  		(format "HISTFILE=%s"
> diff --git a/lisp/net/trampver.el b/lisp/net/trampver.el
> index 8d21133b3b..2351867129 100644
> --- a/lisp/net/trampver.el
> +++ b/lisp/net/trampver.el
> @@ -76,6 +76,13 @@ tramp-repository-version
>              (replace-regexp-in-string "\n" "" (emacs-version))))))
>    (unless (string-equal "ok" x) (error "%s" x)))
>
> +;; `inside-emacs-functions' has been introduces with Emacs 28.1.
                                        ^^^^^^^^^^
                                           typo

> +(when (symbolp inside-emacs-functions)
> +  (add-hook
> +   'inside-emacs-functions
> +   (lambda ()
> +     (when (file-remote-p default-directory) "tramp:2.5.0-pre"))))
                                                ^^^^^^^^^^^^^^^^^

Should this be `tramp-version' instead?

> +
>  ;; Tramp versions integrated into Emacs.  If a user option declares a
>  ;; `:package-version' which doesn't belong to an integrated Tramp
>  ;; version, it must be added here as well (see `tramp-syntax', for
> diff --git a/lisp/progmodes/compile.el b/lisp/progmodes/compile.el
> index a76a3c44a3..65c5257d93 100644
> --- a/lisp/progmodes/compile.el
> +++ b/lisp/progmodes/compile.el
> @@ -1823,7 +1823,7 @@ compilation-start
>  	     (append
>  	      compilation-environment
>                (comint-term-environment)
> -	      (list (format "INSIDE_EMACS=%s,compile" emacs-version))
> +	      (list (inside-emacs-envvar "compile"))
>  	      (copy-sequence process-environment))))
>  	(set (make-local-variable 'compilation-arguments)
>  	     (list command mode name-function highlight-regexp))
> diff --git a/lisp/simple.el b/lisp/simple.el
> index b5ba05426f..70621225ec 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -4295,6 +4295,29 @@ list-processes
>      (tabulated-list-print))
>    (display-buffer buffer)
>    nil)
> +
> +(defvar inside-emacs-functions nil
> +  "List of functions to compose the environment variable INSIDE_EMACS.
> +Every package which needs to set a string in that envirenmont
                                                     ^^^^^^^^^^^
                                                        typo

> +variable shall add a function without arguments, which returns
> +the respective string, or nil.")
> +
> +(defun inside-emacs (&optional context)
> +  "Return the string to be set in environment variable INSIDE_EMACS.
> +CONTEXT could be a string which is added."
> +  (mapconcat
> +   #'identity
> +   (delq nil (append `(,emacs-version
> + 	               ,(and (stringp context) context))
> + 	             (mapcar #'funcall inside-emacs-functions)))
> +   ","))
> +
> +(defun inside-emacs-envvar (&optional context)
> +  "Return the \"INSIDE_EMACS=...\" string.
> +This can be used to modify the `process-environment'.  See
> +`inside-emacs' for the CONTEXT parameter."
> +  (concat "INSIDE_EMACS=" (inside-emacs context)))
> +
>  \f
>  ;;;; Prefix commands
>
> diff --git a/lisp/term.el b/lisp/term.el
> index b990c83cfc..d6a1bdfd1f 100644
> --- a/lisp/term.el
> +++ b/lisp/term.el
> @@ -1535,7 +1535,7 @@ term-exec-1
>  	   (format term-termcap-format "TERMCAP="
>  		   term-term-name term-height term-width)
>
> -	   (format "INSIDE_EMACS=%s,term:%s" emacs-version term-protocol-version)
> +	   (inside-emacs-envvar (format "term:%s" term-protocol-version))
>  	   (format "LINES=%d" term-height)
>  	   (format "COLUMNS=%d" term-width))
>  	  process-environment))

- Fede

Re: INSIDE_EMACS and Tramp

[Michael Albinus]

Federico Tedin <federicotedin@gmail.com> writes:

> Hey Michael,

Hi Federico,

> Thanks for creating this patch! I have a couple of
> comments/observations. Note that this is my first time reviewing a patch
> by mail so formatting may not be very tidy.

Your comments look fine. Thanks for them!

>> diff --git a/lisp/net/trampver.el b/lisp/net/trampver.el
>> index 8d21133b3b..2351867129 100644
>> --- a/lisp/net/trampver.el
>> +++ b/lisp/net/trampver.el
>> @@ -76,6 +76,13 @@ tramp-repository-version
>>              (replace-regexp-in-string "\n" "" (emacs-version))))))
>>    (unless (string-equal "ok" x) (error "%s" x)))
>>
>> +;; `inside-emacs-functions' has been introduces with Emacs 28.1.
>                                         ^^^^^^^^^^
>                                            typo

Fixed.

>> +(when (symbolp inside-emacs-functions)
>> +  (add-hook
>> +   'inside-emacs-functions
>> +   (lambda ()
>> +     (when (file-remote-p default-directory) "tramp:2.5.0-pre"))))
>                                                 ^^^^^^^^^^^^^^^^^
>
> Should this be `tramp-version' instead?

All Tramp files in the Emacs git are synchronized from the Tramp git
repository, where I develop. In the Tramp git repo, trampver.el is a
generated file. The configure script replaces @PACKAGE_VERSION@ by the
Tramp version string, so it is dumped directly here.

>> --- a/lisp/simple.el
>> +++ b/lisp/simple.el
>> @@ -4295,6 +4295,29 @@ list-processes
>>      (tabulated-list-print))
>>    (display-buffer buffer)
>>    nil)
>> +
>> +(defvar inside-emacs-functions nil
>> +  "List of functions to compose the environment variable INSIDE_EMACS.
>> +Every package which needs to set a string in that envirenmont
>                                                      ^^^^^^^^^^^
>                                                         typo

Fixed.

> - Fede

Best regards, Michael.