bug#62958: [PATCH] Set PAGER=cat in comint.el

[Spencer Baugh <sbaugh@janestreet.com>]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Date: Wed, 19 Apr 2023 17:57:38 -0400
>> 
>> Simply adding (setenv "PAGER" "cat") globally is not correct, since
>> that will break modes like term, which support paging quite well.
>> It's only and exactly the comint-derived modes which don't need
>> paging.
>> 
>> Changing the default to "cat" in this way might be a bit
>> controversial...
>
> Sorry, this default cannot be universally correct.  You assume that
> 'cat' is always available, which is not true on non-Posix platforms.
> So at the very least the value should be set according to
> executable-find.

executable-find is not correct in the case of "cat" unfortunately,
because there are programs (git, for one) which if they see PAGER=cat,
just don't start a pager at all, for greater efficiency.  (which is
desirable behavior)

> Having said that, I'm not really sure the default should be "cat" even
> if it is available, since AFAIK you are the only one who is unhappy
> with the current situation.  So why not leave the default value as it
> is, i.e. nil?

Yes okay, I'll leave the default as it is.

>> +(defcustom comint-pager "cat"
>> +  "If non-nil, the value to use for PAGER.
>
> This is too terse.  It should at least say that the value should be a
> program name, a string.  Bonus point for explaining what is PAGER, for
> those who don't necessarily know off-hand.

Hm, yes, I have updated the docstring to explain the issue, it's worth
having a good explanation.  Hopefully this isn't too verbose now :)

>> +gWhen this is nil, comint doesn't set PAGER at all."
>    ^
> Typo.  Also, "set PAGER" is again too terse.
>
>> +  :version "30.1"
>> +  :type '(choice (const :tag "Don't set PAGER" nil)
>> +                 (const :tag "cat" "cat")
>
> The tag of "cat" is too terse again.  It should at least include the
> word "program" somewhere.
>
>> @@ -864,6 +873,7 @@ comint-exec-1
>>  	 (nconc
>>            (comint-term-environment)
>>  	  (list (format "INSIDE_EMACS=%s,comint" emacs-version))
>> +          (when comint-pager (list (format "PAGER=%s" comint-pager)))
>
> Should this test that comint-pager is a string?

I don't think that's necessary; doing
(if (stringp comint-pager) (list (format "PAGER=%s" comint-pager)))
would have unexpected behavior if comint-pager was accidentally set to a
non-string; doing
(when comint-pager (progn (assert (stringp comint-pager))
  (list (format "PAGER=%s" comint-pager))))
is a bit verbose and looks weird and is probably not that important.

Revised patch:

commit a375cb9c1260edb40a92f11fdec7f6910538135d
Author: Spencer Baugh <sbaugh@janestreet.com>
Date:   Wed Apr 19 17:44:54 2023 -0400

  Set PAGER=cat in comint.el
  
  Paging can be undesirable in comint-derived commands such as
  async-shell-command and M-x shell.  It is a frequent footgun for new
  Emacs users when they try to run commands which start a pager in such
  modes.
  
  Simply adding (setenv "PAGER" "cat") globally is not correct, since
  that will break modes like term, which support paging quite well.
  It's only and exactly the comint-derived modes which don't need
  paging.
  
  * lisp/comint.el (comint-pager):
  Add.
  (comint-exec-1):
  Use comint-pager to set PAGER.


diff --git a/lisp/comint.el b/lisp/comint.el
index 682b555a33c..37e189e4bbf 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -258,6 +258,49 @@ comint-input-ring-file-name
 		 file)
   :group 'comint)
 
+(defcustom comint-pager nil
+  "If non-nil, name of the program to use as a pager.
+
+If non-nil, comint sets the PAGER environment variable to this
+value before starting a subprocess.  PAGER controls the pager
+that will be used.  If you prefer to not use a pager, you can set
+this variable to \"cat\".
+
+If nil, the PAGER environment variable is not set and the default
+pager will be used.  On Unix systems, typically this is \"less\".
+
+Some programs start a pager before producing output.  A pager
+supports viewing text page by page, so that if the parent program
+produces more output than will fit on the screen, that output can
+be viewed incrementally.
+
+When a program is running under Emacs, this behavior can be
+undesirable, since Emacs itself supports viewing text page by
+page, and a pager requires input from the user before it will
+show more text.  Furthermore, comint is not a full fledged
+terminal emulator, so a pager will typically behave badly.
+
+However, pagers also provide backpressure: They will not consume
+more output from the parent program than the user has actually
+viewed, which on Unix means the output pipe will fill up and the
+parent program will be stopped from producing unnecessary output.
+Many programs (such as \"git log\") take advantage of this by
+producing large amounts of output by default and relying on the
+pager to not consume text that the user doesn't view.
+
+Emacs and comint do not keep track of what text the user has
+viewed, so they can't provide backpressure like a pager does.
+This means users who do not use a pager should be careful to not
+run commands which produce a lot of output.  Users can avoid this
+by limiting the amount of output (such as with \"git log -n10\")
+or by using native Emacs interfaces instead (such as
+`vc-print-log').")
+  :version "30.1"
+  :type '(choice (const :tag "Don't set PAGER" nil)
+                 (const :tag "Don't do paging (PAGER=cat)" "cat")
+		 string)
+  :group 'comint)
+
 (defvar comint-input-ring-file-prefix nil
   "The prefix to skip when parsing the input ring file.
 This is useful in Zsh when the extended_history option is on.")
@@ -864,6 +907,7 @@ comint-exec-1
 	 (nconc
           (comint-term-environment)
 	  (list (format "INSIDE_EMACS=%s,comint" emacs-version))
+          (when comint-pager (list (format "PAGER=%s" comint-pager)))
 	  process-environment))
 	(default-directory
 	  (if (file-accessible-directory-p default-directory)