cli: resolve "op undo"/"restore" operation against pre-snapshot state

[yuja]

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)
• [x] I have added tests to cover my changes

[yuja]

I think I actually sometimes use that for undoing recent changes. I agree that it's a bit weird to depend on knowing when you last ran a command. But it seems similarly weird to ignore only the latest snapshot. Perhaps we should tag snapshot operations somehow and skip all of them so it becomes more predictable?

Interesting point. I also occasionally use undo to revert the last snapshot, so I don't think all snapshot operations should be skipped. If the dirty working copy isn't snapshotted yet, it makes some sense to undo the new snapshot.

So we'll probably better to add undo -i to deal with this kind of problems.

[ilyagr]

I think this could cause a bit of confusion if the snapshot changes the same line as the operation the user wanted undone. There will be a weird conflict. Perhaps an error would be better for jj undo, explaining what happened.

Separately, I think this should be explained and thought of as an issue of opset resolution (the meaning of "@" changed), not a special property of the jj undo command. After coming up with this, I noticed this is exactly how Yuya implemented it, but it wasn't obvious from the description. If the meaning of @ changes for some other reason, the problem is the same (jj git import would fit here, if it wasn't already a special case).

So, the error condition could be that the opset resolved to a different operation before and after the snapshot (and any other automatic operations that happen before the undo). The error text could mention what this opset previously resolved to.

I wondered for a moment that the meaning of the commit @ (the working copy) could change because of an automatic jj git import, in which case it should get the same check. I think we don't currently allow that, even if commits get abandoned, right?

[yuja]

I think this could cause a bit of confusion if the snapshot changes the same line as the operation the user wanted undone. There will be a weird conflict. Perhaps an error would be better for jj undo, explaining what happened.

An error is also fine, but it's getting unclear to me what the current operation should mean. I thought it should be the last committed operation (so this patch), but the working-copy changes already exist (though not committed yet), and I think it also makes sense to discard the new working-copy changes by jj undo.

FWIW, a weird conflict could be avoided if jj undo were aliased to jj op restore @-.

I wondered for a moment that the meaning of the commit @ (the working copy) could change because of an automatic jj git import, in which case it should get the same check. I think we don't currently allow that, even if commits get abandoned, right?

I don't follow. Automatic git import could move the working copy (to the Git HEAD), but that wouldn't affect the "@" operation resolution.

[ilyagr]

I thought it should be the last committed operation (so this patch), but the working-copy changes already exist (though not committed yet),

I was suggesting making it an error whenever there is disagreement here. The problem is that "@" at the current command (after snapshotting and any automatic jj git import) is different from what "@" meant after the last command was done. If this last command was jj op log, the user might expect "@" to still mean that. Again, the suggestion is to print an error whenever these differ.

My other point is that, in this PR, you are treating the most annoying and visible case of this: jj undo and jj restore. But this applies equally to any other operation that receives an opset that includes "@" in it.

FWIW, a weird conflict could be avoided if jj undo were aliased to jj op restore @-.

I don't know that this would make a difference. There is also confusion here: do we use the value of "@-" from the end of the last operation or the potentially different value of "@-" after snapshotting?

Another possible way to see the problem: I only had a cursory look, but I think that, if there are un-snapshotted changes, jj op restore @- would mean a different thing before and after this PR.

I don't follow. Automatic git import could move the working copy (to the Git HEAD), but that wouldn't affect the "@" operation resolution.

A similar disagreement is conceivable for revsets as it was for opsets above. The problem would occur as long as there's any possibility that, after snapshotting and jj git import , "@" means something different from what "@" meant after the last command (where "@" means the working copy now).

Update: Actually, this would be very common, so perhaps an error here would be distracting. If there are un-snapshotted changes, should jj rebase -r @ rebase the currently stored working copy (the version at the end of the previous command) or the working copy after the snapshot? Probably the latter; the former would rebase a now-abandoned commit and be very confusing. So, maybe the analogy with opsets is not perfect, or maybe this suggests that @ should always mean the snapshotting operation (the version of @ that is correct after the snapshotting).

---

Thank you for tackling this, by the way. It's not something I considered before. I'm hoping it might help to think in terms of the more general problem.

By the way, I don't insist that an error is the best possible resolution, perhaps we can think of something better.

[yuja]

A similar disagreement is conceivable for revsets as it was for opsets above. The problem would occur as long as there's any possibility that, after snapshotting and jj git import , "@" means something different from what "@" meant after the last command (where "@" means the working copy now).

I see. That makes me think the current behavior is more consistent as you mentioned. If I ran jj log && gh pr checkout && jj show @, I would expect the @ revision points to the new HEAD. Op heads might be a bit different, but it makes some sense that everything is resolved after the snapshotting. (--at-operation @ is resolved earlier, but the result isn't visible to the user.)

[yuja]

Closing because

That makes me think the current behavior is more consistent

Perhaps we can improve the doc and status message of jj undo/jj op restore instead.