GNU bug report logs - #72426
29.2.50; comint-pager doesn't affect async-shell-command

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 72426 in the body.
You can then email your comments to 72426 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.2.50; comint-pager doesn't affect async-shell-command
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.


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

Message #8 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Fri, 02 Aug 2024 14:39:41 -0400

[Message part 1 (text/plain, inline)]

Patch fixing this:

[0001-Respect-comint-pager-in-async-shell-command.patch (text/x-patch, inline)]

From 2af83cd922b34e1cde4ab4973e07fdb283acd26a Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> 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

Message #11 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50;
 comint-pager doesn't affect async-shell-command
Date: Sat, 03 Aug 2024 08:48:07 +0300

> From: Spencer Baugh <sbaugh <at> 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.

Message #14 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50;
 comint-pager doesn't affect async-shell-command
Date: Sat, 3 Aug 2024 06:47:10 -0400

[Message part 1 (text/plain, inline)]

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

> > From: Spencer Baugh <sbaugh <at> 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.

>

[Message part 2 (text/html, inline)]

Message #17 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50;
 comint-pager doesn't affect async-shell-command
Date: Sat, 03 Aug 2024 18:38:56 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Date: Sat, 3 Aug 2024 06:47:10 -0400
> Cc: 72426 <at> debbugs.gnu.org
> 
> On Sat, Aug 3, 2024, 1:48 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
> 
>  > From: Spencer Baugh <sbaugh <at> 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".

Message #20 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50;
 comint-pager doesn't affect async-shell-command
Date: Sat, 3 Aug 2024 12:42:00 -0400

[Message part 1 (text/plain, inline)]

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

> > From: Spencer Baugh <sbaugh <at> janestreet.com>
> > Date: Sat, 3 Aug 2024 06:47:10 -0400
> > Cc: 72426 <at> debbugs.gnu.org
> >
> > On Sat, Aug 3, 2024, 1:48 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
> >
> >  > From: Spencer Baugh <sbaugh <at> 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.

>

[Message part 2 (text/html, inline)]

Message #23 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50;
 comint-pager doesn't affect async-shell-command
Date: Sat, 03 Aug 2024 20:18:35 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Date: Sat, 3 Aug 2024 12:42:00 -0400
> Cc: 72426 <at> 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.

Message #26 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Sat, 3 Aug 2024 11:38:35 -0700

On 8/3/2024 8:38 AM, Eli Zaretskii wrote:
>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Date: Sat, 3 Aug 2024 06:47:10 -0400
>> Cc: 72426 <at> 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.

Message #29 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Tue, 06 Aug 2024 11:31:06 -0400

Jim Porter <jporterbugs <at> gmail.com> writes:

> On 8/3/2024 8:38 AM, Eli Zaretskii wrote:
>>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>>> Date: Sat, 3 Aug 2024 06:47:10 -0400
>>> Cc: 72426 <at> 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.

Message #32 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Tue, 06 Aug 2024 11:33:47 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Date: Sat, 3 Aug 2024 12:42:00 -0400
>> Cc: 72426 <at> 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.

Message #35 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Tue, 06 Aug 2024 18:46:14 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: 72426 <at> 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.

Message #38 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: jporterbugs <at> gmail.com, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Tue, 06 Aug 2024 18:50:42 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  72426 <at> debbugs.gnu.org
> Date: Tue, 06 Aug 2024 11:31:06 -0400
> 
> Jim Porter <jporterbugs <at> 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.

Message #41 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>, Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Tue, 6 Aug 2024 09:29:34 -0700

On 8/6/2024 8:46 AM, Eli Zaretskii wrote:
>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Cc: 72426 <at> 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.

Message #44 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: jporterbugs <at> gmail.com, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Tue, 06 Aug 2024 12:42:09 -0400

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Cc: Eli Zaretskii <eliz <at> gnu.org>,  72426 <at> debbugs.gnu.org
>> Date: Tue, 06 Aug 2024 11:31:06 -0400
>> 
>> Jim Porter <jporterbugs <at> 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.)

[0001-Stop-subprocesses-from-using-inherited-or-default-PA.patch (text/x-patch, inline)]

From 14db307fd53a6c5b13a09ef3e023b0ba299d61a8 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> 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

Message #47 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: sbaugh <at> janestreet.com, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Tue, 06 Aug 2024 21:02:00 +0300

> Date: Tue, 6 Aug 2024 09:29:34 -0700
> Cc: 72426 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> On 8/6/2024 8:46 AM, Eli Zaretskii wrote:
> >> From: Spencer Baugh <sbaugh <at> janestreet.com>
> >> Cc: 72426 <at> 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.

Message #50 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: jporterbugs <at> gmail.com, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Tue, 06 Aug 2024 21:07:02 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: jporterbugs <at> gmail.com,  72426 <at> 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.

Message #53 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: jporterbugs <at> gmail.com, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Tue, 06 Aug 2024 14:49:45 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Cc: jporterbugs <at> gmail.com,  72426 <at> 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.

Message #56 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: jporterbugs <at> gmail.com, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Tue, 06 Aug 2024 22:07:27 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: jporterbugs <at> gmail.com,  72426 <at> debbugs.gnu.org
> Date: Tue, 06 Aug 2024 14:49:45 -0400
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> From: Spencer Baugh <sbaugh <at> janestreet.com>
> >> Cc: jporterbugs <at> gmail.com,  72426 <at> 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?

Message #59 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: jporterbugs <at> gmail.com, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Tue, 06 Aug 2024 15:23:25 -0400

[Message part 1 (text/plain, inline)]

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Cc: jporterbugs <at> gmail.com,  72426 <at> debbugs.gnu.org
>> Date: Tue, 06 Aug 2024 14:49:45 -0400
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> 
>> >> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> >> Cc: jporterbugs <at> gmail.com,  72426 <at> 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?

[0001-Stop-subprocesses-from-using-inherited-or-default-PA.patch (text/x-patch, inline)]

From b50a5ef015280a585330d4fcfc1265d65ce1dd88 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> 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

Message #62 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Spencer Baugh <sbaugh <at> janestreet.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Tue, 6 Aug 2024 19:36:15 -0700

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.

Message #65 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: jporterbugs <at> gmail.com, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Wed, 07 Aug 2024 14:18:38 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: jporterbugs <at> gmail.com,  72426 <at> 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?

Message #68 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: sbaugh <at> janestreet.com, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Wed, 07 Aug 2024 14:51:24 +0300

> Date: Tue, 6 Aug 2024 19:36:15 -0700
> Cc: 72426 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> 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?

Message #71 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Jim Porter <jporterbugs <at> gmail.com>, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Wed, 07 Aug 2024 11:05:47 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:
>> Date: Tue, 6 Aug 2024 19:36:15 -0700
>> Cc: 72426 <at> debbugs.gnu.org
>> From: Jim Porter <jporterbugs <at> 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.

Message #74 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: jporterbugs <at> gmail.com, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Wed, 07 Aug 2024 11:09:17 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:
>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Cc: jporterbugs <at> gmail.com,  72426 <at> 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.

Message #77 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: jporterbugs <at> gmail.com, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Wed, 07 Aug 2024 18:26:47 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: Jim Porter <jporterbugs <at> gmail.com>,  72426 <at> debbugs.gnu.org
> Date: Wed, 07 Aug 2024 11:05:47 -0400
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> >> Date: Tue, 6 Aug 2024 19:36:15 -0700
> >> Cc: 72426 <at> debbugs.gnu.org
> >> From: Jim Porter <jporterbugs <at> 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.

Message #80 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: jporterbugs <at> gmail.com, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Wed, 07 Aug 2024 11:31:22 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Cc: Jim Porter <jporterbugs <at> gmail.com>,  72426 <at> debbugs.gnu.org
>> Date: Wed, 07 Aug 2024 11:05:47 -0400
>> 
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>> >> Date: Tue, 6 Aug 2024 19:36:15 -0700
>> >> Cc: 72426 <at> debbugs.gnu.org
>> >> From: Jim Porter <jporterbugs <at> 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.

Message #83 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Spencer Baugh <sbaugh <at> janestreet.com>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Wed, 7 Aug 2024 09:08:12 -0700

On 8/7/2024 8:31 AM, Spencer Baugh wrote:
> Eli Zaretskii <eliz <at> 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.)

Message #88 received at 72426-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: jporterbugs <at> gmail.com, 72426-done <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Sat, 17 Aug 2024 12:25:25 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: jporterbugs <at> gmail.com,  72426 <at> 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.

Message #91 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Eli Zaretskii <eliz <at> gnu.org>, Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Fri, 13 Sep 2024 04:17:11 +0300

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).

Message #94 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: sbaugh <at> janestreet.com, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Fri, 13 Sep 2024 09:08:54 +0300

> Date: Fri, 13 Sep 2024 04:17:11 +0300
> Cc: 72426 <at> debbugs.gnu.org
> From: Dmitry Gutov <dmitry <at> 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!

Message #97 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dmitry <at> gutov.dev>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: sbaugh <at> janestreet.com, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Sat, 14 Sep 2024 02:45:00 +0300

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.

Message #100 received at 72426 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dmitry <at> gutov.dev>
Cc: sbaugh <at> janestreet.com, 72426 <at> debbugs.gnu.org
Subject: Re: bug#72426: 29.2.50; comint-pager doesn't affect
 async-shell-command
Date: Sat, 14 Sep 2024 09:27:46 +0300

> Date: Sat, 14 Sep 2024 02:45:00 +0300
> Cc: sbaugh <at> janestreet.com, 72426 <at> debbugs.gnu.org
> From: Dmitry Gutov <dmitry <at> 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?

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.