jj icon indicating copy to clipboard operation
jj copied to clipboard

FR: jj undo ergonomics

Open benbrittain opened this issue 1 year ago • 14 comments

Is your feature request related to a problem? Please describe. The current behavior of the jj undo command does not seem to be what users intuitively expect. Currently if you run jj undo twice in a row it has the same behavior as jj op undo, it returns you to the same place you were when you started. undo + undo = no change.

Describe the solution you'd like I'd like to preserve the ergonomics of the underlying jj op undo command (as suggested by @PhilipMetzger on Discord), but have the higher level jj undo command track if a user immediately undoes an undo and warn them.

Describe alternatives you've considered Alternatively, the jj undo command could keep track of where you are in the undo log ctrl-Z style. That probably implies a redo command as well?

benbrittain avatar May 16 '24 22:05 benbrittain

I'd like to preserve the ergonomics of the underlying jj op undo command

Why make them different? Oh, I suppose the top-level jj undo would lose its optional operation argument?

martinvonz avatar May 16 '24 22:05 martinvonz

I'm actually starting to question if jj op undo is needed at all. I'd originally been modeling it as a lower level operation, but it's really just a special case of jj op restore, no? I can't imagine I'd ever use jj op undo if jj undo had ctrl-z semantics

benbrittain avatar May 16 '24 22:05 benbrittain

it's really just a special case of jj op restore, no?

No, jj undo @- undoes the second-to-last operation while leaving the changes from the last operation. For example, if you abandoned some commit, and then did a bunch of unrelated changes, you can still recover that commit with jj undo <old operation>. It can be hard to reason about, however, because it also brings back ancestors of the old commit when it makes the old abandoned commit visible.

martinvonz avatar May 16 '24 22:05 martinvonz

Think of it like jj backout but for operations (while jj op restore is jj restore for operations).

martinvonz avatar May 16 '24 22:05 martinvonz

To reword the FR a bit:

Improve the ergonomics of jj undo by making it a separate command, which can do undo tracking like editors. Currently running undo twice in a row, surprises a lot of users as it just rolls back the last operation (not surprising if you know that jj undo is an alias of jj op undo). This would then pave a way for a smart redo, which has the opposite behavior.

Describe alternatives you've considered Alternatively, the jj undo command could keep track of where you are in the undo log ctrl-Z style. That probably implies a redo command as well?

I think that this alternative would be layering violation in UI terms, as imo jj op undo shouldn't be aware of such things anyway. That's where the suggestion came from in Discord anyway.

PhilipMetzger avatar May 17 '24 18:05 PhilipMetzger

Would this have any impact on #3428 (op log waypoints)?

joyously avatar May 17 '24 19:05 joyously

Would this have any impact on #3428 (op log waypoints)?

The higher level jj undo could probably integrate with them, which then simplifies the UX again. op log Waypoints will just be another nice building block for it.

PhilipMetzger avatar May 17 '24 19:05 PhilipMetzger

I think undo is the wrong idiom to think of these things with. The op log is a continuous stream of states that is append only. Attempts to imagine the current state as a pointer to somewhere in this history of operations is appealing, but in truth you are creating a new state at the head that happens to have identical content to an earlier state.

I would recommend removing undo entirely and instead building on restore as the fundamental unit. Give it good usability so you can say restore <timestamp> and then make it clear that what this command does it creates a new state now that has exactly the same contents as the system had at <timestamp>.

fowles avatar Jun 15 '24 18:06 fowles

I would recommend removing undo entirely and instead building on restore as the fundamental unit.

Are you saying that jj op undo <not latest operation> is too hard to understand? I think that's fair. I practically never use it myself.

By the way, I find the suggestion of making jj undo behave differently from jj op undo confusing. That would be resolved by removing jj op undo. Another option is to rename jj op undo to jj op backout (matching jj backout just like jj op restore matches jj restore). Maybe we should start with that rename.

martinvonz avatar Jun 16 '24 00:06 martinvonz

The op log is a continuous stream of states that is append only. Attempts to imagine the current state as a pointer to somewhere in this history of operations is appealing, but in truth you are creating a new state at the head that happens to have identical content to an earlier state.

I agree with this, but still think that jj undo should be kept with the suggested semantics.

By the way, I find the suggestion of making jj undo behave differently from jj op undo confusing. That would be resolved by removing jj op undo. Another option is to rename jj op undo to jj op backout (matching jj backout just like jj op restore matches jj restore). Maybe we should start with that rename.

That sounds great and should make the implementation simpler as it can just shell out to op restore.

PhilipMetzger avatar Jun 19 '24 17:06 PhilipMetzger

I have found jj op undo to be occasionally useful, but I like the idea of renaming it to jj op backout.

I'm not 100% sure what jj undo should do if we rename jj op undo to backout until we create a fancy new behavior. It would be a bit sad to get rid of it. I suppose it could be a version of jj op backout that only works on the last operation, and explains to the user that 1) repeating jj undo will undo the undo 2) try jj op log and jj op restore for anything fancy.

I am also not sure what a new and fancy jj undo would do, exactly. I wrote up a long explanation of how (I think) it worked in Emacs, but that made me realize that almost any text-editor-like undo behavior will run into the same problem: what if you do a bunch of undo-s, and then a non-user-initiated operation happens unexpectedly (like snapshotting from jj log running on a timer or because watchman notices something changed)?


Here's the write-up about the old Emacs behavior (or what I think it was)

I'd like to suggest the old Emacs behavior for consideration: if the op log is at

root -> A -> B -> C

the first jj undo undoes C, and the second jj undo undoes A. At that point, the log would be

root -> A -> B -> C -> undo C -> undo B

Now, if you did another undo, it would undo A. If you do any operation D other than undo (or redo), however, the op log becomes

root -> A -> B -> C -> undo C -> undo B -> D

Now, if you start undoing, you will first undo D, then you'll undo "undo B", then you'll undo "undo A", and so on.

The advantage of this is that it works well with an append-only op log, jj op log will tell you exactly what's going on.

There are some disadvantages, most notably: what do we do if "operation D" is an unexpected non-user-initiated operation, such as snapshotting?

ilyagr avatar Jun 20 '24 03:06 ilyagr

I am also not sure what a new and fancy jj undo would do, exactly. I wrote up a long explanation of how (I think) it worked in Emacs, but that made me realize that almost any text-editor-like undo behavior will run into the same problem: what if you do a bunch of undo-s, and then a non-user-initiated operation happens unexpectedly (like snapshotting from jj log running on a timer or because watchman notices something changed)?

I think we should be able to fix the Emacs use-case you mentioned with separately tagging the op log transactions to distinguish them from automation and human interaction, like #3428 wants. In my opinion we deserve an actual client/daemon for jj where direct forge integration and a VFS is built in and then having the option to tag where an op came from will be needed.

PhilipMetzger avatar Jul 01 '24 17:07 PhilipMetzger

Here’s a nice exposition of a completely linear undo feature that never loses history, even as you perform new changes on top of undos; all past states remain accessible at all times: Resolving the Great Undo-Redo Quandary. I don’t know if it would work for Jujutsu, but we should at least consider and internalize its lessons when thinking about this.

(It’s possible Emacs has behaviour similar to this; I’m not sure.)

emilazy avatar Jul 01 '24 18:07 emilazy

Just spent a couple min trying to repeatedly jj undo to go back through the op log until I remembered that it was just going back and forth :) so +1 - at least in the simple case, repeated jj undo should find the first non-undo operation to undo.

rdamazio avatar Sep 20 '24 22:09 rdamazio

Just spent a couple min trying to repeatedly jj undo to go back through the op log until I remembered that it was just going back and forth :) so +1 - at least in the simple case, repeated jj undo should find the first non-undo operation to undo.

jj-fzf has an implementation of jj undo along the jj op log: screencast When you hit Alt+Z it will pick the first operation from the jj op log that is: a) not an undo op itself, b) has not been subject to a previous undo. That way it effectively implements multi-step undo. The op-log view (Ctrl+O) even shows the jj op log with undo/undone operations marked '-' instead of '○' so using that it is easy to see which operation will be the next subject to Alt+Z.

Since that makes it easy to test how far multi-step undo can be pushed, I tested it a couple of times and ran into cases that cannot be undone:

$ jj op log
[...]
○  0fe687841521 timj@fury 1 day ago, lasted 4 milliseconds
│  import git head
│  args: jj log -s -r ::@
○    efa2eba75c6a timj@fury 1 day ago, lasted 2 milliseconds
├─╮  reconcile divergent operations
│ │  args: jj --ignore-working-copy show --tool true '--color=always' -r @ -T '
│ │        "[@ " ++ concat(
│ │           separate(" ",
│ │             format_short_change_id_with_hidden_and_divergent_info(self),
│ │             format_short_commit_id(commit_id),
│ │             bookmarks,
│ │             if(conflict, label("conflict", "conflict")),
│ │           ),
│ │         ) ++ "]"'
○ │  bc183f57c1b7 timj@fury 1 day ago, lasted 16 milliseconds
│ │  new empty commit
│ │  args: jj new 77d57b3ff6c14698967f312be220c298f67f409f
○ │    e997ad6b277b timj@fury 1 day ago, lasted 7 milliseconds
├───╮  reconcile divergent operations
│ │ │  args: jj --ignore-working-copy show --tool true '--color=always' -r @ -T '
│ │ │        "...[@ ...]"'
○ │ │    037dc2bfb109 timj@fury 1 day ago, lasted 8 milliseconds
[...]
$ jj op undo efa2eba75c6a69a0
Error: Cannot undo a merge operation

So unless there was a way to reliably linearize the op log, endless undo (redo) doesn't really work in practice. Or could the logic I mentioned above be extended to ignore additional steps and continue undo beyond this point?

tim-janik avatar Nov 25 '24 02:11 tim-janik

I would recommend removing undo entirely and instead building on restore as the fundamental unit.

I think it would be a mistake to remove undo. It is so common for newcomers to express their delight when they learn they can jj undo anything.

jj undo is easy to understand with nearly no training (besides that it only works once). jj op restore requires one to first understand what the op log means.

Let people get excited by jj undo, then I think they'll learn jj op restore when they hit its limitations. Better ergonomics like jj op restore --to "monday" would still be great additions.

The op log is a continuous stream of states that is append only. Attempts to imagine the current state as a pointer to somewhere in this history of operations is appealing, but in truth you are creating a new state at the head that happens to have identical content to an earlier state.

Is this a fundamental requirement of the op log? I don't know the data structures so I don't know if it's actually feasible, but if the "current operation" could be a pointer to an old operation, instead of always being the head, then I think undo/redo would be way easier to handle:

  • jj undo would move the op@ pointer back by one operation (with some assistance if there are two parents).
  • jj redo would move the op@ pointer forward by one operation (with some assistance if there are two children).

This would create the semantics I think people expect:

  • jj undo always goes "back in time"
  • jj redo works as long as the last change was a jj undo
  • If you commit a new operation after a few undos, jj redo stops working (since you're now at a head) and jj undo would now undo that operation, then continue backing up from there.
    • If you did this by mistake, you could still recover with jj op restore

If it's not feasible, then I think we'd essentially need to store the undo tree alongside the op log to achieve the following:

  • jj undo "skips over" undo operations. If you have operations A->B->C->@ and @ is an undo of C, then jj undo actually restores to A.
  • jj redo only works if @ is an undo and has similar skipping behavior: A -> B -> C -> @ where @ is an undo of C (and therefore equivalent to B), then jj redo restores to A.

jennings avatar Dec 04 '24 22:12 jennings

And btw, just emitting a warning when you jj undo something that was itself an undo operation would probably go a long way to prevent confusion, even if we don't change the ergonomics :)

rdamazio avatar Dec 05 '24 00:12 rdamazio

Is this a fundamental requirement of the op log?

I would say that it is. @tim-janik just reminded me of this message from me: https://github.com/martinvonz/jj/pull/3455#issuecomment-2038914602. I think that provides some useful context.

martinvonz avatar Dec 05 '24 02:12 martinvonz

I'm wondering if a variation of @jennings 's idea might be viable:

Let's start with the oplog A->B->C. (So, @=C initially)

If the current operation is not undo/redo, jj undo works as it now does. Perhaps we would have it print something like:

$ jj undo
Undid operation C: frobnicate the repo
Do `jj undo --again` to undo operation B: prepare for frobnication

Now, our oplog consists of 4 operations: A->B->C->"undo C", and the current operation is @="undo C".

When the current operation is an undo, I'd forbid jj undo entirely (this is somewhat tangential to the rest of the plan) and require the user to do jj undo --again (better names?) to continue the sequence of undoes. jj undo --again would read the description of the current operation, "undo C", and undo the parent of C, B in this case. As normal (and as we do now), this will be a new operation on top of the operation log. So, then the current operation would be "undo B", and another jj undo --again would undo "A".

jj undo --again would fail if the current operation is not undo/redo.

We could also have jj redo that is only allowed if the current operation is an undo or a redo. It would see that the current operation undoes A and redo A (this is in fact equivalent to what jj undo does now if the top operation is an undo, but I find this simpler to think about if we record this operation as "redo A" in the oplog instead of "undo undo"). After such a redo, jj undo --again[^name] would undo the operation A again, describing the operation as "undo A" (a new operation on top of the op log, like any other operation).

The motivation for requiring --again if the top operation is actually mostly to disallow jj undo --again if the top operation is not an undo, but some concurrent or unexpected operation that breaks a chain of undoes. I don't want the user to unexpectedly undo such an operation; instead they should be informed of the problem. If the unexpected operation doesn't interfered with what they were undoing, they can look up the last successful jj undo, and then do jj undo <the-parent-of-last-undone-operation>, which could be told to them by an error message (by scanning the oplog for the last undo operation).

Update: To relate this back to Stephen's idea, conceptually this stores a pointer to another oplog operation inside the oplog record of the undo operation. We could keep the oplog the same as it is now, and parse the undo pointer from the description of the oplog operation, or we could add a new field to the oplog proto that would only be populated for undo/redo operations and point to the operation being undone/redone.

[^name]: After jj redo, "--again" is not a great name, but I don't have a better idea; "--chain" seems worse.

Instead of `--again`, another option would be to require `jj undo --head` or `jj undo <rev>` to start a sequence of undoes, and have a plain `jj undo` do what I describe as `jj undo --again`. This would be a more breaking change, but otherwise might be better?

ilyagr avatar Dec 05 '24 03:12 ilyagr

I sort of like Ilya's plan, but not this part

When the current operation is an undo, I'd forbid jj undo entirely (this is somewhat tangential to the rest of the plan)

because the default would be @ and that should make sense, and novices are the ones most likely to do undo twice so it should have an intuitive result, and sometimes you just want to undo the undo. If the undone operation is a rebase or split or whatever, does undoing it propagate the changes (as I expect it would)? If so, the old commits still exist? (I don't really understand when garbage collection is done.) I'm wondering if the redo uses the old commit or makes new ones.

joyously avatar Dec 05 '24 15:12 joyously

@ilyagr's solution seems reasonable to me.

The motivation for requiring --again if the top operation is actually mostly to disallow jj undo --again if the top operation is not an undo, but some concurrent or unexpected operation that breaks a chain of undoes. I don't want the user to unexpectedly undo such an operation; instead they should be informed of the problem.

I see this concern, but I don't think the user will appreciate the difference. If I understand correctly, only one of jj undo and jj undo --again can ever succeed, so it feels a bit like, "Nuh-uh, you didn't say the magic word."

  • If the user accidentally creates a new operation while making an undo chain, then they're probably fine undoing that, because by running jj undo again we know their intention is to keep going back in time.
  • If the user purposefully creates a new operation, then it's reasonable to expect them to know that jj undo will now undo that change.

jj undo could instead look at the head op and automatically do the "again" logic if it was an undo operation.

jj undo --again would read the description of the current operation, "undo C", and undo the parent of C, B in this case.

The initial version could parse the operation description, but we might eventually want this to be some kind of structured metadata, either part of the operation itself or linked to it (like git notes).

jennings avatar Dec 05 '24 16:12 jennings

In the meantime, I have updated the jj-fzf undo implementation following @jennings suggestion to use a marker instead of the (slightly involved) procedure I described earlier:

  • Each time Alt+Z is pressed in the jj-fzf op-log, jj undo is executed for the operation that is the parent of the last undo operation executed. Then this op is saved via jj config set --repo jj-fzf.last-undo $OP_ID.
  • There is a check in place, so that the undo operation to be carried out is only taken to be the parent of the jj-fzf.last-undo marker, if the marker matches the topmost (last) operation being undone. Otherwise jj undo works as usual and the topmost op will be undone.

That alone works well enough for reliable multi-step undo, as long as the operation history is linear. If the operation history fans out (due to concurrent repository changes), there is no reliable way to reconstruct older repository states, AFAICS. And jj undo simply refuses to operate at that point.

Below is a screenshot of the op log in jj-fzf after 8 consecutive undo operations (all marked ⋯ together with their respective undo operations). We have been having that in place for a couple weeks and the experiences so far are: a) While it might be nice to have multi-step undo, in the vast majority of cases a single step is sufficient. Except for the cases where a script action needs to be undone - since scripts (like jj-fzf) sometimes do jj new -B && jj squash or similar multi-step operations. b) To confidently engage in multi-step undo, you really want to have a live look at the op log, just to figure out at what point your repository is at in the history of modifications. If you think you need 4, 5 or more undo steps, you probably are better of using restore instead. c) In the very rare case where you want to redo a previous undo step, it feels odd to require some intermediate modifying operation (jj new or describe) just to "reset" the undo marker. That's why I added Alt+K to reset the marker in the op log, so the next Alt+Z is effectively a redo of the last undo.

All in all, I'm quite happy how undo/restore work in the jj-fzf op log view now. As for the jj undo CLI ergonomics, what I would suggest is:

  • Move the above described marker logic into the jj CLI and move jj-fzf.last-undo into jj core. This could look like adding jj undo --parent-of-last-undo which uses and updates the marker.
  • After a while you might want to make that the default jj undo behavior.
  • Add something along the lines of jj undo --reset-last-undo-marker.

And then, leave it at that. As stated above, more than a handful of successive undo ops are probably best left to UI tools with a live view, or are an indication that you probably want to use jj restore / jj op restore. Don't bother with attempts to flatten the operation history, at least not for jj undo when you generally only need a handful of consecutive steps anyway. And, as Martin keeps repeating, that is not what it was designed for anyway: The operation log allows to detect and merge divergent operations

multi-undo

tim-janik avatar Dec 13 '24 23:12 tim-janik

I like the article posted in this comment above: https://github.com/martinvonz/jj/issues/3700#issuecomment-2200738820 -- I think the current operation log in jj is pretty close to what it's describing.

The one major change to jujutsu would just be what the other comments have been saying. To summarize:

  • jj undo should move the undo pointer backward through the op log.
  • jj redo should move it forward.
  • The end state of running jj undo 3 times should be viewing the state of the repo as it was 3 operations before starting to undo.

I softly disagree with the idea that it's okay for the command-line to be hard to understand, because GUI tools will "pick up the slack". This is one of the many problems that makes git irritating to both newbies and power users. The CLI tools can be "basic" but they should be simple, understandable, and complete. If they're hard to understand or use, people may bounce before getting around to trying GUI tools.

robey avatar Dec 14 '24 19:12 robey

I like the article posted in this comment above: #3700 (comment) -- I think the current operation log in jj is pretty close to what it's describing.

I didn't understand the proposal in that article. In particular, why does the "type one character; undo" sequence double the undo stack?

  • jj undo should move the undo pointer backward through the op log.

I thought the article suggested recording the undo as its own operation instead of moving the pointer back (like we already do). Also see my comment above.

I softly disagree with the idea that it's okay for the command-line to be hard to understand, because GUI tools will "pick up the slack".

I think we all agree.

martinvonz avatar Dec 14 '24 22:12 martinvonz

I didn't understand the proposal in that article. In particular, why does the "type one character; undo" sequence double the undo stack?

Admittedly I didn't try to understand the specific encodings & optimizations they were using for their text editor. I read it as an enhancement of "canonical undo/redo" which allows undo-of-an-undo.

This may be the wrong place for a lengthy description, but I was talking to a UX friend about it this morning, and it seems like word processors perform undo/redo like this:

insert "the" --> insert "sleeping" --> insert "cat"
                                       ^
                                       current document
                                       "the sleeping cat"

After 2 undo (command-Z):

insert "the" --> insert "sleeping" --> insert "cat"
^
current document
"the"

Redo (shift-command-Z) twice will still work to bring back "the sleeping cat", or if you start typing "purring" here, the redo stack is lost forever, and you have:

insert "the" --> insert "purring"
                 ^
                 current document
                 "the purring"

I think the GURQ proposal is to behave the same way except to track the undo operations at the end (as jujutsu already does).

commit A --> commit B --> squash
                          ^
                          current tree

After two "undo" commands, the tree is back at the first commit:

commit A --> commit B --> squash --> undo squash --> undo commit B
^
current tree

Doing a new operation bumps the undo pointer to the end of the operation log again so that future undo/redo will treat the previous undo's (undone's?) as operations themselves, which can be navigated through.

commit A --> commit B --> squash --> undo squash --> undo commit B --> commit C
                                                                       ^
                                                                       current tree
  • jj undo should move the undo pointer backward through the op log.

I thought the article suggested recording the undo as its own operation instead of moving the pointer back (like we already do). Also see my comment above.

Yes, I think it does both:

  • undo/redo navigates forward/backward through the history ("canonical undo/redo")
  • if new work happens after an undo, the undo is recorded in the history, and the new work recorded after that (as jj currently does)

robey avatar Dec 15 '24 02:12 robey

Thanks for explaining. I still don't understand the exponential stuff, but maybe we don't need to understand it.

One idea is to make jj undo record in the operation that it represents an undo. Then the next jj undo will search the operation log backwards and count how many undos it finds before the first non-undo. It will then restore 2*n+1 operations back.

Perhaps a more reliable way is to record which operation was undone by which other operation. Then we would instead scan backwards until we find an operation that's not an undo and is not itself undone by a later operation.

martinvonz avatar Dec 16 '24 18:12 martinvonz

Does it make sense to go back to @rdamazio's suggestion of showing a warning as a stop-gap measure? The naming of jj undo and the current behaviour are, I think, unexpected for anyone who is more used to undo/redo in GUIs than in CLIs. (I say this as someone who did exactly the same thing that they did when I first tried to undo multiple changes!)

More concretely, I would expect something like the following:

  1. jj undo with no arguments always undoes the most recent entry in the op log. If this entry is itself an "undo", it additionally warns the user that jj undo may behave unexpectedly, and that if they want to restore the state of the repo at a particular point, they should instead do jj op log and jj op restore <...>. This behaviour should feel similar to the behaviour when jj rebase creates a conflict and provides hints on how to resolve those conflicts.
  2. jj op undo (and potentially jj undo <...>) should not print this warning — the expectation in this case is that the user knows what they are doing and doesn't need unnecessary noise in their terminal.

This still leaves some questions:

  1. Is it possible for jj undo to remain an alias of jj op undo, but still have this (slight) special case? i.e. can the cmd_op_undo function detect when it is being called as jj undo and when as jj op undo?
  2. Is this warning configurable at all? Power users may not need the warning and may want to disable it.
  3. How does the jj undo detect that the most recent entry was also an "undo" operation? That information is definitely already recorded in a human-readable way, do we also need to record it in a machine-readable way as well?

Importantly, this approach would not prevent JJ from implementing the "undo stack"-type semantics in the future. But this issue is already several months old, and I suspect getting this behaviour right will be tricky (albeit useful!). In the meantime, showing a warning helps a lot of new users who are expecting "undo" to behave differently.

I'm happy to work on a PoC PR that implements this behaviour, although it may not answer all of the above questions "correctly"! 😅

MrJohz avatar Feb 04 '25 10:02 MrJohz

I think that improving the power of jj undo could be very benefitial for newcomers and people with little VCS or even CLI experience. Here are two more incremental steps I think we can take in that direction without tackling the more difficult edge cases quite yet:

  1. As Martin suggested, rename jj op undo to jj op revert (now that jj backout was renamed to jj revert). This paves the way for jj undo to behave differently than jj op revert without causing confusion.

  2. Enable jj undo to undo multilpe operations in succession only in the simplest of cases and error out with a hint otherwise. pseudocode of the very simple algo:

fn jj_undo() {
    let parent_op = "@-";

    if !parent_op.is_revert_op() {
        // no undo stack to walk back, easy
        jj_op_revert(parent_op);
        return;
    }

    // check that the existing undo stack is very simple with nested pairs of
    // revert operations that revert earlier, non-revert operations:
    //
    // @
    // |
    // * ----+ (revert op)
    // |     |
    // * --+ | (revert op)
    // |   | |
    // * <-+ | (non-revert op)
    // |     |
    // * <---+ (non-revert op)
    // 
    let (left, right) = (parent_op, parent_op.revert_target());
    while left.is_descendant_of(right) {

        if !left.is_revert_op()
            || right.is_revert_op()
            || left.revert_target() != right
        {
            // left & right are NOT a matching pair, undo stack is not simple!
            println!("ERROR: cannot undo, this is too complicated");
            println!("HINT: use `jj op restore` or `jj op revert` instead");
            return;
        }

        // continue checking stack of nested pairs inwards
        left = left.parent()
        right = right.child()
    }

    // undo stack is simple. to grow the stack, we'd need to revert:
    let op_to_revert = parent_op.revert_target().parent();

    // but for the undo stack to remain simple, it can't be a revert operation:
    if op_to_revert.is_revert_op() {
        println!("ERROR: cannot undo a revert operation");
        println!("HINT: use `jj op restore` or `jj op revert` instead");
        return
    }

    // grow undo stack
    jj_op_revert(op_to_revert)
}

senekor avatar Jul 20 '25 22:07 senekor

I've recently learned that the operation log is not necessarily linear. For example, jj new --at-op=@- will create a merge operation with two parent operations. Anybody have some thoughts about how best to deal with this? Until we have a better idea, we should probably fail and tell the user to use op restore / op revert instead.

senekor avatar Aug 16 '25 17:08 senekor

I've recently learned that the operation log is not necessarily linear. For example, jj new --at-op=@- will create a merge operation with two parent operations. Anybody have some thoughts about how best to deal with this? Until we have a better idea, we should probably fail and tell the user to use op restore / op revert instead.

Yes, there have been several discussions about this in the past (often on discord, sometimes across channels). One major advancement was the idea to use a marker. Please read my comment above about the jj undo implementation in jj-fzf: https://github.com/jj-vcs/jj/issues/3700#issuecomment-2542561508

Initially I was playing around with complicated logic like your is_revert_op() above, but that turned out be fragile, or at least behave unpredictably for the user in some cases.

The marker idea by @jennings turned out to be simple to implement and importantly have predictable behavior for the user. I strongly recommend you test out the op log view in jj-fzf just to get a feel for multi-step undo and to gain visibility into the logic. As long as the op log history is linear, things work fine and feel predictable.

For non-linear op logs, the effort is simply not worth it, even if you find ways to technically minimize conflicts when flattening (required for a linear undo model), that'd still not feel predictable for the user, because most users don't have a mental concept of recent VCS operations they carried out in a non-linear fashion. And in practice the desire for undo seldomly coincides with non-linear operations, so this'd be rarely needed, infrequently tested, hard to predict and require complicated logic.

tim-janik avatar Aug 16 '25 18:08 tim-janik