* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced @ 2023-01-01 13:40 Ihor Radchenko 2023-01-01 14:38 ` Eli Zaretskii ` (2 more replies) 0 siblings, 3 replies; 52+ messages in thread From: Ihor Radchenko @ 2023-01-01 13:40 UTC (permalink / raw) To: 60467 Hi, Original report: https://orgmode.org/list/25520.20685.583180.776610@gargle.gargle.HOWL I am not sure if it is Emacs bug, but I have difficulty understanding the origin of the following error: 1. Create /tmp/bug.org with the following contents * h1 ** h2 2. emacs -Q /tmp/bug.org 3. Move point to h2 4. M-x org-promote-subtree 5. M-x undo 6. Observe "Changes to be undone by function different from announced" error The functions in question are below and I fail to see anything that may be wrong there. (defun org-promote-subtree () "Promote the entire subtree. See also `org-promote'." (interactive) (save-excursion (org-back-to-heading t) (combine-change-calls (point) (save-excursion (org-end-of-subtree t)) (org-with-limited-levels (org-map-tree 'org-promote)))) (org-fix-position-after-promote)) (defun org-promote () "Promote the current heading higher up the tree." (org-with-wide-buffer (org-back-to-heading t) (let* ((after-change-functions (remq 'flyspell-after-change-function after-change-functions)) (level (save-match-data (funcall outline-level))) (up-head (concat (make-string (org-get-valid-level level -1) ?*) " ")) (diff (abs (- level (length up-head) -1)))) (cond ((and (= level 1) org-allow-promoting-top-level-subtree) (replace-match "# " nil t)) ((= level 1) (user-error "Cannot promote to level 0. UNDO to recover if necessary")) (t (replace-match (apply #'propertize up-head (text-properties-at (match-beginning 0))) t))) (unless (= level 1) (when org-auto-align-tags (org-align-tags)) (when org-adapt-indentation (org-fixup-indentation (- diff)))) (run-hooks 'org-after-promote-entry-hook)))) In GNU Emacs 30.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.35, cairo version 1.17.6) of 2022-12-26 built on localhost Repository revision: cc29fab3a66c59e77d0ff67c0f3e2e34ec80a03c Repository branch: master Windowing system distributor 'The X.Org Foundation', version 11.0.12101004 System Description: Gentoo Linux -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-01 13:40 bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced Ihor Radchenko @ 2023-01-01 14:38 ` Eli Zaretskii 2023-01-02 0:46 ` Gregory Heytings 2023-06-22 19:28 ` bug#60467: New problem introduced Frédéric Giquel via Bug reports for GNU Emacs, the Swiss army knife of text editors 2 siblings, 0 replies; 52+ messages in thread From: Eli Zaretskii @ 2023-01-01 14:38 UTC (permalink / raw) To: Ihor Radchenko, Stefan Monnier; +Cc: 60467 > From: Ihor Radchenko <yantar92@posteo.net> > Date: Sun, 01 Jan 2023 13:40:08 +0000 > > 1. Create /tmp/bug.org with the following contents > > * h1 > ** h2 > > 2. emacs -Q /tmp/bug.org > 3. Move point to h2 > 4. M-x org-promote-subtree > 5. M-x undo > 6. Observe "Changes to be undone by function different from announced" error The error message text is confusing, what it wants to say is this: Changes undone by a function are different from the announced ones The problem is here: (apply fun args) ;; Use `save-current-buffer'? ;; Check that the function did what the entry ;; said it would do. (unless (and (= start start-mark) (= (+ delta end) end-mark)) <<<<<<<<<<<<<<< (error "Changes to be undone by function different from announced")) After re-inserting the deleted string "** " with its face information, 'end-marker' is shifted by 3 position, and is now at buffer position 13, whereas 'end' is still 10 and 'delta' is 1, so their sum is 11. Also note that if you begin from an empty Org buffer and insert the initial text "* h1\n** h2\n", and then run the recipe, the problem doesn't happen, because the "* " part is deleted before "apply fun" returns, and then 'end-mark' is in its expected position 11. Adding Stefan, in case how might have ideas for what is wrong here. This problem exists also in Emacs 29. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-01 13:40 bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced Ihor Radchenko 2023-01-01 14:38 ` Eli Zaretskii @ 2023-01-02 0:46 ` Gregory Heytings 2023-01-02 1:50 ` Gregory Heytings 2023-01-02 9:27 ` Ihor Radchenko 2023-06-22 19:28 ` bug#60467: New problem introduced Frédéric Giquel via Bug reports for GNU Emacs, the Swiss army knife of text editors 2 siblings, 2 replies; 52+ messages in thread From: Gregory Heytings @ 2023-01-02 0:46 UTC (permalink / raw) To: Ihor Radchenko; +Cc: 60467 > > I am not sure if it is Emacs bug, but I have difficulty understanding > the origin of the following error: > > 1. Create /tmp/bug.org with the following contents > > * h1 > ** h2 > > 2. emacs -Q /tmp/bug.org > 3. Move point to h2 > 4. M-x org-promote-subtree > 5. M-x undo > 6. Observe "Changes to be undone by function different from announced" error > The culprit is 85e0a69567 (in the Org repository), and the bug is fixed by: diff --git a/lisp/org.el b/lisp/org.el index a98a7f5a0..becb302e1 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -6554,8 +6554,7 @@ See also `org-promote'." (interactive) (save-excursion (org-back-to-heading t) - (combine-change-calls (point) (save-excursion (org-end-of-subtree t)) - (org-with-limited-levels (org-map-tree 'org-promote)))) + (org-with-limited-levels (org-map-tree 'org-promote))) (org-fix-position-after-promote)) (defun org-demote-subtree () @@ -6564,8 +6563,7 @@ See `org-demote' and `org-promote'." (interactive) (save-excursion (org-back-to-heading t) - (combine-change-calls (point) (save-excursion (org-end-of-subtree t)) - (org-with-limited-levels (org-map-tree 'org-demote)))) + (org-with-limited-levels (org-map-tree 'org-demote))) (org-fix-position-after-promote)) (defun org-do-promote () Perhaps another better fix exists. If it's an Emacs bug, which is possible, it's in combine-change-calls. ^ permalink raw reply related [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-02 0:46 ` Gregory Heytings @ 2023-01-02 1:50 ` Gregory Heytings 2023-01-02 9:31 ` Ihor Radchenko 2023-01-02 9:27 ` Ihor Radchenko 1 sibling, 1 reply; 52+ messages in thread From: Gregory Heytings @ 2023-01-02 1:50 UTC (permalink / raw) To: Ihor Radchenko; +Cc: 60467 > > If it's an Emacs bug, which is possible, it's in combine-change-calls. > A few more details on the bug. It's still unclear to me whether the bug is in Org or in Emacs. When the file is opened we start with buffer-undo-list = nil. The (org-with-limited-levels (org-map-tree 'org-promote)) body in org-promote-subtree, called inside a combine-change-calls, adds four entries to buffer-undo-list: ("** " . 6) <timestamp> (9 . 11) <timestamp> which means that three characters have been removed and two have been added in the buffer, which seems correct, although the (9 . 11) entry looks a bit suspicious. Perhaps (6 . 8) would have been expected there? However, this change does not happen outside the region on which combine-change-calls is called, which is 6-11. Upon returning from combine-change-calls, buffer-undo-list contains four elements, when a single element would have been expected, if I understand its docstring correctly: (apply 1 6 10 #'undo--wrap-and-run-primitive-undo 6 10 (("** " . 6))) <timestamp> (9 . 11) <timestamp> ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-02 1:50 ` Gregory Heytings @ 2023-01-02 9:31 ` Ihor Radchenko 2023-01-03 9:41 ` Gregory Heytings 0 siblings, 1 reply; 52+ messages in thread From: Ihor Radchenko @ 2023-01-02 9:31 UTC (permalink / raw) To: Gregory Heytings; +Cc: 60467 Gregory Heytings <gregory@heytings.org> writes: > The (org-with-limited-levels (org-map-tree 'org-promote)) body in > org-promote-subtree, called inside a combine-change-calls, adds four > entries to buffer-undo-list: > > ("** " . 6) <timestamp> (9 . 11) <timestamp> > > which means that three characters have been removed and two have been > added in the buffer, which seems correct The actual function doing the work is (replace-match (apply #'propertize up-head (text-properties-at (match-beginning 0))) t) The match is "** " and the replacement is "* ". -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-02 9:31 ` Ihor Radchenko @ 2023-01-03 9:41 ` Gregory Heytings 2023-01-03 12:46 ` Eli Zaretskii 2023-01-03 15:16 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 2 replies; 52+ messages in thread From: Gregory Heytings @ 2023-01-03 9:41 UTC (permalink / raw) To: Ihor Radchenko; +Cc: 60467 [-- Attachment #1: Type: text/plain, Size: 799 bytes --] It turns out that the bug is indeed not in Org, but in Emacs. The part of combine-change-call which creates the undo-list element was totally broken. For example, with a buffer-undo-list like <undo element 1> <timestamp> <undo element 2> <timestamp> nil <undo element 3> ofter body has been evaluated, the buffer-undo-list after combine-change-call is (apply ... #'undo--wrap-and-run-primitive-undo ... (<undo element 1>)) <timestamp> <undo element 2> <timestamp> nil <undo element 3> which is clearly wrong, because <undo element 1> and <undo element 2> should be considered as a single undo step. IOW, after combine-change-call buffer-undo-list should be: (apply ... #'undo--wrap-and-run-primitive-undo ... (<undo element 1> <undo element 2>)) nil <undo element 3> Patch attached. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Fix-combine-change-call.patch --] [-- Type: text/x-diff; name=Fix-combine-change-call.patch, Size: 2655 bytes --] From 2261de4d20d006c5344615d49ade539541577f7e Mon Sep 17 00:00:00 2001 From: Gregory Heytings <gregory@heytings.org> Date: Tue, 3 Jan 2023 09:23:14 +0000 Subject: [PATCH] Fix combine-change-call * lisp/subr.el (combine-change-calls-1): Rewrite the part which creates the undo-list element. Fixes bug#60467. --- lisp/subr.el | 43 ++++++++++++++++++------------------------- 1 file changed, 18 insertions(+), 25 deletions(-) diff --git a/lisp/subr.el b/lisp/subr.el index 9087f9a404..25ca211225 100644 --- a/lisp/subr.el +++ b/lisp/subr.el @@ -4934,31 +4934,24 @@ combine-change-calls-1 (kill-local-variable 'before-change-functions)) (if local-acf (setq after-change-functions acf) (kill-local-variable 'after-change-functions)))) - (when (not (eq buffer-undo-list t)) - (let ((ap-elt - (list 'apply - (- end end-marker) - beg - (marker-position end-marker) - #'undo--wrap-and-run-primitive-undo - beg (marker-position end-marker) buffer-undo-list)) - (ptr buffer-undo-list)) - (if (not (eq buffer-undo-list old-bul)) - (progn - (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))) - (unless (cdr ptr) - (message "combine-change-calls: buffer-undo-list broken")) - (setcdr ptr nil) - (push ap-elt buffer-undo-list) - (setcdr buffer-undo-list old-bul))))) + (when (and (not (eq buffer-undo-list t)) + (not (eq buffer-undo-list old-bul))) + (let ((ptr buffer-undo-list) body-undo-list) + (while (not (eq ptr old-bul)) + (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)) + (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) + (setcdr buffer-undo-list old-bul))) (if (not inhibit-modification-hooks) (run-hook-with-args 'after-change-functions beg (marker-position end-marker) -- 2.39.0 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-03 9:41 ` Gregory Heytings @ 2023-01-03 12:46 ` Eli Zaretskii 2023-01-03 13:44 ` Gregory Heytings 2023-01-03 15:16 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 1 sibling, 1 reply; 52+ messages in thread From: Eli Zaretskii @ 2023-01-03 12:46 UTC (permalink / raw) To: Gregory Heytings, Stefan Monnier; +Cc: 60467, yantar92 > Cc: 60467@debbugs.gnu.org > Date: Tue, 03 Jan 2023 09:41:52 +0000 > From: Gregory Heytings <gregory@heytings.org> > > It turns out that the bug is indeed not in Org, but in Emacs. The part of > combine-change-call which creates the undo-list element was totally > broken. For example, with a buffer-undo-list like > > <undo element 1> <timestamp> <undo element 2> <timestamp> nil <undo element 3> > > ofter body has been evaluated, the buffer-undo-list after > combine-change-call is > > (apply ... #'undo--wrap-and-run-primitive-undo ... (<undo element 1>)) <timestamp> <undo element 2> <timestamp> nil <undo element 3> > > which is clearly wrong, because <undo element 1> and <undo element 2> > should be considered as a single undo step. IOW, after > combine-change-call buffer-undo-list should be: > > (apply ... #'undo--wrap-and-run-primitive-undo ... (<undo element 1> <undo element 2>)) nil <undo element 3> > > Patch attached. 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. I'd prefer to fix this in Emacs 29 if feasible, but unless the changes are much simpler and/or more clearly documented, so that it becomes possible to assess their risks, I don't see how that could happen. I presume that all the undo tests we have still pass? If so, how about adding a test for this bug? Thanks. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-03 12:46 ` Eli Zaretskii @ 2023-01-03 13:44 ` Gregory Heytings 2023-01-03 14:48 ` Eli Zaretskii 2023-01-03 14:56 ` Gregory Heytings 0 siblings, 2 replies; 52+ messages in thread From: Gregory Heytings @ 2023-01-03 13:44 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 60467, yantar92, Stefan Monnier [-- Attachment #1: Type: text/plain, Size: 1129 bytes --] >> Patch attached. > > 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? > > I'd prefer to fix this in Emacs 29 if feasible, but unless the changes > are much simpler and/or more clearly documented, so that it becomes > possible to assess their risks, I don't see how that could happen. > I don't see how the changes could be made simpler, sorry, but now they are documented. Feel free to ask further questions if something is still unclear. > > I presume that all the undo tests we have still pass? > They do. > > If so, how about adding a test for this bug? > I'll see if I can do that, but it seems tricky. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Fix-combine-change-call.patch --] [-- Type: text/x-diff; name=Fix-combine-change-call.patch, Size: 3207 bytes --] From 8f65a74ec1b86b3f5f295972c99b0b461cc7bd28 Mon Sep 17 00:00:00 2001 From: Gregory Heytings <gregory@heytings.org> Date: Tue, 3 Jan 2023 13:29:06 +0000 Subject: [PATCH] Fix combine-change-call * lisp/subr.el (combine-change-calls-1): Rewrite the part which creates the undo-list element. Fixes bug#60467. --- lisp/subr.el | 56 +++++++++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 25 deletions(-) diff --git a/lisp/subr.el b/lisp/subr.el index 9087f9a404..da105d3474 100644 --- a/lisp/subr.el +++ b/lisp/subr.el @@ -4934,31 +4934,37 @@ combine-change-calls-1 (kill-local-variable 'before-change-functions)) (if local-acf (setq after-change-functions acf) (kill-local-variable 'after-change-functions)))) - (when (not (eq buffer-undo-list t)) - (let ((ap-elt - (list 'apply - (- end end-marker) - beg - (marker-position end-marker) - #'undo--wrap-and-run-primitive-undo - beg (marker-position end-marker) buffer-undo-list)) - (ptr buffer-undo-list)) - (if (not (eq buffer-undo-list old-bul)) - (progn - (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))) - (unless (cdr ptr) - (message "combine-change-calls: buffer-undo-list broken")) - (setcdr ptr nil) - (push ap-elt buffer-undo-list) - (setcdr buffer-undo-list old-bul))))) + ;; 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 . <something>), 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))) (if (not inhibit-modification-hooks) (run-hook-with-args 'after-change-functions beg (marker-position end-marker) -- 2.39.0 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-03 13:44 ` Gregory Heytings @ 2023-01-03 14:48 ` Eli Zaretskii 2023-01-03 15:05 ` Gregory Heytings 2023-01-03 14:56 ` Gregory Heytings 1 sibling, 1 reply; 52+ messages in thread From: Eli Zaretskii @ 2023-01-03 14:48 UTC (permalink / raw) To: Gregory Heytings; +Cc: 60467, yantar92, monnier > Date: Tue, 03 Jan 2023 13:44:01 +0000 > From: Gregory Heytings <gregory@heytings.org> > cc: Stefan Monnier <monnier@iro.umontreal.ca>, 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 . <something>), 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? ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-03 14:48 ` Eli Zaretskii @ 2023-01-03 15:05 ` Gregory Heytings 2023-01-03 16:10 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 52+ messages in thread From: Gregory Heytings @ 2023-01-03 15:05 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 60467, yantar92, monnier >> - ;; 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. By definition, garbage collection cannot collect something that is still referred to. >> - (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. >> + ;; 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 . <something>), 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? > 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. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-03 15:05 ` Gregory Heytings @ 2023-01-03 16:10 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-01-03 16:33 ` Gregory Heytings 0 siblings, 1 reply; 52+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-03 16:10 UTC (permalink / raw) To: Gregory Heytings; +Cc: 60467, Alan Mackenzie, Eli Zaretskii, yantar92 >>> - ;; 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) ^ permalink raw reply related [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-03 16:10 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-03 16:33 ` Gregory Heytings 2023-01-03 16:51 ` Gregory Heytings 2023-01-04 0:16 ` Gregory Heytings 0 siblings, 2 replies; 52+ messages in thread From: Gregory Heytings @ 2023-01-03 16:33 UTC (permalink / raw) To: Stefan Monnier; +Cc: 60467, Alan Mackenzie, Eli Zaretskii, yantar92 >> I don't even understand what this is supposed to do. > > Yet you happily threw it away :-( > Because there are no such precautions elsewhere in the code, and the comment above ("In case garbage collection has removed OLD-BUL") does not explain what its purpose is. A few lines below, old-bul is used without such a precaution. Of course, if it has a purpose, it's okay to keep it. >> 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. > Aha. So this means that garbage-collect could be called between (funcall body) and the (while ...) loop two lines below, and could have removed elements that were added to buffer-undo-list by (funcall body), right? If so, that's what the comment should have said. >> 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). > Given what you write above, that's probably what it meant to do indeed. But it also warns when buffer-undo-list is initially nil, such as with the recipe of this bug report. If adding such a warning is useful, we should check whether old-bul was initially non-nil. >> 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. > It took me a half hour to figure out what the original code was doing with this 'ap-elt' to which 'buffer-undo-list' is added, and subsequenty modified by the loop below, which does not refer to buffer-undo-list, and which also updates 'old-bul' in an unclear way. I'm biased of course, but I think I would have understood the code in my patch much easier: it does one thing (adding the new undo element) after the other (building the list included in that undo element). ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-03 16:33 ` Gregory Heytings @ 2023-01-03 16:51 ` Gregory Heytings 2023-01-04 0:16 ` Gregory Heytings 1 sibling, 0 replies; 52+ messages in thread From: Gregory Heytings @ 2023-01-03 16:51 UTC (permalink / raw) To: Stefan Monnier; +Cc: 60467, Alan Mackenzie, Eli Zaretskii, yantar92 [-- Attachment #1: Type: text/plain, Size: 54 bytes --] Updated patch attached, incorporating your comments. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Fix-combine-change-call.patch --] [-- Type: text/x-diff; name=Fix-combine-change-call.patch, Size: 3563 bytes --] From 1422d8ec3100f49d69fefea488ccda782f983eec Mon Sep 17 00:00:00 2001 From: Gregory Heytings <gregory@heytings.org> Date: Tue, 3 Jan 2023 16:48:58 +0000 Subject: [PATCH] Fix combine-change-call * lisp/subr.el (combine-change-calls-1): Rewrite the part which creates the undo-list element. Fixes bug#60467. --- lisp/subr.el | 64 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 39 insertions(+), 25 deletions(-) diff --git a/lisp/subr.el b/lisp/subr.el index 9087f9a404..3c0268a49d 100644 --- a/lisp/subr.el +++ b/lisp/subr.el @@ -4934,31 +4934,45 @@ combine-change-calls-1 (kill-local-variable 'before-change-functions)) (if local-acf (setq after-change-functions acf) (kill-local-variable 'after-change-functions)))) - (when (not (eq buffer-undo-list t)) - (let ((ap-elt - (list 'apply - (- end end-marker) - beg - (marker-position end-marker) - #'undo--wrap-and-run-primitive-undo - beg (marker-position end-marker) buffer-undo-list)) - (ptr buffer-undo-list)) - (if (not (eq buffer-undo-list old-bul)) - (progn - (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))) - (unless (cdr ptr) - (message "combine-change-calls: buffer-undo-list broken")) - (setcdr ptr nil) - (push ap-elt buffer-undo-list) - (setcdr buffer-undo-list old-bul))))) + ;; 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 (funcall body) did + ;; not add items to buffer-undo-list) ... + (unless (or (eq buffer-undo-list t) + (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, or + ;; ptr is nil (which may happen if garbage-collect has + ;; been called after (funcall body) and has truncated + ;; buffer-undo-list) ... + (while (and (not (eq ptr old-bul)) + ptr) + ;; ... and add the entries to body-undo-list, unless + ;; they are of the form (t . <something>), 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))) + ;; Warn if garbage-collect has truncated buffer-undo-list + ;; behind our back. + (when (and old-bul (not ptr)) + (message "combine-change-calls: buffer-undo-list broken")) + (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))) (if (not inhibit-modification-hooks) (run-hook-with-args 'after-change-functions beg (marker-position end-marker) -- 2.39.0 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-03 16:33 ` Gregory Heytings 2023-01-03 16:51 ` Gregory Heytings @ 2023-01-04 0:16 ` Gregory Heytings 2023-01-04 2:49 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 1 sibling, 1 reply; 52+ messages in thread From: Gregory Heytings @ 2023-01-04 0:16 UTC (permalink / raw) To: Stefan Monnier; +Cc: 60467, Alan Mackenzie, Eli Zaretskii, yantar92 >>> I don't even understand what this is supposed to do. >> >> Yet you happily threw it away :-( > > Because there are no such precautions elsewhere in the code, and the > comment above ("In case garbage collection has removed OLD-BUL") does > not explain what its purpose is. A few lines below, old-bul is used > without such a precaution. Of course, if it has a purpose, it's okay to > keep it. > I didn't remember the main reason why I removed that (cdr ptr). The reason is not only that the comment above it is wrong, but also that this (cdr ptr) condition itself is wrong: it makes that loop stop when the last element of the buffer-undo-list is reached. When buffer-undo-list is initially nil, (funcall body) adds a number of entries in buffer-undo-list, and there is no reason to exit that loop without processing its last element. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-04 0:16 ` Gregory Heytings @ 2023-01-04 2:49 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-01-04 9:24 ` Gregory Heytings 0 siblings, 1 reply; 52+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-04 2:49 UTC (permalink / raw) To: Gregory Heytings; +Cc: 60467, Alan Mackenzie, Eli Zaretskii, yantar92 >>>> I don't even understand what this is supposed to do. >>> Yet you happily threw it away :-( >> >> Because there are no such precautions elsewhere in the code, and the >> comment above ("In case garbage collection has removed OLD-BUL") does not >> explain what its purpose is. A few lines below, old-bul is used without >> such a precaution. Of course, if it has a purpose, it's okay to keep it. >> > > I didn't remember the main reason why I removed that (cdr ptr). The reason > is not only that the comment above it is wrong, but also that this (cdr ptr) > condition itself is wrong: it makes that loop stop when the last element of > the buffer-undo-list is reached. When buffer-undo-list is initially nil, > (funcall body) adds a number of entries in buffer-undo-list, and there is no > reason to exit that loop without processing its last element. I think for your loop you'd need to check `ptr` instead of (cdr ptr), indeed. For the loop currently in `subr.el` I think the (cdr ptr) is OK because we use the `setcdr` to truncate the list, so that last cons (whose `cdr` may be nil) will be in the list included in the (apply ....) entry. Stefan ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-04 2:49 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-04 9:24 ` Gregory Heytings 2023-01-04 10:50 ` Gregory Heytings 2023-01-04 14:36 ` Eli Zaretskii 0 siblings, 2 replies; 52+ messages in thread From: Gregory Heytings @ 2023-01-04 9:24 UTC (permalink / raw) To: Stefan Monnier; +Cc: 60467, Alan Mackenzie, Eli Zaretskii, yantar92 > > For the loop currently in `subr.el` I think the (cdr ptr) is OK because > we use the `setcdr` to truncate the list, so that last cons (whose `cdr` > may be nil) will be in the list included in the (apply ....) entry. > Indeed, now I see it, and I think (without being 100% sure) that it's OK. It's yet another obscure point of the original code, though. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-04 9:24 ` Gregory Heytings @ 2023-01-04 10:50 ` Gregory Heytings 2023-01-04 14:39 ` Eli Zaretskii 2023-01-04 14:36 ` Eli Zaretskii 1 sibling, 1 reply; 52+ messages in thread From: Gregory Heytings @ 2023-01-04 10:50 UTC (permalink / raw) To: Stefan Monnier; +Cc: 60467, Alan Mackenzie, Eli Zaretskii, yantar92 Another data point, about the risk assessment aspect: this function is not used by any package on ELPA, and in core it is only used by Org (with which this bug was identified), newcomment.el (2 uses) and dired (a single use, in which the code is in fact bypassed because buffer-undo-list is let-bound to t in the body). ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-04 10:50 ` Gregory Heytings @ 2023-01-04 14:39 ` Eli Zaretskii 2023-01-04 14:43 ` Gregory Heytings 0 siblings, 1 reply; 52+ messages in thread From: Eli Zaretskii @ 2023-01-04 14:39 UTC (permalink / raw) To: Gregory Heytings; +Cc: 60467, acm, yantar92, monnier > Date: Wed, 04 Jan 2023 10:50:30 +0000 > From: Gregory Heytings <gregory@heytings.org> > cc: 60467@debbugs.gnu.org, Alan Mackenzie <acm@muc.de>, > Eli Zaretskii <eliz@gnu.org>, yantar92@posteo.net > > > Another data point, about the risk assessment aspect: this function is not > used by any package on ELPA, and in core it is only used by Org (with > which this bug was identified), newcomment.el (2 uses) and dired (a single > use, in which the code is in fact bypassed because buffer-undo-list is > let-bound to t in the body). Which version of Org? the one that's on the release branch or a newer one? ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-04 14:39 ` Eli Zaretskii @ 2023-01-04 14:43 ` Gregory Heytings 0 siblings, 0 replies; 52+ messages in thread From: Gregory Heytings @ 2023-01-04 14:43 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 60467, acm, yantar92, monnier >> Another data point, about the risk assessment aspect: this function is >> not used by any package on ELPA, and in core it is only used by Org >> (with which this bug was identified), newcomment.el (2 uses) and dired >> (a single use, in which the code is in fact bypassed because >> buffer-undo-list is let-bound to t in the body). > > Which version of Org? the one that's on the release branch or a newer > one? > All versions or Org, including the one that's on the release branch. The calls to combine-change-call were added to Org on Oct 16 2021 by 85e0a69567 (in the Org repository). ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-04 9:24 ` Gregory Heytings 2023-01-04 10:50 ` Gregory Heytings @ 2023-01-04 14:36 ` Eli Zaretskii 2023-01-04 14:39 ` Gregory Heytings 1 sibling, 1 reply; 52+ messages in thread From: Eli Zaretskii @ 2023-01-04 14:36 UTC (permalink / raw) To: Gregory Heytings; +Cc: 60467, acm, yantar92, monnier > Date: Wed, 04 Jan 2023 09:24:09 +0000 > From: Gregory Heytings <gregory@heytings.org> > cc: 60467@debbugs.gnu.org, Alan Mackenzie <acm@muc.de>, > Eli Zaretskii <eliz@gnu.org>, yantar92@posteo.net > > > For the loop currently in `subr.el` I think the (cdr ptr) is OK because > > we use the `setcdr` to truncate the list, so that last cons (whose `cdr` > > may be nil) will be in the list included in the (apply ....) entry. > > > > Indeed, now I see it, and I think (without being 100% sure) that it's OK. > It's yet another obscure point of the original code, though. I hope all these revelations will find their way into comments to this tricky code. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-04 14:36 ` Eli Zaretskii @ 2023-01-04 14:39 ` Gregory Heytings 2023-01-04 18:02 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 52+ messages in thread From: Gregory Heytings @ 2023-01-04 14:39 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 60467, acm, yantar92, monnier >>> For the loop currently in `subr.el` I think the (cdr ptr) is OK >>> because we use the `setcdr` to truncate the list, so that last cons >>> (whose `cdr` may be nil) will be in the list included in the (apply >>> ....) entry. >> >> Indeed, now I see it, and I think (without being 100% sure) that it's >> OK. It's yet another obscure point of the original code, though. > > I hope all these revelations will find their way into comments to this > tricky code. > Well, I hope this tricky code will be replaced by the non-tricky code I proposed, in which none of these obscurities exist... ;-) ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-04 14:39 ` Gregory Heytings @ 2023-01-04 18:02 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-01-04 18:16 ` Gregory Heytings 0 siblings, 1 reply; 52+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-04 18:02 UTC (permalink / raw) To: Gregory Heytings; +Cc: 60467, acm, Eli Zaretskii, yantar92 >>>> For the loop currently in `subr.el` I think the (cdr ptr) is OK because >>>> we use the `setcdr` to truncate the list, so that last cons (whose `cdr` >>>> may be nil) will be in the list included in the (apply ....) entry. >>> >>> Indeed, now I see it, and I think (without being 100% sure) that it's >>> OK. It's yet another obscure point of the original code, though. >> >> I hope all these revelations will find their way into comments to this >> tricky code. >> > > Well, I hope this tricky code will be replaced by the non-tricky code > I proposed, in which none of these obscurities exist... ;-) My hope is that we can just remove the timestamp special case, in which case we can keep the current code mostly unchanged. Stefan ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-04 18:02 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-04 18:16 ` Gregory Heytings 2023-01-04 18:42 ` Eli Zaretskii 0 siblings, 1 reply; 52+ messages in thread From: Gregory Heytings @ 2023-01-04 18:16 UTC (permalink / raw) To: Stefan Monnier; +Cc: 60467, acm, Eli Zaretskii, yantar92 >> Well, I hope this tricky code will be replaced by the non-tricky code I >> proposed, in which none of these obscurities exist... ;-) > > My hope is that we can just remove the timestamp special case, in which > case we can keep the current code mostly unchanged. > Then our hopes are different. What's wrong exactly with replacing a piece of code that requires a long discussion with question marks everywhere to be understood by a piece of well-documented code that is much more readable and "evidently" does what it is supposed to do? ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-04 18:16 ` Gregory Heytings @ 2023-01-04 18:42 ` Eli Zaretskii 2023-01-04 21:04 ` Gregory Heytings 0 siblings, 1 reply; 52+ messages in thread From: Eli Zaretskii @ 2023-01-04 18:42 UTC (permalink / raw) To: Gregory Heytings; +Cc: 60467, acm, yantar92, monnier > Date: Wed, 04 Jan 2023 18:16:07 +0000 > From: Gregory Heytings <gregory@heytings.org> > cc: 60467@debbugs.gnu.org, acm@muc.de, Eli Zaretskii <eliz@gnu.org>, > yantar92@posteo.net > > > My hope is that we can just remove the timestamp special case, in which > > case we can keep the current code mostly unchanged. > > > > Then our hopes are different. What's wrong exactly with replacing a piece > of code that requires a long discussion with question marks everywhere to > be understood by a piece of well-documented code that is much more > readable and "evidently" does what it is supposed to do? The old code was working for quite a few years, so it isn't all wrong. Minimizing changes also minimizes the risk of introducing new exciting bugs. For a release branch, both these aspects are a clear win. Why do you object so much to leaving the timestamps in the undo-list? ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-04 18:42 ` Eli Zaretskii @ 2023-01-04 21:04 ` Gregory Heytings 0 siblings, 0 replies; 52+ messages in thread From: Gregory Heytings @ 2023-01-04 21:04 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 60467, acm, yantar92, monnier [-- Attachment #1: Type: text/plain, Size: 1460 bytes --] >>> My hope is that we can just remove the timestamp special case, in >>> which case we can keep the current code mostly unchanged. >> >> Then our hopes are different. What's wrong exactly with replacing a >> piece of code that requires a long discussion with question marks >> everywhere to be understood by a piece of well-documented code that is >> much more readable and "evidently" does what it is supposed to do? > > The old code was working for quite a few years, so it isn't all wrong. > It has only a few callers. In fact, its only callers for a long time have been (1) comment-region-default and uncomment-region-default, which, by chance, do not insert timestamps between the undo elements, and (2) dired-readin in which (as I said upthread) this part of the function is simply not executed because buffer-undo-list is let-bound to t around body. In effect, the first caller that actually exercises its logic in full is Org. > > Why do you object so much to leaving the timestamps in the undo-list? > I do not object to leaving timestamps in the undo-list, I object to leaving the code in the state it is, because I think of the future person who will try to make sense of it in 2028. I'm equally fine with the two attached patches, one in which timestamps are kept an the other in which they aren't. And now I believe it's better if I bow out of this thread, and let you and Stefan decide what the best course of action is. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Fix-combine-change-call-without-timestamps.patch --] [-- Type: text/x-diff; name=Fix-combine-change-call-without-timestamps.patch, Size: 6820 bytes --] From 15797b21f663464b5599560e2c62df55a81b77cf Mon Sep 17 00:00:00 2001 From: Gregory Heytings <gregory@heytings.org> Date: Wed, 4 Jan 2023 20:50:49 +0000 Subject: [PATCH] Fix combine-change-call * lisp/subr.el (combine-change-calls-1): Rewrite and document the part which creates the undo-list element. Fixes bug#60467. * test/src/undo-tests.el (undo-test-combine-change-calls-1) (undo-test-combine-change-calls-2) (undo-test-combine-change-calls-3): Add three tests for 'undo' with 'combine-change-calls'. --- lisp/subr.el | 65 +++++++++++++++++++++++--------------- test/src/undo-tests.el | 72 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 25 deletions(-) diff --git a/lisp/subr.el b/lisp/subr.el index 9087f9a404..7e201f4bc9 100644 --- a/lisp/subr.el +++ b/lisp/subr.el @@ -4934,31 +4934,46 @@ combine-change-calls-1 (kill-local-variable 'before-change-functions)) (if local-acf (setq after-change-functions acf) (kill-local-variable 'after-change-functions)))) - (when (not (eq buffer-undo-list t)) - (let ((ap-elt - (list 'apply - (- end end-marker) - beg - (marker-position end-marker) - #'undo--wrap-and-run-primitive-undo - beg (marker-position end-marker) buffer-undo-list)) - (ptr buffer-undo-list)) - (if (not (eq buffer-undo-list old-bul)) - (progn - (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))) - (unless (cdr ptr) - (message "combine-change-calls: buffer-undo-list broken")) - (setcdr ptr nil) - (push ap-elt buffer-undo-list) - (setcdr buffer-undo-list old-bul))))) + ;; If buffer-undo-list is neither t (in which case undo + ;; information is not recorded) nor equal to buffer-undo-list + ;; before body was funcalled (in which case (funcall body) did + ;; not add items to buffer-undo-list) ... + (unless (or (eq buffer-undo-list t) + (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 funcalled is found, or + ;; ptr is nil (which may happen if garbage-collect has + ;; been called after (funcall body) and has removed + ;; entries of buffer-undo-list that were added by (funcall + ;; body)) ... + (while (and ptr (not (eq ptr old-bul))) + ;; ... and add the entries to body-undo-list, unless + ;; they are of the form (t . <something>), 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)) + ;; Warn if garbage-collect has truncated buffer-undo-list + ;; behind our back. + (when (and old-bul (not ptr)) + (message + "combine-change-calls: buffer-undo-list has been truncated")) + ;; 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 funcalled. + (setcdr buffer-undo-list old-bul))) (if (not inhibit-modification-hooks) (run-hook-with-args 'after-change-functions beg (marker-position end-marker) diff --git a/test/src/undo-tests.el b/test/src/undo-tests.el index 84151d3b5d..fd45a9101f 100644 --- a/test/src/undo-tests.el +++ b/test/src/undo-tests.el @@ -439,6 +439,78 @@ undo-test-region-mark-adjustment (should (string= (buffer-string) "aaaFirst line\nSecond line\nbbb")))) +(ert-deftest undo-test-combine-change-calls-1 () + "Test how `combine-change-calls' updates `buffer-undo-list'. +Case 1: a file-visiting buffer with `buffer-undo-list' non-nil +and `buffer-modified-p' non-nil when `combine-change-calls' is +called." + (ert-with-temp-file tempfile + (with-current-buffer (find-file tempfile) + (insert "A") + (undo-boundary) + (insert "B") + (undo-boundary) + (insert "C") + (undo-boundary) + (insert " ") + (undo-boundary) + (insert "D") + (undo-boundary) + (insert "E") + (undo-boundary) + (insert "F") + (should (= (length buffer-undo-list) 14)) + (goto-char (point-min)) + (combine-change-calls (point-min) (point-max) + (re-search-forward "ABC ") + (replace-match "Z ")) + (should (= (length buffer-undo-list) 15))))) + +(ert-deftest undo-test-combine-change-calls-2 () + "Test how `combine-change-calls' updates `buffer-undo-list'. +Case 2: a file-visiting buffer with `buffer-undo-list' non-nil +and `buffer-modified-p' nil when `combine-change-calls' is +called." + (ert-with-temp-file tempfile + (with-current-buffer (find-file tempfile) + (insert "A") + (undo-boundary) + (insert "B") + (undo-boundary) + (insert "C") + (undo-boundary) + (insert " ") + (undo-boundary) + (insert "D") + (undo-boundary) + (insert "E") + (undo-boundary) + (insert "F") + (should (= (length buffer-undo-list) 14)) + (save-buffer) + (goto-char (point-min)) + (combine-change-calls (point-min) (point-max) + (re-search-forward "ABC ") + (replace-match "Z ")) + (should (= (length buffer-undo-list) 15))))) + +(ert-deftest undo-test-combine-change-calls-3 () + "Test how `combine-change-calls' updates `buffer-undo-list'. +Case 3: a file-visiting buffer with `buffer-undo-list' nil and +`buffer-modified-p' nil when `combine-change-calls' is called." + (ert-with-temp-file tempfile + (with-current-buffer (find-file tempfile) + (insert "ABC DEF") + (save-buffer) + (kill-buffer)) + (with-current-buffer (find-file tempfile) + (should (= (length buffer-undo-list) 0)) + (goto-char (point-min)) + (combine-change-calls (point-min) (point-max) + (re-search-forward "ABC ") + (replace-match "Z ")) + (should (= (length buffer-undo-list) 1))))) + (defun undo-test-all (&optional interactive) "Run all tests for \\[undo]." (interactive "p") -- 2.39.0 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: Fix-combine-change-call-with-timestamps.patch --] [-- Type: text/x-diff; name=Fix-combine-change-call-with-timestamps.patch, Size: 6604 bytes --] From a1c435749f1f4346cfd07fdb893f6721233fe9b7 Mon Sep 17 00:00:00 2001 From: Gregory Heytings <gregory@heytings.org> Date: Wed, 4 Jan 2023 20:49:22 +0000 Subject: [PATCH] Fix combine-change-call * lisp/subr.el (combine-change-calls-1): Rewrite and document the part which creates the undo-list element. Fixes bug#60467. * test/src/undo-tests.el (undo-test-combine-change-calls-1) (undo-test-combine-change-calls-2) (undo-test-combine-change-calls-3): Add three tests for 'undo' with 'combine-change-calls'. --- lisp/subr.el | 60 ++++++++++++++++++++--------------- test/src/undo-tests.el | 72 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 25 deletions(-) diff --git a/lisp/subr.el b/lisp/subr.el index 9087f9a404..106fa71b12 100644 --- a/lisp/subr.el +++ b/lisp/subr.el @@ -4934,31 +4934,41 @@ combine-change-calls-1 (kill-local-variable 'before-change-functions)) (if local-acf (setq after-change-functions acf) (kill-local-variable 'after-change-functions)))) - (when (not (eq buffer-undo-list t)) - (let ((ap-elt - (list 'apply - (- end end-marker) - beg - (marker-position end-marker) - #'undo--wrap-and-run-primitive-undo - beg (marker-position end-marker) buffer-undo-list)) - (ptr buffer-undo-list)) - (if (not (eq buffer-undo-list old-bul)) - (progn - (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))) - (unless (cdr ptr) - (message "combine-change-calls: buffer-undo-list broken")) - (setcdr ptr nil) - (push ap-elt buffer-undo-list) - (setcdr buffer-undo-list old-bul))))) + ;; If buffer-undo-list is neither t (in which case undo + ;; information is not recorded) nor equal to buffer-undo-list + ;; before body was funcalled (in which case (funcall body) did + ;; not add items to buffer-undo-list) ... + (unless (or (eq buffer-undo-list t) + (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 funcalled is found, or + ;; ptr is nil (which may happen if garbage-collect has + ;; been called after (funcall body) and has removed + ;; entries of buffer-undo-list that were added by (funcall + ;; body)), and add these entries to body-undo-list. + (while (and ptr (not (eq ptr old-bul))) + (push (car ptr) body-undo-list) + (setq ptr (cdr ptr))) + (setq body-undo-list (nreverse body-undo-list)) + ;; Warn if garbage-collect has truncated buffer-undo-list + ;; behind our back. + (when (and old-bul (not ptr)) + (message + "combine-change-calls: buffer-undo-list has been truncated")) + ;; 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 funcalled. + (setcdr buffer-undo-list old-bul))) (if (not inhibit-modification-hooks) (run-hook-with-args 'after-change-functions beg (marker-position end-marker) diff --git a/test/src/undo-tests.el b/test/src/undo-tests.el index 84151d3b5d..fd45a9101f 100644 --- a/test/src/undo-tests.el +++ b/test/src/undo-tests.el @@ -439,6 +439,78 @@ undo-test-region-mark-adjustment (should (string= (buffer-string) "aaaFirst line\nSecond line\nbbb")))) +(ert-deftest undo-test-combine-change-calls-1 () + "Test how `combine-change-calls' updates `buffer-undo-list'. +Case 1: a file-visiting buffer with `buffer-undo-list' non-nil +and `buffer-modified-p' non-nil when `combine-change-calls' is +called." + (ert-with-temp-file tempfile + (with-current-buffer (find-file tempfile) + (insert "A") + (undo-boundary) + (insert "B") + (undo-boundary) + (insert "C") + (undo-boundary) + (insert " ") + (undo-boundary) + (insert "D") + (undo-boundary) + (insert "E") + (undo-boundary) + (insert "F") + (should (= (length buffer-undo-list) 14)) + (goto-char (point-min)) + (combine-change-calls (point-min) (point-max) + (re-search-forward "ABC ") + (replace-match "Z ")) + (should (= (length buffer-undo-list) 15))))) + +(ert-deftest undo-test-combine-change-calls-2 () + "Test how `combine-change-calls' updates `buffer-undo-list'. +Case 2: a file-visiting buffer with `buffer-undo-list' non-nil +and `buffer-modified-p' nil when `combine-change-calls' is +called." + (ert-with-temp-file tempfile + (with-current-buffer (find-file tempfile) + (insert "A") + (undo-boundary) + (insert "B") + (undo-boundary) + (insert "C") + (undo-boundary) + (insert " ") + (undo-boundary) + (insert "D") + (undo-boundary) + (insert "E") + (undo-boundary) + (insert "F") + (should (= (length buffer-undo-list) 14)) + (save-buffer) + (goto-char (point-min)) + (combine-change-calls (point-min) (point-max) + (re-search-forward "ABC ") + (replace-match "Z ")) + (should (= (length buffer-undo-list) 15))))) + +(ert-deftest undo-test-combine-change-calls-3 () + "Test how `combine-change-calls' updates `buffer-undo-list'. +Case 3: a file-visiting buffer with `buffer-undo-list' nil and +`buffer-modified-p' nil when `combine-change-calls' is called." + (ert-with-temp-file tempfile + (with-current-buffer (find-file tempfile) + (insert "ABC DEF") + (save-buffer) + (kill-buffer)) + (with-current-buffer (find-file tempfile) + (should (= (length buffer-undo-list) 0)) + (goto-char (point-min)) + (combine-change-calls (point-min) (point-max) + (re-search-forward "ABC ") + (replace-match "Z ")) + (should (= (length buffer-undo-list) 1))))) + (defun undo-test-all (&optional interactive) "Run all tests for \\[undo]." (interactive "p") -- 2.39.0 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-03 13:44 ` Gregory Heytings 2023-01-03 14:48 ` Eli Zaretskii @ 2023-01-03 14:56 ` Gregory Heytings 1 sibling, 0 replies; 52+ messages in thread From: Gregory Heytings @ 2023-01-03 14:56 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 60467, yantar92, Stefan Monnier [-- Attachment #1: Type: text/plain, Size: 194 bytes --] >> If so, how about adding a test for this bug? > > I'll see if I can do that, but it seems tricky. > Here you go. Without the patch tests 2 and 3 fail, with the patch all three tests pass. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Three-new-tests-for-undo-with-combine-change-calls.patch --] [-- Type: text/x-diff; name=Three-new-tests-for-undo-with-combine-change-calls.patch, Size: 3474 bytes --] From 333a1b46aa1a430fb8cb7a848946b0a5ea2a0ede Mon Sep 17 00:00:00 2001 From: Gregory Heytings <gregory@heytings.org> Date: Tue, 3 Jan 2023 14:54:09 +0000 Subject: [PATCH] Three new tests for 'undo' with 'combine-change-calls' * test/src/undo-tests.el (undo-test-combine-change-calls-1) (undo-test-combine-change-calls-2) (undo-test-combine-change-calls-3): Add three tests for 'undo' with 'combine-change-calls'. See bug#60467. --- test/src/undo-tests.el | 72 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) diff --git a/test/src/undo-tests.el b/test/src/undo-tests.el index 84151d3b5d..fd45a9101f 100644 --- a/test/src/undo-tests.el +++ b/test/src/undo-tests.el @@ -439,6 +439,78 @@ undo-test-region-mark-adjustment (should (string= (buffer-string) "aaaFirst line\nSecond line\nbbb")))) +(ert-deftest undo-test-combine-change-calls-1 () + "Test how `combine-change-calls' updates `buffer-undo-list'. +Case 1: a file-visiting buffer with `buffer-undo-list' non-nil +and `buffer-modified-p' non-nil when `combine-change-calls' is +called." + (ert-with-temp-file tempfile + (with-current-buffer (find-file tempfile) + (insert "A") + (undo-boundary) + (insert "B") + (undo-boundary) + (insert "C") + (undo-boundary) + (insert " ") + (undo-boundary) + (insert "D") + (undo-boundary) + (insert "E") + (undo-boundary) + (insert "F") + (should (= (length buffer-undo-list) 14)) + (goto-char (point-min)) + (combine-change-calls (point-min) (point-max) + (re-search-forward "ABC ") + (replace-match "Z ")) + (should (= (length buffer-undo-list) 15))))) + +(ert-deftest undo-test-combine-change-calls-2 () + "Test how `combine-change-calls' updates `buffer-undo-list'. +Case 2: a file-visiting buffer with `buffer-undo-list' non-nil +and `buffer-modified-p' nil when `combine-change-calls' is +called." + (ert-with-temp-file tempfile + (with-current-buffer (find-file tempfile) + (insert "A") + (undo-boundary) + (insert "B") + (undo-boundary) + (insert "C") + (undo-boundary) + (insert " ") + (undo-boundary) + (insert "D") + (undo-boundary) + (insert "E") + (undo-boundary) + (insert "F") + (should (= (length buffer-undo-list) 14)) + (save-buffer) + (goto-char (point-min)) + (combine-change-calls (point-min) (point-max) + (re-search-forward "ABC ") + (replace-match "Z ")) + (should (= (length buffer-undo-list) 15))))) + +(ert-deftest undo-test-combine-change-calls-3 () + "Test how `combine-change-calls' updates `buffer-undo-list'. +Case 3: a file-visiting buffer with `buffer-undo-list' nil and +`buffer-modified-p' nil when `combine-change-calls' is called." + (ert-with-temp-file tempfile + (with-current-buffer (find-file tempfile) + (insert "ABC DEF") + (save-buffer) + (kill-buffer)) + (with-current-buffer (find-file tempfile) + (should (= (length buffer-undo-list) 0)) + (goto-char (point-min)) + (combine-change-calls (point-min) (point-max) + (re-search-forward "ABC ") + (replace-match "Z ")) + (should (= (length buffer-undo-list) 1))))) + (defun undo-test-all (&optional interactive) "Run all tests for \\[undo]." (interactive "p") -- 2.39.0 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-03 9:41 ` Gregory Heytings 2023-01-03 12:46 ` Eli Zaretskii @ 2023-01-03 15:16 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-01-03 15:29 ` Gregory Heytings 2023-01-08 15:43 ` Alan Mackenzie 1 sibling, 2 replies; 52+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-03 15:16 UTC (permalink / raw) To: Gregory Heytings; +Cc: 60467, Alan Mackenzie, Ihor Radchenko > <undo element 1> <timestamp> <undo element 2> <timestamp> nil <undo element 3> > > ofter body has been evaluated, the buffer-undo-list after > combine-change-call is > > (apply ... #'undo--wrap-and-run-primitive-undo ... (<undo element 1>)) > <timestamp> <undo element 2> <timestamp> nil <undo element 3> > > which is clearly wrong Indeed. Which begs the question: why does the current code stop when it sees a timestamp? Alan? Do you remember why you did that? What would go wrong if we applied a patch like the one below? Stefan diff --git a/lisp/subr.el b/lisp/subr.el index 5fb150994ec..6f51ac90ce5 100644 --- a/lisp/subr.el +++ b/lisp/subr.el @@ -4972,10 +4972,11 @@ combine-change-calls-1 ;; 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))))) + ;; (not (and (consp (cdr ptr)) + ;; (consp (cadr ptr)) + ;; (eq (caadr ptr) t) + ;; (setq old-bul (cdr ptr)))) + ) (setq ptr (cdr ptr))) (unless (cdr ptr) (message "combine-change-calls: buffer-undo-list broken")) ^ permalink raw reply related [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-03 15:16 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-03 15:29 ` Gregory Heytings 2023-01-03 16:32 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-01-08 15:43 ` Alan Mackenzie 1 sibling, 1 reply; 52+ messages in thread From: Gregory Heytings @ 2023-01-03 15:29 UTC (permalink / raw) To: Stefan Monnier; +Cc: 60467, Alan Mackenzie, Ihor Radchenko > > What would go wrong if we applied a patch like the one below? > With that patch, the "combine-change-calls: buffer-undo-list broken" message would be displayed with the recipe of this bug report. And timestamp entries would be added to what is "body-undo-list" in my patch. It's not clear to me that this could cause problems, but I guess it's safer to not include them, given that they were never included, and that the intention of the original code was to exclude them. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-03 15:29 ` Gregory Heytings @ 2023-01-03 16:32 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 0 replies; 52+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-03 16:32 UTC (permalink / raw) To: Gregory Heytings; +Cc: 60467, Alan Mackenzie, Ihor Radchenko > With that patch, the "combine-change-calls: buffer-undo-list broken" message > would be displayed with the recipe of this bug report. IIUC this is a separate issue which is fairly easy to fix (and doesn't need to be fixed on `emacs-29`) with something like the patch below. > And timestamp entries would be added to what is "body-undo-list" in my > patch. It's not clear to me that this could cause problems, but > I guess it's safer to not include them, given that they were never > included, AFAICT when they were not included, they resulted in a broken undo entry (as evidenced by the current bug report), so anything we do in their presence is "safer" than what we did before :-) Let's wait to see what Alan has to say, Stefan diff --git a/lisp/subr.el b/lisp/subr.el index 5fb150994ec..ff3a7c403d0 100644 --- a/lisp/subr.el +++ b/lisp/subr.el @@ -4970,15 +4970,16 @@ combine-change-calls-1 (progn (while (and (not (eq (cdr ptr) old-bul)) ;; In case garbage collection has removed OLD-BUL. - (cdr ptr) + (or (cdr ptr) + (progn + (message "combine-change-calls: buffer-undo-list broken") + nil)) ;; 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))) - (unless (cdr ptr) - (message "combine-change-calls: buffer-undo-list broken")) (setcdr ptr nil) (push ap-elt buffer-undo-list) (setcdr buffer-undo-list old-bul))))) ^ permalink raw reply related [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-03 15:16 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-01-03 15:29 ` Gregory Heytings @ 2023-01-08 15:43 ` Alan Mackenzie 2023-01-09 6:03 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 1 sibling, 1 reply; 52+ messages in thread From: Alan Mackenzie @ 2023-01-08 15:43 UTC (permalink / raw) To: Stefan Monnier; +Cc: 60467, Gregory Heytings, Ihor Radchenko Hello, Stefan. On Tue, Jan 03, 2023 at 10:16:54 -0500, Stefan Monnier wrote: > > <undo element 1> <timestamp> <undo element 2> <timestamp> nil <undo element 3> > > ofter body has been evaluated, the buffer-undo-list after > > combine-change-call is > > (apply ... #'undo--wrap-and-run-primitive-undo ... (<undo element 1>)) > > <timestamp> <undo element 2> <timestamp> nil <undo element 3> > > which is clearly wrong > Indeed. Which begs the question: why does the current code stop when it > sees a timestamp? > Alan? Do you remember why you did that? I'm afraid not. On 2018-04-01, I noted down that timestamps get "caught up inside the undo--wrap-and-run-primitive-undo entry.". But I neglected to be more specific, and can no longer remember exactly why. > What would go wrong if we applied a patch like the one below? I don't know. Possibly nothing. Maybe the problem that that code was meant to solve was also solved by some other code. > Stefan > diff --git a/lisp/subr.el b/lisp/subr.el > index 5fb150994ec..6f51ac90ce5 100644 > --- a/lisp/subr.el > +++ b/lisp/subr.el > @@ -4972,10 +4972,11 @@ combine-change-calls-1 > ;; 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))))) > + ;; (not (and (consp (cdr ptr)) > + ;; (consp (cadr ptr)) > + ;; (eq (caadr ptr) t) > + ;; (setq old-bul (cdr ptr)))) > + ) > (setq ptr (cdr ptr))) > (unless (cdr ptr) > (message "combine-change-calls: buffer-undo-list broken")) -- Alan Mackenzie (Nuremberg, Germany). ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-08 15:43 ` Alan Mackenzie @ 2023-01-09 6:03 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-01-09 12:16 ` Eli Zaretskii 0 siblings, 1 reply; 52+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-09 6:03 UTC (permalink / raw) To: Alan Mackenzie; +Cc: 60467, Gregory Heytings, Ihor Radchenko >> Alan? Do you remember why you did that? > I'm afraid not. On 2018-04-01, I noted down that timestamps get "caught > up inside the undo--wrap-and-run-primitive-undo entry.". But I neglected > to be more specific, and can no longer remember exactly why. > >> What would go wrong if we applied a patch like the one below? > > I don't know. Possibly nothing. Maybe the problem that that code was > meant to solve was also solved by some other code. Thanks. So maybe we should strip timestamps on the release branch (just to be on the safe side) and keep them on `master` (since it's likely that we don't need to filter them out, which gives simpler code and might even be arguably more correct). On `emacs-29` the patch I suggest is: diff --git a/lisp/subr.el b/lisp/subr.el index 62f72734e14..34dd847e9d5 100644 --- a/lisp/subr.el +++ b/lisp/subr.el @@ -4946,13 +4946,13 @@ combine-change-calls-1 (progn (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))) + (cdr ptr)) + (if (and (consp (cdr ptr)) + (consp (cadr ptr)) + (eq (caadr ptr) t)) + ;; Don't include a timestamp entry. + (setcdr ptr (cddr ptr)) + (setq ptr (cdr ptr)))) (unless (cdr ptr) (message "combine-change-calls: buffer-undo-list broken")) (setcdr ptr nil) I think it's the simplest&safest change to the code that fixes this bug. [ Of course, it should be accompanied by Gregory's tests. ] Then on master I suggest: diff --git a/lisp/subr.el b/lisp/subr.el index d1d3c76caf8..9e50b1e7f91 100644 --- a/lisp/subr.el +++ b/lisp/subr.el @@ -4966,21 +4966,20 @@ combine-change-calls-1 beg (marker-position end-marker) #'undo--wrap-and-run-primitive-undo - beg (marker-position end-marker) buffer-undo-list)) + beg (marker-position end-marker) + ;; We will truncate this list by side-effect below. + buffer-undo-list)) (ptr buffer-undo-list)) (if (not (eq buffer-undo-list old-bul)) (progn (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))))) + (or (cdr ptr) + (progn + (message "combine-change-calls: buffer-undo-list broken") + nil))) (setq ptr (cdr ptr))) - (unless (cdr ptr) - (message "combine-change-calls: buffer-undo-list broken")) + ;; Truncate the list that's in the `apply' entry. (setcdr ptr nil) (push ap-elt buffer-undo-list) (setcdr buffer-undo-list old-bul))))) We could also use Gregory's code on `master`. Obviously Gregory prefers it, tho it's marginally less efficient since it creates a new list instead of re-using the list elements already present. Stefan ^ permalink raw reply related [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-09 6:03 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-09 12:16 ` Eli Zaretskii 2023-01-13 22:45 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 52+ messages in thread From: Eli Zaretskii @ 2023-01-09 12:16 UTC (permalink / raw) To: Stefan Monnier; +Cc: 60467, acm, gregory, yantar92 > Cc: 60467@debbugs.gnu.org, Gregory Heytings <gregory@heytings.org>, > Ihor Radchenko <yantar92@posteo.net> > Date: Mon, 09 Jan 2023 01:03:43 -0500 > From: Stefan Monnier via "Bug reports for GNU Emacs, > the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> > > Thanks. So maybe we should strip timestamps on the release branch > (just to be on the safe side) and keep them on `master` (since it's > likely that we don't need to filter them out, which gives simpler code > and might even be arguably more correct). > > On `emacs-29` the patch I suggest is: Fine with me, thanks. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-09 12:16 ` Eli Zaretskii @ 2023-01-13 22:45 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-01-14 7:06 ` Eli Zaretskii 0 siblings, 1 reply; 52+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-13 22:45 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 60467-done, acm, gregory, yantar92 > Fine with me, thanks. OK, pushed. I also pushed Gregory's new tests. I think we can close this now. Stefan ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-13 22:45 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-01-14 7:06 ` Eli Zaretskii 0 siblings, 0 replies; 52+ messages in thread From: Eli Zaretskii @ 2023-01-14 7:06 UTC (permalink / raw) To: Stefan Monnier; +Cc: 60467-done, acm, gregory, yantar92 > From: Stefan Monnier <monnier@iro.umontreal.ca> > Cc: acm@muc.de, 60467-done@debbugs.gnu.org, gregory@heytings.org, > yantar92@posteo.net > Date: Fri, 13 Jan 2023 17:45:06 -0500 > > > Fine with me, thanks. > > OK, pushed. > I also pushed Gregory's new tests. I think we can close this now. Thanks. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-01-02 0:46 ` Gregory Heytings 2023-01-02 1:50 ` Gregory Heytings @ 2023-01-02 9:27 ` Ihor Radchenko 1 sibling, 0 replies; 52+ messages in thread From: Ihor Radchenko @ 2023-01-02 9:27 UTC (permalink / raw) To: Gregory Heytings; +Cc: 60467 Gregory Heytings <gregory@heytings.org> writes: > The culprit is 85e0a69567 (in the Org repository), and the bug is fixed > by: > > - (combine-change-calls (point) (save-excursion (org-end-of-subtree t)) > - (org-with-limited-levels (org-map-tree 'org-promote)))) > + (org-with-limited-levels (org-map-tree 'org-promote))) > (org-fix-position-after-promote)) Unfortunately, `combine-change-calls' is there for a reason. Heading manipulation involves adding/removing stars. When done in batch, this involves drastic AST changes. `combine-change-calls' here is used to merge AST cache modifications into a single update request. It makes orders of magnitudes performance improvement in some scenarios. -- Ihor Radchenko // yantar92, Org mode contributor, Learn more about Org mode at <https://orgmode.org/>. Support Org development at <https://liberapay.com/org-mode>, or support my work at <https://liberapay.com/yantar92> ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: New problem introduced 2023-01-01 13:40 bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced Ihor Radchenko 2023-01-01 14:38 ` Eli Zaretskii 2023-01-02 0:46 ` Gregory Heytings @ 2023-06-22 19:28 ` Frédéric Giquel via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-06-23 11:01 ` bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced Eli Zaretskii 2 siblings, 1 reply; 52+ messages in thread From: Frédéric Giquel via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-06-22 19:28 UTC (permalink / raw) To: 60467 Hi all, I'm reopening this bug because the fix has introduced a new problem. This can be highlighted with the following procedure (verified with Emacs 29.0.92) : 1. Open a file: emacs -Q <file> 2. Move point to a line you can comment 3. M-x comment-line 4. M-x undo 5. Check buffer modification status in modeline or use (buffer- modified-p) Without the patched version of combine-change-calls-1, the buffer status is "not modified since last save". Same result if you use another command, for example kill-line, at step 2. With new version of combine-change-calls-1, the buffer is seen as modified. I'm not sure but I imagine it has something to do with the lack of timestamp in buffer-undo-list created by combine-change-calls-1. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-06-22 19:28 ` bug#60467: New problem introduced Frédéric Giquel via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-06-23 11:01 ` Eli Zaretskii 2023-06-23 13:06 ` Gregory Heytings 0 siblings, 1 reply; 52+ messages in thread From: Eli Zaretskii @ 2023-06-23 11:01 UTC (permalink / raw) To: Frédéric Giquel, Stefan Monnier, Alan Mackenzie Cc: 60467, Gregory Heytings, Ihor Radchenko [Please don't change the Subject when you reopen a bug.] > Date: Thu, 22 Jun 2023 21:28:44 +0200 > From: Frédéric Giquel via "Bug reports for GNU Emacs, > the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> > > Hi all, > > I'm reopening this bug because the fix has introduced a new problem. > > This can be highlighted with the following procedure (verified with > Emacs 29.0.92) : > 1. Open a file: emacs -Q <file> > 2. Move point to a line you can comment > 3. M-x comment-line > 4. M-x undo > 5. Check buffer modification status in modeline or use (buffer- > modified-p) > > Without the patched version of combine-change-calls-1, the buffer > status is "not modified since last save". Same result if you use > another command, for example kill-line, at step 2. > With new version of combine-change-calls-1, the buffer is seen as > modified. > > I'm not sure but I imagine it has something to do with the lack of > timestamp in buffer-undo-list created by combine-change-calls-1. Stefan, can you please look into this? ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-06-23 11:01 ` bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced Eli Zaretskii @ 2023-06-23 13:06 ` Gregory Heytings 2023-06-26 14:51 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-06-26 14:53 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 2 replies; 52+ messages in thread From: Gregory Heytings @ 2023-06-23 13:06 UTC (permalink / raw) To: Eli Zaretskii Cc: 60467, Frédéric Giquel, Ihor Radchenko, Stefan Monnier, Alan Mackenzie > > Stefan, can you please look into this? > FYI, that issue is fixed with the Fix-combine-change-call-with-timestamps patch. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-06-23 13:06 ` Gregory Heytings @ 2023-06-26 14:51 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-06-26 14:53 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 1 sibling, 0 replies; 52+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-06-26 14:51 UTC (permalink / raw) To: Gregory Heytings Cc: 60467, Frédéric Giquel, Eli Zaretskii, Ihor Radchenko, Alan Mackenzie >> Stefan, can you please look into this? > FYI, that issue is fixed with the > Fix-combine-change-call-with-timestamps patch. Be my guest, Stefan ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-06-23 13:06 ` Gregory Heytings 2023-06-26 14:51 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-06-26 14:53 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-06-26 15:18 ` Gregory Heytings 1 sibling, 1 reply; 52+ messages in thread From: Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-06-26 14:53 UTC (permalink / raw) To: Gregory Heytings Cc: 60467, Frédéric Giquel, Eli Zaretskii, Ihor Radchenko, Alan Mackenzie >> Stefan, can you please look into this? > FYI, that issue is fixed with the > Fix-combine-change-call-with-timestamps patch. Oh, but I don't think this is OK for the `emacs-29` branch, so we still need a more targetted fix for the `emacs-29` branch. Stefan ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-06-26 14:53 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-06-26 15:18 ` Gregory Heytings 2023-06-26 15:30 ` Eli Zaretskii 0 siblings, 1 reply; 52+ messages in thread From: Gregory Heytings @ 2023-06-26 15:18 UTC (permalink / raw) To: Stefan Monnier Cc: 60467, Frédéric Giquel, Eli Zaretskii, Ihor Radchenko, Alan Mackenzie >>> Stefan, can you please look into this? >> >> FYI, that issue is fixed with the >> Fix-combine-change-call-with-timestamps patch. > > Oh, but I don't think this is OK for the `emacs-29` branch, so we still > need a more targetted fix for the `emacs-29` branch. > Well, that bug was already discussed at length six months ago, and the minimal change that was in the end installed (59c3c53efa) introduced a new bug. Should we design another minimal change, at the risk of introducing another bug? ISTM that the safe thing to do is the refactoring that was suggested. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-06-26 15:18 ` Gregory Heytings @ 2023-06-26 15:30 ` Eli Zaretskii 2023-07-01 14:14 ` Gregory Heytings 0 siblings, 1 reply; 52+ messages in thread From: Eli Zaretskii @ 2023-06-26 15:30 UTC (permalink / raw) To: Gregory Heytings; +Cc: 60467, frederic.giquel, yantar92, monnier, acm > Date: Mon, 26 Jun 2023 15:18:45 +0000 > From: Gregory Heytings <gregory@heytings.org> > cc: 60467@debbugs.gnu.org, > Frédéric Giquel <frederic.giquel@laposte.net>, > Eli Zaretskii <eliz@gnu.org>, Ihor Radchenko <yantar92@posteo.net>, > Alan Mackenzie <acm@muc.de> > > >> FYI, that issue is fixed with the > >> Fix-combine-change-call-with-timestamps patch. > > > > Oh, but I don't think this is OK for the `emacs-29` branch, so we still > > need a more targetted fix for the `emacs-29` branch. > > Well, that bug was already discussed at length six months ago, and the > minimal change that was in the end installed (59c3c53efa) introduced a new > bug. Should we design another minimal change, at the risk of introducing > another bug? ISTM that the safe thing to do is the refactoring that was > suggested. If there's no safer way than the refactoring suggested back then, we will have to stay with this regression in Emacs 29.1 and install a better fix only on master. After all, the fact that buffer-modified-p is not undone is a minor inconvenience at best. I'm sorry, but I cannot wait too much time longer before the release. The Emacs 29.1 release cycle already took significantly longer than I ever imagined. So if someone can think about some clever ideas to fix what we have on the release branch by small and safe changes, please speak up soon. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-06-26 15:30 ` Eli Zaretskii @ 2023-07-01 14:14 ` Gregory Heytings 2023-07-01 14:27 ` Eli Zaretskii 0 siblings, 1 reply; 52+ messages in thread From: Gregory Heytings @ 2023-07-01 14:14 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 60467, frederic.giquel, yantar92, monnier, acm >> Well, that bug was already discussed at length six months ago, and the >> minimal change that was in the end installed (59c3c53efa) introduced a >> new bug. Should we design another minimal change, at the risk of >> introducing another bug? ISTM that the safe thing to do is the >> refactoring that was suggested. > > If there's no safer way than the refactoring suggested back then, we > will have to stay with this regression in Emacs 29.1 and install a > better fix only on master. After all, the fact that buffer-modified-p > is not undone is a minor inconvenience at best. > Indeed. A safer way probably exists, but I would not dare writing that patch. I believe Stefan is in a better position to write it. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-07-01 14:14 ` Gregory Heytings @ 2023-07-01 14:27 ` Eli Zaretskii 2023-07-15 7:46 ` Eli Zaretskii 0 siblings, 1 reply; 52+ messages in thread From: Eli Zaretskii @ 2023-07-01 14:27 UTC (permalink / raw) To: Gregory Heytings; +Cc: 60467, frederic.giquel, yantar92, monnier, acm > Date: Sat, 01 Jul 2023 14:14:11 +0000 > From: Gregory Heytings <gregory@heytings.org> > cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net, > monnier@iro.umontreal.ca, acm@muc.de > > > >> Well, that bug was already discussed at length six months ago, and the > >> minimal change that was in the end installed (59c3c53efa) introduced a > >> new bug. Should we design another minimal change, at the risk of > >> introducing another bug? ISTM that the safe thing to do is the > >> refactoring that was suggested. > > > > If there's no safer way than the refactoring suggested back then, we > > will have to stay with this regression in Emacs 29.1 and install a > > better fix only on master. After all, the fact that buffer-modified-p > > is not undone is a minor inconvenience at best. > > > > Indeed. A safer way probably exists, but I would not dare writing that > patch. I believe Stefan is in a better position to write it. How about installing your patch, which you proposed back then, on master, then? ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-07-01 14:27 ` Eli Zaretskii @ 2023-07-15 7:46 ` Eli Zaretskii 2023-08-03 7:38 ` Eli Zaretskii 0 siblings, 1 reply; 52+ messages in thread From: Eli Zaretskii @ 2023-07-15 7:46 UTC (permalink / raw) To: gregory; +Cc: 60467, frederic.giquel, yantar92, monnier, acm Ping! > Cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net, > monnier@iro.umontreal.ca, acm@muc.de > Date: Sat, 01 Jul 2023 17:27:52 +0300 > From: Eli Zaretskii <eliz@gnu.org> > > > Date: Sat, 01 Jul 2023 14:14:11 +0000 > > From: Gregory Heytings <gregory@heytings.org> > > cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net, > > monnier@iro.umontreal.ca, acm@muc.de > > > > > > >> Well, that bug was already discussed at length six months ago, and the > > >> minimal change that was in the end installed (59c3c53efa) introduced a > > >> new bug. Should we design another minimal change, at the risk of > > >> introducing another bug? ISTM that the safe thing to do is the > > >> refactoring that was suggested. > > > > > > If there's no safer way than the refactoring suggested back then, we > > > will have to stay with this regression in Emacs 29.1 and install a > > > better fix only on master. After all, the fact that buffer-modified-p > > > is not undone is a minor inconvenience at best. > > > > > > > Indeed. A safer way probably exists, but I would not dare writing that > > patch. I believe Stefan is in a better position to write it. > > How about installing your patch, which you proposed back then, on > master, then? > > > > ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-07-15 7:46 ` Eli Zaretskii @ 2023-08-03 7:38 ` Eli Zaretskii 2023-08-10 11:28 ` Gregory Heytings 0 siblings, 1 reply; 52+ messages in thread From: Eli Zaretskii @ 2023-08-03 7:38 UTC (permalink / raw) To: gregory; +Cc: 60467, frederic.giquel, yantar92, monnier, acm Ping! Ping! Can we please finish handling of this issue? > Cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net, > monnier@iro.umontreal.ca, acm@muc.de > Date: Sat, 15 Jul 2023 10:46:30 +0300 > From: Eli Zaretskii <eliz@gnu.org> > > Ping! > > > Cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net, > > monnier@iro.umontreal.ca, acm@muc.de > > Date: Sat, 01 Jul 2023 17:27:52 +0300 > > From: Eli Zaretskii <eliz@gnu.org> > > > > > Date: Sat, 01 Jul 2023 14:14:11 +0000 > > > From: Gregory Heytings <gregory@heytings.org> > > > cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net, > > > monnier@iro.umontreal.ca, acm@muc.de > > > > > > > > > >> Well, that bug was already discussed at length six months ago, and the > > > >> minimal change that was in the end installed (59c3c53efa) introduced a > > > >> new bug. Should we design another minimal change, at the risk of > > > >> introducing another bug? ISTM that the safe thing to do is the > > > >> refactoring that was suggested. > > > > > > > > If there's no safer way than the refactoring suggested back then, we > > > > will have to stay with this regression in Emacs 29.1 and install a > > > > better fix only on master. After all, the fact that buffer-modified-p > > > > is not undone is a minor inconvenience at best. > > > > > > > > > > Indeed. A safer way probably exists, but I would not dare writing that > > > patch. I believe Stefan is in a better position to write it. > > > > How about installing your patch, which you proposed back then, on > > master, then? > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-08-03 7:38 ` Eli Zaretskii @ 2023-08-10 11:28 ` Gregory Heytings 2023-08-10 13:41 ` Eli Zaretskii 0 siblings, 1 reply; 52+ messages in thread From: Gregory Heytings @ 2023-08-10 11:28 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 60467, frederic.giquel, yantar92, monnier, acm [-- Attachment #1: Type: text/plain, Size: 408 bytes --] > > Can we please finish handling of this issue? > It's not clear to me what you want: do you want to fix this issue on the emacs-29 branch or on the master branch? We now have two bug reports on this (the one by Frédéric Giquel in this bug, and the one by Eason Huang in bug#64989). I just verified (again) that the Fix-combine-change-call-with-timestamps patch fixes these two recipes. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-08-10 11:28 ` Gregory Heytings @ 2023-08-10 13:41 ` Eli Zaretskii 2023-08-10 13:55 ` Gregory Heytings 0 siblings, 1 reply; 52+ messages in thread From: Eli Zaretskii @ 2023-08-10 13:41 UTC (permalink / raw) To: Gregory Heytings; +Cc: 60467, frederic.giquel, yantar92, monnier, acm > Date: Thu, 10 Aug 2023 11:28:33 +0000 > From: Gregory Heytings <gregory@heytings.org> > cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net, > monnier@iro.umontreal.ca, acm@muc.de > > > Can we please finish handling of this issue? > > It's not clear to me what you want: do you want to fix this issue on the > emacs-29 branch or on the master branch? I suggested to install your fix on master. > We now have two bug reports on this (the one by Frédéric Giquel in > this bug, and the one by Eason Huang in bug#64989). I just verified > (again) that the Fix-combine-change-call-with-timestamps patch fixes > these two recipes. If bug#64989 could be fixed by a simpler, safer change (which remains to be seen), we might do that on emacs-29. If not, at least we will have this fixed in Emacs 30. Thanks. ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-08-10 13:41 ` Eli Zaretskii @ 2023-08-10 13:55 ` Gregory Heytings 2023-08-12 7:09 ` Eli Zaretskii 0 siblings, 1 reply; 52+ messages in thread From: Gregory Heytings @ 2023-08-10 13:55 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 60467, frederic.giquel, yantar92, monnier, acm [-- Attachment #1: Type: text/plain, Size: 1014 bytes --] >> It's not clear to me what you want: do you want to fix this issue on >> the emacs-29 branch or on the master branch? > > I suggested to install your fix on master. > Okay, but see below. >> We now have two bug reports on this (the one by Frédéric Giquel in this >> bug, and the one by Eason Huang in bug#64989). I just verified (again) >> that the Fix-combine-change-call-with-timestamps patch fixes these two >> recipes. > > If bug#64989 could be fixed by a simpler, safer change (which remains to > be seen), we might do that on emacs-29. If not, at least we will have > this fixed in Emacs 30. > Bug#64989 is the same bug, only the recipe differs. And the change I suggested six months ago is safe, it replaces an obscure and bugged piece of code with a refactored and clear piece of code. I attach it again (that patch is for emacs-29, the one for master would be similar). If Stefan (or anyone else) fixes the bug in a safer way, that patch is not necessary. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: Fix-combine-change-call.patch --] [-- Type: text/x-diff; name=Fix-combine-change-call.patch, Size: 3453 bytes --] From 169b2064a4c5200fbfdfd7c6f43cb18e4f854b92 Mon Sep 17 00:00:00 2001 From: Gregory Heytings <gregory@heytings.org> Date: Thu, 10 Aug 2023 13:49:15 +0000 Subject: [PATCH] Fix combine-change-call * lisp/subr.el (combine-change-calls-1): Rewrite and document the part which creates the undo-list element. Fixes bug#60467 and bug#64989. --- lisp/subr.el | 60 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/lisp/subr.el b/lisp/subr.el index 872c701dbe7..d0760e92474 100644 --- a/lisp/subr.el +++ b/lisp/subr.el @@ -4978,31 +4978,41 @@ combine-change-calls-1 (kill-local-variable 'before-change-functions)) (if local-acf (setq after-change-functions acf) (kill-local-variable 'after-change-functions)))) - (when (not (eq buffer-undo-list t)) - (let ((ap-elt - (list 'apply - (- end end-marker) - beg - (marker-position end-marker) - #'undo--wrap-and-run-primitive-undo - beg (marker-position end-marker) buffer-undo-list)) - (ptr buffer-undo-list)) - (if (not (eq buffer-undo-list old-bul)) - (progn - (while (and (not (eq (cdr ptr) old-bul)) - ;; In case garbage collection has removed OLD-BUL. - (cdr ptr)) - (if (and (consp (cdr ptr)) - (consp (cadr ptr)) - (eq (caadr ptr) t)) - ;; Don't include a timestamp entry. - (setcdr ptr (cddr ptr)) - (setq ptr (cdr ptr)))) - (unless (cdr ptr) - (message "combine-change-calls: buffer-undo-list broken")) - (setcdr ptr nil) - (push ap-elt buffer-undo-list) - (setcdr buffer-undo-list old-bul))))) + ;; If buffer-undo-list is neither t (in which case undo + ;; information is not recorded) nor equal to buffer-undo-list + ;; before body was funcalled (in which case (funcall body) did + ;; not add items to buffer-undo-list) ... + (unless (or (eq buffer-undo-list t) + (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 funcalled is found, or + ;; ptr is nil (which may happen if garbage-collect has + ;; been called after (funcall body) and has removed + ;; entries of buffer-undo-list that were added by (funcall + ;; body)), and add these entries to body-undo-list. + (while (and ptr (not (eq ptr old-bul))) + (push (car ptr) body-undo-list) + (setq ptr (cdr ptr))) + (setq body-undo-list (nreverse body-undo-list)) + ;; Warn if garbage-collect has truncated buffer-undo-list + ;; behind our back. + (when (and old-bul (not ptr)) + (message + "combine-change-calls: buffer-undo-list has been truncated")) + ;; 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 funcalled. + (setcdr buffer-undo-list old-bul))) (if (not inhibit-modification-hooks) (run-hook-with-args 'after-change-functions beg (marker-position end-marker) -- 2.39.2 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-08-10 13:55 ` Gregory Heytings @ 2023-08-12 7:09 ` Eli Zaretskii 2023-08-16 16:09 ` Gregory Heytings 0 siblings, 1 reply; 52+ messages in thread From: Eli Zaretskii @ 2023-08-12 7:09 UTC (permalink / raw) To: Gregory Heytings; +Cc: 60467, frederic.giquel, yantar92, monnier, acm > Date: Thu, 10 Aug 2023 13:55:56 +0000 > From: Gregory Heytings <gregory@heytings.org> > cc: 60467@debbugs.gnu.org, frederic.giquel@laposte.net, yantar92@posteo.net, > monnier@iro.umontreal.ca, acm@muc.de > > >> It's not clear to me what you want: do you want to fix this issue on > >> the emacs-29 branch or on the master branch? > > > > I suggested to install your fix on master. > > > > Okay, but see below. I see that you haven't yet installed your fix on master; please do. TIA ^ permalink raw reply [flat|nested] 52+ messages in thread
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced 2023-08-12 7:09 ` Eli Zaretskii @ 2023-08-16 16:09 ` Gregory Heytings 0 siblings, 0 replies; 52+ messages in thread From: Gregory Heytings @ 2023-08-16 16:09 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 60467-done, frederic.giquel, yantar92, monnier, acm > > I see that you haven't yet installed your fix on master; please do. > I was waiting for your and Stefan's comments. Now done (d622602452), and closing this bug (again?). ^ permalink raw reply [flat|nested] 52+ messages in thread
[parent not found: <3eea0a7dff2915453876fc3a2b628886c78a4d4b.camel@laposte.net>]
* bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced [not found] <3eea0a7dff2915453876fc3a2b628886c78a4d4b.camel@laposte.net> @ 2023-07-04 0:03 ` sbaugh 0 siblings, 0 replies; 52+ messages in thread From: sbaugh @ 2023-07-04 0:03 UTC (permalink / raw) To: 60467; +Cc: frederic.giquel, Gregory Heytings, acm, yantar92, monnier Incidentally, this bug has an easy reproduction in emacs -Q on Emacs 29 and before: (with-temp-buffer (insert "foo") (setq-local buffer-undo-list nil comment-start ";") (comment-line 1)) which produces the "combine-change-calls: buffer-undo-list broken" warning. This is easy to trigger in practice: simply open a file already containing text, and comment a line as your first action. Anyway, this was also fixed by Stefan's change 977630b5285809a57e50, * lisp/subr.el (combine-change-calls-1): Fix bug#60467 (I send this email just because I spent some time tracking this down only to find it was already fixed. Maybe my smallish reproducer will somehow be helpful to someone...) ^ permalink raw reply [flat|nested] 52+ messages in thread
end of thread, other threads:[~2023-08-16 16:09 UTC | newest] Thread overview: 52+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-01-01 13:40 bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced Ihor Radchenko 2023-01-01 14:38 ` Eli Zaretskii 2023-01-02 0:46 ` Gregory Heytings 2023-01-02 1:50 ` Gregory Heytings 2023-01-02 9:31 ` Ihor Radchenko 2023-01-03 9:41 ` Gregory Heytings 2023-01-03 12:46 ` Eli Zaretskii 2023-01-03 13:44 ` Gregory Heytings 2023-01-03 14:48 ` Eli Zaretskii 2023-01-03 15:05 ` Gregory Heytings 2023-01-03 16:10 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-01-03 16:33 ` Gregory Heytings 2023-01-03 16:51 ` Gregory Heytings 2023-01-04 0:16 ` Gregory Heytings 2023-01-04 2:49 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-01-04 9:24 ` Gregory Heytings 2023-01-04 10:50 ` Gregory Heytings 2023-01-04 14:39 ` Eli Zaretskii 2023-01-04 14:43 ` Gregory Heytings 2023-01-04 14:36 ` Eli Zaretskii 2023-01-04 14:39 ` Gregory Heytings 2023-01-04 18:02 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-01-04 18:16 ` Gregory Heytings 2023-01-04 18:42 ` Eli Zaretskii 2023-01-04 21:04 ` Gregory Heytings 2023-01-03 14:56 ` Gregory Heytings 2023-01-03 15:16 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-01-03 15:29 ` Gregory Heytings 2023-01-03 16:32 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-01-08 15:43 ` Alan Mackenzie 2023-01-09 6:03 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-01-09 12:16 ` Eli Zaretskii 2023-01-13 22:45 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-01-14 7:06 ` Eli Zaretskii 2023-01-02 9:27 ` Ihor Radchenko 2023-06-22 19:28 ` bug#60467: New problem introduced Frédéric Giquel via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-06-23 11:01 ` bug#60467: 30.0.50; primitive-undo: Changes to be undone by function different from announced Eli Zaretskii 2023-06-23 13:06 ` Gregory Heytings 2023-06-26 14:51 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-06-26 14:53 ` Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-06-26 15:18 ` Gregory Heytings 2023-06-26 15:30 ` Eli Zaretskii 2023-07-01 14:14 ` Gregory Heytings 2023-07-01 14:27 ` Eli Zaretskii 2023-07-15 7:46 ` Eli Zaretskii 2023-08-03 7:38 ` Eli Zaretskii 2023-08-10 11:28 ` Gregory Heytings 2023-08-10 13:41 ` Eli Zaretskii 2023-08-10 13:55 ` Gregory Heytings 2023-08-12 7:09 ` Eli Zaretskii 2023-08-16 16:09 ` Gregory Heytings [not found] <3eea0a7dff2915453876fc3a2b628886c78a4d4b.camel@laposte.net> 2023-07-04 0:03 ` sbaugh
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.