disabling undo boundaries

disabling undo boundaries

[Phillip Lord]

I am a little confused by undo boundaries and how and when they get set.

Consider this, entirely pointless piece of code.


  (defvar fix-test-on nil)
  (defun fix-test-after-change-function (&rest _)
    (when fix-test-on
      (with-current-buffer
          (get-buffer "*scratch*")
        (insert "a"))))

  (add-hook 'after-change-functions 'fix-test-after-change-function)

Now, If I delete buffer-undo-list in the current buffer (not *scratch*)
then type "abc", I get a buffer-undo-list something like...

(nil (355 . 358))

Now if I set fix-test-on to t and do the same, I get this.

(nil (357 . 358) nil (356 . 357) nil (355 . 356))

I am wondering why the "nil" undo-boundaries get inserted when I change
*a different* buffer. This does not really make sense to me, and is
causing breakage in my package.

I can't for the life of me find out where this nil boundary is getting
added; it appears to happen *after* the after-change-function ends, as I
find by logging buffer-undo-list in my a-c-f, so it's neither the
buffer-change nor the insert itself. Unfortunately, this also means I
cannot fix the problem in the a-c-f.

Any other ideas welcome!

Phil

Re: disabling undo boundaries

[Stefan Monnier]

> I am wondering why the "nil" undo-boundaries get inserted when I change
> *a different* buffer. This does not really make sense to me, and is
> causing breakage in my package.

The 3 separate undo entries you're seeing are actually the "normal"
outcome (one undo-boundary per buffer-modifying command).

self-insert-command has special ad-hoc code to try and merge sequences
of single-char insertions by removing the previous undo-boundary.
Which is why you normally only see a single undo entry.

In your second case, I suspect that this ad-hoc code doesn't get to do
what it's intended to do, because it does not recognize the
undo-boundary that is candidate for removal as being "an undo-boundary
we've auto-inserted".  So, self-insert-command thinks that this
undo-boundary may have been inserted by an explicit call to
`undo-boundary' at the end of the previous command, in which case it
should not be removed.

The relevant code is in the remove_excessive_undo_boundaries function,
in src/cmds.c.


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I am wondering why the "nil" undo-boundaries get inserted when I change
>> *a different* buffer. This does not really make sense to me, and is
>> causing breakage in my package.
>
> The 3 separate undo entries you're seeing are actually the "normal"
> outcome (one undo-boundary per buffer-modifying command).
>
> self-insert-command has special ad-hoc code to try and merge sequences
> of single-char insertions by removing the previous undo-boundary.
> Which is why you normally only see a single undo entry.
>
> In your second case, I suspect that this ad-hoc code doesn't get to do
> what it's intended to do, because it does not recognize the
> undo-boundary that is candidate for removal as being "an undo-boundary
> we've auto-inserted".  So, self-insert-command thinks that this
> undo-boundary may have been inserted by an explicit call to
> `undo-boundary' at the end of the previous command, in which case it
> should not be removed.
>
> The relevant code is in the remove_excessive_undo_boundaries function,
> in src/cmds.c.


Unfortunately, this is a general problem. Try enabling the a-c-f I sent
previously, and then run "fill-paragraph" on an overly long line.


With after-change-hook disabled....

Now is the winter of our discontent made glorious summer by the sun over york, whether it is

M-q

Now is the winter of our discontent made glorious summer by the sun over york,
whether it is

C-/

Now is the winter of our discontent made glorious summer by the sun over york, whether it is


With after-change-hook enabled...

Now is the winter of our discontent made glorious summer by the sun over york, whether it is

M-q

Now is the winter of our discontent made glorious summer by the sun over york,
whether it is

C-/

Now is the winter of our discontent made glorious summer by the sun over york,
 whether it is

C-/

Now is the winter of our discontent made glorious summer by the sun over york,
whether it is

So, I have to do two undo's rather than one.

The problem seems to come from here in record_point, in undo.c


  if ((current_buffer != last_undo_buffer)
      /* Don't call Fundo_boundary for the first change.  Otherwise we
	 risk overwriting last_boundary_position in Fundo_boundary with
	 PT of the current buffer and as a consequence not insert an
	 undo boundary because last_boundary_position will equal pt in
	 the test at the end of the present function (Bug#731).  */
      && (MODIFF > SAVE_MODIFF))
    Fundo_boundary ();
  last_undo_buffer = current_buffer;


The a-c-f call causes an undo-able in scratch which means that
"current-buffer != last_undo_buffer" succeeds, which forces an
Fundo_boundary call on the NEXT call to record_point which is why I
cannot see this boundary on a-c-f.

The best thing that I can think of at the moment is to use
post-command-hook to clean up the excessive undo-boundaries, but I am
not sure how I am going to work out which ones are "real" and which ones
not. The obvious solution (delete all those adding since the last
command) will fail for both self-insert-command's logic and anywhere
else that undo-boundary has been explicitly called.

Better ideas welcome.

Phil

Re: disabling undo boundaries

[Stefan Monnier]

> The problem seems to come from here in record_point, in undo.c

>   if ((current_buffer != last_undo_buffer)
>       /* Don't call Fundo_boundary for the first change.  Otherwise we
> 	 risk overwriting last_boundary_position in Fundo_boundary with
> 	 PT of the current buffer and as a consequence not insert an
> 	 undo boundary because last_boundary_position will equal pt in
> 	 the test at the end of the present function (Bug#731).  */
>       && (MODIFF > SAVE_MODIFF))
>     Fundo_boundary ();
>   last_undo_buffer = current_buffer;

Oh, I think you're right, this is even more likely the reason for
those boundaries.

> The best thing that I can think of at the moment is to use
> post-command-hook to clean up the excessive undo-boundaries, but I am
> not sure how I am going to work out which ones are "real" and which ones
> not.  The obvious solution (delete all those adding since the last
> command) will fail for both self-insert-command's logic and anywhere
> else that undo-boundary has been explicitly called.

Indeed, removing those boundaries after the fact seems
difficult/tricky/risky.

I'm not sure exactly what are your design constraints, but I can see
a few alternatives:
- disable undo in the "other buffer" (*scratch* in your example, tho
  I suspect it's a different buffer in your real case).
- delay the modification of the other buffer (e.g. record in a-c-f the
  boundaries of the affected text, and process them later from
  post-command-hook).  Not sure if this would really help.
- change your a-c-f so it records the buffer-undo-list at the beginning,
  and it removes the added boundary (if any) at the end.


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> The problem seems to come from here in record_point, in undo.c
>
>>   if ((current_buffer != last_undo_buffer)
>>       /* Don't call Fundo_boundary for the first change.  Otherwise we
>> 	 risk overwriting last_boundary_position in Fundo_boundary with
>> 	 PT of the current buffer and as a consequence not insert an
>> 	 undo boundary because last_boundary_position will equal pt in
>> 	 the test at the end of the present function (Bug#731).  */
>>       && (MODIFF > SAVE_MODIFF))
>>     Fundo_boundary ();
>>   last_undo_buffer = current_buffer;
>
> Oh, I think you're right, this is even more likely the reason for
> those boundaries.

I'm getting slower better at reading Emacs source code:-) I really need
to bite the bullet and learn C.


>> The best thing that I can think of at the moment is to use
>> post-command-hook to clean up the excessive undo-boundaries, but I am
>> not sure how I am going to work out which ones are "real" and which ones
>> not.  The obvious solution (delete all those adding since the last
>> command) will fail for both self-insert-command's logic and anywhere
>> else that undo-boundary has been explicitly called.
>
> Indeed, removing those boundaries after the fact seems
> difficult/tricky/risky.
>
> I'm not sure exactly what are your design constraints,

I am trying to address a bug in lentic (available in all good
repositories!). Lentic picks up the change in one buffer on the a-c-f
and percolates it too another (or potentially any number of others).


> but I can see a few alternatives: - disable undo in the "other buffer"
> (*scratch* in your example, tho I suspect it's a different buffer in
> your real case).


This fails for me, unfortunately, the user might want to undo in the
other buffer. A priori, though, it seems to be a nice idea. I tried this
code for instance:


#+BEGIN_SRC emacs-lisp
  (defvar-local fix-test-on nil)
  (defun fix-test-after-change-function (&rest _)
    (when fix-test-on
      (let (
            (undo-inhibit-record-point t)
            )
        (with-current-buffer
            (get-buffer "*scratch*")
          (insert "a")))))

  (add-hook 'after-change-functions 'fix-test-after-change-function)
#+END_SRC

And that doesn't have the problem because the inhibit stops record_point
from ever setting the undo_boundary.


> - delay the modification of the other buffer (e.g. record in a-c-f the
>   boundaries of the affected text, and process them later from
>   post-command-hook).  Not sure if this would really help.

Unfortunately that wont work since there there can be many changes on
a-c-f for a single p-c-h call. So, I'd have to amalgamate all the
changes.


> - change your a-c-f so it records the buffer-undo-list at the beginning,
>   and it removes the added boundary (if any) at the end.

The a-c-f doesn't work, unfortunately because the nil boundary is not
present till sometime after the a-c-f has been called (when? not sure).

I did think of doing this with the pre/post-command-hook -- so, take the
car of the b-u-l on the pre-c-h, then on the post-c-h, scan till I get
to the old car and delete all nils placed there.

I can see a few problems with this. First, it will also remove any
undo-boundaries added by explict calls to undo-boundary; perhaps not a
huge problem as there do not appear to be that many of them in the lisp
code base. Second, it's still not going to work for self-insert-command
because each of these are independent commands. Third, I seem to be
reimplementing the undo system which feels ugly.

Still, at the moment, this appears to be my only way forward.

Phil

Re: disabling undo boundaries

[Stefan Monnier]

>> but I can see a few alternatives: - disable undo in the "other buffer"
>> (*scratch* in your example, tho I suspect it's a different buffer in
>> your real case).
> This fails for me, unfortunately, the user might want to undo in the
> other buffer. A priori, though, it seems to be a nice idea. I tried this
> code for instance:
> #+BEGIN_SRC emacs-lisp
>   (defvar-local fix-test-on nil)
>   (defun fix-test-after-change-function (&rest _)
>     (when fix-test-on
>       (let (
>             (undo-inhibit-record-point t)
>             )
>         (with-current-buffer
>             (get-buffer "*scratch*")
>           (insert "a")))))
>   (add-hook 'after-change-functions 'fix-test-after-change-function)
> #+END_SRC
> And that doesn't have the problem because the inhibit stops record_point
> from ever setting the undo_boundary.

So, you're saying that the above let binding fixes your problem?
Then, I guess it's a valid option as well.  It sounds like an obscure
work-around, so it probably deserves a clear comment.

>> - delay the modification of the other buffer (e.g. record in a-c-f the
>> boundaries of the affected text, and process them later from
>> post-command-hook).  Not sure if this would really help.
> Unfortunately that wont work since there there can be many changes on
> a-c-f for a single p-c-h call. So, I'd have to amalgamate all the
> changes.

Amalgamate is exactly what I meant by "record the boundaries".
It's a fairly common approach, typically you only track the beg/end
enclosing boundaries.

E.g. I use it in diff-mode to avoid modifying the buffer right in the
middle of a-c-f, which tends to break all kinds of assumptions.

>> - change your a-c-f so it records the buffer-undo-list at the beginning,
>> and it removes the added boundary (if any) at the end.
> The a-c-f doesn't work, unfortunately because the nil boundary is not
> present till sometime after the a-c-f has been called (when? not sure).

That is weird.  The code you quoted from record_point should be run
"right away".  Are you sure you didn't check in the wrong buffer by
any chance?

I'd expect something like

   (let ((old-ul buffer-undo-list))
     (with-current-buffer "*scratch*"
       <doyourthing>)
     (and (consp buffer-undo-list)
          (eq (cdr buffer-undo-list) old-ul)
          (null (car buffer-undo-list))
          (setq buffer-undo-list (cdr buffer-undo-list))))

to do the trick.  Or, what am I missing?

> I did think of doing this with the pre/post-command-hook -- so, take the
> car of the b-u-l on the pre-c-h, then on the post-c-h, scan till I get
> to the old car and delete all nils placed there.

But at that point you won't know which undo-boundaries were added
because of the buffer-switch and which were added for other reasons.

> I can see a few problems with this. First, it will also remove any
> undo-boundaries added by explict calls to undo-boundary; perhaps not a
> huge problem as there do not appear to be that many of them in the lisp
> code base.

Since `doyourthing' shouldn't touch the source buffer, the source
buffer's undo-list has no reason to have a hand-added undo-boundary.


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> This fails for me, unfortunately, the user might want to undo in the
>> other buffer. A priori, though, it seems to be a nice idea. I tried this
>> code for instance:
>> #+BEGIN_SRC emacs-lisp
>>   (defvar-local fix-test-on nil)
>>   (defun fix-test-after-change-function (&rest _)
>>     (when fix-test-on
>>       (let (
>>             (undo-inhibit-record-point t)
>>             )
>>         (with-current-buffer
>>             (get-buffer "*scratch*")
>>           (insert "a")))))
>>   (add-hook 'after-change-functions 'fix-test-after-change-function)
>> #+END_SRC
>> And that doesn't have the problem because the inhibit stops record_point
>> from ever setting the undo_boundary.
>
> So, you're saying that the above let binding fixes your problem?
> Then, I guess it's a valid option as well.  It sounds like an obscure
> work-around, so it probably deserves a clear comment.

Sort of. In this particular instance above, it does because
inhibit-record-point stops both the recording of point AND the insertion
of an undo boundary.



>>> - delay the modification of the other buffer (e.g. record in a-c-f the
>>> boundaries of the affected text, and process them later from
>>> post-command-hook).  Not sure if this would really help.
>> Unfortunately that wont work since there there can be many changes on
>> a-c-f for a single p-c-h call. So, I'd have to amalgamate all the
>> changes.
>
> Amalgamate is exactly what I meant by "record the boundaries".
> It's a fairly common approach, typically you only track the beg/end
> enclosing boundaries.

It's difficult in my case. The problem is I have two buffers which I am
keeping in sync. So I need to be able to convert a point location in one
with a point location in another. And I have to do this *before* the
change happens because otherwise the two buffers will hold content which
is not in sync (one has changed, the other hasn't been updated yet).

Unfortunately, the scope of the changes reported by the b-c-f and a-c-f
are not necessarily in agreement with each other -- subst-char-in-region
often gets the before-change region wrong. I have some logic for
detecting this, but it can be fouled by changes that both add and remove
from the buffer at the same time; in these cases, I have to copy the
entire buffer. Amalgamating many changes makes this much more likely to
happen.


>
> E.g. I use it in diff-mode to avoid modifying the buffer right in the
> middle of a-c-f, which tends to break all kinds of assumptions.
>
>>> - change your a-c-f so it records the buffer-undo-list at the beginning,
>>> and it removes the added boundary (if any) at the end.
>> The a-c-f doesn't work, unfortunately because the nil boundary is not
>> present till sometime after the a-c-f has been called (when? not sure).
>
> That is weird.  The code you quoted from record_point should be run
> "right away".  Are you sure you didn't check in the wrong buffer by
> any chance?
>
> I'd expect something like
>
>    (let ((old-ul buffer-undo-list))
>      (with-current-buffer "*scratch*"
>        <doyourthing>)
>      (and (consp buffer-undo-list)
>           (eq (cdr buffer-undo-list) old-ul)
>           (null (car buffer-undo-list))
>           (setq buffer-undo-list (cdr buffer-undo-list))))
>
> to do the trick.  Or, what am I missing?


It's rather more sinister than that. The code is this:

  if ((current_buffer != last_undo_buffer)
         && (MODIFF > SAVE_MODIFF))
     Fundo_boundary ();

  last_undo_buffer = current_buffer;

So, when the after-change-function runs the buffer-undo-list has
*already* been modified. But if the a-c-f function makes an undoable
change, then last_undo_buffer gets reset to the other buffer (*scratch*
in my case).

Now, the *next* change happens, and current_buffer != last_undo_buffer
is true, so we add a boundary BEFORE, we add the next undo. So, the
buffer change in a-c-f adds a boundary before the next change not after
the one that has just been added.


>> I did think of doing this with the pre/post-command-hook -- so, take the
>> car of the b-u-l on the pre-c-h, then on the post-c-h, scan till I get
>> to the old car and delete all nils placed there.
>
> But at that point you won't know which undo-boundaries were added
> because of the buffer-switch and which were added for other reasons.

Yep, that's a problem.

>
>> I can see a few problems with this. First, it will also remove any
>> undo-boundaries added by explict calls to undo-boundary; perhaps not a
>> huge problem as there do not appear to be that many of them in the lisp
>> code base.
>
> Since `doyourthing' shouldn't touch the source buffer, the source
> buffer's undo-list has no reason to have a hand-added undo-boundary.

In between the start and end of a command? That can happen. It's the
point of making undo-boundary a lisp callable function.

I had an evil thought on the way home. Rather than working backwards and
trying to remove the undo-boundary that had been added as a result of
the undo action in another buffer, why not just remove that logic from
undo.c? So, I've tried commenting out the four calls to Fundo_boundary
there and now everything works.

I haven't used this for my daily emacs yet, so perhaps there are demons
lurking, but my question would be, why should a undoable change in
*that* buffer cause a marker boundary in *this*? What is the point of
these undo-boundaries that are causing me all this grief?

Phil

Re: disabling undo boundaries

[Stefan Monnier]

[ FWIW, I'm not sure amalgamating would actually help solve this
  problem.  ]

> It's difficult in my case. The problem is I have two buffers which I am
> keeping in sync. So I need to be able to convert a point location in one
> with a point location in another. And I have to do this *before* the
> change happens because otherwise the two buffers will hold content which
> is not in sync (one has changed, the other hasn't been updated yet).

Shouldn't it be sufficient to keep track of the lower bound of the
changes, the higher bound of the changes, and the number of chars
added/removed?
Oh, wait I think I now remember that your two buffers aren't necessarily
identical, in which case indeed it's ...hmm... difficult.

> It's rather more sinister than that. The code is this:

>   if ((current_buffer != last_undo_buffer)
>          && (MODIFF > SAVE_MODIFF))
>      Fundo_boundary ();

>   last_undo_buffer = current_buffer;

> So, when the after-change-function runs the buffer-undo-list has
> *already* been modified. But if the a-c-f function makes an undoable
> change, then last_undo_buffer gets reset to the other buffer (*scratch*
> in my case).

> Now, the *next* change happens, and current_buffer != last_undo_buffer
> is true, so we add a boundary BEFORE, we add the next undo. So, the
> buffer change in a-c-f adds a boundary before the next change not after
> the one that has just been added.

Hmm... so, IIUC the changes in *scratch* done from a-f-c cause the next
modification in the main buffer to have an undo-boundary prepended
because when the new command is run, last_undo_buffer is *scratch*.

Then maybe you could try a hack like:

   (with-current-buffer "*scratch*"
     <doyourthing>)
   (let ((buffer-undo-list))
     (undo-boundary))           ; Set last_undo_buffer.

>> Since `doyourthing' shouldn't touch the source buffer, the source
>> buffer's undo-list has no reason to have a hand-added undo-boundary.
> In between the start and end of a command?

No: during your a-f-c.

> I haven't used this for my daily emacs yet, so perhaps there are demons
> lurking, but my question would be, why should a undoable change in
> *that* buffer cause a marker boundary in *this*? What is the point of
> these undo-boundaries that are causing me all this grief?

It's a heuristic,


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> Shouldn't it be sufficient to keep track of the lower bound of the
> changes, the higher bound of the changes, and the number of chars
> added/removed?
> Oh, wait I think I now remember that your two buffers aren't necessarily
> identical, in which case indeed it's ...hmm... difficult.


Yes, that's the problem. Also, it's not ideal for large changes which
span the buffer.

>
>> It's rather more sinister than that. The code is this:
>
>>   if ((current_buffer != last_undo_buffer)
>>          && (MODIFF > SAVE_MODIFF))
>>      Fundo_boundary ();
>
>>   last_undo_buffer = current_buffer;
>
>> So, when the after-change-function runs the buffer-undo-list has
>> *already* been modified. But if the a-c-f function makes an undoable
>> change, then last_undo_buffer gets reset to the other buffer (*scratch*
>> in my case).
>
>> Now, the *next* change happens, and current_buffer != last_undo_buffer
>> is true, so we add a boundary BEFORE, we add the next undo. So, the
>> buffer change in a-c-f adds a boundary before the next change not after
>> the one that has just been added.
>
> Hmm... so, IIUC the changes in *scratch* done from a-f-c cause the next
> modification in the main buffer to have an undo-boundary prepended
> because when the new command is run, last_undo_buffer is *scratch*.

Yep, that's how I read it.


> Then maybe you could try a hack like:
>
>    (with-current-buffer "*scratch*"
>      <doyourthing>)
>    (let ((buffer-undo-list))
>      (undo-boundary))           ; Set last_undo_buffer.


It doesn't unfortunately. An undo-boundary is not actually an undoable
action in that sense. I tried:

(save-excursion (goto-char (point-min)))

but AFAICT that fails also. I guess save-excursion is clever enough to
remove last_undo_buffer also.


>> I haven't used this for my daily emacs yet, so perhaps there are demons
>> lurking, but my question would be, why should a undoable change in
>> *that* buffer cause a marker boundary in *this*? What is the point of
>> these undo-boundaries that are causing me all this grief?
>
> It's a heuristic,


It is, and one that has been in Emacs for a long time. How wedded to
keeping it are you? Would it be possible to optionalize?

Phil

Re: disabling undo boundaries

[Stefan Monnier]

>> Then maybe you could try a hack like:
>> 
>> (with-current-buffer "*scratch*"
>>   <doyourthing>)
>> (let ((buffer-undo-list))
>>   (undo-boundary))           ; Set last_undo_buffer.
> It doesn't unfortunately.

Oh, indeed when buffer-undo-list, undo-boundary is a no-op, so you need

   (with-current-buffer "*scratch*"
     <doyourthing>)
   (let ((buffer-undo-list '("")))
     (undo-boundary))           ; Set last_undo_buffer.

> It is, and one that has been in Emacs for a long time.  How wedded to
> keeping it are you? Would it be possible to optionalize?

Not sure,


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Then maybe you could try a hack like:
>>> 
>>> (with-current-buffer "*scratch*"
>>>   <doyourthing>)
>>> (let ((buffer-undo-list))
>>>   (undo-boundary))           ; Set last_undo_buffer.
>> It doesn't unfortunately.
>
> Oh, indeed when buffer-undo-list, undo-boundary is a no-op, so you need
>
>    (with-current-buffer "*scratch*"
>      <doyourthing>)
>    (let ((buffer-undo-list '("")))
>      (undo-boundary))           ; Set last_undo_buffer.

My fault for not being clear. AFAICT, undo-boundary does not set
last_undo_buffer at all. I can't directly test this because I can't get
to last_undo_buffer from lisp, but here is my test code.

#+BEGIN_SRC emacs-lisp
  (defvar-local fix-test-on nil)
  (defvar fix-test-fudge t)
  (defun fix-test-after-change-function (&rest _)
      (when fix-test-on
        (with-current-buffer
            (get-buffer "*scratch*")
          (insert "a"))
        (when fix-test-fudge
          (let ((buffer-undo-list '("")))
            (undo-boundary)))))

  (add-hook 'after-change-functions 'fix-test-after-change-function)
#+END_SRC

Setting `fix-test-on' to t still affects undo in the current-buffer.


>> It is, and one that has been in Emacs for a long time.  How wedded to
>> keeping it are you? Would it be possible to optionalize?
>
> Not sure,

Well I have a fork running without it (with some brutal commenting out),
so I'll try running that as my daily emacs, and see if it crashes and
how it behaves.

I'm still trying to understand the reason behind the logic in the first
place; unfortunately, the code seems to predate the earliest VC records.

Thanks for the feedback, it's been as helpful as always.

Phil

Re: disabling undo boundaries

[Stefan Monnier]

> My fault for not being clear.  AFAICT, undo-boundary does not set
> last_undo_buffer at all.

Oh, indeed, it sets last_boundary_buffer but not last_undo_buffer!
Duh!

> I'm still trying to understand the reason behind the logic in the first
> place; unfortunately, the code seems to predate the earliest VC records.

IIUC the reason is for modifications in another buffer that take place
either within a command or between commands (e.g. process output), so
they don't accumulate arbitrarily within a single undo-boundary.

E.g. if you have a command "add timestamp to log buffer" which you
typically use from another buffer then, without this logic, running this
command 100 times would result in all entries in the buffer being lumped
together into a single undo-unit, so you couldn't undo them separately.


        Stefab

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> My fault for not being clear.  AFAICT, undo-boundary does not set
>> last_undo_buffer at all.
>
> Oh, indeed, it sets last_boundary_buffer but not last_undo_buffer!
> Duh!
>
>> I'm still trying to understand the reason behind the logic in the first
>> place; unfortunately, the code seems to predate the earliest VC records.
>
> IIUC the reason is for modifications in another buffer that take place
> either within a command or between commands (e.g. process output), so
> they don't accumulate arbitrarily within a single undo-boundary.
>
> E.g. if you have a command "add timestamp to log buffer" which you
> typically use from another buffer then, without this logic, running this
> command 100 times would result in all entries in the buffer being lumped
> together into a single undo-unit, so you couldn't undo them separately.


In the other buffer? So, you can undo the changes to the log buffer one
after the other? I agree that makes sense, but it does seem to me that
this could be the responsibility of "add timestamp to log buffer".

I'm checking at the moment how often this undo-boundary gets called. So
far, it's about 1.5% of the time that an undo event gets added, so it's
not very often (obviously this is without an a-c-f entry or it would be
much higher!).

Phil

Re: disabling undo boundaries

[Stefan Monnier]

> In the other buffer? So, you can undo the changes to the log buffer one
> after the other? I agree that makes sense, but it does seem to me that
> this could be the responsibility of "add timestamp to log buffer".

Right.

> I'm checking at the moment how often this undo-boundary gets called. So
> far, it's about 1.5% of the time that an undo event gets added, so it's
> not very often (obviously this is without an a-c-f entry or it would be
> much higher!).

My guess is that the vast majority of the cases where this code is
triggered is for process output (inserted in a buffer different from
the one that's current).


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> In the other buffer? So, you can undo the changes to the log buffer one
>> after the other? I agree that makes sense, but it does seem to me that
>> this could be the responsibility of "add timestamp to log buffer".
>
> Right.
>
>> I'm checking at the moment how often this undo-boundary gets called. So
>> far, it's about 1.5% of the time that an undo event gets added, so it's
>> not very often (obviously this is without an a-c-f entry or it would be
>> much higher!).
>
> My guess is that the vast majority of the cases where this code is
> triggered is for process output (inserted in a buffer different from
> the one that's current).

That doesn't really make sense to me. Say I am editing a file in this
buffer while compiling something in that buffer. Why would I want an
update in the compilation buffer to force an undo-boundary in this buffer?

I guess I could try and print this-command to see what is happening when
the condition comes into affect.

Sorry to be persistant about this, but at the moment, changing the code
in undo.c is the only good solution I can see to my problem. It would be
different if explicitally called and automatic boundaries were
distinguishable, but they aren't.

Phil

Re: disabling undo boundaries

[Stefan Monnier]

> That doesn't really make sense to me. Say I am editing a file in this
> buffer while compiling something in that buffer. Why would I want an
> update in the compilation buffer to force an undo-boundary in this buffer?

It probably won't make any difference in "this buffer" because the
process filter will be run *between* commands (at which point the
read-eval command loop already inserts undo-boundaries anyway).

The difference is in the compilation buffer where it'll insert undo
boundaries every time you run a command in "this buffer".

> Sorry to be persistant about this, but at the moment, changing the code
> in undo.c is the only good solution I can see to my problem.  It would be
> different if explicitally called and automatic boundaries were
> distinguishable, but they aren't.

We could make them distinguishable, OTOH (e.g. using a (weak) hash-table where
we insert every explicitly added undo-boundary).


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> That doesn't really make sense to me. Say I am editing a file in this
>> buffer while compiling something in that buffer. Why would I want an
>> update in the compilation buffer to force an undo-boundary in this buffer?
>
> It probably won't make any difference in "this buffer" because the
> process filter will be run *between* commands (at which point the
> read-eval command loop already inserts undo-boundaries anyway).
>
> The difference is in the compilation buffer where it'll insert undo
> boundaries every time you run a command in "this buffer".

Yes, I can see that. Although iff this is the reason for the
undo-boundary, it would still make more sense to me to have the process
code insert this. Aside from being a more discrete effect, it would also
avoid the current "do nothing now, but insert an undo-boundary before
the next change where ever that is" semantics.

>
>> Sorry to be persistant about this, but at the moment, changing the code
>> in undo.c is the only good solution I can see to my problem.  It would be
>> different if explicitally called and automatic boundaries were
>> distinguishable, but they aren't.
>
> We could make them distinguishable, OTOH (e.g. using a (weak) hash-table where
> we insert every explicitly added undo-boundary).

I don't understand how that would that work. undo-boundaries are nil, so
surely they are all the same object?

Using symbols like:

'(boundary . user)
'(boundary . internal)

would enable this, I think. But it would break code like this:

  (while (or (null (car buffer-undo-list))
	     (and discard-pos (integerp (car buffer-undo-list))))
    (setq buffer-undo-list (cdr buffer-undo-list)))

from undo-tree, and I would guess quite a lot of other places.


I have thought of a partial solution -- which is to add a symbol to
b-u-l in the pre-command-hook, the delete all the boundaries added since
that in the post-command-hook. That gets me to a clean "boundary at
every command" semantics. I couldn't work out how to do this before,
again, because boundaries are all the same object.


Phil

Re: disabling undo boundaries

[Stefan Monnier]

>>> That doesn't really make sense to me. Say I am editing a file in this
>>> buffer while compiling something in that buffer. Why would I want an
>>> update in the compilation buffer to force an undo-boundary in this buffer?
>> It probably won't make any difference in "this buffer" because the
>> process filter will be run *between* commands (at which point the
>> read-eval command loop already inserts undo-boundaries anyway).
>> The difference is in the compilation buffer where it'll insert undo
>> boundaries every time you run a command in "this buffer".
> Yes, I can see that. Although iff this is the reason for the
> undo-boundary, it would still make more sense to me to have the process
> code insert this. Aside from being a more discrete effect, it would also
> avoid the current "do nothing now, but insert an undo-boundary before
> the next change where ever that is" semantics.

I don't claim it's the best solution, indeed.  It's just that if we
remove it, we'll have to replace it with other mechanisms.
So we need a clear description of the current cases that are considered
misbehaviors and those that are considered good (tho not necessarily
perfect).

> I don't understand how that would that work. undo-boundaries are nil, so
> surely they are all the same object?

No, we'd record put the cons cell in which the nil is placed.


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Yes, I can see that. Although iff this is the reason for the
>> undo-boundary, it would still make more sense to me to have the process
>> code insert this. Aside from being a more discrete effect, it would also
>> avoid the current "do nothing now, but insert an undo-boundary before
>> the next change where ever that is" semantics.
>
> I don't claim it's the best solution, indeed.  It's just that if we
> remove it, we'll have to replace it with other mechanisms.
> So we need a clear description of the current cases that are considered
> misbehaviors and those that are considered good (tho not necessarily
> perfect).

I entirely agree with this, of course. This is why I am trying to find
positive behaviour.

So, I have tested this, and indeed commenting out that code does affect
undo behaviour. To test this, I wrote a "countdown" script which, well,
counts down, and then ran this in *shell*. Then I typed into *scratch*
at defined points. With the undo-boundary code, indeed, the typing in
*scratch* does add boundaries -- so that a countdown from 100 say, gets
split up into multiple undos. Without this code, the countdown gets
undone in one go.

So, while it clearly has an effect, I am not sure why this is better.
For example, if I launch two shell buffers, then run "countdown 20" in
both at the same time, then both buffers now undo in 20 steps, one
second at a time, because their output interleaves. Again, the "add an
undo boundary because of a change in another buffer" semantics does not
seem intuitive to me.


>> I don't understand how that would that work. undo-boundaries are nil, so
>> surely they are all the same object?
>
> No, we'd record put the cons cell in which the nil is placed.

Oh, yes, of course. That does assume, though, that any operations on
buffer-undo-list are destructive. For instance, if I filter the
buffer-undo-list with seq.el or dash, do I not get all new cons cells?
Still, most of the undo.c code seems to change in place, so I guess it
would work in most cases.

Phil

Re: disabling undo boundaries

[Phillip Lord]

Phillip Lord <phillip.lord@newcastle.ac.uk> writes:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>> I don't claim it's the best solution, indeed.  It's just that if we
>> remove it, we'll have to replace it with other mechanisms.
>> So we need a clear description of the current cases that are considered
>> misbehaviors and those that are considered good (tho not necessarily
>> perfect).
>
> I entirely agree with this, of course. This is why I am trying to find
> positive behaviour.
>
> So, I have tested this, and indeed commenting out that code does affect
> undo behaviour. To test this, I wrote a "countdown" script which, well,
> counts down, and then ran this in *shell*. Then I typed into *scratch*
> at defined points. With the undo-boundary code, indeed, the typing in
> *scratch* does add boundaries -- so that a countdown from 100 say, gets
> split up into multiple undos. Without this code, the countdown gets
> undone in one go.
>
> So, while it clearly has an effect, I am not sure why this is better.
> For example, if I launch two shell buffers, then run "countdown 20" in
> both at the same time, then both buffers now undo in 20 steps, one
> second at a time, because their output interleaves. Again, the "add an
> undo boundary because of a change in another buffer" semantics does not
> seem intuitive to me.


Just wanted to bump this. I am still not sure why this is a good
behaviour. My own feeling is that the "undo-boundary when a buffer
change happens" logic may not have been implemented for this reason, but
as a belts-and-braces way of detecting the end of a command -- after
all, buffer changes (followed by an undoable event) normally happen
between commands rather than within.



>>> I don't understand how that would that work. undo-boundaries are nil, so
>>> surely they are all the same object?
>>
>> No, we'd record put the cons cell in which the nil is placed.
>
> Oh, yes, of course. That does assume, though, that any operations on
> buffer-undo-list are destructive. For instance, if I filter the
> buffer-undo-list with seq.el or dash, do I not get all new cons cells?
> Still, most of the undo.c code seems to change in place, so I guess it
> would work in most cases.

I did start working on this, but haven't got it functional yet. One
problem is that if I record the cons cell at the head of
buffer-undo-list, I have to search the whole buffer-undo-list to see if
it is there (unless I get a match). A cons cell with a nil gets added
for every self-insert-command and then removed again, so this tends to
happen quite a lot.

Phil

Re: disabling undo boundaries

[Stefan Monnier]

>> So, while it clearly has an effect, I am not sure why this is better.
>> For example, if I launch two shell buffers, then run "countdown 20" in
>> both at the same time, then both buffers now undo in 20 steps, one
>> second at a time, because their output interleaves. Again, the "add an
>> undo boundary because of a change in another buffer" semantics does not
>> seem intuitive to me.

Agreed.

> Just wanted to bump this. I am still not sure why this is a good
> behaviour. My own feeling is that the "undo-boundary when a buffer
> change happens" logic may not have been implemented for this reason, but
> as a belts-and-braces way of detecting the end of a command -- after
> all, buffer changes (followed by an undoable event) normally happen
> between commands rather than within.

For process filters, I think the "right way" would be to add an
undo-boundary every N insertions.

> I did start working on this, but haven't got it functional yet. One
> problem is that if I record the cons cell at the head of
> buffer-undo-list, I have to search the whole buffer-undo-list to see if
> it is there (unless I get a match). A cons cell with a nil gets added
> for every self-insert-command and then removed again, so this tends to
> happen quite a lot.

Sounds rather ugly.  Let's see how far we can go with the option of
changing the current behavior rather than trying to add workarounds in
your code.

Could you describe the exact case that bothers you, so we could start by
thinking what should be the ideal behavior for that one?


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Just wanted to bump this. I am still not sure why this is a good
>> behaviour. My own feeling is that the "undo-boundary when a buffer
>> change happens" logic may not have been implemented for this reason, but
>> as a belts-and-braces way of detecting the end of a command -- after
>> all, buffer changes (followed by an undoable event) normally happen
>> between commands rather than within.
>
> For process filters, I think the "right way" would be to add an
> undo-boundary every N insertions.

"undo" for a process filter seems to be quite a difficult one to me. In
many cases ("M-x compile") undo is just switched off. For things like
M-x shell undo is on and makes sense. My initial guess would be that
"undo the last command and all the output from that command" would seem
the most intuitive.


>> I did start working on this, but haven't got it functional yet. One
>> problem is that if I record the cons cell at the head of
>> buffer-undo-list, I have to search the whole buffer-undo-list to see if
>> it is there (unless I get a match). A cons cell with a nil gets added
>> for every self-insert-command and then removed again, so this tends to
>> happen quite a lot.
>
> Sounds rather ugly.  Let's see how far we can go with the option of
> changing the current behavior rather than trying to add workarounds in
> your code.

Agreed. I'd rather fix the core Emacs behaviour than workaround it, so
it works better for everyone. Of course, I have to acknowledge that my
public spirited behaviour is partly motivated by the workaround being a
PITA.

> Could you describe the exact case that bothers you, so we could start by
> thinking what should be the ideal behavior for that one?


Yes. Any after-change-function that changes another buffer breaks undo.

My "noisy-change" package which just logs the before and
after-change-function args is an example of this.

https://raw.githubusercontent.com/phillord/lentic/master/noisy-change.el

My real use case is lentic.

Ideal behaviour: is just not to do it. Uncommenting all the
"undo_boundary" calls in undo.c seems to achieve this for me, although
obviously that's a blunt fix.

Phil

Re: disabling undo boundaries

[Stefan Monnier]

>> Could you describe the exact case that bothers you, so we could start by
>> thinking what should be the ideal behavior for that one?
> Yes. Any after-change-function that changes another buffer breaks undo.
> My "noisy-change" package which just logs the before and
> after-change-function args is an example of this.
> https://raw.githubusercontent.com/phillord/lentic/master/noisy-change.el
> My real use case is lentic.
> Ideal behaviour: is just not to do it. Uncommenting all the
> "undo_boundary" calls in undo.c seems to achieve this for me, although
> obviously that's a blunt fix.

Could you be more specific.  Give a very concrete example of a sequence
of commands, and then explain which the undo-boundaries end up being and
what they should be instead.


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Could you describe the exact case that bothers you, so we could start by
>>> thinking what should be the ideal behavior for that one?
>> Yes. Any after-change-function that changes another buffer breaks undo.
>> My "noisy-change" package which just logs the before and
>> after-change-function args is an example of this.
>> https://raw.githubusercontent.com/phillord/lentic/master/noisy-change.el
>> My real use case is lentic.
>> Ideal behaviour: is just not to do it. Uncommenting all the
>> "undo_boundary" calls in undo.c seems to achieve this for me, although
>> obviously that's a blunt fix.
>
> Could you be more specific.  Give a very concrete example of a sequence
> of commands, and then explain which the undo-boundaries end up being and
> what they should be instead.


Yes, okay.

So, in *scratch* I remove comments from the second and third line and
put everything onto one line.

;; This buffer is for notes you don't want to save, and for Lisp evaluation. If you want to create a file, visit that file with C-x C-f, then enter the text in that file's own buffer.


I load noisy-change.el (but not enable it).

(setq buffer-undo-list nil)
M-x fill-paragraph (with point on the one line)

(nil
 (156 . 159)
 (#(" " 0 1
    (fontified nil face
               (whitespace-line font-lock-comment-face)))
  . -156)
 (nil face nil 155 . 156)
 (155 . 156)
 (78 . 81)
 (#(" " 0 1
    (fontified nil face font-lock-comment-face))
  . -78)
 (#<marker
  (moves after insertion)
  at 81 in *scratch*> . 1)
 (#<marker
  (moves after insertion)
  at 81 in *scratch*> . 1)
 (#<marker
  (moves after insertion)
  at 81 in *scratch*> . 1)
 (#<marker
  (moves after insertion)
  at 81 in *scratch*> . 1)
 (nil face nil 77 . 78)
 (77 . 78))

M-x undo
(setq noisy-change-undo t)
(setq buffer-undo-list nil)

M-x fill-paragraph

(nil
 (156 . 159)
 nil
 (#(" " 0 1
    (fontified nil face
               (whitespace-line font-lock-comment-face)))
  . -156)
 nil
 (nil face nil 155 . 156)
 nil
 (155 . 156)
 nil
 (78 . 81)
 nil
 (#(" " 0 1
    (fontified nil face font-lock-comment-face))
  . -78)
 nil
 (nil face nil 77 . 78)
 nil
 (77 . 78))

At my count it takes 8 undo operations to undo "fill-paragraph".
Obviously, a longer paragraph will take more operations.

Phil

Re: disabling undo boundaries

[Stefan Monnier]

> M-x fill-paragraph
[...]
> At my count it takes 8 undo operations to undo "fill-paragraph".

Good example, thank you.  But IIUC just removing the "undo-boundary on
buffer-change" code still wouldn't give you the perfect behavior,
because you still do want an undo-boundary in the second buffer, right?


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> M-x fill-paragraph
> [...]
>> At my count it takes 8 undo operations to undo "fill-paragraph".
>
> Good example, thank you.  But IIUC just removing the "undo-boundary on
> buffer-change" code still wouldn't give you the perfect behavior,
> because you still do want an undo-boundary in the second buffer, right?



Yes, you are right, the second buffer is an issue, although I am not
convinced that the behaviour is worse. Below I have described two tasks
both of which change another buffer on a-c-f. I'm comparing between an
Emacs 24.5 (as released, but built from git by me) and my own Emacs
(based on 24.4 actually) with the relevant sections of undo.c commented
out. Again, built by me.

With the existing logic I get a lot of undo boundaries. Without the
existing logic I get a sane number of boundaries. I do not get the same
undo-boundaries that I would have got in the other buffer if I had made
changes by typing commands rather than using a-c-f. But I could add
these programmatically with as much or as little fidelity as I choose.

The data I make these conclusions on is below.

Phil


* Noisy-change

** Task

Type "abcdef" into *scratch* with (setq noisy-change-log t), then check
buffer-undo-list in the created noisy-change-log buffer.

I pick this task because it's about as simple as it gets.


** With existing undo-boundary code

(nil
 (402 . 439)
 nil
 (329 . 402)
 nil
 (256 . 329)
 nil
 (183 . 256)
 nil
 (110 . 183)
 nil
 (37 . 110)
 nil
 (1 . 37)
 (t . 0))


** Without existing undo-boundary logic

(nil (1 . 439) (t . 0))


** Conclusion

The amalgamation of several events is working.


* lentic-view

** Task

Use "Create Lentic View" (this creates an exact clone, rather like an
indirect-buffer).

** With existing undo-boundary logic

(nil
 (197 . 198)
 nil
 (196 . 197)
 nil
 (195 . 196)
 nil
 (194 . 195)
 nil
 (193 . 194)
 nil
 (192 . 193))


** Without existing undo-boundary logic

 (nil (192 . 198))


** Conclusion

Same as last, but with a simpler set of events.


* Task 3

Use lentic-view, type in the entire alphabet

** Without existing undo-boundary logig

(nil (192 . 217) nil (1 . 192) (t . 0))


** Conclusion

The "undo-boundary every 20 self-insert-command"s logic is not working
or it should be something like this...

(nil (212 . 218) nil (192 . 212))

I could add this logic in, although it's a bit of pain and requires
effectively duplicating logic from cmds.c in lisp.


* Task 4

Using lentic view, type in 

abcdefghijk


abcdefghijk

** In *scratch*

(nil (208 . 222) nil (206 . 208) nil (192 . 206))

** In "*lentic: scratch*"

 (nil (192 . 222) nil (1 . 193) (t . 0))


** Conclusion

All of the insertion events have been amalgamated, so the undo is much too
"lumpy".

Again, I can add this logic in, but I have to duplicate the "add an undo-boundary after
a command" logic from the command loop.

Re: disabling undo boundaries

[Stefan Monnier]

> Yes, you are right, the second buffer is an issue, although I am not
> convinced that the behaviour is worse.

Worse than the ideal or worse than the current one?
Right now, I'm trying to understand what the ideal behavior should be.

After that, we can try to figure out how to get this ideal behavior, or
how close we can get to it, and which part of the work can be made
generic in undo.c and which part of the work has to be done manually by
each and every particular command/package.

> ** Conclusion
> All of the insertion events have been amalgamated, so the undo is much
> too "lumpy".  Again, I can add this logic in, but I have to duplicate
> the "add an undo-boundary after a command" logic from the
> command loop.

[ You only used self-insert-command in your examples, which gives
  a slightly twisted view of what's going on in general.  ]

But ideally, we could have undo.c do most of that for you.
E.g. instead of adding a boundary every time we switch buffer, undo.c
could simply keep track of which buffers have seen changes during the
current command, and then push undo-boundaries on all of them at the end
of the command.

Except for self-insert-command, I think this would give you the ideal
behavior (without any manual work on your side) *and* it would mostly
preserve the existing behavior.

For self-insert-command, it's more tricky, and you'd probably need to do
the extra manual work.  E.g. add a post-self-insert-hook which checks if
this self-insert-command removed the previous undo-boundary and then
remove it as well in the sibling buffer.  I can't see how undo.c could
do that for you magically, since it doesn't know that the insertions in
the sibling buffer are "clones" of the insertions in the main buffer,
nor that it's done by self-insert-command.  Another approach would be
for you to arrange such that your a-f-c uses self-insert-command rather
than `insert' when cloning the insertion from a self-insert-command, but
that's probably just as hard if not harder.



        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Yes, you are right, the second buffer is an issue, although I am not
>> convinced that the behaviour is worse.
>
> Worse than the ideal or worse than the current one?

The current situation.

> Right now, I'm trying to understand what the ideal behavior should be.

I don't have a clear handle on this either.

For my two use cases, with noisy-change-log, I think the answer is, I
don't really care. I don't need undo in a log buffer, and if I had
thought about it, I would have switched it off.

For lentic, I would like the undo-boundaries to be what they would have
been if the commands had been typed manually.

> After that, we can try to figure out how to get this ideal behavior, or
> how close we can get to it, and which part of the work can be made
> generic in undo.c and which part of the work has to be done manually by
> each and every particular command/package.

That would be good.

Another package worth looking at would be gnus, which AFAICT,
reimplements undo all for itself, to cope with the fact that it's a
multi buffer sort of package.
 


>> ** Conclusion
>> All of the insertion events have been amalgamated, so the undo is much
>> too "lumpy".  Again, I can add this logic in, but I have to duplicate
>> the "add an undo-boundary after a command" logic from the
>> command loop.
>
> [ You only used self-insert-command in your examples, which gives
>   a slightly twisted view of what's going on in general.  ]
>
> But ideally, we could have undo.c do most of that for you.
> E.g. instead of adding a boundary every time we switch buffer, undo.c
> could simply keep track of which buffers have seen changes during the
> current command, and then push undo-boundaries on all of them at the end
> of the command.


Yeah, thought about that and that would get me most of the way there.

I think that should not affect *shell* buffers or equivalent right? They
actually only update outside of the command loop, so can't have changed
in the middle?


> Except for self-insert-command, I think this would give you the ideal
> behavior (without any manual work on your side) *and* it would mostly
> preserve the existing behavior.
>
> For self-insert-command, it's more tricky, and you'd probably need to do
> the extra manual work.  E.g. add a post-self-insert-hook which checks if
> this self-insert-command removed the previous undo-boundary and then
> remove it as well in the sibling buffer.  I can't see how undo.c could
> do that for you magically, since it doesn't know that the insertions in
> the sibling buffer are "clones" of the insertions in the main buffer,
> nor that it's done by self-insert-command.


I'd agree with this. One possibility would be to have an undo-boundary
command which is implements the same sort of "20 steps" logic as
self-insert-command. But, even that is going to be hard, since there can
be more changes in *that* buffer than in *this* (the two are not
necessarily exactly the same).

I think this would all be a little easier if buffer-undo-list didn't use
"nil" but allowed the developer to differentiate between different kinds
of boundary.


> Another approach would be for you to arrange such that your a-f-c uses
> self-insert-command rather than `insert' when cloning the insertion
> from a self-insert-command, but that's probably just as hard if not
> harder.

That doesn't sound great to me either, as I have to behave quite
differently depending on the last-command.

It seems to me, that you are not adverse to the idea of changing undo.c
to stop it putting boundaries in. Can I suggest a way forward? I will
carry on with my hacked Emacs with removed undo-boundaries in undo.c. I
can add the "undo-boundary to all changed buffers" using
post-command-hook. This will let me test everything makes sense and
works as expected, and I don't mind spending the effort, if emacs will
support it later.

If you are prepared to implement the "add an undo-boundary to every
buffer thats changed" logic into undo.c/cmd.c, then we can do that once
the logic works. I would be willing to help with that, but my C
programming is very poor, and in practice, I'm likely to learn more than
I actually help.


Phil

Re: disabling undo boundaries

[Phillip Lord]

Phillip Lord <phillip.lord@newcastle.ac.uk> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Another approach would be for you to arrange such that your a-f-c uses
>> self-insert-command rather than `insert' when cloning the insertion
>> from a self-insert-command, but that's probably just as hard if not
>> harder.
>
> That doesn't sound great to me either, as I have to behave quite
> differently depending on the last-command.
>
> It seems to me, that you are not adverse to the idea of changing undo.c
> to stop it putting boundaries in. Can I suggest a way forward? I will
> carry on with my hacked Emacs with removed undo-boundaries in undo.c. I
> can add the "undo-boundary to all changed buffers" using
> post-command-hook. This will let me test everything makes sense and
> works as expected, and I don't mind spending the effort, if emacs will
> support it later.
>
> If you are prepared to implement the "add an undo-boundary to every
> buffer thats changed" logic into undo.c/cmd.c, then we can do that once
> the logic works. I would be willing to help with that, but my C
> programming is very poor, and in practice, I'm likely to learn more than
> I actually help.


Actually, I have been thinking about this. What I worry about with this
is that we may be adding the same form of heuristic into undo.c as the
"insert an undo-boundary if the buffer has changed". And once is in the
C layer it is effectively unfixable from lisp. The existing logic does
not seem unsensible, but actually does not work in my (admitted niche)
set of circumstances.

So, it may be that simply removing the existing logic is the right thing
to do!

Phil

Re: disabling undo boundaries

[Stefan Monnier]

> Actually, I have been thinking about this. What I worry about with this
> is that we may be adding the same form of heuristic into undo.c as the
> "insert an undo-boundary if the buffer has changed".

Yes, because we do want this to happen, pretty much always (having
changes accumulate without intervening boundary is actually a problem
that can lead to nasty behaviors).

There are exceptions, but they're rare.


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Actually, I have been thinking about this. What I worry about with this
>> is that we may be adding the same form of heuristic into undo.c as the
>> "insert an undo-boundary if the buffer has changed".
>
> Yes, because we do want this to happen, pretty much always (having
> changes accumulate without intervening boundary is actually a problem
> that can lead to nasty behaviors).
>
> There are exceptions, but they're rare.


Sorry for late reply -- been travelling.

The heuristic I was refering to is the "insert an undo-boundary if the
current-buffer has changed". Or, rather, insert an undo-boundary if some
*other* buffer has changed.

I think that the current undo-boundary behaviour wrt a single buffer
makes sense. It's the fact that inserting content into *that* buffer
forces an undo-boundary into *this* buffer. I don't know why Emacs does
this. In most cases, this is irrelevant and doesn't happen anyway, and
where you have a p-c-h or a-c-f it changes undo behaviour. I worry that
I am missing a good reason for this behaviour, but I have thought about
it and failed to come up with a reason; negative conclusions are always
worrisome, but what else can you do?

I'll write a patch to trunk and send it in, once my life has caught up,
then I can happily pass the buck of making the final decision to you:-)

Phil

Re: disabling undo boundaries

[Phillip Lord]

Phillip Lord <phillip.lord@newcastle.ac.uk> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> Actually, I have been thinking about this. What I worry about with this
>>> is that we may be adding the same form of heuristic into undo.c as the
>>> "insert an undo-boundary if the buffer has changed".
>>
>> Yes, because we do want this to happen, pretty much always (having
>> changes accumulate without intervening boundary is actually a problem
>> that can lead to nasty behaviors).
>>
>> There are exceptions, but they're rare.
>
>
> Sorry for late reply -- been travelling.
>
> The heuristic I was refering to is the "insert an undo-boundary if the
> current-buffer has changed". Or, rather, insert an undo-boundary if some
> *other* buffer has changed.
>
> I think that the current undo-boundary behaviour wrt a single buffer
> makes sense. It's the fact that inserting content into *that* buffer
> forces an undo-boundary into *this* buffer. I don't know why Emacs does
> this. In most cases, this is irrelevant and doesn't happen anyway, and
> where you have a p-c-h or a-c-f it changes undo behaviour. I worry that
> I am missing a good reason for this behaviour, but I have thought about
> it and failed to come up with a reason; negative conclusions are always
> worrisome, but what else can you do?
>
> I'll write a patch to trunk and send it in, once my life has caught up,
> then I can happily pass the buck of making the final decision to you:-)

I have added the change that I am suggesting to:

fix/no-undo-boundary-on-secondary-buffer-change

The change simplifies undo.c somewhat. I can see no reason for the
existing logic, and it is has extremely counter-intuitive consequences
when using as discussed when using post-c-h or a-c-f.

But clearly it also changes some deep (and old!) logic in undo.c. So,
making this my first commit to trunk would clearly be reckless.

Let me know what you think. If you are unhappy, I will drop the idea
entirely and delete the branch. If you are happy, I will leave you to
merge/rebase as you choose. If you don't like my commit message grammar,
I can fix and squash on a branch!

Cheers

Phil

Re: disabling undo boundaries

[Stefan Monnier]

> I think that the current undo-boundary behaviour wrt a single buffer
> makes sense. It's the fact that inserting content into *that* buffer
> forces an undo-boundary into *this* buffer.  I don't know why Emacs
> does this.

I think the reason is not very deep: for reasons of efficiency (back in
the days when 8MB was a lot of memory), Emacs keeps track of a few
undo-related pieces of information in global variables, so it can only
handle a single buffer at a time.  If a command touches some other
buffer, the C code has to choose between "just switch" and "push
a boundary and then switch", and the choice it makes is to err on the
"safe" side of adding a potentially unnecessary undo-boundary rather
than risking to let the undo-log grow without intervening boundaries.

In your cases, this is complicated by the fact that you're concerned
about self-insert-command which is very special in that it was the only
command that doesn't push a boundary every time, but only once every
N repetitions.


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I think that the current undo-boundary behaviour wrt a single buffer
>> makes sense. It's the fact that inserting content into *that* buffer
>> forces an undo-boundary into *this* buffer.  I don't know why Emacs
>> does this.
>
> I think the reason is not very deep: for reasons of efficiency (back in
> the days when 8MB was a lot of memory), Emacs keeps track of a few
> undo-related pieces of information in global variables, so it can only
> handle a single buffer at a time.  If a command touches some other
> buffer, the C code has to choose between "just switch" and "push
> a boundary and then switch", and the choice it makes is to err on the
> "safe" side of adding a potentially unnecessary undo-boundary rather
> than risking to let the undo-log grow without intervening boundaries.


I meant "deep" in the sense of "deep in version control history" -- or
in this case, before the version control system. Still, I didn't know
this was the reason.

Arguing against this are my experiments which suggest that
undo-boundaries are, in practice, rarely inserted in this way (about 1%
of times these functions are called, although, of course, it will depend
on how you are using Emacs).


> In your cases, this is complicated by the fact that you're concerned
> about self-insert-command which is very special in that it was the only
> command that doesn't push a boundary every time, but only once every
> N repetitions.


I am slightly concerned about this, but not overly concerned. The main
thing that I am worried about is here:

https://github.com/phillord/lentic/issues/20

where, for example, a single "fill-paragraph" command takes many undos
to undo because of multiple changes in *another* buffer.

Anyway, the patch is there. Any chance? If not, anything (further) that
I can add (or remove!) to the branch to make it happen?

Failing that, I will have to think about how to remove the undo-boundary
post-hoc, or give up and accept that lentic will interfere with undo.
Haven't decided which yet.

BTW, I did a demonstration at a conference last, which pitted Emacs
(lagging badly on a badly overloaded netbook) with CIDER, and lentic
against several slick websites. We won the best demo prize. You have to
laugh...

Phil

Re: disabling undo boundaries

[David Kastrup]

phillip.lord@newcastle.ac.uk (Phillip Lord) writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> I think that the current undo-boundary behaviour wrt a single buffer
>>> makes sense. It's the fact that inserting content into *that* buffer
>>> forces an undo-boundary into *this* buffer.  I don't know why Emacs
>>> does this.
>>
>> I think the reason is not very deep: for reasons of efficiency (back in
>> the days when 8MB was a lot of memory), Emacs keeps track of a few
>> undo-related pieces of information in global variables, so it can only
>> handle a single buffer at a time.  If a command touches some other
>> buffer, the C code has to choose between "just switch" and "push
>> a boundary and then switch", and the choice it makes is to err on the
>> "safe" side of adding a potentially unnecessary undo-boundary rather
>> than risking to let the undo-log grow without intervening boundaries.
>
>
> I meant "deep" in the sense of "deep in version control history" -- or
> in this case, before the version control system. Still, I didn't know
> this was the reason.

The problem _I_ see is that it is in general not just "a command" which
may touch another buffer, but also compilation buffers, timer events,
font lock and a number of other background activities not immediately
caused by key presses.

-- 
David Kastrup

Re: disabling undo boundaries

[Stefan Monnier]

> I am slightly concerned about this, but not overly concerned. The main
> thing that I am worried about is here:
> https://github.com/phillord/lentic/issues/20
> where, for example, a single "fill-paragraph" command takes many undos
> to undo because of multiple changes in *another* buffer.

Yes, this is a clear bug.

> Anyway, the patch is there. Any chance? If not, anything (further) that
> I can add (or remove!) to the branch to make it happen?

I'll try and look at it soon,


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

David Kastrup <dak@gnu.org> writes:

> phillip.lord@newcastle.ac.uk (Phillip Lord) writes:
>
>> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>
>>>> I think that the current undo-boundary behaviour wrt a single buffer
>>>> makes sense. It's the fact that inserting content into *that* buffer
>>>> forces an undo-boundary into *this* buffer.  I don't know why Emacs
>>>> does this.
>>>
>>> I think the reason is not very deep: for reasons of efficiency (back in
>>> the days when 8MB was a lot of memory), Emacs keeps track of a few
>>> undo-related pieces of information in global variables, so it can only
>>> handle a single buffer at a time.  If a command touches some other
>>> buffer, the C code has to choose between "just switch" and "push
>>> a boundary and then switch", and the choice it makes is to err on the
>>> "safe" side of adding a potentially unnecessary undo-boundary rather
>>> than risking to let the undo-log grow without intervening boundaries.
>>
>>
>> I meant "deep" in the sense of "deep in version control history" -- or
>> in this case, before the version control system. Still, I didn't know
>> this was the reason.
>
> The problem _I_ see is that it is in general not just "a command" which
> may touch another buffer, but also compilation buffers, timer events,
> font lock and a number of other background activities not immediately
> caused by key presses.


I don't think this is an issue, because Emacs is single-threaded. So,
all of these events happen *between* commands which, in general, means
that an undo-boundary has gone onto the current-buffer anyway. So, they
don't break the undo behaviour in the current-buffer.

If it were not, the current semantics of Emacs undo would have been
shown to be problematic a long time ago.

As it stands (as I think I mentioned a long time ago!), I've checked how
often undo-boundaries are added by the current logic and the answer is
hardly ever. The code is almost entirely benign AFAICT, in that it never
runs. It just makes undo.c more complex than necessary, and is a total
disaster for my use case.

As always, it depends on what you do with Emacs and I may be missing
something where it is the right thing. I can't tell.

Phil

Re: disabling undo boundaries

[Davis Herring]

[I'm late to this thread, but after reading the more recent portion,
this still seems a useful reply.]

> For self-insert-command, it's more tricky, and you'd probably need to do
> the extra manual work.  E.g. add a post-self-insert-hook which checks if
> this self-insert-command removed the previous undo-boundary and then
> remove it as well in the sibling buffer.  I can't see how undo.c could
> do that for you magically, since it doesn't know that the insertions in
> the sibling buffer are "clones" of the insertions in the main buffer,
> nor that it's done by self-insert-command.  Another approach would be
> for you to arrange such that your a-f-c uses self-insert-command rather
> than `insert' when cloning the insertion from a self-insert-command, but
> that's probably just as hard if not harder.

Shouldn't it be easy to write a function (callable from Lisp) that
emulates self-insert-command (and its special behavior w.r.t. undo),
except that it takes a character as an argument?

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or
too sparse, it is because mass-energy conversion has occurred during
shipping.

Re: disabling undo boundaries

[David Kastrup]

phillip.lord@newcastle.ac.uk (Phillip Lord) writes:

> David Kastrup <dak@gnu.org> writes:
>>
>> The problem _I_ see is that it is in general not just "a command" which
>> may touch another buffer, but also compilation buffers, timer events,
>> font lock and a number of other background activities not immediately
>> caused by key presses.
>
>
> I don't think this is an issue, because Emacs is single-threaded. So,
> all of these events happen *between* commands which, in general, means
> that an undo-boundary has gone onto the current-buffer anyway.

Not for self-insert-command.  Not for stuff like shell-command-on-region
that has to wait for material to arrive.

-- 
David Kastrup

Re: disabling undo boundaries

[Phillip Lord]

David Kastrup <dak@gnu.org> writes:

> phillip.lord@newcastle.ac.uk (Phillip Lord) writes:
>
>> David Kastrup <dak@gnu.org> writes:
>>>
>>> The problem _I_ see is that it is in general not just "a command" which
>>> may touch another buffer, but also compilation buffers, timer events,
>>> font lock and a number of other background activities not immediately
>>> caused by key presses.
>>
>>
>> I don't think this is an issue, because Emacs is single-threaded. So,
>> all of these events happen *between* commands which, in general, means
>> that an undo-boundary has gone onto the current-buffer anyway.
>
> Not for self-insert-command.  Not for stuff like shell-command-on-region
> that has to wait for material to arrive.

I thought shell-command-on-region still arrives between commands, but I
have not looked at the code. You are more likely to be correct than I.

My feeling is that my patch is likely to remove problems caused by
asynchronicity rather than cause it. The logic is simpler and undo
boundaries are not added based on something that has happened in the
past.

Phil

Re: disabling undo boundaries

[Phillip Lord]

Davis Herring <herring@lanl.gov> writes:

> [I'm late to this thread, but after reading the more recent portion,
> this still seems a useful reply.]
>
>> For self-insert-command, it's more tricky, and you'd probably need to do
>> the extra manual work.  E.g. add a post-self-insert-hook which checks if
>> this self-insert-command removed the previous undo-boundary and then
>> remove it as well in the sibling buffer.  I can't see how undo.c could
>> do that for you magically, since it doesn't know that the insertions in
>> the sibling buffer are "clones" of the insertions in the main buffer,
>> nor that it's done by self-insert-command.  Another approach would be
>> for you to arrange such that your a-f-c uses self-insert-command rather
>> than `insert' when cloning the insertion from a self-insert-command, but
>> that's probably just as hard if not harder.
>
> Shouldn't it be easy to write a function (callable from Lisp) that
> emulates self-insert-command (and its special behavior w.r.t. undo),
> except that it takes a character as an argument?

That would help a little in my use case, although the
self-insert-command behaviour is a tweak for me, not a main concern.

I think that the main issue is the use of "nil" for an undo-boundary.
This makes all undo-boundaries equivalent and impossible to tag.
Something like

'(:boundary)

or

(:boundary :reason)

would be far easier, at the cost of two cons cells rather than one, at
least for a naive implementation.

Phil

Re: disabling undo boundaries

[Stefan Monnier]

>> Anyway, the patch is there. Any chance? If not, anything (further) that
>> I can add (or remove!) to the branch to make it happen?
> I'll try and look at it soon,

IIUC this only removes the code which adds a boundary when we change
buffer.   IOW it switches from the conservative option of adding
a boundary even if it might not be needed, to the risky option of hoping
that someone else will add the needed boundary.

As explained, this will most likely introduce problems with ever-growing
undo-logs in buffers filled by process output and timers.

It's an acceptable first step, but it needs to be compensated by
a second step which keeps track of all buffers modified during the
execution of a command and change the "add a boundary in current-buffer
at the end of a command" to "add boundaries in all modified buffers
at the end of a command".


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Anyway, the patch is there. Any chance? If not, anything (further) that
>>> I can add (or remove!) to the branch to make it happen?
>> I'll try and look at it soon,
>
> IIUC this only removes the code which adds a boundary when we change
> buffer.   IOW it switches from the conservative option of adding
> a boundary even if it might not be needed, to the risky option of hoping
> that someone else will add the needed boundary.

I'd tend to characterise it as the risky option of disrupting the user
experience by added unnecessary boundaries on the off-chance of using
too much memory, against the conservative option of not adding a
boundary.

But, otherwise, yes.


> As explained, this will most likely introduce problems with ever-growing
> undo-logs in buffers filled by process output and timers.

Will buffer-undo-list not be truncated by GC anyway? I can't quite
understand where the ever-growing logs come from, nor how the
functionality I have removed would prevent it.

> It's an acceptable first step, but it needs to be compensated by
> a second step which keeps track of all buffers modified during the
> execution of a command and change the "add a boundary in current-buffer
> at the end of a command" to "add boundaries in all modified buffers
> at the end of a command".

I'm struggling with understanding this also. I've tried tracing when the
code my patch removes actually runs and it is pretty rarely. And, yet,
undo works correctly, generally on a per-command basis (except for the
self-insert-command special handling).

Also, adding boundaries in all modified buffers strikes me as fairly
stochastic. Assuming a well-behaved timer (i.e. one that releases
control with `sit-for'), this logic essentially translates to "we may
choose to show an undo-boundary in at this point, or we may not".

So I'd need an example, though, or some way of causing this sort of
problem that you are worried about. A failing ert test in
test/automated would be ideal, but anything is good!

Phil

Re: disabling undo boundaries

[Stefan Monnier]

> Will buffer-undo-list not be truncated by GC anyway?

Truncation is only done at undo-boundaries.  Hence the problem.

> I'm struggling with understanding this also. I've tried tracing when the
> code my patch removes actually runs and it is pretty rarely.

When you're editing buffer FOO while some process is inserting data in
buffer BAR.  Or when your commands modify two buffers (e.g. in your case).

> Also, adding boundaries in all modified buffers strikes me as fairly
> stochastic.

Not sure what you mean.

> Assuming a well-behaved timer (i.e. one that releases
> control with `sit-for'),

Hmm?  timers usually shouldn't call sit-for.


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> Will buffer-undo-list not be truncated by GC anyway?
>
> Truncation is only done at undo-boundaries.  Hence the problem.

Ah, yes, okay.


>> I'm struggling with understanding this also. I've tried tracing when the
>> code my patch removes actually runs and it is pretty rarely.
>
> When you're editing buffer FOO while some process is inserting data in
> buffer BAR.

Okay. So, buffer BAR should get undo-boundaries when a command runs in
FOO? And FOO should get an undo-boundary when the process adds something
to BAR?

> Or when your commands modify two buffers (e.g. in your case).

Well, I think that this is my problem (as the author of the command,
rather than as the user). I mean, I modify two buffers, I need to fix
the undo, to decide what I want to do about this.

For my use case, this would work much better because I could even do the
"undo-boundary unless it's self-insert in which case wait for 20 of
them" logic.

In the case of a logging mode (i.e. which logs all events), I'd switch
the undo off in that buffer anyway.


>> Also, adding boundaries in all modified buffers strikes me as fairly
>> stochastic.
>
> Not sure what you mean.

I am editing a buffer, FOO. I type something in the usual way, with some
pauses for me to think.

Mean time, an idle-timer is running, doing something to BAR. If I think
long, the undo will behave in one way, if I think quick, the undo will
behave in another, depending on whether the idle-timer has modified BAR
or not.

>
>> Assuming a well-behaved timer (i.e. one that releases
>> control with `sit-for'),
>
> Hmm?  timers usually shouldn't call sit-for.

Oh, now I am sure that this used to be recommended (which is why I did
it). I use this when I have a long running idle timer -- I sit-for a
very short gap, then carry on if there is no input pending, then carry
on.

Well, that's more code to fix!

Phil

Re: disabling undo boundaries

[Stefan Monnier]

>>> I'm struggling with understanding this also. I've tried tracing when the
>>> code my patch removes actually runs and it is pretty rarely.
>> When you're editing buffer FOO while some process is inserting data in
>> buffer BAR.
> Okay. So, buffer BAR should get undo-boundaries when a command runs in
> FOO? And FOO should get an undo-boundary when the process adds something
> to BAR?

The general goal is: add undo-boundaries often enough so the undo-log
doesn't grow too large, both in FOO and in BAR, and ideally in ways
which are somewhat predictable for the user.

If you ask my ideal: FOO would get an undo-boundary after each command,
and BAR would get an undo boundary after each N bytes inserted, or after
every N seconds.

>>> Also, adding boundaries in all modified buffers strikes me as fairly
>>> stochastic.
>> Not sure what you mean.
> I am editing a buffer, FOO. I type something in the usual way, with some
> pauses for me to think.
> Mean time, an idle-timer is running, doing something to BAR. If I think
> long, the undo will behave in one way, if I think quick, the undo will
> behave in another, depending on whether the idle-timer has modified BAR
> or not.

In the scenario of "add boundaries to every buffer modified since last
time"?  Why do you think so?  The boundaries in Foo will not be
influenced by the timer.  The boundaries in Bar will, indeed, because
they'll only be inserted as a side-effect of the commands in Foo and
depending on how much you think, the timer could fire several times
between commands.  Hence my ideal wish of "get an undo boundary after
each N bytes inserted, or after every N seconds" for things like process
output or timers.

>>> Assuming a well-behaved timer (i.e. one that releases
>>> control with `sit-for'),
>> Hmm?  timers usually shouldn't call sit-for.
> Oh, now I am sure that this used to be recommended (which is why I did
> it). I use this when I have a long running idle timer -- I sit-for a
> very short gap, then carry on if there is no input pending, then carry
> on.

Ah, you're thinking of a long-running idle-timer, basically a poor man's
background task.  Yes, then it needs to call sit-for, but I don't think
it makes much difference in the end result in the scenarios
discussed here.


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>> I'm struggling with understanding this also. I've tried tracing when the
>>>> code my patch removes actually runs and it is pretty rarely.
>>> When you're editing buffer FOO while some process is inserting data in
>>> buffer BAR.
>> Okay. So, buffer BAR should get undo-boundaries when a command runs in
>> FOO? And FOO should get an undo-boundary when the process adds something
>> to BAR?
>
> The general goal is: add undo-boundaries often enough so the undo-log
> doesn't grow too large, both in FOO and in BAR, and ideally in ways
> which are somewhat predictable for the user.
>
> If you ask my ideal: FOO would get an undo-boundary after each command,
> and BAR would get an undo boundary after each N bytes inserted, or after
> every N seconds.

My own feeling is that undo-boundaries should be the task of the mode
author, with sensible default behaviour (like the current one) for
user-centric modes (i.e. non process buffers). My guess is that most
buffers connected to non-interactive processes have undo switched off.

As an alternative, why not handle this at GC? So, change the current
logic to "iff buffer undo-log is too large, and you have no
undo-boundaries, GC to 50% of length regardless". This addresses your
concern (overly long undo-logs, i.e. memory leaks) within the memory
management system, rather than in as a heuristic in a user system (i.e.
undo).


>>>> Also, adding boundaries in all modified buffers strikes me as fairly
>>>> stochastic.
>>> Not sure what you mean.
>> I am editing a buffer, FOO. I type something in the usual way, with some
>> pauses for me to think.
>> Mean time, an idle-timer is running, doing something to BAR. If I think
>> long, the undo will behave in one way, if I think quick, the undo will
>> behave in another, depending on whether the idle-timer has modified BAR
>> or not.
>
> In the scenario of "add boundaries to every buffer modified since last
> time"?  Why do you think so?  The boundaries in Foo will not be
> influenced by the timer.

Sorry, my mistake. "command" != function call. So, the call to the
idle-timer function will not trigger this.


>>>> Assuming a well-behaved timer (i.e. one that releases
>>>> control with `sit-for'),
>>> Hmm?  timers usually shouldn't call sit-for.
>> Oh, now I am sure that this used to be recommended (which is why I did
>> it). I use this when I have a long running idle timer -- I sit-for a
>> very short gap, then carry on if there is no input pending, then carry
>> on.
>
> Ah, you're thinking of a long-running idle-timer, basically a poor man's
> background task.  Yes, then it needs to call sit-for, but I don't think
> it makes much difference in the end result in the scenarios
> discussed here.

Yeah, a "background" task. And you are correct.

Phil

Re: disabling undo boundaries

[Phillip Lord]

Phillip Lord <phillip.lord@russet.org.uk> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>>>> I'm struggling with understanding this also. I've tried tracing when the
>>>>> code my patch removes actually runs and it is pretty rarely.
>>>> When you're editing buffer FOO while some process is inserting data in
>>>> buffer BAR.
>>> Okay. So, buffer BAR should get undo-boundaries when a command runs in
>>> FOO? And FOO should get an undo-boundary when the process adds something
>>> to BAR?
>>
>> The general goal is: add undo-boundaries often enough so the undo-log
>> doesn't grow too large, both in FOO and in BAR, and ideally in ways
>> which are somewhat predictable for the user.
>>
>> If you ask my ideal: FOO would get an undo-boundary after each command,
>> and BAR would get an undo boundary after each N bytes inserted, or after
>> every N seconds.
>
> My own feeling is that undo-boundaries should be the task of the mode
> author, with sensible default behaviour (like the current one) for
> user-centric modes (i.e. non process buffers). My guess is that most
> buffers connected to non-interactive processes have undo switched off.
>
> As an alternative, why not handle this at GC? So, change the current
> logic to "iff buffer undo-log is too large, and you have no
> undo-boundaries, GC to 50% of length regardless". This addresses your
> concern (overly long undo-logs, i.e. memory leaks) within the memory
> management system, rather than in as a heuristic in a user system (i.e.
> undo).

Okay, to answer my own question, this *is* already handled at GC, with
the undo-outer-limit logic. Not handled ideally -- undo-outer-limit is
two orders greater than normal and the compaction function is noisy. But
it should stop Emacs from memory leaking to crash.

Phil

Re: disabling undo boundaries

[Stefan Monnier]

> My own feeling is that undo-boundaries should be the task of the mode
> author, with sensible default behaviour (like the current one) for
> user-centric modes (i.e. non process buffers).

In 99.9% of the cases, authors don't want to know about undo boundaries,
they should "just work", so the default is very important.


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> My own feeling is that undo-boundaries should be the task of the mode
>> author, with sensible default behaviour (like the current one) for
>> user-centric modes (i.e. non process buffers).
>
> In 99.9% of the cases, authors don't want to know about undo boundaries,
> they should "just work", so the default is very important.

I would agree, but I still need to something that breaks so I can check
it is all working.

The closest I have got so far is with cider interaction mode (i.e. the
repl). I managed to get this to crash out with the "excessively large
undo this is probably a bug" error message after my patch, running

(doall (map #(println %)) (range 100000))

However, without my patch, emacs just runs to hang as it gets into a
tight GC cycle, presumably as the buffer gets enormous, so in fact this
really is a bug (i.e. the buffer is never truncated), and my patch
improves things.

Implementing "add an undo-boundary if there isn't one in any buffer with
a big buffer-undo-list" I can certainly do. Any reason to not do this on
a timer (i.e. why tie it to the command cycle)? In lisp? This is a ten
minute job. In C, it will take longer.

Phil

Re: disabling undo boundaries

[Stefan Monnier]

> a big buffer-undo-list" I can certainly do. Any reason to not do this on
> a timer (i.e. why tie it to the command cycle)?

As mentioned, for buffers modified by timers/processes I like the idea
of adding undo-boundaries every N seconds or every N modifications.


        Stefan

Re: disabling undo boundaries

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> a big buffer-undo-list" I can certainly do. Any reason to not do this on
>> a timer (i.e. why tie it to the command cycle)?
>
> As mentioned, for buffers modified by timers/processes I like the idea
> of adding undo-boundaries every N seconds or every N modifications.


I've add an update for this to the same branch
(fix/no-undo-boundary-on-secondary-buffer-change).

Is this the sort of thing that you had in mind? Is it not complete
yet -- I was not considering leaving it noisy (it messages at the
moment). And I am not sure it is in the right place -- simple.el does
not currently require timer. Finally, the timer needs to be manually
started.

Phil