From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#68272: [PATCH] Fix -1 leaking from C to lisp in 'read-event' etc. Date: Sat, 13 Jan 2024 12:40:31 -0500 Message-ID: References: <46480759b6d89b5a4864e8ee1b986817366a56e5.camel@timruffing.de> <83wmsmucuo.fsf@gnu.org> <1b3fa12138838a3fe5643a9e76a65d32a677e34d.camel@timruffing.de> Reply-To: Stefan Monnier Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="1456"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: Eli Zaretskii , 68272@debbugs.gnu.org To: Tim Ruffing Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Jan 13 18:41:24 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rOi0Z-0000DK-UN for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 13 Jan 2024 18:41:24 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rOi0K-0001y9-D8; Sat, 13 Jan 2024 12:41:08 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rOi0H-0001xs-8S for bug-gnu-emacs@gnu.org; Sat, 13 Jan 2024 12:41:05 -0500 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1rOi0F-0005xB-F7 for bug-gnu-emacs@gnu.org; Sat, 13 Jan 2024 12:41:04 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rOi0D-0007PZ-Qu for bug-gnu-emacs@gnu.org; Sat, 13 Jan 2024 12:41:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 13 Jan 2024 17:41:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 68272 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 68272-submit@debbugs.gnu.org id=B68272.170516764128452 (code B ref 68272); Sat, 13 Jan 2024 17:41:01 +0000 Original-Received: (at 68272) by debbugs.gnu.org; 13 Jan 2024 17:40:41 +0000 Original-Received: from localhost ([127.0.0.1]:40981 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rOhzt-0007Oq-2g for submit@debbugs.gnu.org; Sat, 13 Jan 2024 12:40:41 -0500 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:3057) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rOhzr-0007Od-6W for 68272@debbugs.gnu.org; Sat, 13 Jan 2024 12:40:39 -0500 Original-Received: from pmg3.iro.umontreal.ca (localhost [127.0.0.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id B3AA5440FD6; Sat, 13 Jan 2024 12:40:34 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1705167632; bh=8Iok5tK96sa5HVFYEPRBps5s9mueN9Bg70RfqNegJSU=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=NkvfwTzJ/X22lCKdSQUvM89KPfnXpmnI7GhGwyh1WJ+4v/grAF7OoHBu09ckw6A6F vE05EJvZn3EmgztvGqcsSSDxT5v+9f7iqNY3g118X0O6eN5qgho6z0UgxPlux3mv3V Kl1MPVUJR5YbuvyLs4K4nJES7vbX6O1PrhIdZoqacsDwg51LueiB8U7l/pyDdXo2At 02OxnQmJXMT+i7hzImPg/BJpkcGu82qEkPisXwtpcIgLpZnqqhKE3oTJiK4tK4jI5g lN51w87jsmxvwIvQ1l3OYulW0jSORxdF9dnQTWgLc62GVCibFWyiBrkFL7IIuRs/zF M8ZqylpLnN1iQ== Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id A837B440B2E; Sat, 13 Jan 2024 12:40:32 -0500 (EST) Original-Received: from pastel (65-110-221-238.cpe.pppoe.ca [65.110.221.238]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 7C88812082B; Sat, 13 Jan 2024 12:40:32 -0500 (EST) In-Reply-To: <1b3fa12138838a3fe5643a9e76a65d32a677e34d.camel@timruffing.de> (Tim Ruffing's message of "Sat, 06 Jan 2024 15:32:23 +0100") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:278138 Archived-At: 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