From: Eli Zaretskii <eliz@gnu.org>
To: Noam Postavsky <npostavs@gmail.com>, Lars Ingebrigtsen <larsi@gnus.org>
Cc: abliss@gmail.com, 36591@debbugs.gnu.org
Subject: bug#36591: 26.2; Term's pager seems broken
Date: Wed, 24 Jul 2019 18:07:53 +0300 [thread overview]
Message-ID: <83y30no4hi.fsf@gnu.org> (raw)
In-Reply-To: <87imrsw5gj.fsf@gmail.com> (message from Noam Postavsky on Tue, 23 Jul 2019 22:07:24 -0400)
> 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?)
next prev parent reply other threads:[~2019-07-24 15:07 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-11 0:06 bug#36591: 26.2; Term's pager seems broken Adam Bliss
2019-07-23 13:53 ` Noam Postavsky
2019-07-23 17:40 ` Eli Zaretskii
2019-07-23 22:33 ` Adam Bliss
2019-07-24 2:07 ` Noam Postavsky
2019-07-24 15:07 ` Eli Zaretskii [this message]
2019-07-25 0:55 ` Noam Postavsky
2019-07-25 10:02 ` Lars Ingebrigtsen
2019-07-25 13:01 ` Eli Zaretskii
2019-07-25 16:58 ` Lars Ingebrigtsen
2019-07-25 13:01 ` Noam Postavsky
2019-07-25 17:01 ` Lars Ingebrigtsen
2019-07-25 17:28 ` Noam Postavsky
2019-07-25 17:44 ` Lars Ingebrigtsen
2019-07-25 17:57 ` Noam Postavsky
2019-07-25 13:11 ` Eli Zaretskii
2019-07-25 22:38 ` Noam Postavsky
2020-08-21 12:58 ` Lars Ingebrigtsen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=83y30no4hi.fsf@gnu.org \
--to=eliz@gnu.org \
--cc=36591@debbugs.gnu.org \
--cc=abliss@gmail.com \
--cc=larsi@gnus.org \
--cc=npostavs@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.