Re: Distinguish between regional undo and undo to the beginning in undo-equiv-table

[Stefan Monnier]

> Here is a patch, I mapped undo-in-region records to 'undo-in-region. I also
> fixed a bug in undo-make-selective-list: before this change, if you
> undo-in-region, break the undo chain, then undo-in-region again with
> undo-only, ulist would be set to t and it breaks at (setq undo-elt (car
> ulist)).

Thanks a lot (especially for improving the docstring ;-).  In my
experience, this part of the code is quite delicate and I wouldn't be
surprised to find yet more bugs for other corner cases.  So I think it's
very important to document those corner cases with tests.  Any chance
you could add matching tests to test/lisp/simple-tests.el in your patch?


        Stefan


> Yuan
>
> From 8c4d50409a7ad88eb85d51e79b04f3587cd36976 Mon Sep 17 00:00:00 2001
> From: Yuan Fu <casouri@gmail.com>
> Date: Wed, 3 Mar 2021 09:50:15 -0500
> Subject: [PATCH] Map redo records for undo-in-region to 'undo-in-region
>
> * lisp/simple.el (undo-equiv-table): Add explaination for
> undo-in-region and undo to the beginning of undo list.
> (undo): If equiv is 'undo-in-region or t, set to pending-undo-list to
> t. If the redo is undo-in-region, map buffer-undo-list to
> 'undo-in-region instead of t.
> (undo-make-selective-list): Only continue when ulist is not t or
> undo-in-region.
> ---
>  lisp/simple.el | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/lisp/simple.el b/lisp/simple.el
> index f8050091d5..05bf4b8784 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -2824,8 +2824,9 @@ 'advertised-undo
>  
>  (defconst undo-equiv-table (make-hash-table :test 'eq :weakness t)
>    "Table mapping redo records to the corresponding undo one.
> -A redo record for undo-in-region maps to t.
> -A redo record for ordinary undo maps to the following (earlier) undo.")
> +A redo record for undo-in-region maps to 'undo-in-region.
> +A redo record for ordinary undo maps to the following (earlier) undo.
> +An undo record that undoes to the beginning of the undo list maps to t.")
>  
>  (defvar undo-in-region nil
>    "Non-nil if `pending-undo-list' is not just a tail of `buffer-undo-list'.")
> @@ -2901,7 +2902,7 @@ undo
>       ;; undo-redo-undo-redo-... so skip to the very last equiv.
>       (while (let ((next (gethash equiv undo-equiv-table)))
>                (if next (setq equiv next))))
> -     (setq pending-undo-list equiv)))
> +     (setq pending-undo-list (if (consp equiv) equiv t))))
>      (undo-more
>       (if (numberp arg)
>        (prefix-numeric-value arg)
> @@ -2917,11 +2918,12 @@ undo
>        (while (eq (car list) nil)
>       (setq list (cdr list)))
>        (puthash list
> -               ;; Prevent identity mapping.  This can happen if
> -               ;; consecutive nils are erroneously in undo list.
> -               (if (or undo-in-region (eq list pending-undo-list))
> -                   t
> -                 pending-undo-list)
> +               (cond
> +                (undo-in-region 'undo-in-region)
> +                ;; Prevent identity mapping.  This can happen if
> +                ;; consecutive nils are erroneously in undo list.
> +                ((eq list pending-undo-list) t)
> +                (t pending-undo-list))
>              undo-equiv-table))
>      ;; Don't specify a position in the undo record for the undo command.
>      ;; Instead, undoing this should move point to where the change is.
> @@ -3234,7 +3236,7 @@ undo-make-selective-list
>          undo-elt)
>      (while ulist
>        (when undo-no-redo
> -        (while (gethash ulist undo-equiv-table)
> +        (while (consp (gethash ulist undo-equiv-table))
>            (setq ulist (gethash ulist undo-equiv-table))))
>        (setq undo-elt (car ulist))
>        (cond