Re: Support for undo-amalgamate in a version of the atomic-change-group macro (with patch)

[Campbell Barton]

On Mon, Nov 8, 2021 at 12:21 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > Here is an updated patch with a separate macro to amalgamate undo barriers.
>
> Looks pretty good, thanks.
> I was about to install it into `master` but noticed the following:
>
> - You seem not to have signed copyright paperwork yet.  If you're OK
>   with it, please fill the form below and send it to the FSF so they can
>   send you the relevant paperwork to sign.
>   [ The change is sufficiently small that we can accept it right away,
>     but since the paperwork process takes some time, it's good to do it
>     "in advance" so it's out of the way for your next contributions.  ]

Thanks for checking the patch.
I'll look into signing the paperwork, I may do other contributions in
the future, so best get that handled sooner than later.

> - I see the macro binds undo limits, but AFAICT this is an "accident"
>   resulting from copy&pasting code from the other macro: for the
>   atomic-change macro it's very important that undo info is not thrown
>   away since the macro uses the undo info internally to cancel changes
>   on error, but for this macro I can't see any harm in having the undo
>   info truncated, so I think we shouldn't change the undo limit
>   vars.  WDYT?

This was intentional, the reasoning is that it should not be possible
for the amalgamated undo information to form a partial undo step.

In other words, I believe there should be a guarantee that a single
undo restores the state before `with-undo-amalgamate-change-group' is
used. Or, in the unlikely case a single undo-step exceeds memory
limits, that undo fails entirely instead of undoing into a state
part-way through the body within the macro.
This is in keeping with how you would expect undo to work for any
other command AFAICS.

In my use case this is an important guarantee since undo/redoing an
action needs to ensure the action was fully undone before performing
the new action. Failure to do so will cause bugs that could be
difficult to track down.

This should be noted in the code-comments, if the rationale above
seems reasonable I'll update the patch.

Another note on naming, this could be called `with-undo-amalgamate',
not sure if the "-change-group" suffix is worth including (I did this
to fit in with existing names, although it seems a bit verbose).

>
>         Stefan
>


-- 
- Campbell