cli: hint at recovered changes after workspace update-stale#9786
Conversation
PhilipMetzger
left a comment
There was a problem hiding this comment.
LG with minor nits. I guess Joseph will also chime in with some wording fixes.
| "Uncommitted changes were saved in commit {} before the working \ | ||
| copy was updated.\nRestore them with `jj restore --from {}`.", |
There was a problem hiding this comment.
nit: indoc seems useful here instead of manually inserting newlines and stuff.
There was a problem hiding this comment.
Done — now uses writedoc! with an inline {stale_commit} capture, so there are no manual \ns or \ line-continuations.
| * When `jj workspace update-stale` resets a working copy that had un-snapshotted | ||
| changes, it now prints a hint naming the commit those changes were saved in and | ||
| the `jj restore` command to recover them, so the update no longer looks like | ||
| data loss. Disable with `hints.recovered-changes = false`. |
There was a problem hiding this comment.
nit: This is extremely wordy for no good reason. Also this is missing a Issue link.
"When jj workspace update-stale resets a working copy with unsnapshotted changes it now provides a hint on how to recover these changes.
There was a problem hiding this comment.
Trimmed to one line and added the #7229 link.
| @@ -1,2 +1,3 @@ | |||
| [hints] | |||
| resolving-conflicts = true | |||
| recovered-changes = true | |||
There was a problem hiding this comment.
nit: I don't think we should add this option to this table until people complain.
There was a problem hiding this comment.
Removed — reverted hints.toml.
| // overwrote them on disk. Point the user at that commit so the | ||
| // update doesn't look like data loss. | ||
| if recovered_local_changes | ||
| && self.settings().get_bool("hints.recovered-changes")? |
There was a problem hiding this comment.
ditto, its to early to provide an option for that.
There was a problem hiding this comment.
Dropped the config gate; the hint is unconditional now (shown only when the recovery actually preserved changes).
|
I also think this addresses the mentioned issue. |
fc94b49 to
6d3008a
Compare
|
Thanks for the review! Addressed all four nits in the latest push:
Also took a small pass on the hint wording and the code comment, and rebased onto current |
|
A broader thought this message surfaces — not all of which belongs in the hint itself, but it frames why the wording matters here. This fires at a genuinely high-stress moment. The user is also pushed down this path fairly blind. The upstream stale error — — doesn't mention that running — is cryptic about what became of the user's actual source (the Given that, this feels like one of the few jj messages where a reassurance-first phrasing is worth considering, e.g.: Happy to keep the terse version for house-style consistency — mostly flagging that the emotional context here is unusually high for a hint, and that the surrounding flow (especially the stale error not warning that files will change) might merit its own follow-up. Curious what you and @josephlou5 think. |
|
Haha funny to see that I was mentioned for wording :p Seems mostly fine to me, though I have never used Also the commit description (and PR description too) are outdated after recent changes I think. |
`jj workspace update-stale` snapshots any un-snapshotted changes in the stale working copy before it checks out the fresh commit and overwrites them on disk. Since the only output named the fresh commit, the update looked like data loss even though the changes were preserved and recoverable. When the pre-update snapshot preserved such changes, print a hint naming the commit they were saved in and the `jj restore --from <commit>` to recover them. The lost-operation recovery path already prints a similar explanation; this gives the far more common path the same legibility. Addresses jj-vcs#7229.
6d3008a to
d585f17
Compare
|
Good catch, thanks — updated both the commit and PR descriptions to drop the config-gate mention (removed in the earlier round) and match the current wording. |
Problem
When
jj workspace update-staleresets a stale working copy, it first snapshots any un-snapshotted on-disk changes into a commit, then checks out the fresh commit — overwriting those changes on disk. The only output names the fresh commit, so the update looks like data loss even though the changes were preserved and are recoverable:This has confused several users (e.g. #7229, and reports on Discord).
jj undodoesn't help here either, because update-stale ends on a reconcile/merge operation (Error: Cannot undo a merge operation), so the command's own output is the only reasonable place to surface the recovery path.Change
When the pre-update snapshot actually preserved un-snapshotted changes, print a hint naming the commit they were saved in and the
jj restoreto get them back:RECOVERY COMMIT FROM ...explanation; this gives the far more common normal path the same legibility.Tests
test_workspaces_update_stale_recovers_changes: makes a workspace stale with an un-snapshotted edit, runsjj workspace update-stale, asserts the hint, then runs the exactjj restore --from <hinted commit>and asserts the edit is recovered — a full round-trip.update-stale, and the auto-update-stale case). The clean-stale and "attempted recovery, but not stale" tests are unchanged, pinning the gating.jj-clisuite passes;cargo fmtclean.Addresses the confusion reported in #7229.