Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make other working copies immutable #4421

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Make other working copies immutable #4421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e333620
revsets: add a `other_working_copies()` revset
martinvonz Sep 6, 2024
c42a941
cli: don't create a new working-copy commit for already immutable com…
martinvonz Sep 6, 2024
61339c0
cli: consider other working copies immutable by default
martinvonz Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

working_copies() ~ @ is close, but it's not correct if the
same commit is being edited in the current workspace and in another
workspace.

Wouldn't it simpler to allow mutation in that case? If the wc commit is already editing, I wouldn't expect it to become immutable by editing the exact commit in other workspace.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It certainly would be simpler, but I think it seems more correct. Since the goal is to avoid accidentally creating stale workspaces, we should avoid that also if another workspace is editing the same commit (or a descendant).

There's also not currently a way to correctly calculate the set of commits edited in other workspaces, but maybe that doesn't really matter if we ignore this use case (i.e., I haven't thought of other use cases).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the goal is to avoid accidentally creating stale workspaces, we should avoid that also if another workspace is editing the same commit (or a descendant).

That's correct, but in order to edit the same commit, the user would have to use --ignore-immutable edit in another workspace, so I (as a user) wouldn't mind if it made the other workspace ignore immutability of the wc commit. (There might be other ways to enter such situation, but that's rare?)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could also happen if you do jj edit <parent> in one workspace and jj edit <child> in another workspace. Going back to the first workspace would then result in it becoming immutable. That seems consistent to me. However, since we don't prevent snapshotting even if the commit is immutable, it's already not going to work very well for preventing stale workspaces in that case, so I think it makes sense to simply add working_copies() ~ @ as I think you're suggesting. I'll update this PR to do just that.

Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
(inherit from parent; default), `full` (full working copy), or `empty` (the
empty working copy).

* New revset function `other_working_copies()` selects working-copy commits
in all but the current workspace.

### Fixed bugs

* Fixed panic when parsing invalid conflict markers of a particular form.
Expand Down
6 changes: 6 additions & 0 deletions cli/src/cli_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1476,9 +1476,15 @@ See https://martinvonz.github.io/jj/latest/working-copy/#stale-working-copy \
tx.mut_repo().view().wc_commit_ids().clone().iter().sorted()
//sorting otherwise non deterministic order (bad for tests)
{
// Create a new working-copy commit in the workspace if the working copy became
// immutable (but not if it already was)
if self
.check_repo_rewritable(tx.repo(), [wc_commit_id])
.is_err()
&& tx.base_repo().index().has_id(wc_commit_id)
&& !self
.check_repo_rewritable(tx.base_repo().as_ref(), [wc_commit_id])
.is_err()
{
let wc_commit = tx.repo().store().get_commit(wc_commit_id)?;
tx.mut_repo()
Expand Down
6 changes: 3 additions & 3 deletions cli/src/config/revsets.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

[revsets]
fix = "reachable(@, mutable())"
log = "@ | ancestors(immutable_heads().., 2) | trunk()"
log = "working_copies() | ancestors(immutable_heads().., 2) | trunk()"

[revset-aliases]
'trunk()' = '''
Expand All @@ -18,7 +18,7 @@ latest(
)
'''

'builtin_immutable_heads()' = 'trunk() | tags() | untracked_remote_branches()'
'builtin_immutable_heads()' = 'trunk() | tags() | untracked_remote_branches() | other_working_copies()'
'immutable_heads()' = 'builtin_immutable_heads()'
'immutable()' = '::(immutable_heads() | root())'
'mutable()' = '~immutable()'
'mutable()' = '~immutable()'
3 changes: 0 additions & 3 deletions cli/tests/test_git_private_commits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,6 @@ fn test_git_private_commits_are_not_checked_if_immutable() {
insta::assert_snapshot!(stderr, @r###"
Branch changes to push to origin:
Move forward branch main from 7eb97bf230ad to aa3058ff8663
Warning: The working-copy commit in workspace 'default' became immutable, so a new commit has been created on top of it.
Working copy now at: yostqsxw dce4a15c (empty) (no description set)
Parent commit : yqosqzyt aa3058ff main | (empty) private 1
"###);
}

Expand Down
6 changes: 3 additions & 3 deletions cli/tests/test_immutable_commits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,11 @@ fn test_immutable_heads_set_to_working_copy() {
test_env.jj_cmd_ok(test_env.env_root(), &["git", "init"]);
test_env.jj_cmd_ok(test_env.env_root(), &["branch", "create", "main"]);
test_env.add_config(r#"revset-aliases."immutable_heads()" = "@""#);
// No new commit since it was already immutable
let (_, stderr) = test_env.jj_cmd_ok(test_env.env_root(), &["new", "-m=a"]);
insta::assert_snapshot!(stderr, @r###"
Warning: The working-copy commit in workspace 'default' became immutable, so a new commit has been created on top of it.
Working copy now at: pmmvwywv 7278b2d8 (empty) (no description set)
Parent commit : kkmpptxz a713ef56 (empty) a
Working copy now at: kkmpptxz a713ef56 (empty) a
Parent commit : qpvuntsm e0360db1 main | (no description set)
"###);
}

Expand Down
109 changes: 57 additions & 52 deletions cli/tests/test_workspaces.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,17 @@ fn test_workspaces_add_second_workspace() {
// Can see the working-copy commit in each workspace in the log output. The "@"
// node in the graph indicates the current workspace's working-copy commit.
insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
○ 5ed2222c28e2 second@
◆ 5ed2222c28e2 second@
│ @ 8183d0fcaa4c default@
├─╯
○ 751b12b7b981
◆ 751b12b7b981
◆ 000000000000
"###);
insta::assert_snapshot!(get_log_output(&test_env, &secondary_path), @r###"
@ 5ed2222c28e2 second@
│ ○ 8183d0fcaa4c default@
│ ◆ 8183d0fcaa4c default@
├─╯
○ 751b12b7b981
◆ 751b12b7b981
◆ 000000000000
"###);

Expand Down Expand Up @@ -144,12 +144,12 @@ fn test_workspaces_add_second_workspace_on_merge() {

// The new workspace's working-copy commit shares all parents with the old one.
insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
○ 7013a493bd09 second@
◆ 7013a493bd09 second@
├─╮
│ │ @ 35e47bff781e default@
╭─┬─╯
│ ○ 444b77e99d43
○ │ 1694f2ddf8ec
│ ◆ 444b77e99d43
◆ │ 1694f2ddf8ec
├─╯
◆ 000000000000
"###);
Expand Down Expand Up @@ -282,19 +282,19 @@ fn test_workspaces_add_workspace_at_revision() {
// Can see the working-copy commit in each workspace in the log output. The "@"
// node in the graph indicates the current workspace's working-copy commit.
insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
○ e374e74aa0c8 second@
◆ e374e74aa0c8 second@
│ @ dadeedb493e8 default@
│ ○ c420244c6398
├─╯
○ f6097c2f7cac
◆ f6097c2f7cac
◆ 000000000000
"###);
insta::assert_snapshot!(get_log_output(&test_env, &secondary_path), @r###"
@ e374e74aa0c8 second@
│ ○ dadeedb493e8 default@
│ ○ c420244c6398
│ ◆ dadeedb493e8 default@
│ ◆ c420244c6398
├─╯
○ f6097c2f7cac
◆ f6097c2f7cac
◆ 000000000000
"###);
}
Expand Down Expand Up @@ -352,12 +352,12 @@ fn test_workspaces_add_workspace_multiple_revisions() {
"###);

insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
○ f4fa64f40944 merge@
◆ f4fa64f40944 merge@
├─┬─╮
│ │ ○ f6097c2f7cac
│ ○ │ 544cd61f2d26
│ │ ◆ f6097c2f7cac
│ ◆ │ 544cd61f2d26
│ ├─╯
○ │ 6c843d62ca29
◆ │ 6c843d62ca29
├─╯
│ @ 5b36783cd11c default@
├─╯
Expand Down Expand Up @@ -471,10 +471,10 @@ fn test_workspaces_conflicting_edits() {
test_env.jj_cmd_ok(&main_path, &["workspace", "add", "../secondary"]);

insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
○ 3224de8ae048 secondary@
◆ 3224de8ae048 secondary@
│ @ 06b57f44a3ca default@
├─╯
○ 506f4ec3c2c6
◆ 506f4ec3c2c6
◆ 000000000000
"###);

Expand All @@ -483,7 +483,7 @@ fn test_workspaces_conflicting_edits() {
std::fs::write(secondary_path.join("file"), "changed in second\n").unwrap();
// Squash the changes from the main workspace into the initial commit (before
// running any command in the secondary workspace
let (stdout, stderr) = test_env.jj_cmd_ok(&main_path, &["squash"]);
let (stdout, stderr) = test_env.jj_cmd_ok(&main_path, &["squash", "--ignore-immutable"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 1 descendant commits
Expand All @@ -494,9 +494,9 @@ fn test_workspaces_conflicting_edits() {
// The secondary workspace's working-copy commit was updated
insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
@ a58c9a9b19ce default@
│ ○ e82cd4ee8faa secondary@
│ ◆ e82cd4ee8faa secondary@
├─╯
○ d41244767d45
◆ d41244767d45
◆ 000000000000
"###);
let stderr = test_env.jj_cmd_failure(&secondary_path, &["st"]);
Expand Down Expand Up @@ -526,23 +526,23 @@ fn test_workspaces_conflicting_edits() {
insta::assert_snapshot!(get_log_output(&test_env, &secondary_path),
@r###"
× a28c85ce128b (divergent)
│ ○ a58c9a9b19ce default@
│ ◆ a58c9a9b19ce default@
├─╯
│ @ e82cd4ee8faa secondary@ (divergent)
├─╯
○ d41244767d45
◆ d41244767d45
◆ 000000000000
"###);
// The stale working copy should have been resolved by the previous command
let stdout = get_log_output(&test_env, &secondary_path);
assert!(!stdout.starts_with("The working copy is stale"));
insta::assert_snapshot!(stdout, @r###"
× a28c85ce128b (divergent)
│ ○ a58c9a9b19ce default@
│ ◆ a58c9a9b19ce default@
├─╯
│ @ e82cd4ee8faa secondary@ (divergent)
├─╯
○ d41244767d45
◆ d41244767d45
◆ 000000000000
"###);
}
Expand All @@ -561,16 +561,16 @@ fn test_workspaces_updated_by_other() {
test_env.jj_cmd_ok(&main_path, &["workspace", "add", "../secondary"]);

insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
○ 3224de8ae048 secondary@
◆ 3224de8ae048 secondary@
│ @ 06b57f44a3ca default@
├─╯
○ 506f4ec3c2c6
◆ 506f4ec3c2c6
◆ 000000000000
"###);

// Rewrite the check-out commit in one workspace.
std::fs::write(main_path.join("file"), "changed in main\n").unwrap();
let (stdout, stderr) = test_env.jj_cmd_ok(&main_path, &["squash"]);
let (stdout, stderr) = test_env.jj_cmd_ok(&main_path, &["squash", "--ignore-immutable"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Rebased 1 descendant commits
Expand All @@ -581,9 +581,9 @@ fn test_workspaces_updated_by_other() {
// The secondary workspace's working-copy commit was updated.
insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
@ a58c9a9b19ce default@
│ ○ e82cd4ee8faa secondary@
│ ◆ e82cd4ee8faa secondary@
├─╯
○ d41244767d45
◆ d41244767d45
◆ 000000000000
"###);
let stderr = test_env.jj_cmd_failure(&secondary_path, &["st"]);
Expand All @@ -602,10 +602,10 @@ fn test_workspaces_updated_by_other() {
"###);
insta::assert_snapshot!(get_log_output(&test_env, &secondary_path),
@r###"
○ a58c9a9b19ce default@
◆ a58c9a9b19ce default@
│ @ e82cd4ee8faa secondary@
├─╯
○ d41244767d45
◆ d41244767d45
◆ 000000000000
"###);
}
Expand Down Expand Up @@ -647,7 +647,7 @@ fn test_workspaces_current_op_discarded_by_other() {

// Create an op by abandoning the parent commit. Importantly, that commit also
// changes the target tree in the secondary workspace.
test_env.jj_cmd_ok(&main_path, &["abandon", "@-"]);
test_env.jj_cmd_ok(&main_path, &["abandon", "@-", "--ignore-immutable"]);

let stdout = test_env.jj_cmd_success(
&main_path,
Expand All @@ -659,7 +659,7 @@ fn test_workspaces_current_op_discarded_by_other() {
],
);
insta::assert_snapshot!(stdout, @r###"
@ 7337338f0b abandon commit 20dd439c4bd12c6ad56c187ac490bd0141804618f638dc5c4dc92ff9aecba20f152b23160db9dcf61beb31a5cb14091d9def5a36d11c9599cc4d2e5689236af1
@ c43504554c abandon commit 20dd439c4bd12c6ad56c187ac490bd0141804618f638dc5c4dc92ff9aecba20f152b23160db9dcf61beb31a5cb14091d9def5a36d11c9599cc4d2e5689236af1
○ f4bd4d046b create initial working-copy commit in workspace secondary
○ 0f99641958 add workspace 'secondary'
○ 5641361f60 new empty commit
Expand All @@ -676,10 +676,10 @@ fn test_workspaces_current_op_discarded_by_other() {
test_env.jj_cmd_ok(&main_path, &["util", "gc", "--expire=now"]);

insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
○ 96b31dafdc41 secondary@
◆ 96b31dafdc41 secondary@
│ @ 6c051bd1ccd5 default@
├─╯
○ 7c5b25a4fc8f
◆ 7c5b25a4fc8f
◆ 000000000000
"###);

Expand All @@ -698,11 +698,11 @@ fn test_workspaces_current_op_discarded_by_other() {
insta::assert_snapshot!(stdout, @"");

insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
○ b0b400439a82 secondary@
○ 96b31dafdc41
◆ b0b400439a82 secondary@
◆ 96b31dafdc41
│ @ 6c051bd1ccd5 default@
├─╯
○ 7c5b25a4fc8f
◆ 7c5b25a4fc8f
◆ 000000000000
"###);

Expand Down Expand Up @@ -795,10 +795,10 @@ fn test_workspaces_update_stale_snapshot() {

insta::assert_snapshot!(get_log_output(&test_env, &secondary_path), @r###"
@ e672fd8fefac secondary@
│ ○ ea37b073f5ab default@
│ ○ b13c81dedc64
│ ◆ ea37b073f5ab default@
│ ◆ b13c81dedc64
├─╯
○ e6e9989f1179
◆ e6e9989f1179
◆ 000000000000
"###);
}
Expand Down Expand Up @@ -836,8 +836,8 @@ fn test_workspaces_forget() {
// there's only one workspace. We should show it when the command is not run
// from that workspace.
insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
○ 18463f438cc9
○ 4e8f9d2be039
◆ 18463f438cc9
◆ 4e8f9d2be039
◆ 000000000000
"###);

Expand Down Expand Up @@ -902,14 +902,19 @@ fn test_workspaces_forget_multi_transaction() {
"###);

// now, undo, and that should restore both workspaces
test_env.jj_cmd_ok(&main_path, &["op", "undo"]);
let (stdout, stderr) = test_env.jj_cmd_ok(&main_path, &["op", "undo"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Warning: The working-copy commit in workspace 'second' became immutable, so a new commit has been created on top of it.
Warning: The working-copy commit in workspace 'third' became immutable, so a new commit has been created on top of it.
"###);

// finally, there should be three workspaces at the end
let stdout = test_env.jj_cmd_success(&main_path, &["workspace", "list"]);
insta::assert_snapshot!(stdout, @r###"
default: rlvkpnrz 909d51b1 (empty) (no description set)
second: pmmvwywv 18463f43 (empty) (no description set)
third: rzvqmyuk cc383fa2 (empty) (no description set)
second: yostqsxw c6fb1379 (empty) (no description set)
third: wmwvqwsz 2c09d1de (empty) (no description set)
"###);
}

Expand All @@ -925,9 +930,9 @@ fn test_workspaces_forget_abandon_commits() {
test_env.jj_cmd_ok(&main_path, &["workspace", "add", "../third"]);
test_env.jj_cmd_ok(&main_path, &["workspace", "add", "../fourth"]);
let third_path = test_env.env_root().join("third");
test_env.jj_cmd_ok(&third_path, &["edit", "second@"]);
test_env.jj_cmd_ok(&third_path, &["edit", "second@", "--ignore-immutable"]);
let fourth_path = test_env.env_root().join("fourth");
test_env.jj_cmd_ok(&fourth_path, &["edit", "second@"]);
test_env.jj_cmd_ok(&fourth_path, &["edit", "second@", "--ignore-immutable"]);

// there should be four workspaces, three of which are at the same empty commit
let stdout = test_env.jj_cmd_success(&main_path, &["workspace", "list"]);
Expand All @@ -938,7 +943,7 @@ fn test_workspaces_forget_abandon_commits() {
third: uuqppmxq 57d63245 (empty) (no description set)
"###);
insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
○ 57d63245a308 fourth@ second@ third@
◆ 57d63245a308 fourth@ second@ third@
│ @ 4e8f9d2be039 default@
├─╯
◆ 000000000000
Expand All @@ -947,7 +952,7 @@ fn test_workspaces_forget_abandon_commits() {
// delete the default workspace (should not abandon commit since not empty)
test_env.jj_cmd_success(&main_path, &["workspace", "forget", "default"]);
insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
○ 57d63245a308 fourth@ second@ third@
◆ 57d63245a308 fourth@ second@ third@
│ ○ 4e8f9d2be039
├─╯
◆ 000000000000
Expand All @@ -957,7 +962,7 @@ fn test_workspaces_forget_abandon_commits() {
// still have commit checked out)
test_env.jj_cmd_success(&main_path, &["workspace", "forget", "second"]);
insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r###"
○ 57d63245a308 fourth@ third@
◆ 57d63245a308 fourth@ third@
│ ○ 4e8f9d2be039
├─╯
◆ 000000000000
Expand Down
Loading
Loading