bug#68272: [PATCH] Fix -1 leaking from C to lisp in 'read-event' etc.

[Stefan Monnier]

Hi Tim,

> Hey, thank you very much for the long email. I was somewhat prepared
> for hesitant reply, and I totally understand that "there be dragons".

:-)

> And I'm also aware that it doesn't spark confidence when someone with
> (almost) zero code contributions submits a patch of this kind.

[ Actually, such carefully written code and (even more so) the
  convention-abiding commit messages coming from someone who's not
  a regular contributor spiked my ears.  It made me think
  that this is either a carefully crafted trojan, or it's a really
  welcome new kid on the block :-)  ]

> First of all, I think the bug is real.

No doubt.  Bug#62018 brought to my attention to nasty situation, and I'm
really glad you went through the trouble to come up with a cleanup.

> But the problem is not just that this is not elegant.  It's worse
> because it's also pretty unclear what the caller should do in
> this case.

Yes, I think we should document in a comment somewhere how the end of
kmacro is handled from a "global" perspective: what `read_char`
does when it's the case, where/when it's supposed to be detected and how
this "signal" is propagated from there to the corresponding call to
`execute-kbd-macro`, how that relates to setting `executing-kbd-macro`
back to nil, ...

> One thing the caller can simply do is to error out.

My first intuition is that we should do that more often.
That's what `calc.el` does.

In `read-char-choice-with-read-key`, OTOH we apparently decided to
consider that not as an error but to "transparently" continue execution
past the end of the kmacro by reading from the keyboard (which
is the behavior that your patch implements).

In the case of `dbus.el` my head hurts trying to understand what the code
really does, really should do, and how your patch changes its behavior.
First, the `dbus.el` code is problematic in any case because it relies
on "pre-reading" input events and pushing them back on
`unread-command-events`, which is sadly not 100% "a no-op".  We should
provide better primitives to handle such parallel input streams
without having to "read" all events and them push them back.

IIUC, with your patch, we have the following scenario:
- Say we're inside a kmacro with N events left to execute.
- Dbus reads those N events and stashes them them onto `unread-command-events`.
- Dbus finally can read the actual dbus event and does its thing.
- Good!
- But now `at_end_of_macro_p` will return true, even though we've yet to
  execute the last N events.  We'll presumably still execute them
  (since they're in `unread-command-events`) but that won't be
  considered as coming from a kmacro any more.

> But if the caller wants an event, the only thing it can do is to touch
> executing_kbd_macro and set it nil explicitly (this is what subr.el
> does currently).

My head also hurts trying to understand what are the consequences of
doing that.

> We could also document this, but I feel something like
> "this function can return -1 and if it does, set executing_kbd_macro to
> nil and try again" is really a bit too unelegant (and it has more
> downsides, see next paragraph).

Agreed.  I think the problem is that we haven't clearly decided what
`execute-kbd-macro` is supposed to do.  The problem is that the way it's
expected to work implies a form of "nesting" such that at the end of
processing those events we return from the function, but sometimes
that's not what happens.  E.g. a kmacro can end in the middle of
a recursive edit.

Your patch brings is closer to a non-nesting semantics, a bit as if
`execute-kbd-macro` pushed all those events into the incoming input
queue and returned immediately.

Maybe that's what `execute-kbd-macro` should do, really: push all the
events into `unread-command-events` and return.  But then, how would we
do the boolean tests for `executing-kbd-macro` which we perform at
various places?  [ I guess we could label/wrap the events such that they
get executed in a special way (which let-binds `executing-kbd-macro` to
t?  ]

Seeing how `calc.el` used the -1 to signal an error (i.e. forcing
kmacros to contain "complete sequences"), maybe a half-way
behavior between the current one and your new one would be for
`read_char` to return -1 (or rather a more explicit `end-of-kbd-macro`)
event *once* and then on the next call to go on and read from
the keyboard.

> 3. commit: only change is that -1 is not propagated to lisp, other
> callers of read_char are unaffected as I explained above. (modulo
> Vlast_event_device and Vlast_event_frame -- which I can also still set
> if you want me to to).

"-1 is not propagated to lisp" doesn't say what happens in its stead.
What this does really is transparently continue reading input from the
keyboard when reaching the end of a kmacro.

I tend to agree that it's closer to the ideal behavior (even though
I still don't know what that is :-).


        Stefan