bug#36591: 26.2; Term's pager seems broken

[Noam Postavsky <npostavs@gmail.com>]

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

Eli Zaretskii <eliz@gnu.org> writes:

> I see that it's a small mess, but I don't think I understand your
> reasoning about setting the filter to t twice: it looks to me that
> both calls will clear the bit of p->infd, because they both will
> trigger this clause:
>
>       if (EQ (p->filter, Qt) && !EQ (p->status, Qlisten))

> Am I missing something?

Argh, no, I was confused.  The code in Emacs 25 is correct, although
(IMO) somewhat misleading.

>   . if the filter is not t, we will now call add_process_read_fd every
>     time, for no good reason

(This is a moot point due to the second problem, but)
add_process_read_fd just sets some bits, which is harmless even if
repeated.

>   . the patch changes how connect_network_socket works in ways that we
>     don't sufficiently understand

Yes, I agree this is a serious problem.

> I would like to leave connect_network_socket alone on the release
> branch, and just fix the problem with set-process-filter.  I think the
> easiest way is simply not to call set_process_filter_masks in
> set-process-filter, and instead restore the way that code worked
> before 9755b753, i.e. let it test the argument FILTER _before_ that
> filter is installed.  Do you see any problems with this fix?

I think that makes sense, patch attached.


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1719 bytes --]

From ee13774efa2ac4b288f339c668b6002e27fbbe8f Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Wed, 24 Jul 2019 20:33:18 -0400
Subject: [PATCH] Fix subproc listening when setting filter to non-t
 (Bug#36591)

* src/process.c (Fset_process_filter): Call add_process_read_fd
according to the state of process filter before it's updated.  This
restores the correct functioning as it was before 2016-02-16 "Allow
setting the filter masks later".  Inline the set_process_filter_masks
call instead of fixing it that function, because it is also called
from connect_network_socket, and we don't want to change the behavior
of that function so close to release.
---
 src/process.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/process.c b/src/process.c
index 2df51cfd99..b602507765 100644
--- a/src/process.c
+++ b/src/process.c
@@ -1268,10 +1268,19 @@ DEFUN ("set-process-filter", Fset_process_filter, Sset_process_filter,
   if (NILP (filter))
     filter = Qinternal_default_process_filter;
 
-  pset_filter (p, filter);
-
   if (p->infd >= 0)
-    set_process_filter_masks (p);
+    {
+      /* If filter WILL be t, stop reading output.  */
+      if (EQ (filter, Qt) && !EQ (p->status, Qlisten))
+        delete_read_fd (p->infd);
+      else if (/* If filter WAS t, then resume reading output.  */
+               EQ (p->filter, Qt)
+               /* Network or serial process not stopped:  */
+               && !EQ (p->command, Qt))
+        add_process_read_fd (p->infd);
+    }
+
+  pset_filter (p, filter);
 
   if (NETCONN1_P (p) || SERIALCONN1_P (p) || PIPECONN1_P (p))
     pset_childp (p, Fplist_put (p->childp, QCfilter, filter));
-- 
2.11.0


[-- Attachment #3: Type: text/plain, Size: 640 bytes --]


> On the master branch we should clean up the confusing set of if
> clauses, both in set-process-filter and in connect_network_socket.
> Perhaps Lars could describe his reasoning for making the change which
> introduced set_process_filter_masks and what problem it tried to
> solve.  (Btw, the log message for that change seems to imply that
> set-process-filter should not have called set_process_filter_masks,
> something that the change itself disagrees with.  An omission?)

Hmm, true, I didn't pay that close attention to the log message.
Maybe "we may not have a socket yet" refers to the already existing
'if (p->infd >= 0)' check?