Show a warning when undoing an undo operation #5580

MrJohz · 2025-02-04T14:06:57Z

One common issue that users run into with jj undo is that they expect it to behave like an "undo stack", in which every consecutive jj undo execution moves the current state further back in history. This is (currently) an incorrect intuition - running multiple jj undo commands back-to-back will only result in the previous non-undo change being reverted, then un-reverted, then reverted, and so on.

This change adds a hint when the user runs jj undo multiple times consecutively, suggesting that they may be looking for jj restore, which allows them to revert the whole state of the repository back to a previous snapshot.

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (cli/src/config-schema.json)
I have added tests to cover my changes

cli/src/commands/operation/undo.rs

PhilipMetzger

This works but I'd rather not see it here, like I've already talked about this morning in Discord. I won't block it, but it still should imo be in the new "freestanding" undo, instead of in the lower-level plumbing.

CHANGELOG.md

MrJohz · 2025-02-04T17:05:31Z

This works but I'd rather not see it here, like I've already talked about this morning in Discord. I won't block it, but it still should imo be in the new "freestanding" undo, instead of in the lower-level plumbing.

I kind of agree, this feels a bit ugly right now because of the "is this jj undo or jj op undo?" check. However, it feels worse to just duplicate most of the code between the two commands, because then they'll probably diverge more and they really should be doing the same thing.

Is there an existing way to delegate from one command to some other command? If there is, and it's not too complicated to get this working with model, I'm happy to try that approach. But if the alternative is just duplicating the code, then I'd rather just make the small change here.

PhilipMetzger · 2025-02-04T17:24:32Z

Is there an existing way to delegate from one command to some other command? If there is, and it's not too complicated to get this working with model, I'm happy to try that approach. But if the alternative is just duplicating the code, then I'd rather just make the small change here.

Beside extracting the core out of the current jj op undo and calling it both from "freestanding undo" and "op undo" there are not many variants, as making it dependent on a single parameter is probably quite ugly.

arxanas · 2025-02-04T23:44:05Z

I think this is overall a good hint to have (while we don't have a more ergonomic undo solution). It would be fine with me if it were implemented for both jj undo and jj op undo, but I don't feel strongly.

martinvonz · 2025-02-04T23:59:19Z

I kind of agree, this feels a bit ugly right now because of the "is this jj undo or jj op undo?" check. However, it feels worse to just duplicate most of the code between the two commands, because then they'll probably diverge more and they really should be doing the same thing.

I don't follow. You can just remove the command.matches().subcommand_matches("operation").is_none() bit to make it apply to both commands, right? Isn't that what we want?

martinvonz · 2025-02-05T00:03:27Z

Oh, that was a deliberate attempt to change only jj undo without changing jj op undo? I'd rather keep the same for now. I'd make it a separate change to rename jj op undo to jj op backout.

arxanas

(Oops, my inline comment got stuck, something seems wrong with the mobile app.)

arxanas · 2025-02-04T23:41:47Z

cli/src/commands/operation/undo.rs

+        if is_bare_undo && was_undo_operation {
+            writeln!(
+                ui.hint_default(),
+                "Consider using `jj restore` if your intention was to undo multiple operations"


issue: Message needs more detail. It should explain why it's being printed, why that's a problem, and only then the solution (written here). Example: "It appears you're attempting to undo an undo operation; this will leave the repo in the same state as when you started, rather than undoing the last two operations; consider..."

comment (non-blocking): I don't know if we have a general framework for defining hints and the configuration to silence them. If we do, then it would be good to use it.

I agree, I struggled to find a good concise way of explaining the problem. Therefore I've given up on conciseness, let's see if that helps! ;) The new text is as follows:

Hint: This action reverted an 'undo' operation. The repository is now in the same state as it was before the original 'undo'. Hint: If your goal is to undo multiple operations, consider using `jj op log` to see past states, and `jj op restore` to restore one of these states.

I have some colleagues who haven't really used JJ much before, I'd like to see how they react to this message and if it points them in the right direction. I should get a chance to do that today or tomorrow.

comment (non-blocking): I don't know if we have a general framework for defining hints and the configuration to silence them. If we do, then it would be good to use it.

I think the ui.hint_default() is the framework as it stands. This handles e.g. --quiet correctly (although this disables most output, as opposed to just disabling hints). Joining these together, and potentially allowing users to selectively disable them could be a good follow-up task, though.

MrJohz · 2025-02-05T08:51:40Z

cli/src/commands/operation/undo.rs

@@ -75,6 +75,20 @@ pub fn cmd_op_undo(
        let template = tx.base_workspace_helper().operation_summary_template();
        template.format(&bad_op, formatter.as_mut())?;
        writeln!(formatter)?;
+
+        let was_undo_op = *tx.repo().view() == parent_op.view()?;
+        if args.operation == "@" && was_undo_op {


This is a review to discuss the op undo vs undo question, but I think at this point we're fairly decided: op undo and undo now behave identically, and both will display the hint.

The hint will still not be displayed if the user attempts to undo a specific operation (e.g. jj [op] undo 1234abc), with the assumption that they know what they're doing in that case.

One common issue that users run into with `jj undo` is that they expect it to behave like an "undo stack", in which every consecutive `jj undo` execution moves the current state further back in history. This is (currently) an incorrect intuition - running multiple `jj undo` commands back-to-back will only result in the previous non-undo change being reverted, then un-reverted, then reverted, and so on. This change adds a hint when the user runs `jj undo` multiple times consecutively, suggesting that they may be looking for `jj restore`, which allows them to revert the whole state of the repository back to a previous snapshot.

MrJohz requested a review from PhilipMetzger February 4, 2025 14:08

MrJohz commented Feb 4, 2025

View reviewed changes

cli/src/commands/operation/undo.rs Outdated Show resolved Hide resolved

MrJohz force-pushed the jf/push-wptnznuqqxzm branch 3 times, most recently from 0689655 to 463b1a3 Compare February 4, 2025 14:40

PhilipMetzger reviewed Feb 4, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

arxanas reviewed Feb 5, 2025

View reviewed changes

MrJohz force-pushed the jf/push-wptnznuqqxzm branch 2 times, most recently from bd27145 to 023fad6 Compare February 5, 2025 08:48

MrJohz commented Feb 5, 2025

View reviewed changes

MrJohz force-pushed the jf/push-wptnznuqqxzm branch from 023fad6 to 85876be Compare February 5, 2025 08:52

MrJohz force-pushed the jf/push-wptnznuqqxzm branch from 85876be to f5cd562 Compare February 5, 2025 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show a warning when undoing an undo operation #5580

Show a warning when undoing an undo operation #5580

MrJohz commented Feb 4, 2025

PhilipMetzger left a comment

MrJohz commented Feb 4, 2025

PhilipMetzger commented Feb 4, 2025 •

edited

Loading

arxanas commented Feb 4, 2025

martinvonz commented Feb 4, 2025

martinvonz commented Feb 5, 2025

arxanas left a comment

arxanas Feb 4, 2025

MrJohz Feb 5, 2025

MrJohz Feb 5, 2025

Show a warning when undoing an undo operation #5580

Are you sure you want to change the base?

Show a warning when undoing an undo operation #5580

Conversation

MrJohz commented Feb 4, 2025

Checklist

PhilipMetzger left a comment

Choose a reason for hiding this comment

MrJohz commented Feb 4, 2025

PhilipMetzger commented Feb 4, 2025 • edited Loading

arxanas commented Feb 4, 2025

martinvonz commented Feb 4, 2025

martinvonz commented Feb 5, 2025

arxanas left a comment

Choose a reason for hiding this comment

arxanas Feb 4, 2025

Choose a reason for hiding this comment

MrJohz Feb 5, 2025

Choose a reason for hiding this comment

MrJohz Feb 5, 2025

Choose a reason for hiding this comment

PhilipMetzger commented Feb 4, 2025 •

edited

Loading