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#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced Date: Tue, 03 Jan 2023 11:10:06 -0500 Message-ID: References: <87o7ri74qv.fsf@localhost> <9bc9c69ac20a37ded741@heytings.org> <9bc9c69ac282c0148962@heytings.org> <87h6x9mgdv.fsf@localhost> <838rij23by.fsf@gnu.org> <83zgazznau.fsf@gnu.org> 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="20358"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: 60467@debbugs.gnu.org, Alan Mackenzie , Eli Zaretskii , yantar92@posteo.net To: Gregory Heytings Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Tue Jan 03 17:11:13 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 1pCjse-00054V-Sh for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 03 Jan 2023 17:11:13 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pCjsW-00054j-IM; Tue, 03 Jan 2023 11:11:04 -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 1pCjsU-00054Q-S2 for bug-gnu-emacs@gnu.org; Tue, 03 Jan 2023 11:11: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 1pCjsU-0008Lu-K3 for bug-gnu-emacs@gnu.org; Tue, 03 Jan 2023 11:11:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1pCjsU-0001nT-32 for bug-gnu-emacs@gnu.org; Tue, 03 Jan 2023 11:11:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 03 Jan 2023 16:11:02 +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.16727622206854 (code B ref 60467); Tue, 03 Jan 2023 16:11:02 +0000 Original-Received: (at 60467) by debbugs.gnu.org; 3 Jan 2023 16:10:20 +0000 Original-Received: from localhost ([127.0.0.1]:46541 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pCjrn-0001mT-Ig for submit@debbugs.gnu.org; Tue, 03 Jan 2023 11:10:19 -0500 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:41020) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1pCjrj-0001mA-GE for 60467@debbugs.gnu.org; Tue, 03 Jan 2023 11:10:17 -0500 Original-Received: from pmg3.iro.umontreal.ca (localhost [127.0.0.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id 2104644123E; Tue, 3 Jan 2023 11:10:10 -0500 (EST) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id 2ED6E441072; Tue, 3 Jan 2023 11:10:08 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1672762208; bh=a1IU7+RqfC2KPxyACilkNEpI3MhWQxP7TcokhWrgppM=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=lM+QfkVOCAQHUoYQ+zzgK+DOwiz/U9Wu+rrFJf81eaDAnuZMsbnsvbJNRWGGqLaqE fXQbzIv7kiS+XItAqn0mq433Y/8cA53CI4ukZxVQ8un/92f5cC3RrYAzRI+Pkc9hiP xPyexsCVAjrJmMtG6mK2yRTt63MBJaG6+MXPf1XBBDrHz2BjkP7oek2Af/IQDdthPE 7yUFoV1q/pW18RE7PnMcvkpv7h5Ysojd3DgiByAMTZ2Qk3paHRwdiLW7molZxrn79B BCBUq9qdsAD5GaFwoByumxQpK70OsJenq9xVNJxc7XlhhWxjwmN0Y+l7dyOdKSfYuh aXVk18563+qTQ== Original-Received: from pastel (unknown [45.72.200.228]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 91EBF1201C2; Tue, 3 Jan 2023 11:10:07 -0500 (EST) In-Reply-To: (Gregory Heytings's message of "Tue, 03 Jan 2023 15:05:29 +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:252418 Archived-At: >>> - ;; In case garbage collection has removed OLD-BUL. >>> - (cdr ptr) >> Why should we toss this precaution? > I don't even understand what this is supposed to do. Yet you happily threw it away :-( > By definition, garbage collection cannot collect something that is > still referred to. `garbage-collect` does a bit more than collect garbage. It also truncates undo lists among other things. >>> - (unless (cdr ptr) >>> - (message "combine-change-calls: buffer-undo-list broken")) >> And this one? > This one is just plain wrong. It assumes that buffer-undo-list is > non-nil initially. AFAICT it warns when the GC's truncation has thrown stuff away (in which case there's a good chance the `apply` element we're building is incorrect). But indeed, it probably misfires if `buffer-undo-list` is nil initially. > Yes. The current code apparently meant to skip these entries, but it does > not work at all. Replacing a broken code that does not work with > a clean(er) code that does work seems the right thing. FWIW, I don't see what's cleaner about the new code. Don't get me wrong: it's not worse either. The two look pretty much equivalent to me (well, the layout and ordering is different and your code builds a new list where the old code reuses the old list elements, but these are details that don't make much difference to neither the complexity nor the size of the code, from where I stand). Also, I agree that it's arguably easier/cleaner to install your change (which drops timestamp entries instead of exiting the loop at the first such entry) into your version of the code than into the current one (see patch below for my attempt to do that). But I don't understand why we should drop timestamp entries at all, so I'd first like to hear if Alan has an explanation for his choice to stop at timestamp entries: he presumably went to the trouble to write that extra code for a good reason. Stefan diff --git a/lisp/subr.el b/lisp/subr.el index 5fb150994ec..d84cd33c3a9 100644 --- a/lisp/subr.el +++ b/lisp/subr.el @@ -4971,12 +4971,13 @@ combine-change-calls-1 (while (and (not (eq (cdr ptr) old-bul)) ;; In case garbage collection has removed OLD-BUL. (cdr ptr) - ;; Don't include a timestamp entry. - (not (and (consp (cdr ptr)) - (consp (cadr ptr)) - (eq (caadr ptr) t) - (setq old-bul (cdr ptr))))) - (setq ptr (cdr ptr))) + ) + (if (and (consp (cdr ptr)) + (consp (cadr ptr)) + (eq (caadr ptr) t)) + ;; Don't include timestamp entries. + (setcdr ptr (cddr ptr)) + (setq ptr (cdr ptr)))) (unless (cdr ptr) (message "combine-change-calls: buffer-undo-list broken")) (setcdr ptr nil)