From c1ca1ed32c7046d4314b8cd915c1be7c706cab9d Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Sat, 20 Apr 2024 09:28:04 -0400 Subject: [PATCH] WIP: Don't advance branches to a target that already has branches --- cli/src/cli_util.rs | 20 ++++++- cli/src/commands/commit.rs | 2 +- cli/src/commands/new.rs | 2 +- cli/tests/test_advance_branches.rs | 91 +++++++++++++++++++++++------- 4 files changed, 93 insertions(+), 22 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index eee53c53b3..383c635c9f 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1576,7 +1576,25 @@ impl WorkspaceCommandTransaction<'_> { /// the branch is conflicted before the update, it will remain conflicted /// after the update, but the conflict will involve the `move_to` commit /// instead of the old commit. - pub fn advance_branches(&mut self, branches: Vec, move_to: &CommitId) { + pub fn advance_branches( + &mut self, + ui: &Ui, + branches: Vec, + move_to: &CommitId, + ) { + let current_branches = self + .base_repo() + .view() + .local_branches_for_commit(move_to) + .count(); + if current_branches != 0 { + writeln!( + ui.warning_default(), + "Refusing to advance branches to a destination with a branch." + ) + .ok(); + return; + } for branch in branches { // This removes the old commit ID from the branch's RefTarget and // replaces it with the `move_to` ID. diff --git a/cli/src/commands/commit.rs b/cli/src/commands/commit.rs index a33e58624e..2d0f210e75 100644 --- a/cli/src/commands/commit.rs +++ b/cli/src/commands/commit.rs @@ -124,7 +124,7 @@ new working-copy commit. .write()?; // Does nothing if there's no branches to advance. - tx.advance_branches(advanceable_branches, new_commit.id()); + tx.advance_branches(ui, advanceable_branches, new_commit.id()); for workspace_id in workspace_ids { tx.mut_repo().edit(workspace_id, &new_wc_commit).unwrap(); diff --git a/cli/src/commands/new.rs b/cli/src/commands/new.rs index 9655b7bb57..4c75b4728b 100644 --- a/cli/src/commands/new.rs +++ b/cli/src/commands/new.rs @@ -212,7 +212,7 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.", // Does nothing if there's no branches to advance. if let Some(target) = advance_branches_target { - tx.advance_branches(advanceable_branches, &target); + tx.advance_branches(ui, advanceable_branches, &target); } tx.finish(ui, "new empty commit")?; diff --git a/cli/tests/test_advance_branches.rs b/cli/tests/test_advance_branches.rs index afa25c2f9d..fe75799707 100644 --- a/cli/tests/test_advance_branches.rs +++ b/cli/tests/test_advance_branches.rs @@ -43,18 +43,33 @@ fn set_advance_branches(test_env: &TestEnvironment, enabled: bool) { // Runs a command in the specified test environment and workspace path that // describes the current commit with `commit_message` and creates a new commit -// on top of it. -type CommitFn = fn(env: &TestEnvironment, workspace_path: &Path, commit_message: &str); +// on top of it. Returns the stdout and stderr of the command the creates the +// commit. +type CommitFn = + fn(env: &TestEnvironment, workspace_path: &Path, commit_message: &str) -> (String, String); + +enum CommitFnType { + Commit, + DescribeNew, +} // Implements CommitFn using the `jj commit` command. -fn commit_cmd(env: &TestEnvironment, workspace_path: &Path, commit_message: &str) { - env.jj_cmd_ok(workspace_path, &["commit", "-m", commit_message]); +fn commit_cmd( + env: &TestEnvironment, + workspace_path: &Path, + commit_message: &str, +) -> (String, String) { + env.jj_cmd_ok(workspace_path, &["commit", "-m", commit_message]) } // Implements CommitFn using the `jj describe` and `jj new`. -fn describe_new_cmd(env: &TestEnvironment, workspace_path: &Path, commit_message: &str) { +fn describe_new_cmd( + env: &TestEnvironment, + workspace_path: &Path, + commit_message: &str, +) -> (String, String) { env.jj_cmd_ok(workspace_path, &["describe", "-m", commit_message]); - env.jj_cmd_ok(workspace_path, &["new"]); + env.jj_cmd_ok(workspace_path, &["new"]) } // Check that enabling and disabling advance-branches works as expected. @@ -131,19 +146,6 @@ fn test_advance_branches_at_minus(make_commit: CommitFn) { ◉ branches{} desc: "###); } - - // Create a second branch pointing to @. On the next commit, only the first - // branch, which points to @-, will advance. - test_env.jj_cmd_ok(&workspace_path, &["branch", "create", "test_branch2"]); - make_commit(&test_env, &workspace_path, "second"); - insta::allow_duplicates! { - insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" - @ branches{} desc: - ◉ branches{test_branch test_branch2} desc: second - ◉ branches{} desc: first - ◉ branches{} desc: - "###); - } } // Test that per-branch overrides invert the behavior of @@ -295,6 +297,57 @@ fn test_advance_branches_multiple_branches(make_commit: CommitFn) { } } +// Branches will not be advanced to a target commit that already has a branch. +#[test_case(commit_cmd, CommitFnType::Commit; "commit")] +#[test_case(describe_new_cmd, CommitFnType::DescribeNew; "new")] +fn test_advance_branches_no_branch_on_target(make_commit: CommitFn, cmd_type: CommitFnType) { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let workspace_path = test_env.env_root().join("repo"); + + set_advance_branches(&test_env, true); + test_env.jj_cmd_ok(&workspace_path, &["branch", "create", "-r", "@-", "b1"]); + test_env.jj_cmd_ok(&workspace_path, &["branch", "create", "-r", "@", "b2"]); + + insta::allow_duplicates! { + // Check the initial state of the repo. + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ branches{b2} desc: + ◉ branches{b1} desc: + "###); + } + + // Both branches are eligible and both will advance. + let (stdout, stderr) = make_commit(&test_env, &workspace_path, "first"); + insta::allow_duplicates! { + insta::assert_snapshot!(stdout, @""); + } + match cmd_type { + CommitFnType::Commit => { + insta::allow_duplicates! { + insta::assert_snapshot!(stderr, @r###" + Working copy now at: mzvwutvl 24bb7f9d (empty) (no description set) + Parent commit : qpvuntsm 95f2456c b1 b2 | (empty) first + "###); + } + } + _ => { + insta::assert_snapshot!(stderr, @r###" + Warning: Refusing to advance branches to a destination with a branch. + Working copy now at: royxmykx 2e6f2cdd (empty) (no description set) + Parent commit : qpvuntsm 95f2456c b2 | (empty) first + "###); + } + } + insta::allow_duplicates! { + insta::assert_snapshot!(get_log_output_with_branches(&test_env, &workspace_path), @r###" + @ branches{} desc: + ◉ branches{b2} desc: first + ◉ branches{b1} desc: + "###); + } +} + // Call `jj new` on an interior commit and see that the branch pointing to its // parent's parent is advanced. #[test]