unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: Robert Pluim <rpluim@gmail.com>
To: Helmut Eller <eller.helmut@gmail.com>
Cc: 64975@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
Subject: bug#64975: 30.0.50; accept-process-output and async connect
Date: Tue, 08 Aug 2023 17:16:52 +0200	[thread overview]
Message-ID: <87350ta7iz.fsf@gmail.com> (raw)
In-Reply-To: <m2pm3x1u0f.fsf@gmail.com> (Helmut Eller's message of "Tue, 08 Aug 2023 16:36:00 +0200")

>>>>> On Tue, 08 Aug 2023 16:36:00 +0200, Helmut Eller <eller.helmut@gmail.com> said:

    Helmut> On Tue, Aug 08 2023, Robert Pluim wrote:
    >> I think itʼs correct, as I have a change locally setting
    >> got_some_output for a different test case, but Iʼm going to be a pain,
    >> and ask Helmut to explain why, and see if I agree with his explanation
    >> (thatʼs a very hairy loop)

    Helmut> Setting got_some_output=1, was the first thing that came to mind that
    Helmut> made the test case pass :-).  A reasonable strategy, if we have a
    Helmut> comprehensive test suite.  If the test suite is lacking, then writing
    Helmut> more tests is a good investment too.  Ahem.

Submitting new tests is always good :-)

    Helmut> Setting got_some_output=1 will terminate the while(1) loop, but only on
    Helmut> the next iteration (around line process.c:5753) and only after another
    Helmut> useless call to xg_select.  So maybe a change like below might be
    Helmut> better.

It also means we finish looping through all the channels, unlike your
patch below. I think thatʼs a smaller and thus better change, and
aligns more with the docstring:

    Allow any pending output from subprocesses to be read by Emacs.
    It is given to their filter functions.

So if the rest of the test cases pass, I think we should apply your
original patch.

    Helmut> The variable got_some_output is also the return value of
    Helmut> wait_reading_process_output.  So I thought that 1 is a reasonable value
    Helmut> to indicate "some event happened".  0 and negative values are converted
    Helmut> to nil in accept-process-output, so there isn't an obvious way to
    Helmut> indicate "not a timeout, 0 bytes read, but some other event".  Maybe
    Helmut> MAX_INT could be used.

1 is ok as a value.

    Helmut> If you're asking why accept-process-output should return at all, then
    Helmut> the answer is that the socket is now writable and the caller probably
    Helmut> want's to know that.

I think thatʼs ok, since any actual input received from the process
will get passed to the filter function anyway.

Eli, the docstring also says

    Optional argument PROCESS means to return only after output is
    received from PROCESS or PROCESS closes the connection.

Do we need to add something like "or the underlying network connection
becomes available"? (I wonder if thatʼs too strong a guarantee).

Thanks

Robert
-- 





  reply	other threads:[~2023-08-08 15:16 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-31 13:31 bug#64975: 30.0.50; accept-process-output and async connect Helmut Eller
2023-08-05  9:26 ` Eli Zaretskii
2023-08-08  9:10   ` Robert Pluim
2023-08-08 12:15     ` Eli Zaretskii
2023-08-08 12:32       ` Robert Pluim
2023-08-08 14:36     ` Helmut Eller
2023-08-08 15:16       ` Robert Pluim [this message]
2023-08-08 15:33         ` Eli Zaretskii

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

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87350ta7iz.fsf@gmail.com \
    --to=rpluim@gmail.com \
    --cc=64975@debbugs.gnu.org \
    --cc=eliz@gnu.org \
    --cc=eller.helmut@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 public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).