bug#67837: 29.1.90; inhibit-interaction breaks keyboard macros

[Stefan Monnier]

> I'm mostly opposed to making this kind of changes for reasons that are
> very weak, and basically are based on a special use case Spencer
> bumped into in his practice, a use case that can be resolved in a
> different way without any changes.

I can't remember what Spencer's use case was, but for me the use case
was simply that I was getting tired of having to kill Emacs processes
that were hanging waiting for input while running the test suite.

The specific reason why Emacs was waiting for input varied, it was
usually linked to some bug I had introduced.

AFAIK, `inhibit-interaction` is meant for those kinds of circumstances,
but currently it can't be used for that because it basically rules out
tests that have to wait for some async operation.

You say:

    And I'm still concerned that making these changes will be an
    incompatible change, because the functions that signaled the error
    right at the beginning will now do part of their job before
    signaling, and that could affect the net result of calling them in
    those cases.

Why are you concerned?  Which code do you think will break?
More specifically, have you ever seen or heard about a piece of code
using `inhibit-interaction`?  I have not, *except* in the context of
bugs like this one.  IOW, AFAICT, `inhibit-interaction` fails at
providing the only feature for which it's useful.

> And it seems to include unrelated change(s)?  E.g., what is this
> about:
>
>> +  defsubr (&Sadjust_point_for_property);
>> +  DEFVAR_LISP ("point-adjustment-function",
>> +               Vpoint_adjustment_function,
>> +               doc: /* Function called to adjust point after commands.
>> +This function is run after each command that moved point,
>> +unless `disable-point-adjustment' or `global-disable-point-adjustment'
>> +is non-nil.  */);
>> +  Vpoint_adjustment_function = intern ("adjust-point-for-property");
>
> ?

My changes are WiP in my big-hunk-of-unrelated changes.
So yes, I failed to excise every bit of the unrelated changes.
[ FWIW, the above change is one I couldn't remember I had in my tree,
  and while I vaguely remember making it, I can't remember what it was
  for. 🙂  ]

They're meant for Spencer or whoever else wants to work on this.
They're definitely not meant to be anywhere near ready for inclusion.
I can't even remember if I had convinced myself that this was the right
way to go about it.  Its main quality is that it pretty much works, so
it's a useful starting point.

>> diff --git a/test/lisp/autorevert-tests.el b/test/lisp/autorevert-tests.el
>> index c202970e0b2..58002d597f0 100644
>> --- a/test/lisp/autorevert-tests.el
>> +++ b/test/lisp/autorevert-tests.el
>> @@ -110,7 +110,7 @@ auto-revert--wait-for-revert
>>        (if (and (or file-notify--library
>>                     (file-remote-p temporary-file-directory))
>>                 (with-current-buffer buffer auto-revert-use-notify))
>> -          (read-event nil nil 0.05)
>> +          (sit-for 0.05)
>>          (sleep-for 0.05)))))
>
> So we are supposed to replace all calls to read-event in our test
> suite with sit-for, and to be able to do that, we are now changing how
> sit-for behaves in non-interactive sessions?

Can't remember why I did it this way, TBH.  Apparently using
`read-event` was a problem I bumped into along the way.  I probably made
or kept that change thinking it's better anyway because it more clearly
reflects the intention.

> That sounds like a very serious incompatible change in behavior for
> a very weak reason -- the possibility to run the test suite with
> inhibit-interaction non-nil.

IIUC you're talking about the `sit-for` change to not use `sleep-for`?
Yes, it's an incompatible change.  I think I made it thinking that it's
probably a good idea anyway.

We have a kind of a mess when it comes to waiting for external events:
if you look at various pieces of code that have to do such waits, you'll
tend to see that they all do it a bit differently, and if you look at
the surrounding comments and Git history, you'll tend to see that it's
the result of frustrating trial-and-failure bumping into various corner
cases.  The fact that `sit-for` currently allows async processes in
interactive mode but not in batch mode is probably one of the sources of
such problems.

> I sincerely hope we will not make such changes for such reasons.

We'll cross that bridge when we get there.  Right now, I just sent
Spencer a patch that kinds of summarizes what I've learned when I looked
at that problem.  If/when Spencer comes back with a patch he thinks is
appropriate for `master` and it includes such changes, we'll
discuss it then.

> With all due respect to our test suite, let's not forget that its main
> purpose is to allow us to test the various Emacs features.

That doesn't mean that problems encountered while writing the test suite
(e.g. the fact that `sit-for` doesn't read from async processes when
used in batch) can't reflect real problems that also affect real code
and thus deserve changes in Emacs proper.


        Stefan