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

[Eli Zaretskii <eliz@gnu.org>]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: abliss@gmail.com,  36591@debbugs.gnu.org
> Date: Tue, 23 Jul 2019 22:07:24 -0400
> 
>     static void
>     set_process_filter_masks (struct Lisp_Process *p)
>     {
>       if (EQ (p->filter, Qt) && !EQ (p->status, Qlisten))
>         delete_read_fd (p->infd);
>       else if (EQ (p->filter, Qt)
>            /* Network or serial process not stopped:  */
>            && !EQ (p->command, Qt))
>         add_process_read_fd (p->infd);
>     }
> 
> In Emacs 25 it looks like the 'if' cases are the same, but there is a
> subtle difference: the first 'if' checks 'filter', while the second
> checks 'p->filter'.  And furthermore note that pset_filter is called
> after this check against 'p->filter', so it is checking the "original"
> 'p->filter' value (which means that Emacs 25 has a bug that the fd is
> incorrectly enabled if setting the filter to t twice, i.e., (progn
> (set-process-filter PROC t) (set-process-filter PROC t))).
> 
>     DEFUN ("set-process-filter", Fset_process_filter, Sset_process_filter,
>       ...
>       (register Lisp_Object process, Lisp_Object filter)
>     {
>       ...
>       if (p->infd >= 0)
>         {
>           if (EQ (filter, Qt) && !EQ (p->status, Qlisten))
>             {
>               FD_CLR (p->infd, &input_wait_mask);
>               FD_CLR (p->infd, &non_keyboard_wait_mask);
>             }
>           else if (EQ (p->filter, Qt)
>                    /* Network or serial process not stopped:  */
>                    && !EQ (p->command, Qt))
>             {
>               FD_SET (p->infd, &input_wait_mask);
>               FD_SET (p->infd, &non_keyboard_wait_mask);
>             }
>         }
> 
>       pset_filter (p, filter);

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:

  current emacs-26:

      if (EQ (p->filter, Qt) && !EQ (p->status, Qlisten))
        delete_read_fd (p->infd);

  previous code:

          if (EQ (filter, Qt) && !EQ (p->status, Qlisten))
            {
              FD_CLR (p->infd, &input_wait_mask);
              FD_CLR (p->infd, &non_keyboard_wait_mask);
            }

Am I missing something?

> The patch found by Adam's bisect put the pset_filter call before this
> check, so that Emacs 26 checks the 'p->filter' after it's been set
> (i.e., the value of the 'filter' parameter).  So the second case is no
> longer entered when calling (set-filter-process PROC FILTER).

Right, this part is clear.  My problem was with the solution that just
reverses the 2nd test.  See below.

> > Also, the same function is called in another
> > place, so what will this change do to that other caller?
> 
> Hmm, it's difficult to say, there are a lot of optional code paths.  I
> suspect socket subprocesses might suffer from the same bug, but there
> are also some (redundant?) calls add_process_read_fd that might cover it
> up (notice that all calls are guarded by !EQ (p->filter, Qt), i.e., the
> corrected version of the check in set_process_filter_masks).

Yes, that's another small mess.

I think we need to be more careful here if we want to have this fixed
in Emacs 26.3.  The problem with your proposal is that it has
potentially far-reaching consequences:

  . if the filter is not t, we will now call add_process_read_fd every
    time, for no good reason
  . the patch changes how connect_network_socket works in ways that we
    don't sufficiently understand

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?

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