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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  abliss@gmail.com,  36591@debbugs.gnu.org
> Date: Thu, 25 Jul 2019 12:02:09 +0200
> 
> Noam Postavsky <npostavs@gmail.com> writes:
> 
> >> 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?
> 
> Let's see...  this was part of the patch series that allowed for
> asynchronous connection setup?
> 
> I think Noam is right -- the "we may not have the socket yet" refers to
> this bit:
> 
>   if (p->infd >= 0)
>     set_process_filter_masks (p);

So you are saying that the commit log message wanted to explain the
code which existed there already?  Because the condition that tested
p->infd was already there before you refactored the code into
set_process_filter_masks.

That's somewhat strange, but I guess is OK.  However, I still wonder
what was the rationale for making the code change in the first place.
It seems to me that the real reason was the addition of the call to
set_process_filter_masks in connect_network_socket, but why was that
necessary?  IOW, what was missing from this code in
connect_network_socket, which already existed and manipulated some of
the same flags and descriptor bits:

  if (p->is_non_blocking_client)
    {
      /* We may get here if connect did succeed immediately.  However,
         in that case, we still need to signal this like a non-blocking
         connection.  */
      if (! (connecting_status (p->status)
             && EQ (XCDR (p->status), addrinfos)))
        pset_status (p, Fcons (Qconnect, addrinfos));
      if ((fd_callback_info[inch].flags & NON_BLOCKING_CONNECT_FD) == 0)
        add_non_blocking_write_fd (inch);
    }
  else
    /* A server may have a client filter setting of Qt, but it must
       still listen for incoming connects unless it is stopped.  */
    if ((!EQ (p->filter, Qt) && !EQ (p->command, Qt))
        || (EQ (p->status, Qlisten) && NILP (p->command)))
      add_process_read_fd (inch);

And in what use case did you see the need for the additional handling
in set_process_filter_masks?  I'd like to have a good understanding of
this, so that we could make this code less confusing on the master
branch.

TIA