A strange issue with buffer-undo-list

A strange issue with buffer-undo-list

[Marcin Borkowski]

Hi all,

I was writing a command to move the current line past the next one (for
educational purposes), and I encountered a strange issue.

Here is my function:

--8<---------------cut here---------------start------------->8---
(defun move-line-down (count)
  "Move the current line down."
  (interactive "p")
  (let ((position-in-line (current-column)))
    (line-move 1)
    (transpose-lines count)
    (line-move -1)
    (move-to-column position-in-line)))
--8<---------------cut here---------------end--------------->8---

(It is definitely far from optimal, and I know that - the next version
will get rid of `transpose-lines' dependency and will just delete the
line below and recreate it above within a `save-excursion', which will
render the `position-in-line' stuff unnecessary.  But never mind this.)

So, my problem is that if I say M-x move-line-down, everything works,
but after undoing that change, the point lands at the bol of the next
line instead if where it had been at the beginning.  (I suppose this is
because only the `(transpose-lines count)' is effectively undone, so
`(line-move 1)` is not undone - that makes sense.)

So, I changed the code to this:

--8<---------------cut here---------------start------------->8---
(defun move-line-down (count)
  "Move the current line down."
  (interactive "p")
  (let ((position-in-line (current-column)))
    (push (point) buffer-undo-list)
    (line-move 1)
    (transpose-lines count)
    (line-move -1)
    (move-to-column position-in-line)))
--8<---------------cut here---------------end--------------->8---

which triggers an error.

Now, I did (of course) M-x toggle-debug-on-error, and so I learned about
"change groups".  I haven't heard about them before, so I consulted the
docs, and they seem cool (much like transactions in databases).  But
I have questions.

1. Why did my code confuse the change group mechanism?

2. How do I use `undo-amalgamate-change-group'?  The manual does not
provide any examples, and I only found one occurrence in the Emacs
source, and frankly, it didn't help a lot.  (Well, once I know the
answer to my next question, this will most probably be easy.)

3. Also, I am not even sure how the change group mechanism is supposed
to be used.  The docs tell me to use `unwind-protect', but this:

--8<---------------cut here---------------start------------->8---
(defun move-line-down (count)
  "Move the current line down."
  (interactive "p")
  (let ((position-in-line (current-column))
	(change-group (prepare-change-group)))
    (unwind-protect
	(progn
	  (activate-change-group change-group)
	  (line-move 1)
	  (transpose-lines count)
	  (line-move -1)
	  (move-to-column position-in-line)
	  (accept-change-group change-group))
      (cancel-change-group change-group))))
--8<---------------cut here---------------end--------------->8---

of course doesn't work, since both `accept-change-group' and
`cancel-change-group' are evaluated.  Is there a way to get the "state"
of the change group to only cancel it if it was not finished?

Anyway, it seems that this whole machinery is an overkill for the
purpose of just moving a line down - but as I said, I want to learn and
teach here, so this is kind of intentionally a toy example.

TIA,

-- 
Marcin Borkowski
http://mbork.pl

Re: A strange issue with buffer-undo-list

[Michael Heerdegen]

Marcin Borkowski <mbork@mbork.pl> writes:

> Is there a way to get the "state" of the change group to only cancel
> it if it was not finished?

I think yes, it's called `atomic-change-group' - see how that macro is
implemented.

Michael.

Re: A strange issue with buffer-undo-list

[Marcin Borkowski]

On 2021-02-24, at 01:43, Michael Heerdegen <michael_heerdegen@web.de> wrote:

> Marcin Borkowski <mbork@mbork.pl> writes:
>
>> Is there a way to get the "state" of the change group to only cancel
>> it if it was not finished?
>
> I think yes, it's called `atomic-change-group' - see how that macro is
> implemented.

Ah, that's simple and clever.  Thank you.

Still, these "change groups" seem a strange feature to me - they are
hardly ever used in Emacs itself, right?

Anyway, thanks a lot to both of you.

-- 
Marcin Borkowski
http://mbork.pl

Re: A strange issue with buffer-undo-list

[Emanuel Berg via Users list for the GNU Emacs text editor]

Marcin Borkowski wrote:

>>> Is there a way to get the "state" of the change group to
>>> only cancel it if it was not finished?
>>
>> I think yes, it's called `atomic-change-group' - see how
>> that macro is implemented.
>
> Ah, that's simple and clever. Thank you.
>
> Still, these "change groups" seem a strange feature to me -
> they are hardly ever used in Emacs itself, right?

Maybe people just don't understand the docstring:

  Like ‘progn’ but perform BODY as an atomic change group.
  This means that if BODY exits abnormally, all of its changes
  to the current buffer are undone. This works regardless of
  whether undo is enabled in the buffer.

  This mechanism is transparent to ordinary use of undo;
  if undo is enabled in the buffer and BODY succeeds, the
  user can undo the change normally.

How about:

  1. Do BODY
  2. On error: redo all

But if they are to drop buzzwords, isn't this a transaction?
Maybe that's something else...

-- 
underground experts united
http://user.it.uu.se/~embe8573
https://dataswamp.org/~incal

Re: A strange issue with buffer-undo-list

[Stefan Monnier]

>>> I think yes, it's called `atomic-change-group' - see how
[...]
> But if they are to drop buzzwords, isn't this a transaction?

No.  It's only part of the properties of a transaction.  The one called
"atomicity".  In the context of programming language, atomicity is also
important for concurrency (where people usually use locks and stuff
where the purpose is to get atomicity of the "critical region").


        Stefan

Re: A strange issue with buffer-undo-list

[Emanuel Berg via Users list for the GNU Emacs text editor]

Stefan Monnier wrote:

>> But if they are to drop buzzwords, isn't this
>> a transaction?
>
> No. It's only part of the properties of a transaction.
> The one called "atomicity". In the context of programming
> language, atomicity is also important for concurrency (where
> people usually use locks and stuff where the purpose is to
> get atomicity of the "critical region").

Interesting...

Semaphores and mutex to avoid race conditions?

Can you do that in Emacs even tho Emacs isn't Unix (POSIX)?

-- 
underground experts united
http://user.it.uu.se/~embe8573
https://dataswamp.org/~incal

Re: A strange issue with buffer-undo-list

[Marcin Borkowski]

On 2021-02-25, at 06:28, Emanuel Berg via Users list for the GNU Emacs text editor <help-gnu-emacs@gnu.org> wrote:

> Marcin Borkowski wrote:
>
>>>> Is there a way to get the "state" of the change group to
>>>> only cancel it if it was not finished?
>>>
>>> I think yes, it's called `atomic-change-group' - see how
>>> that macro is implemented.
>>
>> Ah, that's simple and clever. Thank you.
>>
>> Still, these "change groups" seem a strange feature to me -
>> they are hardly ever used in Emacs itself, right?
>
> Maybe people just don't understand the docstring:
>
>   Like ‘progn’ but perform BODY as an atomic change group.
>   This means that if BODY exits abnormally, all of its changes
>   to the current buffer are undone. This works regardless of
>   whether undo is enabled in the buffer.
>
>   This mechanism is transparent to ordinary use of undo;
>   if undo is enabled in the buffer and BODY succeeds, the
>   user can undo the change normally.

Well, I understand that.  But isn't it strange that almost no code in
Emacs itself uses this?

Best,

-- 
Marcin Borkowski
http://mbork.pl

Re: A strange issue with buffer-undo-list

[Michael Heerdegen]

Marcin Borkowski <mbork@mbork.pl> writes:

> Still, these "change groups" seem a strange feature to me - they are
> hardly ever used in Emacs itself, right?

Seems so.  Where they are used - e.g. in the implementation of
`transpose-subr', they are important however.  You wouldn't want that
undoing a transposition would happen in multiple steps uncovering
implementation details.

Could be in other places they would also be appropriate but were
forgotten.  May also be that automatic undo amalgamation conceals cases
were change group applications are missing.

BTW, in master your code example now works without errors, the problem
has been fixed (by Stefan).  And your code also works as you intend.  I
have tried.  It doesn't work as intended however when using
`undo-tree-mode' (another minor bug I would say).


Regards,

Michael.

Re: A strange issue with buffer-undo-list

[Marcin Borkowski]

On 2021-02-26, at 01:53, Michael Heerdegen <michael_heerdegen@web.de> wrote:

> Marcin Borkowski <mbork@mbork.pl> writes:
>
>> Still, these "change groups" seem a strange feature to me - they are
>> hardly ever used in Emacs itself, right?
>
> Seems so.  Where they are used - e.g. in the implementation of
> `transpose-subr', they are important however.  You wouldn't want that
> undoing a transposition would happen in multiple steps uncovering
> implementation details.

Agreed, although I don't think undo would kick in within one command
(without explicit undo boundaries).

> Could be in other places they would also be appropriate but were
> forgotten.  May also be that automatic undo amalgamation conceals cases
> were change group applications are missing.

As above - isn't it rather that unless you put explicit undo boundaries,
they are only inserted when the command loop takes over?

>
> BTW, in master your code example now works without errors, the problem
> has been fixed (by Stefan).  And your code also works as you intend.  I
> have tried.  It doesn't work as intended however when using
> `undo-tree-mode' (another minor bug I would say).

Thanks for the answer, and sorry for the delay.  Yes, I noticed it's
fixed; I don't use undo-tree.

Best,

-- 
Marcin Borkowski
http://mbork.pl

Re: A strange issue with buffer-undo-list

[Michael Heerdegen]

Marcin Borkowski <mbork@mbork.pl> writes:

> > Seems so.  Where they are used - e.g. in the implementation of
> > `transpose-subr', they are important however.  You wouldn't want that
> > undoing a transposition would happen in multiple steps uncovering
> > implementation details.
>
> Agreed, although I don't think undo would kick in within one command
> (without explicit undo boundaries).

Maybe I had not yet completely understood how this works.  It seems one
(primitive) undo undoes until it finds a boundary, and boundaries seem
to be implicitly inserted after any command.  Then I think the change
group is used in `transpose-subr' to prevent that the transposition or
its undoing is left in a half-done state in case of an error, and it has
more or less no user visible effect if no errors happen.

Someone should really write a short summary on how the undo system is
implemented, it is not trivial and finding out is also not.

Regards,

Michael.

Re: A strange issue with buffer-undo-list

[Marcin Borkowski]

On 2021-03-09, at 01:32, Michael Heerdegen <michael_heerdegen@web.de> wrote:

> Marcin Borkowski <mbork@mbork.pl> writes:
>
>> > Seems so.  Where they are used - e.g. in the implementation of
>> > `transpose-subr', they are important however.  You wouldn't want that
>> > undoing a transposition would happen in multiple steps uncovering
>> > implementation details.
>>
>> Agreed, although I don't think undo would kick in within one command
>> (without explicit undo boundaries).
>
> Maybe I had not yet completely understood how this works.  It seems one
> (primitive) undo undoes until it finds a boundary, and boundaries seem
> to be implicitly inserted after any command.  Then I think the change
> group is used in `transpose-subr' to prevent that the transposition or
> its undoing is left in a half-done state in case of an error, and it has
> more or less no user visible effect if no errors happen.
>
> Someone should really write a short summary on how the undo system is
> implemented, it is not trivial and finding out is also not.

Yeah, I'd also like to read that.  (I'd like to write it, too, but I'm
afraid I might not be competent enough...)

Best,

-- 
Marcin Borkowski
http://mbork.pl

Re: A strange issue with buffer-undo-list

[Michael Heerdegen]

Marcin Borkowski <mbork@mbork.pl> writes:

>   (let ((position-in-line (current-column)))
>     (push (point) buffer-undo-list)

I wonder if it is legitimate to do this.

>     (line-move 1)
>     (transpose-lines count)
>     (line-move -1)
>     (move-to-column position-in-line)))
>
>
> which triggers an error.

Sometimes it helps in such situations to add an explicit
`undo-boundary'.


> 1. Why did my code confuse the change group mechanism?

I can't answer that, I dunno if it is legal.  Don't have enough
knowledge about that (mine is also only from reading code).

> 2. How do I use `undo-amalgamate-change-group'?  The manual does not
> provide any examples, and I only found one occurrence in the Emacs
> source, and frankly, it didn't help a lot.

Well, it seems very simple: you have the handle, you call
`undo-amalgamate-change-group' with it, typically directly before
accepting the group, and that's it...?

The only example use I found is in viper-cmd.el.  Seems the code never
closes the opened change groups.  AFAIU that's not ok.


Regards,

Michael.

Re: A strange issue with buffer-undo-list

[Michael Heerdegen]

Marcin Borkowski <mbork@mbork.pl> writes:

> (defun move-line-down (count)
>   "Move the current line down."
>   (interactive "p")
>   (let ((position-in-line (current-column)))
>     (push (point) buffer-undo-list)
>     (line-move 1)
>     (transpose-lines count)
>     (line-move -1)
>     (move-to-column position-in-line)))
>
>
> which triggers an error.

[when invoking undo after using it]

Seems the error happens in the code for fixing Bug#33341 by Stefan:

(defun activate-change-group (handle)
  "Activate a change group made with `prepare-change-group' (which see)."
  (dolist (elt handle)
    (with-current-buffer (car elt)
      (if (eq buffer-undo-list t)
	  (setq buffer-undo-list nil)
	;; [huge comment here]
        (when (numberp (caar buffer-undo-list)) ;; <--- here
          (push (cons (caar buffer-undo-list) (caar buffer-undo-list))
                buffer-undo-list))))))

I guess that `caar' should be (car-safe (car buffer-undo-list)) or so -
right?

Michael.

Re: A strange issue with buffer-undo-list

[Stefan Monnier]

> (defun activate-change-group (handle)
>   "Activate a change group made with `prepare-change-group' (which see)."
>   (dolist (elt handle)
>     (with-current-buffer (car elt)
>       (if (eq buffer-undo-list t)
> 	  (setq buffer-undo-list nil)
> 	;; [huge comment here]
>         (when (numberp (caar buffer-undo-list)) ;; <--- here
>           (push (cons (caar buffer-undo-list) (caar buffer-undo-list))
>                 buffer-undo-list))))))
>
> I guess that `caar' should be (car-safe (car buffer-undo-list)) or so -
> right?

Yup!


        Stefan