Skip to content

Commit

Permalink
workspace: don't lose sparsed-away paths when recovering workspace
Browse files Browse the repository at this point in the history
When an operation is missing and we recover the workspace, we create a
new working-copy commit on top of the desired working-copy commit (per
the available head operation). We then reset the working copy to an
empty tree because it shouldn't really matter much which commit we
reset to. However, when the workspace is sparse, it does matter, as
the test case from the previous patch shows. This patch fixes it by
replacing the `reset_to_empty()` method by a new `recover(&Commit)`,
which effectively resets to the empty tree and then resets to the
commit. That way, any subsequent snapshotting will result keep the
paths from that tree for paths outside the sparse patterns.
  • Loading branch information
martinvonz committed Mar 16, 2024
1 parent ffb1268 commit c55e080
Show file tree
Hide file tree
Showing 5 changed files with 15 additions and 13 deletions.
4 changes: 2 additions & 2 deletions cli/examples/custom-working-copy/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,8 @@ impl LockedWorkingCopy for LockedConflictsWorkingCopy {
self.inner.reset(commit)
}

fn reset_to_empty(&mut self) -> Result<(), ResetError> {
self.inner.reset_to_empty()
fn recover(&mut self, commit: &Commit) -> Result<(), ResetError> {
self.inner.recover(commit)
}

fn sparse_patterns(&self) -> Result<&[RepoPathBuf], WorkingCopyStateError> {
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ fn create_and_check_out_recovery_commit(
mut_repo.set_wc_commit(workspace_id, new_commit.id().clone())?;
let repo = tx.commit("recovery commit");

locked_workspace.locked_wc().reset_to_empty()?;
locked_workspace.locked_wc().recover(&new_commit)?;
locked_workspace.finish(repo.op_id().clone())?;

writeln!(
Expand Down
8 changes: 3 additions & 5 deletions cli/tests/test_workspaces.rs
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ fn test_workspaces_current_op_discarded_by_other() {
insta::assert_snapshot!(stdout, @"");

insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
8fd911b4e595 secondary@
b93a924213f3 secondary@
◉ ec4904a30161
│ @ 74769415363f default@
├─╯
Expand All @@ -511,14 +511,12 @@ fn test_workspaces_current_op_discarded_by_other() {
"###);
let (stdout, stderr) = test_env.jj_cmd_ok(&secondary_path, &["st"]);
insta::assert_snapshot!(stderr, @"");
// TODO: The file outside the sparse patterns should still be there
insta::assert_snapshot!(stdout, @r###"
Working copy changes:
A added
D deleted
M modified
D sparse
Working copy : kmkuslsw 8fd911b4 (no description set)
Working copy : kmkuslsw b93a9242 (no description set)
Parent commit: rzvqmyuk ec4904a3 (empty) (no description set)
"###);
// The modified file should have the same contents it had before (not reset to
Expand All @@ -530,7 +528,7 @@ fn test_workspaces_current_op_discarded_by_other() {
let (stdout, stderr) = test_env.jj_cmd_ok(&secondary_path, &["obslog"]);
insta::assert_snapshot!(stderr, @"");
insta::assert_snapshot!(stdout, @r###"
@ kmkuslsw [email protected] 2001-02-03 04:05:18.000 +07:00 secondary@ 8fd911b4
@ kmkuslsw [email protected] 2001-02-03 04:05:18.000 +07:00 secondary@ b93a9242
│ (no description set)
◉ kmkuslsw hidden [email protected] 2001-02-03 04:05:18.000 +07:00 30ee0d1f
(empty) (no description set)
Expand Down
9 changes: 6 additions & 3 deletions lib/src/local_working_copy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1457,9 +1457,10 @@ impl TreeState {
Ok(())
}

pub fn reset_to_empty(&mut self) {
pub async fn recover(&mut self, new_tree: &MergedTree) -> Result<(), ResetError> {
self.file_states.clear();
self.tree_id = self.store.empty_merged_tree_id();
self.reset(new_tree).await
}
}

Expand Down Expand Up @@ -1778,14 +1779,16 @@ impl LockedWorkingCopy for LockedLocalWorkingCopy {
Ok(())
}

fn reset_to_empty(&mut self) -> Result<(), ResetError> {
fn recover(&mut self, commit: &Commit) -> Result<(), ResetError> {
let new_tree = commit.tree()?;
self.wc
.tree_state_mut()
.map_err(|err| ResetError::Other {
message: "Failed to read the working copy state".to_string(),
err: err.into(),
})?
.reset_to_empty();
.recover(&new_tree)
.block_on()?;
self.tree_state_dirty = true;
Ok(())
}
Expand Down
5 changes: 3 additions & 2 deletions lib/src/working_copy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,9 @@ pub trait LockedWorkingCopy {
/// Update to another commit without touching the files in the working copy.
fn reset(&mut self, commit: &Commit) -> Result<(), ResetError>;

/// Update to the empty tree without touching the files in the working copy.
fn reset_to_empty(&mut self) -> Result<(), ResetError>;
/// Update to another commit without touching the files in the working copy,
/// without assuming that the previous tree exists.
fn recover(&mut self, commit: &Commit) -> Result<(), ResetError>;

/// See `WorkingCopy::sparse_patterns()`
fn sparse_patterns(&self) -> Result<&[RepoPathBuf], WorkingCopyStateError>;
Expand Down

0 comments on commit c55e080

Please sign in to comment.