From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced Date: Tue, 03 Jan 2023 16:48:41 +0200 Message-ID: <83zgazznau.fsf@gnu.org> References: <87o7ri74qv.fsf@localhost> <9bc9c69ac20a37ded741@heytings.org> <9bc9c69ac282c0148962@heytings.org> <87h6x9mgdv.fsf@localhost> <838rij23by.fsf@gnu.org> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="1714"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 60467@debbugs.gnu.org, yantar92@posteo.net, monnier@iro.umontreal.ca To: Gregory Heytings Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Tue Jan 03 15:49:19 2023 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 1pCibO-0000CH-UR for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 03 Jan 2023 15:49:18 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pCib9-0005Ws-P9; Tue, 03 Jan 2023 09:49:03 -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 1pCib8-0005Wf-BU for bug-gnu-emacs@gnu.org; Tue, 03 Jan 2023 09:49:02 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1pCib8-0003Hp-2v for bug-gnu-emacs@gnu.org; Tue, 03 Jan 2023 09:49:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1pCib7-0007hJ-VA for bug-gnu-emacs@gnu.org; Tue, 03 Jan 2023 09:49:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 03 Jan 2023 14:49:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 60467 X-GNU-PR-Package: emacs Original-Received: via spool by 60467-submit@debbugs.gnu.org id=B60467.167275732329560 (code B ref 60467); Tue, 03 Jan 2023 14:49:01 +0000 Original-Received: (at 60467) by debbugs.gnu.org; 3 Jan 2023 14:48:43 +0000 Original-Received: from localhost ([127.0.0.1]:45043 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pCiao-0007gh-Mj for submit@debbugs.gnu.org; Tue, 03 Jan 2023 09:48:43 -0500 Original-Received: from eggs.gnu.org ([209.51.188.92]:57344) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pCiam-0007gU-98 for 60467@debbugs.gnu.org; Tue, 03 Jan 2023 09:48:41 -0500 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pCiag-0003H5-4Y; Tue, 03 Jan 2023 09:48:34 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=VijVrdx0F1ZDUU9VMAhJC8+h8YD5temuZWAClg4dfcw=; b=d/vhOlxidZLz Nax2Ml43NZsSLbLeykoqjWwkhn6/6b4dNwgNHZSWIWcl+0Y1DQ39CCCWRTi9yer4XzXlpE1xSA8a9 LIs2RiVmwl6X1fqDqmGFh3GT11DnVHM6tCWB3vmOs9+1grR89sGGlhSR4FLsZBd3YHAMYuScyJBSZ SXEesYJc5yMJmqmRZQMUiOKR07qVYlpCrLYNN4A9I4tt2rERLvK2cSoV1g96dvKfFupoPsreUZMfw ozkbPpR65VEKSiI/L1V7kv7JmsrxKaWoqNNeOH0+fqrpQeCLRVi+4Lhv+/SAegFyKXK8os7VY8zKv Kgh6n0RLys8DR8/+0M8Idw==; Original-Received: from [87.69.77.57] (helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pCiae-00085n-2r; Tue, 03 Jan 2023 09:48:33 -0500 In-Reply-To: (message from Gregory Heytings on Tue, 03 Jan 2023 13:44:01 +0000) 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:252408 Archived-At: > Date: Tue, 03 Jan 2023 13:44:01 +0000 > From: Gregory Heytings > cc: Stefan Monnier , 60467@debbugs.gnu.org, > yantar92@posteo.net > > > Hmm... scary, even if this is intended for the master branch. Could you > > at least keep the few comments which were in the original code, and > > perhaps also add comments explaining all those tricky tests with consp, > > equality to t and to old-bul, the significance of (car ptr) and (caar > > ptr), etc.? This would make the code more maintainer-friendly, as the > > need to constantly consult the spec of the various undo-list elements > > (and hope the spec is complete and accurate ;-) would be avoided. > > Is the attached patch better in that respect? To some extent, yes. But there's too much of comments that just say what the code does without explaining. So let me ask some questions instead: > - ;; In case garbage collection has removed OLD-BUL. > - (cdr ptr) Why should we toss this precaution? > - (unless (cdr ptr) > - (message "combine-change-calls: buffer-undo-list broken")) And this one? > + ;; If buffer-undo-list is neither t (in which case undo > + ;; information is not recorded) nor equal to buffer-undo-list > + ;; before body was evaluated (in which case evaluating body > + ;; did not add items to buffer-undo-list) ... > + (when (and (not (eq buffer-undo-list t)) > + (not (eq buffer-undo-list old-bul))) > + (let ((ptr buffer-undo-list) body-undo-list) > + ;; ... then loop over buffer-undo-list, until the head of > + ;; buffer-undo-list before body was evaluated is found ... > + (while (not (eq ptr old-bul)) > + ;; ... and add the entries to body-undo-list, unless > + ;; they are of the form (t . ), which are > + ;; entries that record buffer modification timestamps. > + (unless (and (consp (car ptr)) > + (eq (caar ptr) t)) > + (push (car ptr) body-undo-list)) > + (setq ptr (cdr ptr))) > + (setq body-undo-list (nreverse body-undo-list)) > + ;; Add an (apply ...) entry to buffer-undo-list, using > + ;; body-undo-list ... > + (push (list 'apply > + (- end end-marker) > + beg > + (marker-position end-marker) > + #'undo--wrap-and-run-primitive-undo > + beg (marker-position end-marker) > + body-undo-list) > + buffer-undo-list) > + ;; ... and set the cdr of buffer-undo-list to > + ;; buffer-undo-list before body was evaluated. > + (setcdr buffer-undo-list old-bul))) So basically you are saying that the current code stops too early and doesn't collect all the undo entries that need to be combined, because timestamp entries get in the way, is that right?