bug#77580: [PATCH] New command ediff-undo

[Sean Whitton]

Hello,

On Sun 06 Apr 2025 at 05:45pm +02, Paul D. Nelson wrote:

> I'd like to propose a command 'ediff-undo' that compares the current
> buffer with how it would look after undo.  It supports a prefix arg (to
> specify multiple undo steps) and respects the active region (to restrict
> undo operations to that region).
>
> Some less obvious situations where I've found this helpful:
>
> - When a buffer has been modified by an external program or a
>   complicated Emacs command.
>
> - After replacing part of a buffer with text obtained externally (e.g.,
>   copied from an external email client).
>
> - For a quick recap of recent edits to a buffer or region, via something
>   like C-u C-u C-u M-x ediff-undo.
>
> The implementation is a straightforward combination of parts of
> 'ediff-current-file' and 'undo'.  Any feedback would be welcome.

Thank you for an interesting idea!  I'm especially interested in the
external commands and complex Emacs commands case.

I've made a couple of minor code comments below.
Some higher level thoughts:

- Why ediff?  Why not plain diff?  The latter is lighter weight, and is
  what diff-buffer-with-file uses, and your new command feels similar to
  diff-buffer-with-file, to me.

- Have you thought about automatically detecting how far to go back,
  somehow?  We already have a feature whereby autosaves are disabled if
  the buffer text has shrunk a lot; maybe that heuristic could help
  here.  Probably this would want to be a separate command so the manual
  version was still available.

> diff --git a/etc/NEWS b/etc/NEWS
> index 35e6edcd712..261ff68605c 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1288,6 +1288,12 @@ name of the directory now reverts the Dired buffer.
>  With a new value of the prefix argument (1), this command copies file
>  names relative to the root directory of the current project.
>
> +*** New command 'ediff-undo' to compare buffer with its undo states.
> +This command compares the current buffer and how it would look if undo
> +were performed.  With a prefix argument, it considers that many undo
> +operations.  With an active region, it restricts to changes in that
> +region.

... it restricts itself to changes in that region ...

> +  (let* ((use-region (use-region-p))
> +         (beg (and use-region (region-beginning)))
> +         (end (and use-region (region-end)))

See use-region-beginning and use-region-end to make this shorter.

> +         (arg (if (numberp arg) arg 1))

It's more idiomatic to use an arguments list of (arg &optional ...) with
(interactive "p").

> +         (orig-buf (current-buffer))
> +         (current-major major-mode)
> +         (undo-buf-name (concat "UNDO=" (buffer-name orig-buf)))

A more usual name would be " *ediff-undo*<N>", I think.  The space means
it is considered an internal buffer.  (I know you're copying other ediff
practice with this FOO= thing but let's not introduce more.)

> +         (undo-buf (get-buffer undo-buf-name)))
> +    (unless (>= arg 0)
> +      (error "Negative argument %d" arg))

(unless (plusp arg)

> +    (when undo-buf
> +      (kill-buffer undo-buf)
> +      (setq undo-buf nil))
> +    (setq undo-buf (get-buffer-create undo-buf-name))

Usually in Elisp we call erase-buffer on the buffer rather than
killing and recreating it.

> +    (with-current-buffer undo-buf
> +      (insert-buffer-substring orig-buf)
> +      (setq buffer-undo-list
> +            (undo-copy-list (with-current-buffer orig-buf
> +                              buffer-undo-list)))

You can use buffer-local-value instead here.

> +      (funcall current-major)

Could you explain why the temporary buffer needs to be set to the major
mode?  Is it for font lock/syntax highlighting, essentially?

-- 
Sean Whitton