Add a dry_run mode to LockedWorkingCopy snapshot method. #6375
Conversation
At Google, we are building a tool that needs to know the state of a jj repo constantly. Always snapshotting and updating .jj/working_copy/checkout increases the likelihood of write races and should be avoided. I came across jj-vcs#2562 that encountered the same problem. From the post, the suggested solution is to add a --dry-run/--no-commit-transaction flag. This commit does a part of that by adding a `dry_run` mode to the WorkingCopy snapshot method. I don't intend to address the whole FR.
| }); | ||
| if dry_run { | ||
| return Ok((false, stats)); | ||
| } |
There was a problem hiding this comment.
Always snapshotting and updating .jj/working_copy/checkout increases the likelihood of write races and should be avoided.
Is it important to skip updates of in-memory data? If the write races are the problem, maybe we can just avoid calling .finish(). (We might have to restore the in-meory state, though.)
There was a problem hiding this comment.
Is it important to skip updates of in-memory data?
No, we're concerned about parallel updates to the .jj/working_copy/checkout file only. Since Google uses a network file system, races to the file system are far more common.
maybe we can just avoid calling .finish()
That's a great idea, not sure why I haven't thought of that. In our case, we need the trees to be written, but not the checkout file, and not calling finish() seems to do exactly that. I will put this PR on hold, and see if that works.
We might have to restore the in-memory state, though.
Because the snapshot method has updated the internal variables? Our usage pattern does not call other methods after the dry run snapshot, but maybe this can be a problem for others.
There was a problem hiding this comment.
Yay it worked! I'll abandon this PR soon.
| trace_span!("write tree").in_scope(|| { | ||
| let new_tree_id = tree_builder.write_tree(&self.store).unwrap(); | ||
| if dry_run { | ||
| stats.dry_run_tree_id = Some(new_tree_id); |
There was a problem hiding this comment.
nit: It seems odd that we have dry_run_tree_id which is conditionally set. It might be better to add LockedWorkignCopy::snapshot_dry_run() -> (MergedTreeId, SnapshotStats). It might also make sense to add the function to WorkingCopy because dry-run snapshot() wouldn't need locking.
At Google, we are building a tool that needs to know the state of a jj repo constantly. Always snapshotting and updating .jj/working_copy/checkout increases the likelihood of write races and should be avoided.
I came across #2562 that encountered the same problem. From the post, the suggested solution is to add a --dry-run/--no-commit-transaction flag. This commit does a part of that by adding a
dry_runmode to the WorkingCopy snapshot method. I don't intend to address the whole FR.Checklist
If applicable:
CHANGELOG.md