Fix new --after ignoring immutable commits + update hint for root commit being immutable

CHANGELOG.md

@@ -78,6 +78,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * Conflicts in executable files can now be resolved just like conflicts in
   non-executable files ([#1279](https://github.com/martinvonz/jj/issues/1279)).
 
+* `jj new --insert-before` and `--insert-after` now respect immutable revisions
+  ([#2468](https://github.com/martinvonz/jj/pull/2468)).
+
 ## [0.10.0] - 2023-10-04
 
 ### Breaking changes

cli/src/cli_util.rs

@@ -1268,10 +1268,19 @@ Set which revision the branch points to with `jj branch set {branch_name} -r <RE
         let revset = self.evaluate_revset(to_rewrite_revset.intersection(&immutable_revset))?;
         if let Some(commit) = revset.iter().commits(self.repo().store()).next() {
             let commit = commit?;
-            return Err(user_error_with_hint(
-                format!("Commit {} is immutable", short_commit_hash(commit.id()),),
-                "Configure the set of immutable commits via `revset-aliases.immutable_heads()`.",
-            ));
+            let error = if commit.id() == self.repo().store().root_commit_id() {
+                user_error(format!(
+                    "The root commit {} is immutable",
+                    short_commit_hash(commit.id()),
+                ))
+            } else {
+                user_error_with_hint(
+                    format!("Commit {} is immutable", short_commit_hash(commit.id()),),
+                    "Configure the set of immutable commits via \
+                     `revset-aliases.immutable_heads()`.",
+                )
+            };
+            return Err(error);
         }
 
         Ok(())

cli/src/commands/new.rs

@@ -81,17 +81,15 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.",
         .collect_vec();
     let target_ids = target_commits.iter().map(|c| c.id().clone()).collect_vec();
     let mut tx = workspace_command.start_transaction("new empty commit");
-    let mut num_rebased = 0;
+    let mut num_rebased;
     let new_commit;
     if args.insert_before {
         // Instead of having the new commit as a child of the changes given on the
         // command line, add it between the changes' parents and the changes.
         // The parents of the new commit will be the parents of the target commits
         // which are not descendants of other target commits.
-        let root_commit = tx.repo().store().root_commit();
-        if target_ids.contains(root_commit.id()) {
-            return Err(user_error("Cannot insert a commit before the root commit"));
-        }
+        tx.base_workspace_helper()
+            .check_rewritable(&target_commits)?;
         let new_children = RevsetExpression::commits(target_ids.clone());
         let new_parents = new_children.parents();
         if let Some(commit_id) = new_children
@@ -116,6 +114,7 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.",
         // The git backend does not support creating merge commits involving the root
         // commit.
         if new_parents_commits.len() > 1 {
+            let root_commit = tx.repo().store().root_commit();
             new_parents_commits.retain(|c| c != &root_commit);
         }
         let merged_tree = merge_commit_trees(tx.repo(), &new_parents_commits)?;
@@ -135,43 +134,46 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.",
             )?;
         }
     } else {
+        let old_parents = RevsetExpression::commits(target_ids.clone());
+        let commits_to_rebase: Vec<Commit> = if args.insert_after {
+            // Each child of the targets will be rebased: its set of parents will be updated
+            // so that the targets are replaced by the new commit.
+            // Exclude children that are ancestors of the new commit
+            let to_rebase = old_parents.children().minus(&old_parents.ancestors());
+            to_rebase
+                .resolve(tx.base_repo().as_ref())?
+                .evaluate(tx.base_repo().as_ref())?
+                .iter()
+                .commits(tx.base_repo().store())
+                .try_collect()?
+        } else {
+            vec![]
+        };
+        tx.base_workspace_helper()
+            .check_rewritable(&commits_to_rebase)?;
         let merged_tree = merge_commit_trees(tx.repo(), &target_commits)?;
         new_commit = tx
             .mut_repo()
             .new_commit(command.settings(), target_ids.clone(), merged_tree.id())
             .set_description(cli_util::join_message_paragraphs(&args.message_paragraphs))
             .write()?;
-        if args.insert_after {
-            // Each child of the targets will be rebased: its set of parents will be updated
-            // so that the targets are replaced by the new commit.
-            let old_parents = RevsetExpression::commits(target_ids);
-            // Exclude children that are ancestors of the new commit
-            let to_rebase = old_parents.children().minus(&old_parents.ancestors());
-            let commits_to_rebase: Vec<Commit> = to_rebase
+        num_rebased = commits_to_rebase.len();
+        for child_commit in commits_to_rebase {
+            let commit_parents = RevsetExpression::commits(child_commit.parent_ids().to_owned());
+            let new_parents = commit_parents.minus(&old_parents);
+            let mut new_parent_commits: Vec<Commit> = new_parents
                 .resolve(tx.base_repo().as_ref())?
                 .evaluate(tx.base_repo().as_ref())?
                 .iter()
                 .commits(tx.base_repo().store())
                 .try_collect()?;
-            num_rebased = commits_to_rebase.len();
-            for child_commit in commits_to_rebase {
-                let commit_parents =
-                    RevsetExpression::commits(child_commit.parent_ids().to_owned());
-                let new_parents = commit_parents.minus(&old_parents);
-                let mut new_parent_commits: Vec<Commit> = new_parents
-                    .resolve(tx.base_repo().as_ref())?
-                    .evaluate(tx.base_repo().as_ref())?
-                    .iter()
-                    .commits(tx.base_repo().store())
-                    .try_collect()?;
-                new_parent_commits.push(new_commit.clone());
-                rebase_commit(
-                    command.settings(),
-                    tx.mut_repo(),
-                    &child_commit,
-                    &new_parent_commits,
-                )?;
-            }
+            new_parent_commits.push(new_commit.clone());
+            rebase_commit(
+                command.settings(),
+                tx.mut_repo(),
+                &child_commit,
+                &new_parent_commits,
+            )?;
         }
     }
     num_rebased += tx.mut_repo().rebase_descendants(command.settings())?;

cli/tests/test_immutable_commits.rs

@@ -56,8 +56,7 @@ fn test_rewrite_immutable_generic() {
     test_env.add_config(r#"revset-aliases."immutable_heads()" = "none()""#);
     let stderr = test_env.jj_cmd_failure(&repo_path, &["edit", "root()"]);
     insta::assert_snapshot!(stderr, @r###"
-    Error: Commit 000000000000 is immutable
-    Hint: Configure the set of immutable commits via `revset-aliases.immutable_heads()`.
+    Error: The root commit 000000000000 is immutable
     "###);
     // Error if we redefine immutable_heads() with an argument
     // TODO: This error comes from the built-in definition of
@@ -154,6 +153,18 @@ fn test_rewrite_immutable_commands() {
     Error: Commit 16ca9d800b08 is immutable
     Hint: Configure the set of immutable commits via `revset-aliases.immutable_heads()`.
     "###);
+    // new --insert-before
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["new", "--insert-before", "main"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Error: Commit 16ca9d800b08 is immutable
+    Hint: Configure the set of immutable commits via `revset-aliases.immutable_heads()`.
+    "###);
+    // new --insert-after parent_of_main
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["new", "--insert-after", "description(b)"]);
+    insta::assert_snapshot!(stderr, @r###"
+    Error: Commit 16ca9d800b08 is immutable
+    Hint: Configure the set of immutable commits via `revset-aliases.immutable_heads()`.
+    "###);
     // rebase -s
     let stderr = test_env.jj_cmd_failure(&repo_path, &["rebase", "-s=main", "-d=@"]);
     insta::assert_snapshot!(stderr, @r###"

cli/tests/test_new_command.rs

@@ -400,7 +400,7 @@ fn test_new_insert_before_root() {
     let stderr =
         test_env.jj_cmd_failure(&repo_path, &["new", "--insert-before", "-m", "G", "root()"]);
     insta::assert_snapshot!(stderr, @r###"
-    Error: Cannot insert a commit before the root commit
+    Error: The root commit 000000000000 is immutable
     "###);
 }