GNU bug report logs - #77580
[PATCH] New command ediff-undo

Previous Next

To reply to this bug, email your comments to 77580 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "Paul D. Nelson" <ultrono <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] New command ediff-undo
Date: Sun, 06 Apr 2025 17:45:16 +0200

[Message part 1 (text/plain, inline)]

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.

[0001-Add-ediff-undo-command.patch (text/x-patch, attachment)]

Message #8 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: "Paul D. Nelson" <ultrono <at> gmail.com>
Cc: 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Mon, 07 Apr 2025 09:22:55 +0800

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

Message #11 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: "Paul D. Nelson" <ultrono <at> gmail.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Mon, 07 Apr 2025 08:20:16 +0200

[Message part 1 (text/plain, inline)]

Hi Sean, thanks for your comments.

> - 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.

"ediff" because that's what I've typically used more, but I agree that
it makes sense to have a "diff" analogue, so I've added one (attached).

> - 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.
>

In practice, I've just spammed C-u enough times.  I considered allowing
a negative argument to signal "undo everything", which would be most
useful with an active region; how does that sound?

I hadn't thought about automatic detection along the lines that you
suggest.  I think I'm happy to leave that for now, either to a
refinement of this command or a future one.

>> +         (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.)

I'm hesitant to mark it as internal since other Ediff functions don't
automatically delete modified non-indirect buffers without user-defined
cleanup hooks.  In particular, I think it would be confusing if this
function behaved differently from ediff-current-file, although I see
your point about naming conventions.  Perhaps "*ediff-undo*<N>" would be
more suitable?

>> +      (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?

Another reason is that users might edit the "undo" buffer during the
Ediff session, in which case standard keybindings are useful.  Other
suggestions would be welcome here.

Paul

[0001-Add-diff-with-undo-and-ediff-undo-commands.patch (text/x-patch, attachment)]

Message #14 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: "Paul D. Nelson" <ultrono <at> gmail.com>
Cc: 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Tue, 08 Apr 2025 11:04:12 +0800

Hello,

On Mon 07 Apr 2025 at 08:20am +02, Paul D. Nelson wrote:

> Hi Sean, thanks for your comments.
>
>> - 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.
>
> "ediff" because that's what I've typically used more, but I agree that
> it makes sense to have a "diff" analogue, so I've added one (attached).

Cool.  There is ediff-with-current-buffer so probably it should be named
ediff-with-undo, like you already have diff-with-undo.

Btw I noticed that in undo-to-buffer it would be better to use
'(when (minusp' instead of '(unless (>='

>> - 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.
>>
>
> In practice, I've just spammed C-u enough times.  I considered allowing
> a negative argument to signal "undo everything", which would be most
> useful with an active region; how does that sound?

Wouldn't undoing everything just be the existing diff-buffer-with-file?
That doesn't seem right.

> I hadn't thought about automatic detection along the lines that you
> suggest.  I think I'm happy to leave that for now, either to a
> refinement of this command or a future one.

To be honest, I don't like adding a command where the normal way to use
it is to spam C-u.  The normal way to use it should be easy to access.
So let's try to think of something better.

>>> +         (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.)
>
> I'm hesitant to mark it as internal since other Ediff functions don't
> automatically delete modified non-indirect buffers without user-defined
> cleanup hooks.  In particular, I think it would be confusing if this
> function behaved differently from ediff-current-file, although I see
> your point about naming conventions.  Perhaps "*ediff-undo*<N>" would be
> more suitable?

Yes, sure, sounds good.

>>> +      (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?
>
> Another reason is that users might edit the "undo" buffer during the
> Ediff session, in which case standard keybindings are useful.  Other
> suggestions would be welcome here.

Cool.

-- 
Sean Whitton

Message #17 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: "Paul D. Nelson" <ultrono <at> gmail.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Tue, 08 Apr 2025 07:25:20 +0200

> Cool.  There is ediff-with-current-buffer so probably it should be named
> ediff-with-undo, like you already have diff-with-undo.

I appreciate this suggestion, but it's less clear to me for a few
reasons:

- ediff-with-current-buffer is an internal macro.

- the user-facing Ediff commands omit words like "with".

- I used "diff-with-undo" just because "diff-undo" was taken by a
  more-or-less internal helper function (undo inhibiting read-only).

As I see it, the advantages of "ediff-undo" are internal consistency
with user-facing ediff-* commands, and brevity (for the command name,
and buffer names like "*ediff-undo*").  The disadvantage is that it is
not quite parallel to "diff-with-undo".  On the other hand, the two sets
of commands are already not particularly consistent in their naming.

I still lean towards "ediff-undo" given its brevity and internal
consistency, but would be happy to discuss further.

>> In practice, I've just spammed C-u enough times.  I considered
>> allowing a negative argument to signal "undo everything", which would
>> be most useful with an active region; how does that sound?
>
> Wouldn't undoing everything just be the existing diff-buffer-with-file?
> That doesn't seem right.

The proposed operation of diff-with-undo with a negative argument C--
would differ from diff-buffer-with-file in some cases:

- With an active region, it would preview undo restricted to that
  region.

- If you've saved the file more than once in the current session, "undo
  everything" goes to the state at the beginning of the session rather
  than the last save.

Let me know if you think this feature seems worth including.  I lean
towards "no" out of a desire to keep the new commands consistent with
"undo" itself, which does not accept a negative argument.

>
>> I hadn't thought about automatic detection along the lines that you
>> suggest.  I think I'm happy to leave that for now, either to a
>> refinement of this command or a future one.
>
> To be honest, I don't like adding a command where the normal way to use
> it is to spam C-u.  The normal way to use it should be easy to access.
> So let's try to think of something better.

Agreed.

Thinking further, this scenario seems niche rather than normal.  The
original intent might be met more directly by some new command such as
revert-buffer-in-region, diff-buffer-with-file-in-region,
vc-diff-in-region, diff-backup-in-region, (...).

The situation resembles that of the undo command itself.  Typically, we
should either undo small amounts, or resort to one of the structural
undo-like commands tied to file or VC status.  Only rarely should we
need to do something like C-99 C-/ (possibly in a region), but that
option remains a useful fallback.

Message #20 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Paul Nelson <ultrono <at> gmail.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Tue, 8 Apr 2025 07:28:05 +0200

[Message part 1 (text/plain, inline)]

Attaching the updated patch.

[Message part 2 (text/html, inline)]

[0001-Add-diff-with-undo-and-ediff-undo-commands.patch (application/octet-stream, attachment)]

Message #23 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: "Paul D. Nelson" <ultrono <at> gmail.com>
Cc: 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Tue, 08 Apr 2025 15:00:57 +0800

Hello,

On Tue 08 Apr 2025 at 07:25am +02, Paul D. Nelson wrote:

> As I see it, the advantages of "ediff-undo" are internal consistency
> with user-facing ediff-* commands, and brevity (for the command name,
> and buffer names like "*ediff-undo*").  The disadvantage is that it is
> not quite parallel to "diff-with-undo".  On the other hand, the two sets
> of commands are already not particularly consistent in their naming.

Yes, fine, ediff-undo and diff-with-undo it is.

> Let me know if you think this feature seems worth including.  I lean
> towards "no" out of a desire to keep the new commands consistent with
> "undo" itself, which does not accept a negative argument.

Yes, I think that we should leave this out.

>>> I hadn't thought about automatic detection along the lines that you
>>> suggest.  I think I'm happy to leave that for now, either to a
>>> refinement of this command or a future one.
>>
>> To be honest, I don't like adding a command where the normal way to use
>> it is to spam C-u.  The normal way to use it should be easy to access.
>> So let's try to think of something better.
>
> Agreed.
>
> Thinking further, this scenario seems niche rather than normal.  The
> original intent might be met more directly by some new command such as
> revert-buffer-in-region, diff-buffer-with-file-in-region,
> vc-diff-in-region, diff-backup-in-region, (...).
>
> The situation resembles that of the undo command itself.  Typically, we
> should either undo small amounts, or resort to one of the structural
> undo-like commands tied to file or VC status.  Only rarely should we
> need to do something like C-99 C-/ (possibly in a region), but that
> option remains a useful fallback.

So just to confirm, we both think that it would be good to come up with
a different command to satisfy the original use cases, because doing it
with ediff-undo and diff-with-undo requires spamming C-u, and that's
unergonomic?  (I ask because you posted an updated patch after writing
the above).

Of your suggestions here, my first thought is that maybe C-x x g could
start operating only on an active region, if there is one.  That seems
like a great new feature.

-- 
Sean Whitton

Message #26 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: "Paul D. Nelson" <ultrono <at> gmail.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Tue, 08 Apr 2025 09:25:17 +0200

Hi Sean,

>>>> I hadn't thought about automatic detection along the lines that you
>>>> suggest.  I think I'm happy to leave that for now, either to a
>>>> refinement of this command or a future one.
>>>
>>> To be honest, I don't like adding a command where the normal way to use
>>> it is to spam C-u.  The normal way to use it should be easy to access.
>>> So let's try to think of something better.
>>
>> Agreed.
>>
>> Thinking further, this scenario seems niche rather than normal.  The
>> original intent might be met more directly by some new command such as
>> revert-buffer-in-region, diff-buffer-with-file-in-region,
>> vc-diff-in-region, diff-backup-in-region, (...).
>>
>> The situation resembles that of the undo command itself.  Typically, we
>> should either undo small amounts, or resort to one of the structural
>> undo-like commands tied to file or VC status.  Only rarely should we
>> need to do something like C-99 C-/ (possibly in a region), but that
>> option remains a useful fallback.
>
> So just to confirm, we both think that it would be good to come up with
> a different command to satisfy the original use cases, because doing it
> with ediff-undo and diff-with-undo requires spamming C-u, and that's
> unergonomic?  (I ask because you posted an updated patch after writing
> the above).

To clarify, by "original intent", I was referring to your suggestion of
detecting automatically how far back to undo.  The C-u spamming is
relevant in an edge case, not the typical use case for ediff-undo and
diff-with-undo.  It's analogous to how for regular undo, it's
occasionally useful to do C-999 C-/.  In my view, the current patch
meets its primary goals well, but I'd welcome a broader discussion of
related enhancements or other commands.

> Of your suggestions here, my first thought is that maybe C-x x g could
> start operating only on an active region, if there is one.  That seems
> like a great new feature.

Yes, that sounds excellent.

Paul

Message #29 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Paul Nelson <ultrono <at> gmail.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Tue, 8 Apr 2025 16:50:31 +0200

[Message part 1 (text/plain, inline)]

And (with apologies for the spam) a further, hopefully final update, that
works in narrowed buffers.

[Message part 2 (text/html, inline)]

[0001-Add-diff-with-undo-and-ediff-undo-commands.patch (application/octet-stream, attachment)]

Message #32 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: "Paul D. Nelson" <ultrono <at> gmail.com>
Cc: 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Wed, 09 Apr 2025 10:32:24 +0800

Hello,

On Tue 08 Apr 2025 at 09:25am +02, Paul D. Nelson wrote:

> To clarify, by "original intent", I was referring to your suggestion of
> detecting automatically how far back to undo.  The C-u spamming is
> relevant in an edge case, not the typical use case for ediff-undo and
> diff-with-undo.  It's analogous to how for regular undo, it's
> occasionally useful to do C-999 C-/.  In my view, the current patch
> meets its primary goals well, but I'd welcome a broader discussion of
> related enhancements or other commands.

Thanks for clarifying.  So your thought is that the first two bullet
points in your original mail are covered by the command as-is?

>> Of your suggestions here, my first thought is that maybe C-x x g could
>> start operating only on an active region, if there is one.  That seems
>> like a great new feature.
>
> Yes, that sounds excellent.

If you would be interested in working on that, I'd be very happy to
review and shepherd the change.

-- 
Sean Whitton

Message #35 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: "Paul D. Nelson" <ultrono <at> gmail.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Wed, 09 Apr 2025 07:43:40 +0200

> Thanks for clarifying.  So your thought is that the first two bullet
> points in your original mail are covered by the command as-is?

Yes.  A key justification in my view is that these uses compound with
all commands (in Emacs or external) that substantially modify a buffer.

>>> Of your suggestions here, my first thought is that maybe C-x x g could
>>> start operating only on an active region, if there is one.  That seems
>>> like a great new feature.
>>
>> Yes, that sounds excellent.
>
> If you would be interested in working on that, I'd be very happy to
> review and shepherd the change.

Thanks, sounds good (although happy if others want to try, too).  A
related idea is to make vc-diff restrict to hunks in the active region
(in the "without arg" case).  Does that sound to you like a good change?

Message #38 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: "Paul D. Nelson" <ultrono <at> gmail.com>
Cc: 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Wed, 09 Apr 2025 10:48:43 +0200

Hello Paul and Sean,

I want to hint at the Gnu Elpa package "vundo" that is able to
Ediff undo states.

It's ok if you aim at a different interface - but I want to avoid that
at the end you are more or less end developing the same thing again.


Michael.

Message #41 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: "Paul D. Nelson" <ultrono <at> gmail.com>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Wed, 09 Apr 2025 18:38:08 +0800

Hello,

On Wed 09 Apr 2025 at 10:48am +02, Michael Heerdegen via "Bug reports for GNU Emacs, the Swiss army knife of text editors" wrote:

> Hello Paul and Sean,
>
> I want to hint at the Gnu Elpa package "vundo" that is able to
> Ediff undo states.
>
> It's ok if you aim at a different interface - but I want to avoid that
> at the end you are more or less end developing the same thing again.

Huh.  Thanks!  Paul, could you take a look?

-- 
Sean Whitton

Message #44 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Michael Heerdegen via "Bug reports for GNU Emacs, the Swiss army knife
 of text editors" <bug-gnu-emacs <at> gnu.org>
Cc: 77580 <at> debbugs.gnu.org, "Paul D. Nelson" <ultrono <at> gmail.com>
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Wed, 09 Apr 2025 15:17:55 +0200

Michael Heerdegen via "Bug reports for GNU Emacs, the Swiss army knife
of text editors" <bug-gnu-emacs <at> gnu.org> writes:

> I want to hint at the Gnu Elpa package "vundo" that is able to
> Ediff undo states.

Small correction: currently it uses diff.  It's possible to get Ediff
too, though.  The author of vundo is open to good ideas, so it's at
least an option to improve this (quite nice) package to make it even
better.


Michael.

Message #50 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: "Paul D. Nelson" <ultrono <at> gmail.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: michael_heerdegen <at> web.de, 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Wed, 09 Apr 2025 22:27:24 +0200

Hi Michael and Sean,

Many thanks for the pointer to vundo.  It's a great tool for navigating
undo histories visually.

The commands I've proposed (ediff-undo and diff-with-undo) serve a
somewhat different niche, primarily addressing a simpler use case:
quick, one-off comparisons with recent undo states, typically one or two
steps back.  Their implementation is straightforward and minimal.  vundo
is more innovative, but also more complex.  For the common scenarios
where I've found the proposed commands useful, vundo would generally be
overkill.

Regarding the "how far back to undo?" problem that Sean raised earlier
in this thread, vundo indeed seems like a natural interface.  It might
fully address that problem after a couple enhancements: Ediff support
(as you mentioned, likely straightforward) and region-sensitivity
(potentially more involved).

Paul

Message #53 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: "Paul D. Nelson" <ultrono <at> gmail.com>
Cc: michael_heerdegen <at> web.de, 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Thu, 10 Apr 2025 09:21:35 +0800

Hello,

On Wed 09 Apr 2025 at 10:27pm +02, Paul D. Nelson wrote:

> Many thanks for the pointer to vundo.  It's a great tool for navigating
> undo histories visually.
>
> The commands I've proposed (ediff-undo and diff-with-undo) serve a
> somewhat different niche, primarily addressing a simpler use case:
> quick, one-off comparisons with recent undo states, typically one or two
> steps back.  Their implementation is straightforward and minimal.  vundo
> is more innovative, but also more complex.  For the common scenarios
> where I've found the proposed commands useful, vundo would generally be
> overkill.

It might be more complex, but in general we really don't want to add
functionality to Emacs core that is already available on ELPA, because
we are working towards shipping the whole of GNU ELPA with our release
tarballs.  Then we would be shipping the same functionality twice.

So if vundo can already do what ediff-undo and diff-with-undo are for,
then I'm afraid we probably shouldn't continue working on the latter
two.  I hope this is not too disappointing.

-- 
Sean Whitton

Message #56 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: "Paul D. Nelson" <ultrono <at> gmail.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: michael_heerdegen <at> web.de, 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Thu, 10 Apr 2025 07:23:57 +0200

Hi Sean,

> It might be more complex, but in general we really don't want to add
> functionality to Emacs core that is already available on ELPA, because
> we are working towards shipping the whole of GNU ELPA with our release
> tarballs.  Then we would be shipping the same functionality twice.

I understand the goal of avoiding duplicated functionality with ELPA -
thanks for clarifying.

> So if vundo can already do what ediff-undo and diff-with-undo are for,

I don't think "can already do" applies here (and not just because vundo
currently lacks Ediff and region support).  To clarify, here's the
equivalent of M-x diff-with-undo with vundo:

  M-x vundo, m, b, d, q, C-x b *vundo-diff<TAB>.

These ergonomics scale nicely when exploring undo histories, but for
quick comparisons, they're hardly better than "fully manual" (copying to
a temp buffer and doing diff-buffers).

It's like asking "vc-annotate can show diffs, so why do we need
vc-diff?"

Should diff-with-undo be a "shortcut" command built into vundo rather
than Emacs?  I don't think so: this would bifurcate vundo's interface,
add some complexity, and wouldn't meaningfully reuse any existing vundo
code.  I think the proposed implementation is at the appropriate level,
alongside commands like diff-buffer-with-file.

Thanks, best,

Paul

Message #59 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: "Paul D. Nelson" <ultrono <at> gmail.com>
Cc: michael_heerdegen <at> web.de, 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Thu, 10 Apr 2025 15:16:47 +0800

Hello,

Okay.  Let me take some time to read this bug again and think about it.

-- 
Sean Whitton

Message #62 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: michael_heerdegen <at> web.de, 77580 <at> debbugs.gnu.org, ultrono <at> gmail.com
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Thu, 10 Apr 2025 10:35:19 +0300

> Cc: michael_heerdegen <at> web.de, 77580 <at> debbugs.gnu.org
> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Date: Thu, 10 Apr 2025 09:21:35 +0800
> 
> On Wed 09 Apr 2025 at 10:27pm +02, Paul D. Nelson wrote:
> 
> > Many thanks for the pointer to vundo.  It's a great tool for navigating
> > undo histories visually.
> >
> > The commands I've proposed (ediff-undo and diff-with-undo) serve a
> > somewhat different niche, primarily addressing a simpler use case:
> > quick, one-off comparisons with recent undo states, typically one or two
> > steps back.  Their implementation is straightforward and minimal.  vundo
> > is more innovative, but also more complex.  For the common scenarios
> > where I've found the proposed commands useful, vundo would generally be
> > overkill.
> 
> It might be more complex, but in general we really don't want to add
> functionality to Emacs core that is already available on ELPA, because
> we are working towards shipping the whole of GNU ELPA with our release
> tarballs.  Then we would be shipping the same functionality twice.

There are exceptions, though.  VC vs Magit comes to mind, and there
are others.

Basically, there's nothing wrong with having in Emacs a feature which
is simple and supports a subset of what an ELPA package offers.

Message #65 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: "Paul D. Nelson" <ultrono <at> gmail.com>
Cc: 77580 <at> debbugs.gnu.org, Sean Whitton <spwhitton <at> spwhitton.name>
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Thu, 10 Apr 2025 16:42:58 +0200

Hello,

>   M-x vundo, m, b, d, q, C-x b *vundo-diff<TAB>.
>
> These ergonomics scale nicely when exploring undo histories, but for
> quick comparisons, they're hardly better than "fully manual" (copying
> to a temp buffer and doing diff-buffers).

My main concern is whether your proposed interface is really convenient.
Sure, vundo is for exploring, and requires hitting more keys.  But in my
experience, most of the time some exploration _is_ required: undo steps
are small or tiny, and later even may be amalgamated in a partly random
way.  So the problem is that often that the last undo step is not what
you think would be, and you don't know how far you must look back.  Then
you end up guessing or bisecting the prefix arg range.  Only to see that
it would have been easier to use a different method.

I don't say vundo is the optimal approach, but I would suggest to
experiment a bit to find out what is really helpful, before installing
anything.

As a start I find the idea of a separate "slave" buffer where one can
undo and redo without messing up the original buffer appealing.  I would
play back until the state I need stuff from or want to compare with, and
then invoke Ediff manually to copy the changes I want to restore.

Apart from that, something to visualize the latest undo history is
definitely useful, too.  But personally I would rather like to see
something like a popup displaying a chain (history) of poststamp like
diffs, or ediff like side-by-side views, and then clicking on them would
give me a diff or an Ediff session.  Wouldn't that be nice?


Michael.

Message #68 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: "Paul D. Nelson" <ultrono <at> gmail.com>
Cc: 77580 <at> debbugs.gnu.org, Sean Whitton <spwhitton <at> spwhitton.name>
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Thu, 10 Apr 2025 19:01:31 +0300

> In practice, I've just spammed C-u enough times.  I considered allowing
> a negative argument to signal "undo everything", which would be most
> useful with an active region; how does that sound?

To me a negative argument would be the most important feature
of the new command 'diff-with-undo'.  Let me explain my workflow:
sometimes I open a file with uncommitted changes and make
more changes before committing it, but then later discover that
some changes were wrong, so need to revert to the previous state.

There is no other command to do this: 'vc-diff' includes other
uncommitted changes from other editing sessions, there is no backup for
'diff-backup', 'diff-buffer-with-file' can't compare the saved buffer.

So a negative argument for 'diff-with-undo' would be perfect to show
a diff with all recent changes.  Then could reverse the diff and remove
some hunks by applying them from the reverse diff, etc.

Message #71 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: "Paul D. Nelson" <ultrono <at> gmail.com>
Cc: Michael Heerdegen <michael_heerdegen <at> web.de>, 77580 <at> debbugs.gnu.org
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Fri, 11 Apr 2025 18:40:35 +0800

Hello,

On Thu 10 Apr 2025 at 04:42pm +02, Michael Heerdegen wrote:

> Hello,
>
>>   M-x vundo, m, b, d, q, C-x b *vundo-diff<TAB>.
>>
>> These ergonomics scale nicely when exploring undo histories, but for
>> quick comparisons, they're hardly better than "fully manual" (copying
>> to a temp buffer and doing diff-buffers).
>
> My main concern is whether your proposed interface is really convenient.

I was about to write pretty much the same thing: I think that these
commands are not yet fully baked.  I think that they are too difficult
to use.

The problem is, it seems too difficult for the user to know what prefix
argument to supply.  We don't keep a count of buffer changes in our
heads so that we could know how many changes back we need to go to get a
diff that will contain the information we want.

In fact, showing a diff for a given number of undo operations backwards
seems more like a function to be called from Lisp than an interactive
command, because Lisp could calculate the number of operations to go
back, somehow.

If there is a more minimal version of vundo that is accessible and
generally useful, we haven't found it yet.  It's fine if this takes some
time, we can keep the bug open and let the ideas simmer and come up with
something really good.

-- 
Sean Whitton

Message #74 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: "Paul D. Nelson" <ultrono <at> gmail.com>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: michael_heerdegen <at> web.de, 77580 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Sat, 12 Apr 2025 23:15:26 +0200

Thanks so much for taking the time to think and offer feedback.

>> often that the last undo step is not what you think would be, and you
>> don't know how far you must look back

> We don't keep a count of buffer changes in our heads so that we could
> know how many changes back we need to go to get a diff that will
> contain the information we want.

The proposed commands can be useful when the number of undo steps is 1,
2 or infinity.  Examples:

1. After a buffer reverts because a file changes.

2. When you need to revert, perhaps just in a region, across file-save
boundaries without suitable VC or backup endpoints available, or (Juri's
example) across file and VC boundaries within a session.  (Good reason
to add the "minus prefix" option -- thanks Juri.)

3. After any complicated Emacs command.  Suppose you've just done a
query-replace-regexp with "!" that had unexpected effects.  As an
alternative to undoing and redoing the query-replace-regexp more
carefully, you can ediff-undo.  This is really a family of examples, one
for each complicated Emacs command.

In these cases, the ergonomics seem good to me.  For cases (1) and (2),
I'm not aware of an available alternative.

It's been pointed out that the proposed commands are unergonomic when
the prefix arg is large but not infinite.  In my view, there's nothing
intrinsically wrong with that.  By comparison, undo itself is not a
catch-all -- it's often better to use revert-buffer, backup files, VC,
or vundo.  We could steer the user away from such unergonomic cases by
prepending the new commands with

  (when (>= arg 3)
    (user-error "Find a better way"))

Jokes aside, can we improve the ergonomics in the "large but finite
argument" case without effectively reinventing vundo?  My first thought
(following Michael's "slave buffer" suggestion) is to consolidate the
two proposed commands into a single command, undo-preview, that displays
a read-only buffer *undo-preview* containing a clone of the current
buffer after ARG undo steps, with a transient keymap binding b, f, d, e,
q, and RET to undo, redo, diff, ediff, quit and apply.

Michael's more ambitious suggestion (pop-up views showing a visual
history) sounds excellent, but I don't have immediate thoughts on how to
achieve it, and wonder if it'd fit more naturally as a vundo
enhancement.  I've tried to keep the current proposal minimalist.

Message #77 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: "Paul D. Nelson" <ultrono <at> gmail.com>
Cc: michael_heerdegen <at> web.de, 77580 <at> debbugs.gnu.org, juri <at> linkov.net
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Sun, 13 Apr 2025 11:13:57 +0800

Hello,

On Sat 12 Apr 2025 at 11:15pm +02, Paul D. Nelson wrote:

> Thanks so much for taking the time to think and offer feedback.
>
>>> often that the last undo step is not what you think would be, and you
>>> don't know how far you must look back
>
>> We don't keep a count of buffer changes in our heads so that we could
>> know how many changes back we need to go to get a diff that will
>> contain the information we want.
>
> The proposed commands can be useful when the number of undo steps is 1,
> 2 or infinity.  Examples:
>
> 1. After a buffer reverts because a file changes.
>
> 2. When you need to revert, perhaps just in a region, across file-save
> boundaries without suitable VC or backup endpoints available, or (Juri's
> example) across file and VC boundaries within a session.  (Good reason
> to add the "minus prefix" option -- thanks Juri.)
>
> 3. After any complicated Emacs command.  Suppose you've just done a
> query-replace-regexp with "!" that had unexpected effects.  As an
> alternative to undoing and redoing the query-replace-regexp more
> carefully, you can ediff-undo.  This is really a family of examples, one
> for each complicated Emacs command.
>
> In these cases, the ergonomics seem good to me.

I'm afraid I can't agree.  I'm glad they work out for you, but I'm not
sure they will for others.  For example, if a complicated command or
external process has made large edits to the buffer you might make
further, small edits yourself before even realising that large edits had
been made.  And then you're into counting undo operations again.

-- 
Sean Whitton

Message #80 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Paul Nelson <ultrono <at> gmail.com>
Cc: 77580 <at> debbugs.gnu.org, Sean Whitton <spwhitton <at> spwhitton.name>
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Tue, 15 Apr 2025 18:18:43 +0200

Paul Nelson <ultrono <at> gmail.com> writes:

> And (with apologies for the spam) a further, hopefully final update,
> that works in narrowed buffers.

Some comments about your code, independent of how the end result will
look like:

 - Consider to handle an active region similar to how you handle
 narrowing: undo is able to restrict undoing to the region.

 - There is a potential problem with the target buffers of
 `undo-to-buffer': they share the `buffer-file-name' with the original
 buffer.  If they are modified, Emacs will potentially save the contents
 of these buffers to that file, resulting in a mess.

 - Since you mentioned reverting as a use case: doesn't the undo history
 get lost when a buffer is reverted?  I remember that this is the case
 at least sometimes.


Michael.

Message #83 received at 77580 <at> debbugs.gnu.org (full text, mbox):

From: "Paul D. Nelson" <ultrono <at> gmail.com>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: 77580 <at> debbugs.gnu.org, spwhitton <at> spwhitton.name
Subject: Re: bug#77580: [PATCH] New command ediff-undo
Date: Tue, 22 Apr 2025 15:20:27 +0200

Hi Michael, thanks for your feedback.

>  - Consider to handle an active region similar to how you handle
>  narrowing: undo is able to restrict undoing to the region.

I think feature is already there in my patch -- if the region is active,
then it previews undoing in the region, just like undo.  Would be happy
to hear if I missed something.

>  - There is a potential problem with the target buffers of
>  `undo-to-buffer': they share the `buffer-file-name' with the original
>  buffer.  If they are modified, Emacs will potentially save the contents
>  of these buffers to that file, resulting in a mess.

I tested it just now with ediff-undo, and it seems on my end that
buffer-file-name is new in the target buffer.  If I try saving, then I'm
prompted for a file name.  Again, happy to hear if I missed something.

>
>  - Since you mentioned reverting as a use case: doesn't the undo history
>  get lost when a buffer is reverted?  I remember that this is the case
>  at least sometimes.

I'm not familiar with how reverting affects undo history.  I see it as a
mostly orthogonal issue (since the idea is to use previewed undo in a
region as a region-specific way to preview reverting, rather than to use
existing reverting functions), but I'll keep this in mind.  Thanks!

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.