vundo-diff

[jdtsmith]

This provides vundo-diff support with the following capabilities:

• It's optional, enabled by the vundo-diff-mode global minor mode (now builtin)
• Adds keys (m)ark, (u)nmark, and (d)iff
• Marked node has a separate (new) color, blue by default
• Diff pair defaults to parent if no node is marked
• Automatically swaps states based on index
• Includes additional color-coded header information in the diff file, including save timestamp (if available)

[casouri]

I'm ok with setting these keybindings by default in vundo.el, and autoload them in vundo-diff.el.

[casouri]

That way we don't need a minor mode.

[jdtsmith]

OK, great, done. Should we declare-function in vundo.el to avoid linter issues? I'm always worried autoloads won't get compiled or run correctly (for no good reason).

[jdtsmith]

Note that I had missed a key conflict, and moved (d)ebug to (D)ebug.

[casouri]

I know what you want to to, but are you sure this is the right way? Don't you need to assign vundo-highlight-saved-nodes to some variable?

[jdtsmith]

This basically means you only get "Saved at: " in the diff buffer when you have enabled highlight-saved-nodes. But that's sort of silly; I have removed it. We are storing TS data anyway. I also update vundo.el so that TS info is always calculated (including on save), whether or not we are showing it. No sense mixing display and backend logic.

[casouri]

Thanks, looks nice! But please make sure only indent with spaces, I'm seeing tabs again. Apart from the comments above, I also see terminal color codes in the terminal. I know you are using Emacs facility for diffing, is there anything we can do? Or at least teach users how to resolve it in the docs.

[jdtsmith]

Thanks. Indent fixed. For your color codes in terminal, maybe try (setq diff-switches "-u --color=never")?

The one other feature I considered overnight is just replacing those terrible tmp file names with the Current/Marked (with TS). But it sounds a bit fragile, so maybe not worth it for the improved cleanliness.

[jdtsmith]

I want to double-check. Is it your understanding that you need to (vundo--refresh-buffer ... 'incremental) after every move-to-node like this? Somehow I got the impression that this is needed only once (i.e. you can move as much as you like). But I get aref and no route errors without both of these calls.

[jdtsmith]

Actually maybe this was because I wasn't doing trim-undo-list. Just added that and dropped to a single refresh-buffer and all seems OK.

[jdtsmith]

I take it back. I get no route without a refresh-buffer after each move. Restored that. Any idea why? Perhaps because I'm "reversing direction", i.e. moving from current->marked then marked->current?

[jdtsmith]

I have added a sanity check wrapper prior to the moves, and pushed some README/NEWS changes for your consideration. I'm still playing with the formatting in the diff buffer, will wrap that up soon.

[jdtsmith]

I made a few more improvements today:

• Turns out that diff can be called async, and if so, I need to update/cleanup the diff buffer in the process sentinel.
• Since vundo-diff displays timestamps independent of whether the user has green circles on (vundo-highlight-saved-nodes), we need last-saved-idx to always be saved (further separating display/other logic).
• Similarly, properly report the last-saved-idx timestamp for the node, when it is just the most recent saved buffer state — it gets a green circle, after all. (New function vundo--any-timestamp).
• Cleaned up the vundo-diff results buffer to remove the ugly tmp file names and substitute vundo-specific info. (New function vundo-diff--cleanup-diff-buffer).
• Created a tiny vundo-diff-mode derived from diff-mode, to fontify said file header info.
• Added declare-function's to silence compiler.

This PR is pretty well complete IMO, pending any further review.

[casouri]

Looks nice!

Turns out that diff can be called async, and if so, I need to update/cleanup the diff buffer in the process sentinel.

Why use the async version? IMO the sync version is perfectly fine and should be simpler to use, right?

And I wonder if we can use a single key to mark/unmark nodes, instead of using both m and u. You know, make it vundo-toggle-mark or something.

[jdtsmith]

Turns out that diff can be called async, and if so, I need to update/cleanup the diff buffer in the process sentinel.

Why use the async version? IMO the sync version is perfectly fine and should be simpler to use, right?

Since it's the default, people might be used to that behavior of diff. The cost to support async is not high: just a lambda for the process sentinel. And sometimes tmp files are across a tramp link. [Update: since all the undo data are in process, I don't buy my own argument. I've switched to a sync-call for simplicity. ]

And I wonder if we can use a single key to mark/unmark nodes, instead of using both m and u. You know, make it vundo-toggle-mark or something.

Good idea. Maybe just m. OK, just tried and I don't love it. The problem is this: I often cruise around a few nearby nodes, and set a few different marks in a row, and re-diff looking for "something specific". This would double the number of m's I have to hit.

We could make it so that m on an unmarked node sets is, m on a marked node unsets. But that's getting fancy/non-intuitive. I think m + u is pretty simple and clean, and leverages dired knowledge.

And if you don't mind taking a look up at my question about vundo-refresh-buffer, I'm still a bit confused about the need for that to be there twice.

[casouri]

We could make it so that m on an unmarked node sets is, m on a marked node unsets. But that's getting fancy/non-intuitive. I think m + u is pretty simple and clean, and leverages dired knowledge.

Ok, I trust your judgement here.

And if you don't mind taking a look up at my question about vundo-refresh-buffer, I'm still a bit confused about the need for that to be there twice.

Yeah, moving to a node pushes new undo entries onto undo-list, and we need to process those with vundo-refresh-buffer (update mod-list, etc).

[jdtsmith]

Thanks for the feedback.

And if you don't mind taking a look up at my question about vundo-refresh-buffer, I'm still a bit confused about the need for that to be there twice.

Yeah, moving to a node pushes new undo entries onto undo-list, and we need to process those with vundo-refresh-buffer (update mod-list, etc).

I think what confuses me is this note from the vundo--trim-undo-list doc:

We can call vundo--move-to-node multiple times between two vundo--refresh-buffer.

That doesn't seem to be the case here. I.e. you must call refresh-buffer after each of the pair of moves (there and back) or you get no route. Probably not important.

Found one more little doc tweak I pushed, so this is ready to go.

In terms of version bump, I do have a couple of small things I'd like to send in as a small additional PR once this is merged:

1. Make l key move to a prior saved node, if already on one (so you can l, l, l, to go back through saved states), and have it report the saved time too.
2. Ensure the pattern undo-to-node, save it, alter it correctly marks that node as saved: this requires checking all a node's equivalents nodes for any subsequent TS-ful mod. This does not solve the issue outlined in A possible approach to the "lost timestamp" issue #66, but that is rare in practice.

[casouri]

That doesn't seem to be the case here. I.e. you must call refresh-buffer after each of the pair of moves (there and back) or you get no route.

Oh, I would trust the me that wrote the comment much more than I trust the me right now. If the comment says so, we should be able to run move-to-node multiple times.

I think I know why you'll get "no route". One example: in an undo list [1 2 3 4 5], you'd have mod list [1 2 3 4 5], and if you go back to node 3, your undo list becomes [1 2 3 4 5 4' 3'], but without calling refresh, the mod list remains [1 2 3 4 5]. Now, if you want to go back to 5, there's no route available in the mod list (you can think of it as you can only go left in the list, so from 5 to 1-4, but not the other way around). Once you call refresh, the mod list becomes [1 2 3 4 5 4' 3'], now you have a route from 3 to 5 (in the form of 3' to 5, which is going left).

Good call though, I never thought about this before. I should add a comment somewhere calling this out.

So the branch is ready for a final review, right? It'd be great if you can squash everything and write a commit message. Then I'll review and merge it.

[jdtsmith]

I think I know why you'll get "no route".

Right, you can't change direction between refresh's.

I'll review and merge it.

Great thanks, commits squashed, ready for review. Once merged I'll submit my small PR for timestamp stuff (I'm already loving multiple l).

[casouri]

Ok, some final nits:

• Could you leave the defvar of vundo--orig-buffer and vundo--last-saved-idx in their original place, and just place (defvar vundo--last-saved-idx) in the timestamp section (to suppress compiler errors)?
• Could you write the commit message in GNU format, such that it explain what functions/variable are changed? For the new file (vundo-diff.el), simply saying "new file" is enough.
• I feel like the "current node" highlight (red bold) should override any other highlights, including the "marked node" highlight.
• Could you put (setq diff-switches "-u --color=never") somewhere in the documentation?

[jdtsmith]

OK should be all set, thanks for the review. I also threw in a docstring escape of single quotes to silence the compiler. Once this is merged I have a TS-related PR I'll post.

[casouri]

Merged, thanks!