bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Spencer Baugh]

1. PAGER=less emacs -Q
2. (setq comint-pager "cat")
3. (async-shell-command "echo $PAGER")
4. Observe "less" rather than "cat".

I intended async-shell-command to also be affected when I added
comint-pager; a patch to fix this will follow.


In GNU Emacs 29.2.50 (build 9, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2024-07-30 built on
 igm-qws-u22796a
Repository revision: cd9604db959c439c5695cf79f6533b5cbd340851
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.10 (Green Obsidian)

Configured using:
 'configure --with-x-toolkit=lucid --without-gpm --without-gconf
 --without-selinux --without-imagemagick --with-modules --with-gif=no
 --with-cairo --with-rsvg --without-compress-install
 --with-native-compilation=aot --with-tree-sitter
 PKG_CONFIG_PATH=/usr/local/home/garnish/libtree-sitter/0.22.6-1/lib/pkgconfig/'

Configured features:
CAIRO DBUS FREETYPE GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG
SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER X11
XDBE XIM XINPUT2 XPM LUCID ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Spencer Baugh]

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


Patch fixing this:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Respect-comint-pager-in-async-shell-command.patch --]
[-- Type: text/x-patch, Size: 2617 bytes --]

From 2af83cd922b34e1cde4ab4973e07fdb283acd26a Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Fri, 2 Aug 2024 14:39:08 -0400
Subject: [PATCH] Respect comint-pager in async-shell-command

comint-pager now also affects async-shell-command.

As a side benefit, this also allows it to be configured with
connection-local variables.

* lisp/comint.el (comint-exec-1): Remove check on comint-pager.
(comint-term-environment): Add check on comint-pager. (bug#72426)
---
 lisp/comint.el | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/lisp/comint.el b/lisp/comint.el
index c7cd22d840a..4f28ddc3165 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -893,10 +893,6 @@ comint-exec-1
 	 (nconc
           (comint-term-environment)
 	  (list (format "INSIDE_EMACS=%s,comint" emacs-version))
-          (when comint-pager
-            (if (stringp comint-pager)
-                (list (format "PAGER=%s" comint-pager))
-              (error "comint-pager should be a string: %s" comint-pager)))
 	  process-environment))
 	(default-directory
 	  (if (file-accessible-directory-p default-directory)
@@ -925,7 +921,9 @@ comint-exec-1
     proc))
 
 (defun comint-term-environment ()
-  "Return an environment variable list for terminal configuration."
+  "Return an environment variable list for terminal configuration.
+
+Includes a value for PAGER based on `comint-pager'."
   ;; If using termcap, we specify `emacs' as the terminal type
   ;; because that lets us specify a width.
   ;; If using terminfo, we default to `dumb' because that is
@@ -934,12 +932,17 @@ comint-term-environment
   ;; Some programs that use terminfo get very confused
   ;; if TERM is not a valid terminal type.
   (with-connection-local-variables
-   (if system-uses-terminfo
-       (list (format "TERM=%s" comint-terminfo-terminal)
-             "TERMCAP="
-             (format "COLUMNS=%d" (window-width)))
-     (list "TERM=emacs"
-           (format "TERMCAP=emacs:co#%d:tc=unknown:" (window-width))))))
+   (nconc
+    (when comint-pager
+      (if (stringp comint-pager)
+          (list (format "PAGER=%s" comint-pager))
+        (error "comint-pager should be a string: %s" comint-pager)))
+    (if system-uses-terminfo
+        (list (format "TERM=%s" comint-terminfo-terminal)
+              "TERMCAP="
+              (format "COLUMNS=%d" (window-width)))
+      (list "TERM=emacs"
+            (format "TERMCAP=emacs:co#%d:tc=unknown:" (window-width)))))))
 
 (defun comint-nonblank-p (str)
   "Return non-nil if STR contains non-whitespace syntax."
-- 
2.39.3

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Date: Fri, 02 Aug 2024 14:35:25 -0400
> 
> 
> 1. PAGER=less emacs -Q
> 2. (setq comint-pager "cat")
> 3. (async-shell-command "echo $PAGER")
> 4. Observe "less" rather than "cat".
> 
> I intended async-shell-command to also be affected when I added
> comint-pager; a patch to fix this will follow.

Thanks, I don't think this is right: comint stuff should not affect
lower-level primitives, it should only affect comint and its callers.

Lisp programs that use async-shell-command can arrange for
process-environment to have PAGER=SOMETHING as they see fit.

So I don't think we should install your patch.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Spencer Baugh]

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

On Sat, Aug 3, 2024, 1:48 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Spencer Baugh <sbaugh@janestreet.com>
> > Date: Fri, 02 Aug 2024 14:35:25 -0400
> >
> >
> > 1. PAGER=less emacs -Q
> > 2. (setq comint-pager "cat")
> > 3. (async-shell-command "echo $PAGER")
> > 4. Observe "less" rather than "cat".
> >
> > I intended async-shell-command to also be affected when I added
> > comint-pager; a patch to fix this will follow.
>
> Thanks, I don't think this is right: comint stuff should not affect
> lower-level primitives, it should only affect comint and its callers.
>

comint-terminfo-terminal affects async-shell-command, why not this?

If the fact that the variable is in comint is the problem, I can rename it
and move it elsewhere.

Lisp programs that use async-shell-command can arrange for
> process-environment to have PAGER=SOMETHING as they see fit.
>

My intention is primarily to affect interactive usage of
async-shell-command.

>

[-- Attachment #2: Type: text/html, Size: 1922 bytes --]

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Date: Sat, 3 Aug 2024 06:47:10 -0400
> Cc: 72426@debbugs.gnu.org
> 
> On Sat, Aug 3, 2024, 1:48 AM Eli Zaretskii <eliz@gnu.org> wrote:
> 
>  > From: Spencer Baugh <sbaugh@janestreet.com>
>  > Date: Fri, 02 Aug 2024 14:35:25 -0400
>  > 
>  > 
>  > 1. PAGER=less emacs -Q
>  > 2. (setq comint-pager "cat")
>  > 3. (async-shell-command "echo $PAGER")
>  > 4. Observe "less" rather than "cat".
>  > 
>  > I intended async-shell-command to also be affected when I added
>  > comint-pager; a patch to fix this will follow.
> 
>  Thanks, I don't think this is right: comint stuff should not affect
>  lower-level primitives, it should only affect comint and its callers.
> 
> comint-terminfo-terminal affects async-shell-command, why not this?

Ugh!  A mistake, IMNSHO.  But that ship sailed a long time ago, so we
cannot fix the mistake.  We can avoid enlarging the mistake, though.

> If the fact that the variable is in comint is the problem, I can rename it and move it elsewhere.

I don't think functions that are almost primitives should pay
attention to application-level features such as this one.

>  Lisp programs that use async-shell-command can arrange for
>  process-environment to have PAGER=SOMETHING as they see fit.
> 
> My intention is primarily to affect interactive usage of async-shell-command.

But comint-term-environment also affects "M-x compile".

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Spencer Baugh]

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

On Sat, Aug 3, 2024, 11:39 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Spencer Baugh <sbaugh@janestreet.com>
> > Date: Sat, 3 Aug 2024 06:47:10 -0400
> > Cc: 72426@debbugs.gnu.org
> >
> > On Sat, Aug 3, 2024, 1:48 AM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> >  > From: Spencer Baugh <sbaugh@janestreet.com>
> >  > Date: Fri, 02 Aug 2024 14:35:25 -0400
> >  >
> >  >
> >  > 1. PAGER=less emacs -Q
> >  > 2. (setq comint-pager "cat")
> >  > 3. (async-shell-command "echo $PAGER")
> >  > 4. Observe "less" rather than "cat".
> >  >
> >  > I intended async-shell-command to also be affected when I added
> >  > comint-pager; a patch to fix this will follow.
> >
> >  Thanks, I don't think this is right: comint stuff should not affect
> >  lower-level primitives, it should only affect comint and its callers.
> >
> > comint-terminfo-terminal affects async-shell-command, why not this?
>
> Ugh!  A mistake, IMNSHO.  But that ship sailed a long time ago, so we
> cannot fix the mistake.  We can avoid enlarging the mistake, though.
>
> > If the fact that the variable is in comint is the problem, I can rename
> it and move it elsewhere.
>
> I don't think functions that are almost primitives should pay
> attention to application-level features such as this one.
>

If we're not going to make the variable apply to all usage, I think it
should just be deleted before 30 is released.  There's no point to it in
that case, because it's still necessary to add (setenv "PAGER" "cat") to
one's configuration.

Alternatively, the default of comint-pager should be set to "cat".

But I see no value in a variable like this if it neither applies to all (or
at least most) users, nor provides better default behavior.


> >  Lisp programs that use async-shell-command can arrange for
> >  process-environment to have PAGER=SOMETHING as they see fit.
> >
> > My intention is primarily to affect interactive usage of
> async-shell-command.
>
> But comint-term-environment also affects "M-x compile".
>

Yes, I would like comint-pager to also affect that.

>

[-- Attachment #2: Type: text/html, Size: 3530 bytes --]

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Date: Sat, 3 Aug 2024 12:42:00 -0400
> Cc: 72426@debbugs.gnu.org
> 
>  > comint-terminfo-terminal affects async-shell-command, why not this?
> 
>  Ugh!  A mistake, IMNSHO.  But that ship sailed a long time ago, so we
>  cannot fix the mistake.  We can avoid enlarging the mistake, though.
> 
>  > If the fact that the variable is in comint is the problem, I can rename it and move it elsewhere.
> 
>  I don't think functions that are almost primitives should pay
>  attention to application-level features such as this one.
> 
> If we're not going to make the variable apply to all usage, I think it should just be deleted before 30 is
> released.  There's no point to it in that case, because it's still necessary to add (setenv "PAGER" "cat") to
> one's configuration.
> 
> Alternatively, the default of comint-pager should be set to "cat".
> 
> But I see no value in a variable like this if it neither applies to all (or at least most) users, nor provides better
> default behavior.
> 
>  >  Lisp programs that use async-shell-command can arrange for
>  >  process-environment to have PAGER=SOMETHING as they see fit.
>  > 
>  > My intention is primarily to affect interactive usage of async-shell-command.
> 
>  But comint-term-environment also affects "M-x compile".
> 
> Yes, I would like comint-pager to also affect that.

That makes very little sense to me, as "M-x compile" is used for
non-interactive programs.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Jim Porter]

On 8/3/2024 8:38 AM, Eli Zaretskii wrote:
>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Date: Sat, 3 Aug 2024 06:47:10 -0400
>> Cc: 72426@debbugs.gnu.org
>>
>> comint-terminfo-terminal affects async-shell-command, why not this?
> 
> Ugh!  A mistake, IMNSHO.  But that ship sailed a long time ago, so we
> cannot fix the mistake.  We can avoid enlarging the mistake, though.
> 
>> If the fact that the variable is in comint is the problem, I can rename it and move it elsewhere.
> 
> I don't think functions that are almost primitives should pay
> attention to application-level features such as this one.

Perhaps we should be setting the pager in a similar way to how TERM is 
set in startup.el?

     ;; Subprocesses of Emacs do not have direct access to the terminal, so
     ;; unless told otherwise they should only assume a dumb terminal.
     ;; We are careful to do it late (after term-setup-hook), although the
     ;; new multi-tty code does not use $TERM any more there anyway.
     (setenv "TERM" "dumb")

I think the reasoning in that comment applies to PAGER as well: unless 
told otherwise, subprocesses probably shouldn't use pager like "less"; 
it's very unlikely to work correctly.

In that case, would it make sense to add something along these lines to 
startup.el?

     (when (executable-find "cat")
       (setenv "PAGER" "cat"))

The 'comint-pager' variable could still be useful though, since you can 
set it to a pager that's fancier than "cat" but that actually works in 
Comint (unlike "less"). For example, you could set a pager that does 
automatic syntax highlighting with ANSI escapes; Comint would be able to 
handle that.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Spencer Baugh]

Jim Porter <jporterbugs@gmail.com> writes:

> On 8/3/2024 8:38 AM, Eli Zaretskii wrote:
>>> From: Spencer Baugh <sbaugh@janestreet.com>
>>> Date: Sat, 3 Aug 2024 06:47:10 -0400
>>> Cc: 72426@debbugs.gnu.org
>>>
>>> comint-terminfo-terminal affects async-shell-command, why not this?
>> Ugh!  A mistake, IMNSHO.  But that ship sailed a long time ago, so
>> we
>> cannot fix the mistake.  We can avoid enlarging the mistake, though.
>> 
>>> If the fact that the variable is in comint is the problem, I can rename it and move it elsewhere.
>> I don't think functions that are almost primitives should pay
>> attention to application-level features such as this one.
>
> Perhaps we should be setting the pager in a similar way to how TERM is
> set in startup.el?
>
>     ;; Subprocesses of Emacs do not have direct access to the terminal, so
>     ;; unless told otherwise they should only assume a dumb terminal.
>     ;; We are careful to do it late (after term-setup-hook), although the
>     ;; new multi-tty code does not use $TERM any more there anyway.
>     (setenv "TERM" "dumb")
>
> I think the reasoning in that comment applies to PAGER as well: unless
> told otherwise, subprocesses probably shouldn't use pager like "less";
> it's very unlikely to work correctly.
>
> In that case, would it make sense to add something along these lines
> to startup.el?
>
>     (when (executable-find "cat")
>       (setenv "PAGER" "cat"))

Yes, I'd be very in favor of that.  Fixing this is exactly my
motivation.

> The 'comint-pager' variable could still be useful though, since you
> can set it to a pager that's fancier than "cat" but that actually
> works in Comint (unlike "less"). For example, you could set a pager
> that does automatic syntax highlighting with ANSI escapes; Comint
> would be able to handle that.

Maybe, but I'd rather delete it in the meantime, since it doesn't serve
its main purpose.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Date: Sat, 3 Aug 2024 12:42:00 -0400
>> Cc: 72426@debbugs.gnu.org
>> 
>>  > comint-terminfo-terminal affects async-shell-command, why not this?
>> 
>>  Ugh!  A mistake, IMNSHO.  But that ship sailed a long time ago, so we
>>  cannot fix the mistake.  We can avoid enlarging the mistake, though.
>> 
>>  > If the fact that the variable is in comint is the problem, I can rename it and move it elsewhere.
>> 
>>  I don't think functions that are almost primitives should pay
>>  attention to application-level features such as this one.
>> 
>> If we're not going to make the variable apply to all usage, I think it should just be deleted before 30 is
>> released.  There's no point to it in that case, because it's still necessary to add (setenv "PAGER" "cat") to
>> one's configuration.
>> 
>> Alternatively, the default of comint-pager should be set to "cat".
>> 
>> But I see no value in a variable like this if it neither applies to all (or at least most) users, nor provides better
>> default behavior.
>> 
>>  >  Lisp programs that use async-shell-command can arrange for
>>  >  process-environment to have PAGER=SOMETHING as they see fit.
>>  > 
>>  > My intention is primarily to affect interactive usage of async-shell-command.
>> 
>>  But comint-term-environment also affects "M-x compile".
>> 
>> Yes, I would like comint-pager to also affect that.
>
> That makes very little sense to me, as "M-x compile" is used for
> non-interactive programs.

Yes.  If you run a program which is supposed to be non-interactive, and
it tries to run PAGER (as some do, e.g. "hg log"), then it will break.
So setting PAGER would be helpful for M-x compile.

Anyway, can we just remove comint-pager for now, to avoid adding
something broken that has to be maintained?  I can try it again for
Emacs 31.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: 72426@debbugs.gnu.org
> Date: Tue, 06 Aug 2024 11:33:47 -0400
> 
> Anyway, can we just remove comint-pager for now, to avoid adding
> something broken that has to be maintained?  I can try it again for
> Emacs 31.

I think it's too late for that, sorry.  It is already supported in
Eshell, and Emacs 30 is frozen for such changes anyway.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  72426@debbugs.gnu.org
> Date: Tue, 06 Aug 2024 11:31:06 -0400
> 
> Jim Porter <jporterbugs@gmail.com> writes:
> 
> > Perhaps we should be setting the pager in a similar way to how TERM is
> > set in startup.el?
> >
> >     ;; Subprocesses of Emacs do not have direct access to the terminal, so
> >     ;; unless told otherwise they should only assume a dumb terminal.
> >     ;; We are careful to do it late (after term-setup-hook), although the
> >     ;; new multi-tty code does not use $TERM any more there anyway.
> >     (setenv "TERM" "dumb")
> >
> > I think the reasoning in that comment applies to PAGER as well: unless
> > told otherwise, subprocesses probably shouldn't use pager like "less";
> > it's very unlikely to work correctly.
> >
> > In that case, would it make sense to add something along these lines
> > to startup.el?
> >
> >     (when (executable-find "cat")
> >       (setenv "PAGER" "cat"))
> 
> Yes, I'd be very in favor of that.  Fixing this is exactly my
> motivation.

We could perhaps try this on master.  If it doesn't cause trouble,
maybe this is the right way, indeed.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Jim Porter]

On 8/6/2024 8:46 AM, Eli Zaretskii wrote:
>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: 72426@debbugs.gnu.org
>> Date: Tue, 06 Aug 2024 11:33:47 -0400
>>
>> Anyway, can we just remove comint-pager for now, to avoid adding
>> something broken that has to be maintained?  I can try it again for
>> Emacs 31.
> 
> I think it's too late for that, sorry.  It is already supported in
> Eshell, and Emacs 30 is frozen for such changes anyway.

For what it's worth, from the perspective of Eshell, I see the current 
implementation as a half-measure that makes things better, but may be 
obviated by a better implementation (e.g. setting PAGER in startup.el). 
While it's disappointing to have a not-quite-right solution make it to 
30.1, I don't think that solution paints us into a corner either: it 
doesn't make it any harder for us to make the startup.el change for 31.1.

On the plus(?) side, it looks like the only place 'comint-pager' is 
mentioned is in the Eshell manual (and the docstrings, of course). So 
since we're not really "advertising" this option, and since I think it 
would still be useful even with the startup.el change, I don't think we 
have to worry too much. We can just consider the underlying issue to be 
something we'll (try to) fix in 31.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Spencer Baugh]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  72426@debbugs.gnu.org
>> Date: Tue, 06 Aug 2024 11:31:06 -0400
>> 
>> Jim Porter <jporterbugs@gmail.com> writes:
>> 
>> > Perhaps we should be setting the pager in a similar way to how TERM is
>> > set in startup.el?
>> >
>> >     ;; Subprocesses of Emacs do not have direct access to the terminal, so
>> >     ;; unless told otherwise they should only assume a dumb terminal.
>> >     ;; We are careful to do it late (after term-setup-hook), although the
>> >     ;; new multi-tty code does not use $TERM any more there anyway.
>> >     (setenv "TERM" "dumb")
>> >
>> > I think the reasoning in that comment applies to PAGER as well: unless
>> > told otherwise, subprocesses probably shouldn't use pager like "less";
>> > it's very unlikely to work correctly.
>> >
>> > In that case, would it make sense to add something along these lines
>> > to startup.el?
>> >
>> >     (when (executable-find "cat")
>> >       (setenv "PAGER" "cat"))
>> 
>> Yes, I'd be very in favor of that.  Fixing this is exactly my
>> motivation.
>
> We could perhaps try this on master.  If it doesn't cause trouble,
> maybe this is the right way, indeed.

Here's a patch which does this, for master.

(Everyone please feel free to CC me on any bugs or emacs-devel
discussions about this; I will try hard to fix any issues that crop up.)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Stop-subprocesses-from-using-inherited-or-default-PA.patch --]
[-- Type: text/x-patch, Size: 1425 bytes --]

From 14db307fd53a6c5b13a09ef3e023b0ba299d61a8 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Tue, 6 Aug 2024 12:39:37 -0400
Subject: [PATCH] Stop subprocesses from using inherited or default PAGER

At startup, set PAGER to cat so that any inherited or default
value of PAGER does not affect subprocesses of Emacs.  Pagers
generally won't work when a subprocess runs under Emacs.

A user can use comint-pager (or other customizations) to tell
subprocesses to use a different specific pager.

* lisp/startup.el (normal-top-level): Set PAGER to cat, if cat
is available. (bug#72426)
---
 lisp/startup.el | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lisp/startup.el b/lisp/startup.el
index f18795ae6ac..324f3aeee60 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -854,6 +854,10 @@ normal-top-level
     ;; We are careful to do it late (after term-setup-hook), although the
     ;; new multi-tty code does not use $TERM any more there anyway.
     (setenv "TERM" "dumb")
+    ;; Likewise, subprocesses should not use a pager unless told
+    ;; otherwise, since it generally won't work.
+    (when (executable-find "cat")
+      (setenv "PAGER" "cat"))
     ;; Remove DISPLAY from the process-environment as well.  This allows
     ;; `callproc.c' to give it a useful adaptive default which is either
     ;; the value of the `display' frame-parameter or the DISPLAY value
-- 
2.39.3

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Eli Zaretskii]

> Date: Tue, 6 Aug 2024 09:29:34 -0700
> Cc: 72426@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> On 8/6/2024 8:46 AM, Eli Zaretskii wrote:
> >> From: Spencer Baugh <sbaugh@janestreet.com>
> >> Cc: 72426@debbugs.gnu.org
> >> Date: Tue, 06 Aug 2024 11:33:47 -0400
> >>
> >> Anyway, can we just remove comint-pager for now, to avoid adding
> >> something broken that has to be maintained?  I can try it again for
> >> Emacs 31.
> > 
> > I think it's too late for that, sorry.  It is already supported in
> > Eshell, and Emacs 30 is frozen for such changes anyway.
> 
> For what it's worth, from the perspective of Eshell, I see the current 
> implementation as a half-measure that makes things better, but may be 
> obviated by a better implementation (e.g. setting PAGER in startup.el). 
> While it's disappointing to have a not-quite-right solution make it to 
> 30.1, I don't think that solution paints us into a corner either: it 
> doesn't make it any harder for us to make the startup.el change for 31.1.
> 
> On the plus(?) side, it looks like the only place 'comint-pager' is 
> mentioned is in the Eshell manual (and the docstrings, of course). So 
> since we're not really "advertising" this option, and since I think it 
> would still be useful even with the startup.el change, I don't think we 
> have to worry too much. We can just consider the underlying issue to be 
> something we'll (try to) fix in 31.

Yes, I think the fix will have to be postponed to Emacs 31, and
meanwhile we need to test the fix well enough to be sure it (a) fixes
the problem and (b) doesn't cause new ones.

There's a lesson to be learned here, but I'll leave to each one of use
to spell it out to him/herself.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: jporterbugs@gmail.com,  72426@debbugs.gnu.org
> Date: Tue, 06 Aug 2024 12:42:09 -0400
> 
> --- a/lisp/startup.el
> +++ b/lisp/startup.el
> @@ -854,6 +854,10 @@ normal-top-level
>      ;; We are careful to do it late (after term-setup-hook), although the
>      ;; new multi-tty code does not use $TERM any more there anyway.
>      (setenv "TERM" "dumb")
> +    ;; Likewise, subprocesses should not use a pager unless told
> +    ;; otherwise, since it generally won't work.
> +    (when (executable-find "cat")
> +      (setenv "PAGER" "cat"))

We need to work on the comment, because it doesn't explain any of what
it needs to.  Worse, it says something very confusing and at least
inaccurate.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: jporterbugs@gmail.com,  72426@debbugs.gnu.org
>> Date: Tue, 06 Aug 2024 12:42:09 -0400
>> 
>> --- a/lisp/startup.el
>> +++ b/lisp/startup.el
>> @@ -854,6 +854,10 @@ normal-top-level
>>      ;; We are careful to do it late (after term-setup-hook), although the
>>      ;; new multi-tty code does not use $TERM any more there anyway.
>>      (setenv "TERM" "dumb")
>> +    ;; Likewise, subprocesses should not use a pager unless told
>> +    ;; otherwise, since it generally won't work.
>> +    (when (executable-find "cat")
>> +      (setenv "PAGER" "cat"))
>
> We need to work on the comment, because it doesn't explain any of what
> it needs to.  Worse, it says something very confusing and at least
> inaccurate.

Could you just say what you'd prefer the comment to be?  I'm not sure
what exactly you'd like it to explain.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: jporterbugs@gmail.com,  72426@debbugs.gnu.org
> Date: Tue, 06 Aug 2024 14:49:45 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Spencer Baugh <sbaugh@janestreet.com>
> >> Cc: jporterbugs@gmail.com,  72426@debbugs.gnu.org
> >> Date: Tue, 06 Aug 2024 12:42:09 -0400
> >> 
> >> --- a/lisp/startup.el
> >> +++ b/lisp/startup.el
> >> @@ -854,6 +854,10 @@ normal-top-level
> >>      ;; We are careful to do it late (after term-setup-hook), although the
> >>      ;; new multi-tty code does not use $TERM any more there anyway.
> >>      (setenv "TERM" "dumb")
> >> +    ;; Likewise, subprocesses should not use a pager unless told
> >> +    ;; otherwise, since it generally won't work.
> >> +    (when (executable-find "cat")
> >> +      (setenv "PAGER" "cat"))
> >
> > We need to work on the comment, because it doesn't explain any of what
> > it needs to.  Worse, it says something very confusing and at least
> > inaccurate.
> 
> Could you just say what you'd prefer the comment to be?  I'm not sure
> what exactly you'd like it to explain.

I don't have a clear enough idea; I hoped you did, since you initiated
this change to begin with.  I just know that the text you wrote cannot
be it, because it didn't explain to me anything about the reasons we
should be doing this.  The comment should explain why PAGER=cat is a
good idea to go with TERM=dumb, for example.  Maybe begin by saying
why Emacs needs to set the variable at all, why not leave it unset?

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Spencer Baugh]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: jporterbugs@gmail.com,  72426@debbugs.gnu.org
>> Date: Tue, 06 Aug 2024 14:49:45 -0400
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Spencer Baugh <sbaugh@janestreet.com>
>> >> Cc: jporterbugs@gmail.com,  72426@debbugs.gnu.org
>> >> Date: Tue, 06 Aug 2024 12:42:09 -0400
>> >> 
>> >> --- a/lisp/startup.el
>> >> +++ b/lisp/startup.el
>> >> @@ -854,6 +854,10 @@ normal-top-level
>> >>      ;; We are careful to do it late (after term-setup-hook), although the
>> >>      ;; new multi-tty code does not use $TERM any more there anyway.
>> >>      (setenv "TERM" "dumb")
>> >> +    ;; Likewise, subprocesses should not use a pager unless told
>> >> +    ;; otherwise, since it generally won't work.
>> >> +    (when (executable-find "cat")
>> >> +      (setenv "PAGER" "cat"))
>> >
>> > We need to work on the comment, because it doesn't explain any of what
>> > it needs to.  Worse, it says something very confusing and at least
>> > inaccurate.
>> 
>> Could you just say what you'd prefer the comment to be?  I'm not sure
>> what exactly you'd like it to explain.
>
> I don't have a clear enough idea; I hoped you did, since you initiated
> this change to begin with.  I just know that the text you wrote cannot
> be it, because it didn't explain to me anything about the reasons we
> should be doing this.  The comment should explain why PAGER=cat is a
> good idea to go with TERM=dumb, for example.  Maybe begin by saying
> why Emacs needs to set the variable at all, why not leave it unset?

OK, how about this?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Stop-subprocesses-from-using-inherited-or-default-PA.patch --]
[-- Type: text/x-patch, Size: 1593 bytes --]

From b50a5ef015280a585330d4fcfc1265d65ce1dd88 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Tue, 6 Aug 2024 12:39:37 -0400
Subject: [PATCH] Stop subprocesses from using inherited or default PAGER

At startup, set PAGER to cat so that any inherited or default
value of PAGER does not affect subprocesses of Emacs.  Pagers
generally won't work when a subprocess runs under Emacs.

A user can use comint-pager (or other customizations) to tell
subprocesses to use a different specific pager.

* lisp/startup.el (normal-top-level): Set PAGER to cat, if cat
is available. (bug#72426)
---
 lisp/startup.el | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lisp/startup.el b/lisp/startup.el
index f18795ae6ac..738eec772ec 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -854,6 +854,12 @@ normal-top-level
     ;; We are careful to do it late (after term-setup-hook), although the
     ;; new multi-tty code does not use $TERM any more there anyway.
     (setenv "TERM" "dumb")
+    ;; Similarly, a subprocess should not try to invoke a pager, as most
+    ;; pagers will fail in a dumb terminal.  Many programs default to
+    ;; using "less" when PAGER is unset, so set PAGER to "cat"; using cat
+    ;; as a pager is equivalent to not using a pager at all.
+    (when (executable-find "cat")
+      (setenv "PAGER" "cat"))
     ;; Remove DISPLAY from the process-environment as well.  This allows
     ;; `callproc.c' to give it a useful adaptive default which is either
     ;; the value of the `display' frame-parameter or the DISPLAY value
-- 
2.39.3

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Jim Porter]

On 8/6/2024 12:23 PM, Spencer Baugh wrote:
> OK, how about this?
[snip]
> +    ;; Similarly, a subprocess should not try to invoke a pager, as most
> +    ;; pagers will fail in a dumb terminal.  Many programs default to
> +    ;; using "less" when PAGER is unset, so set PAGER to "cat"; using cat
> +    ;; as a pager is equivalent to not using a pager at all.
> +    (when (executable-find "cat")
> +      (setenv "PAGER" "cat"))

This makes sense to me.

Just to be extra-sure, I tried using 'async-shell-command' to run "git 
log" with PAGER unset, and sure enough it tried to use "less" for 
paging, which didn't go very well. "PAGER=cat" was much better.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: jporterbugs@gmail.com,  72426@debbugs.gnu.org
> Date: Tue, 06 Aug 2024 15:23:25 -0400
> 
> > I don't have a clear enough idea; I hoped you did, since you initiated
> > this change to begin with.  I just know that the text you wrote cannot
> > be it, because it didn't explain to me anything about the reasons we
> > should be doing this.  The comment should explain why PAGER=cat is a
> > good idea to go with TERM=dumb, for example.  Maybe begin by saying
> > why Emacs needs to set the variable at all, why not leave it unset?
> 
> OK, how about this?

Much better, thanks.

Do we care about platforms which don't have 'cat'?

Also, is it true that this issue is only relevant to sub-processes
that communicate via PTYs?

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Eli Zaretskii]

> Date: Tue, 6 Aug 2024 19:36:15 -0700
> Cc: 72426@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> Just to be extra-sure, I tried using 'async-shell-command' to run "git 
> log" with PAGER unset, and sure enough it tried to use "less" for 
> paging, which didn't go very well. "PAGER=cat" was much better.

Did you try with process-connection-type nil?

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> Date: Tue, 6 Aug 2024 19:36:15 -0700
>> Cc: 72426@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>> 
>> Just to be extra-sure, I tried using 'async-shell-command' to run "git 
>> log" with PAGER unset, and sure enough it tried to use "less" for 
>> paging, which didn't go very well. "PAGER=cat" was much better.
>
> Did you try with process-connection-type nil?

With process-connection-type nil, git will never run a pager (since like
many programs it checks whether stdout is a terminal before doing so).

So both:

(let ((process-connection-type nil)
      (process-environment (cons '("PAGER" "less") process-environment)))
  (async-shell-command "git log"))

and

(let ((process-connection-type nil)
      (process-environment (cons "PAGER" process-environment)))
  (async-shell-command "git log"))

behave identically.

Setting PAGER=cat is only necessary for process-connection-type=t.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: jporterbugs@gmail.com,  72426@debbugs.gnu.org
>> Date: Tue, 06 Aug 2024 15:23:25 -0400
>> 
>> > I don't have a clear enough idea; I hoped you did, since you initiated
>> > this change to begin with.  I just know that the text you wrote cannot
>> > be it, because it didn't explain to me anything about the reasons we
>> > should be doing this.  The comment should explain why PAGER=cat is a
>> > good idea to go with TERM=dumb, for example.  Maybe begin by saying
>> > why Emacs needs to set the variable at all, why not leave it unset?
>> 
>> OK, how about this?
>
> Much better, thanks.
>
> Do we care about platforms which don't have 'cat'?

No - those platforms don't have less either, and don't respect PAGER.

> Also, is it true that this issue is only relevant to sub-processes
> that communicate via PTYs?

Yes - that's also the case for the TERM environment variable; when
communicating via a pipe, programs don't check TERM anyway.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: Jim Porter <jporterbugs@gmail.com>,  72426@debbugs.gnu.org
> Date: Wed, 07 Aug 2024 11:05:47 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> >> Date: Tue, 6 Aug 2024 19:36:15 -0700
> >> Cc: 72426@debbugs.gnu.org
> >> From: Jim Porter <jporterbugs@gmail.com>
> >> 
> >> Just to be extra-sure, I tried using 'async-shell-command' to run "git 
> >> log" with PAGER unset, and sure enough it tried to use "less" for 
> >> paging, which didn't go very well. "PAGER=cat" was much better.
> >
> > Did you try with process-connection-type nil?
> 
> With process-connection-type nil, git will never run a pager (since like
> many programs it checks whether stdout is a terminal before doing so).
> 
> So both:
> 
> (let ((process-connection-type nil)
>       (process-environment (cons '("PAGER" "less") process-environment)))
>   (async-shell-command "git log"))
> 
> and
> 
> (let ((process-connection-type nil)
>       (process-environment (cons "PAGER" process-environment)))
>   (async-shell-command "git log"))
> 
> behave identically.
> 
> Setting PAGER=cat is only necessary for process-connection-type=t.

As expected.  The problem is that a Lisp program could let-bind this
variable around a call to async-shell-command (or some other similar
API), in which case a setting in startup.el will not catch that.  But
maybe we don't care, since a program whose stdout is not a console
device will ignore PAGER anyway.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: Jim Porter <jporterbugs@gmail.com>,  72426@debbugs.gnu.org
>> Date: Wed, 07 Aug 2024 11:05:47 -0400
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> >> Date: Tue, 6 Aug 2024 19:36:15 -0700
>> >> Cc: 72426@debbugs.gnu.org
>> >> From: Jim Porter <jporterbugs@gmail.com>
>> >> 
>> >> Just to be extra-sure, I tried using 'async-shell-command' to run "git 
>> >> log" with PAGER unset, and sure enough it tried to use "less" for 
>> >> paging, which didn't go very well. "PAGER=cat" was much better.
>> >
>> > Did you try with process-connection-type nil?
>> 
>> With process-connection-type nil, git will never run a pager (since like
>> many programs it checks whether stdout is a terminal before doing so).
>> 
>> So both:
>> 
>> (let ((process-connection-type nil)
>>       (process-environment (cons '("PAGER" "less") process-environment)))
>>   (async-shell-command "git log"))
>> 
>> and
>> 
>> (let ((process-connection-type nil)
>>       (process-environment (cons "PAGER" process-environment)))
>>   (async-shell-command "git log"))
>> 
>> behave identically.
>> 
>> Setting PAGER=cat is only necessary for process-connection-type=t.
>
> As expected.  The problem is that a Lisp program could let-bind this
> variable around a call to async-shell-command (or some other similar
> API), in which case a setting in startup.el will not catch that.  But
> maybe we don't care, since a program whose stdout is not a console
> device will ignore PAGER anyway.

Yep - I think it's fine for the same reason it's fine to have TERM set
by default, even though it's ignored by programs whose stdout is a pipe.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Jim Porter]

On 8/7/2024 8:31 AM, Spencer Baugh wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>> As expected.  The problem is that a Lisp program could let-bind this
>> variable around a call to async-shell-command (or some other similar
>> API), in which case a setting in startup.el will not catch that.  But
>> maybe we don't care, since a program whose stdout is not a console
>> device will ignore PAGER anyway.
> 
> Yep - I think it's fine for the same reason it's fine to have TERM set
> by default, even though it's ignored by programs whose stdout is a pipe.

Git, at least, ignores PAGER entirely when stdout isn't a PTY. (I tested 
by setting PAGER to "rev" and trying with and without a PTY. I get 
reversed lines with a PTY, but everything is non-reversed without.)

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: jporterbugs@gmail.com,  72426@debbugs.gnu.org
> Date: Tue, 06 Aug 2024 15:23:25 -0400
> 
> >> Could you just say what you'd prefer the comment to be?  I'm not sure
> >> what exactly you'd like it to explain.
> >
> > I don't have a clear enough idea; I hoped you did, since you initiated
> > this change to begin with.  I just know that the text you wrote cannot
> > be it, because it didn't explain to me anything about the reasons we
> > should be doing this.  The comment should explain why PAGER=cat is a
> > good idea to go with TERM=dumb, for example.  Maybe begin by saying
> > why Emacs needs to set the variable at all, why not leave it unset?
> 
> OK, how about this?

Thanks, installed on master, and closing the bug.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Dmitry Gutov]

On 03/08/2024 18:38, Eli Zaretskii wrote:
>> comint-terminfo-terminal affects async-shell-command, why not this?
> Ugh!  A mistake, IMNSHO.  But that ship sailed a long time ago, so we
> cannot fix the mistake.  We can avoid enlarging the mistake, though.
> 
>> If the fact that the variable is in comint is the problem, I can rename it and move it elsewhere.
> I don't think functions that are almost primitives should pay
> attention to application-level features such as this one.

FWIW async-shell-command's behavior is affected by a bunch of other 
comint-* variables as well because it uses a major mode which inherits 
from comint-mode (previously it was shell-mode, now shell-command-mode, 
but that aspect didn't change).

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Eli Zaretskii]

> Date: Fri, 13 Sep 2024 04:17:11 +0300
> Cc: 72426@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 03/08/2024 18:38, Eli Zaretskii wrote:
> >> comint-terminfo-terminal affects async-shell-command, why not this?
> > Ugh!  A mistake, IMNSHO.  But that ship sailed a long time ago, so we
> > cannot fix the mistake.  We can avoid enlarging the mistake, though.
> > 
> >> If the fact that the variable is in comint is the problem, I can rename it and move it elsewhere.
> > I don't think functions that are almost primitives should pay
> > attention to application-level features such as this one.
> 
> FWIW async-shell-command's behavior is affected by a bunch of other 
> comint-* variables as well because it uses a major mode which inherits 
> from comint-mode (previously it was shell-mode, now shell-command-mode, 
> but that aspect didn't change).

Yes, and that is baaad!

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Dmitry Gutov]

On 13/09/2024 09:08, Eli Zaretskii wrote:
>> FWIW async-shell-command's behavior is affected by a bunch of other
>> comint-* variables as well because it uses a major mode which inherits
>> from comint-mode (previously it was shell-mode, now shell-command-mode,
>> but that aspect didn't change).
> Yes, and that is baaad!

It might be non-obvious, but I don't see how else it would work: comint 
is the only built-in subsystem we have that implement a general 
read-evel-print-loop. So we have 'M-x shell' and 'M-x ielm', for 
example, both using it as well (not to mention inf-python, inf-ruby, etc).

And since we wanted, apparently, to have async-shell-command support 
user input (the command can be 'sh', for example), it uses parts of 
comint for its work too (comint-output-filter and the major mode with 
all its local bindings -- not sure if something else too, but this is 
already a lot).

An alternative would be to have separate user options for each of the 
above features which would bind comint's options under the cover. But 
that's not very economical.

bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command

[Eli Zaretskii]

> Date: Sat, 14 Sep 2024 02:45:00 +0300
> Cc: sbaugh@janestreet.com, 72426@debbugs.gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 13/09/2024 09:08, Eli Zaretskii wrote:
> >> FWIW async-shell-command's behavior is affected by a bunch of other
> >> comint-* variables as well because it uses a major mode which inherits
> >> from comint-mode (previously it was shell-mode, now shell-command-mode,
> >> but that aspect didn't change).
> > Yes, and that is baaad!
> 
> It might be non-obvious, but I don't see how else it would work: comint 
> is the only built-in subsystem we have that implement a general 
> read-evel-print-loop. So we have 'M-x shell' and 'M-x ielm', for 
> example, both using it as well (not to mention inf-python, inf-ruby, etc).

Very simply: such options should be specific to applications
(shell-command, inf-python, etc.), not to comint.  Comint should have
_variables_ that applications could bind depending on the
application-level user options.

> An alternative would be to have separate user options for each of the 
> above features which would bind comint's options under the cover.

Exactly!

> But that's not very economical.

I don't see why.  It would certainly be much cleaner and more
flexible: users could have different behaviors in different callers of
comint.

But all of this is academical at this point, so why are we still
arguing?