On Mon, Nov 8, 2021 at 10:09 AM Campbell Barton <ideasman42@gmail.com> wrote:
>
> 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).

Send the copyright assignment email to `assign(_at_)gnu.org'
updated the patch with added comment,